From 5d8a253785c9ea2c6b3b57373824c7b0227ce861 Mon Sep 17 00:00:00 2001 From: Alexander Ivash Date: Fri, 19 Oct 2018 02:53:15 +0300 Subject: [PATCH 1/3] FB19400 during shutdown -- TypeError: Cannot read property 'buttons' of null note: change also resolve a lot of other shutdown errors from QML (related to null parent) --- interface/resources/qml/hifi/Desktop.qml | 4 ++-- interface/resources/qml/hifi/tablet/TabletHome.qml | 4 ++-- libraries/qml/src/qml/OffscreenSurface.cpp | 12 ++++++++++-- libraries/qml/src/qml/impl/SharedObject.cpp | 4 +++- libraries/qml/src/qml/impl/SharedObject.h | 2 ++ 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/interface/resources/qml/hifi/Desktop.qml b/interface/resources/qml/hifi/Desktop.qml index 4d342fe775..1d20587643 100644 --- a/interface/resources/qml/hifi/Desktop.qml +++ b/interface/resources/qml/hifi/Desktop.qml @@ -70,8 +70,8 @@ OriginalDesktop.Desktop { anchors.horizontalCenter: settings.constrainToolbarToCenterX ? desktop.horizontalCenter : undefined; // Literal 50 is overwritten by settings from previous session, and sysToolbar.x comes from settings when not constrained. x: sysToolbar.x - buttonModel: tablet.buttons; - shown: tablet.toolbarMode; + buttonModel: tablet ? tablet.buttons : null; + shown: tablet ? tablet.toolbarMode : false; } Settings { diff --git a/interface/resources/qml/hifi/tablet/TabletHome.qml b/interface/resources/qml/hifi/tablet/TabletHome.qml index 1922b02f93..616cdf7fee 100644 --- a/interface/resources/qml/hifi/tablet/TabletHome.qml +++ b/interface/resources/qml/hifi/tablet/TabletHome.qml @@ -115,9 +115,9 @@ Item { property int previousIndex: -1 Repeater { id: pageRepeater - model: Math.ceil(tabletProxy.buttons.rowCount() / TabletEnums.ButtonsOnPage) + model: tabletProxy != null ? Math.ceil(tabletProxy.buttons.rowCount() / TabletEnums.ButtonsOnPage) : null onItemAdded: { - item.proxyModel.sourceModel = tabletProxy.buttons; + item.proxyModel.sourceModel = tabletProxy != null ? tabletProxy.buttons : null; item.proxyModel.pageIndex = index; } diff --git a/libraries/qml/src/qml/OffscreenSurface.cpp b/libraries/qml/src/qml/OffscreenSurface.cpp index cbcafe9c7d..37c3ae7a37 100644 --- a/libraries/qml/src/qml/OffscreenSurface.cpp +++ b/libraries/qml/src/qml/OffscreenSurface.cpp @@ -387,8 +387,16 @@ void OffscreenSurface::finishQmlLoad(QQmlComponent* qmlComponent, if (!parent) { parent = getRootItem(); } - // Allow child windows to be destroyed from JS - QQmlEngine::setObjectOwnership(newObject, QQmlEngine::JavaScriptOwnership); + // manually control children items lifetime + QQmlEngine::setObjectOwnership(newObject, QQmlEngine::CppOwnership); + // some objects we are going to delete might be already deleted (children of parent we already deleted) so use QPointer to track lifetime + QPointer trackedObject(newObject); + connect(_sharedObject, &SharedObject::onBeforeDestroyed, [trackedObject]() { + if (trackedObject) { + delete trackedObject.data(); + } + }); + newObject->setParent(parent); newItem->setParentItem(parent); } else { diff --git a/libraries/qml/src/qml/impl/SharedObject.cpp b/libraries/qml/src/qml/impl/SharedObject.cpp index 259defdb48..7612a54173 100644 --- a/libraries/qml/src/qml/impl/SharedObject.cpp +++ b/libraries/qml/src/qml/impl/SharedObject.cpp @@ -79,9 +79,11 @@ SharedObject::SharedObject() { SharedObject::~SharedObject() { + + emit onBeforeDestroyed(); + // After destroy returns, the rendering thread should be gone destroy(); - // _renderTimer is created with `this` as the parent, so need no explicit destruction #ifndef DISABLE_QML // Destroy the event hand diff --git a/libraries/qml/src/qml/impl/SharedObject.h b/libraries/qml/src/qml/impl/SharedObject.h index 002679c44d..57f4756e79 100644 --- a/libraries/qml/src/qml/impl/SharedObject.h +++ b/libraries/qml/src/qml/impl/SharedObject.h @@ -67,6 +67,8 @@ public: bool isPaused() const; bool fetchTexture(TextureAndFence& textureAndFence); +signals: + void onBeforeDestroyed(); private: bool event(QEvent* e) override; From 5fef331995512044a7499b51820c9d2feed02e30 Mon Sep 17 00:00:00 2001 From: Alexander Ivash Date: Fri, 19 Oct 2018 19:35:12 +0300 Subject: [PATCH 2/3] address CR comment --- interface/resources/qml/hifi/tablet/TabletHome.qml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interface/resources/qml/hifi/tablet/TabletHome.qml b/interface/resources/qml/hifi/tablet/TabletHome.qml index 616cdf7fee..4a1648baaf 100644 --- a/interface/resources/qml/hifi/tablet/TabletHome.qml +++ b/interface/resources/qml/hifi/tablet/TabletHome.qml @@ -115,7 +115,7 @@ Item { property int previousIndex: -1 Repeater { id: pageRepeater - model: tabletProxy != null ? Math.ceil(tabletProxy.buttons.rowCount() / TabletEnums.ButtonsOnPage) : null + model: tabletProxy != null ? Math.ceil(tabletProxy.buttons.rowCount() / TabletEnums.ButtonsOnPage) : 0 onItemAdded: { item.proxyModel.sourceModel = tabletProxy != null ? tabletProxy.buttons : null; item.proxyModel.pageIndex = index; From 657ff7f6f882d8392fadbbe1cfed66034e34f758 Mon Sep 17 00:00:00 2001 From: Alexander Ivash Date: Mon, 22 Oct 2018 23:29:39 +0300 Subject: [PATCH 3/3] eliminate signal, introduce deletion list --- libraries/qml/src/qml/OffscreenSurface.cpp | 10 +++------- libraries/qml/src/qml/impl/SharedObject.cpp | 14 +++++++++++--- libraries/qml/src/qml/impl/SharedObject.h | 6 +++--- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/libraries/qml/src/qml/OffscreenSurface.cpp b/libraries/qml/src/qml/OffscreenSurface.cpp index 37c3ae7a37..1f1d50ed1f 100644 --- a/libraries/qml/src/qml/OffscreenSurface.cpp +++ b/libraries/qml/src/qml/OffscreenSurface.cpp @@ -389,13 +389,9 @@ void OffscreenSurface::finishQmlLoad(QQmlComponent* qmlComponent, } // manually control children items lifetime QQmlEngine::setObjectOwnership(newObject, QQmlEngine::CppOwnership); - // some objects we are going to delete might be already deleted (children of parent we already deleted) so use QPointer to track lifetime - QPointer trackedObject(newObject); - connect(_sharedObject, &SharedObject::onBeforeDestroyed, [trackedObject]() { - if (trackedObject) { - delete trackedObject.data(); - } - }); + + // add object to the manual deletion list + _sharedObject->addToDeletionList(newObject); newObject->setParent(parent); newItem->setParentItem(parent); diff --git a/libraries/qml/src/qml/impl/SharedObject.cpp b/libraries/qml/src/qml/impl/SharedObject.cpp index 7612a54173..1f41fefb02 100644 --- a/libraries/qml/src/qml/impl/SharedObject.cpp +++ b/libraries/qml/src/qml/impl/SharedObject.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -79,9 +80,6 @@ SharedObject::SharedObject() { SharedObject::~SharedObject() { - - emit onBeforeDestroyed(); - // After destroy returns, the rendering thread should be gone destroy(); // _renderTimer is created with `this` as the parent, so need no explicit destruction @@ -98,6 +96,11 @@ SharedObject::~SharedObject() { } #endif + // already deleted objects will be reset to null by QPointer so it should be safe just iterate here + for (auto qmlObject : _deletionList) { + delete qmlObject; + } + if (_rootItem) { delete _rootItem; _rootItem = nullptr; @@ -414,6 +417,11 @@ bool SharedObject::fetchTexture(TextureAndFence& textureAndFence) { return true; } +void hifi::qml::impl::SharedObject::addToDeletionList(QObject * object) +{ + _deletionList.append(QPointer(object)); +} + void SharedObject::setProxyWindow(QWindow* window) { #ifndef DISABLE_QML _proxyWindow = window; diff --git a/libraries/qml/src/qml/impl/SharedObject.h b/libraries/qml/src/qml/impl/SharedObject.h index 57f4756e79..ce9fcd46d2 100644 --- a/libraries/qml/src/qml/impl/SharedObject.h +++ b/libraries/qml/src/qml/impl/SharedObject.h @@ -66,9 +66,7 @@ public: void resume(); bool isPaused() const; bool fetchTexture(TextureAndFence& textureAndFence); - -signals: - void onBeforeDestroyed(); + void addToDeletionList(QObject* object); private: bool event(QEvent* e) override; @@ -93,6 +91,8 @@ private: void onAboutToQuit(); void updateTextureAndFence(const TextureAndFence& newTextureAndFence); + QList> _deletionList; + // Texture management TextureAndFence _latestTextureAndFence{ 0, 0 }; QQuickItem* _item{ nullptr };