Merge pull request #12726 from jherico/fix/rc66_crashes

Attempt to resolve potential overlay crashes at shutdown

interface/src/Application.cpp

@@ -7455,7 +7455,7 @@ DisplayPluginPointer Application::getActiveDisplayPlugin() const {
         return _displayPlugin;
     }
 
-    if (!_displayPlugin) {
+    if (!_aboutToQuit && !_displayPlugin) {
         const_cast<Application*>(this)->updateDisplayMode();
         Q_ASSERT(_displayPlugin);
     }

interface/src/ui/overlays/Overlays.cpp

@@ -54,6 +54,7 @@ Overlays::Overlays() {
 }
 
 void Overlays::cleanupAllOverlays() {
+    _shuttingDown = true;
     QMap<OverlayID, Overlay::Pointer> overlaysHUD;
     QMap<OverlayID, Overlay::Pointer> overlaysWorld;
     {
@@ -147,6 +148,10 @@ void Overlays::enable() {
 // Note, can't be invoked by scripts, but can be called by the InterfaceParentFinder
 // class on packet processing threads
 Overlay::Pointer Overlays::getOverlay(OverlayID id) const {
+    if (_shuttingDown) {
+        return nullptr;
+    }
+
     QMutexLocker locker(&_mutex);
     if (_overlaysHUD.contains(id)) {
         return _overlaysHUD[id];
@@ -157,6 +162,10 @@ Overlay::Pointer Overlays::getOverlay(OverlayID id) const {
 }
 
 OverlayID Overlays::addOverlay(const QString& type, const QVariant& properties) {
+    if (_shuttingDown) {
+        return UNKNOWN_OVERLAY_ID;
+    }
+
     if (QThread::currentThread() != thread()) {
         OverlayID result;
         PROFILE_RANGE(script, __FUNCTION__);
@@ -261,6 +270,10 @@ OverlayID Overlays::addOverlay(const QString& type, const QVariant& properties)
 }
 
 OverlayID Overlays::addOverlay(const Overlay::Pointer& overlay) {
+    if (_shuttingDown) {
+        return UNKNOWN_OVERLAY_ID;
+    }
+
     OverlayID thisID = OverlayID(QUuid::createUuid());
     overlay->setOverlayID(thisID);
     overlay->setStackOrder(_stackOrder++);
@@ -283,6 +296,10 @@ OverlayID Overlays::addOverlay(const Overlay::Pointer& overlay) {
 }
 
 OverlayID Overlays::cloneOverlay(OverlayID id) {
+    if (_shuttingDown) {
+        return UNKNOWN_OVERLAY_ID;
+    }
+
     if (QThread::currentThread() != thread()) {
         OverlayID result;
         PROFILE_RANGE(script, __FUNCTION__);
@@ -301,6 +318,10 @@ OverlayID Overlays::cloneOverlay(OverlayID id) {
 }
 
 bool Overlays::editOverlay(OverlayID id, const QVariant& properties) {
+    if (_shuttingDown) {
+        return false;
+    }
+
     auto thisOverlay = getOverlay(id);
     if (!thisOverlay) {
         return false;
@@ -320,6 +341,10 @@ bool Overlays::editOverlay(OverlayID id, const QVariant& properties) {
 }
 
 bool Overlays::editOverlays(const QVariant& propertiesById) {
+    if (_shuttingDown) {
+        return false;
+    }
+
     bool defer2DOverlays = QThread::currentThread() != thread();
 
     QVariantMap deferrred;
@@ -351,6 +376,10 @@ bool Overlays::editOverlays(const QVariant& propertiesById) {
 }
 
 void Overlays::deleteOverlay(OverlayID id) {
+    if (_shuttingDown) {
+        return;
+    }
+
     if (QThread::currentThread() != thread()) {
         QMetaObject::invokeMethod(this, "deleteOverlay", Q_ARG(OverlayID, id));
         return;
@@ -374,6 +403,9 @@ void Overlays::deleteOverlay(OverlayID id) {
 }
 
 QString Overlays::getOverlayType(OverlayID overlayId) {
+    if (_shuttingDown) {
+        return "";
+    }
     if (QThread::currentThread() != thread()) {
         QString result;
         PROFILE_RANGE(script, __FUNCTION__);
@@ -404,7 +436,7 @@ QObject* Overlays::getOverlayObject(OverlayID id) {
 }
 
 OverlayID Overlays::getOverlayAtPoint(const glm::vec2& point) {
-    if (!_enabled) {
+    if (_shuttingDown || !_enabled) {
         return UNKNOWN_OVERLAY_ID;
     }
 

interface/src/ui/overlays/Overlays.h

@@ -724,6 +724,7 @@ private:
     unsigned int _stackOrder { 1 };
 
     bool _enabled = true;
+    std::atomic<bool> _shuttingDown{ false };
 
     PointerEvent calculateOverlayPointerEvent(OverlayID overlayID, PickRay ray, RayToOverlayIntersectionResult rayPickResult,
         QMouseEvent* event, PointerEvent::EventType eventType);

interface/src/ui/overlays/QmlOverlay.cpp

@@ -39,18 +39,20 @@ void QmlOverlay::buildQmlElement(const QUrl& url) {
 
     auto offscreenUi = DependencyManager::get<OffscreenUi>();
     offscreenUi->load(url, [=](QQmlContext* context, QObject* object) {
-        QQuickItem* rawPtr = dynamic_cast<QQuickItem*>(object);
-        // Create a shared ptr with a custom deleter lambda, that calls deleteLater
-        _qmlElement = std::shared_ptr<QQuickItem>(rawPtr, [](QQuickItem* ptr) {
-            if (ptr) {
-                ptr->deleteLater();
-            }
-        });
+        _qmlElement = dynamic_cast<QQuickItem*>(object);
+        connect(_qmlElement, &QObject::destroyed, this, &QmlOverlay::qmlElementDestroyed);
     });
 }
 
+void QmlOverlay::qmlElementDestroyed() {
+    _qmlElement = nullptr;
+}
+
 QmlOverlay::~QmlOverlay() {
-    _qmlElement.reset();
+    if (_qmlElement) {
+        _qmlElement->deleteLater();
+    }
+    _qmlElement = nullptr;
 }
 
 // QmlOverlay replaces Overlay's properties with those defined in the QML file used but keeps Overlay2D's properties.
@@ -62,15 +64,13 @@ void QmlOverlay::setProperties(const QVariantMap& properties) {
 
     Overlay2D::setProperties(properties);
     auto bounds = _bounds;
-    std::weak_ptr<QQuickItem> weakQmlElement = _qmlElement;
     // check to see if qmlElement still exists
-    auto qmlElement = weakQmlElement.lock();
-    if (qmlElement) {
-        qmlElement->setX(bounds.left());
-        qmlElement->setY(bounds.top());
-        qmlElement->setWidth(bounds.width());
-        qmlElement->setHeight(bounds.height());
-        QMetaObject::invokeMethod(qmlElement.get(), "updatePropertiesFromScript", Qt::DirectConnection, Q_ARG(QVariant, properties));
+    if (_qmlElement) {
+        _qmlElement->setX(bounds.left());
+        _qmlElement->setY(bounds.top());
+        _qmlElement->setWidth(bounds.width());
+        _qmlElement->setHeight(bounds.height());
+        QMetaObject::invokeMethod(_qmlElement, "updatePropertiesFromScript", Qt::DirectConnection, Q_ARG(QVariant, properties));
     }
 }
 

interface/src/ui/overlays/QmlOverlay.h

@@ -32,10 +32,11 @@ public:
     void render(RenderArgs* args) override;
 
 private:
+    Q_INVOKABLE void qmlElementDestroyed();
     Q_INVOKABLE void buildQmlElement(const QUrl& url);
 
 protected:
-    std::shared_ptr<QQuickItem> _qmlElement;
+    QQuickItem* _qmlElement{ nullptr };
 };
 
 #endif // hifi_QmlOverlay_h

libraries/qml/src/qml/impl/SharedObject.cpp

@@ -69,6 +69,7 @@ SharedObject::SharedObject() {
     _quickWindow->setColor(QColor(255, 255, 255, 0));
     _quickWindow->setClearBeforeRendering(true);
 
+
     QObject::connect(qApp, &QCoreApplication::aboutToQuit, this, &SharedObject::onAboutToQuit);
 }
 
@@ -124,6 +125,7 @@ void SharedObject::setRootItem(QQuickItem* rootItem) {
     _renderThread->setObjectName(objectName());
     _renderThread->start();
 
+
     // Create event handler for the render thread
     _renderObject = new RenderEventHandler(this, _renderThread);
     QCoreApplication::postEvent(this, new OffscreenEvent(OffscreenEvent::Initialize));
@@ -152,9 +154,16 @@ void SharedObject::destroy() {
     QObject::disconnect(_renderControl);
     QObject::disconnect(qApp);
 
-    QMutexLocker lock(&_mutex);
-    _quit = true;
-    QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit));
+    {
+        QMutexLocker lock(&_mutex);
+        _quit = true;
+        QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority);
+    }
+    // Block until the rendering thread has stopped
+    // FIXME this is undesirable because this is blocking the main thread,
+    // but I haven't found a reliable way to do this only at application 
+    // shutdown
+    _renderThread->wait();
 }