QmlPuppet: Fix crash at puppet reset

Puppet cleanup was not handled properly, so derefFromEffectItem() was
not called for all objects that needed it, causing puppet to crash
at shutdown.

Fixes: QDS-3461
Change-Id: I22c0552fe1223789fa42b276ab377d4a9e929955
Reviewed-by: Mahmoud Badri <mahmoud.badri@qt.io>
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>

8 files changed, 45 insertions, 27 deletions

diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.cpp b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.cpp
index 64d790c6a7..a4175fe992 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.cpp
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.cpp
@@ -771,25 +771,18 @@ QList<QObject*> NodeInstanceServer::allSubObjectsForObject(QObject *object)
 
 void NodeInstanceServer::removeAllInstanceRelationships()
 {
-    // prevent destroyed() signals calling back
-
-    foreach (ServerNodeInstance instance, m_objectInstanceHash) {
+    for (ServerNodeInstance &instance : m_objectInstanceHash) {
         if (instance.isValid())
-            instance.setId(QString());
+            instance.setId({});
     }
 
-    //first  the root object
-    if (rootNodeInstance().internalObject())
-        rootNodeInstance().internalObject()->disconnect();
-
+    // First the root object
+    // This also cleans up all objects that have root object as ancestor
     rootNodeInstance().makeInvalid();
 
-
-    foreach (ServerNodeInstance instance, m_objectInstanceHash) {
-        if (instance.internalObject())
-            instance.internalObject()->disconnect();
+    // Invalidate any remaining objects
+    for (ServerNodeInstance &instance : m_objectInstanceHash)
         instance.makeInvalid();
-    }
 
     m_idInstances.clear();
     m_objectInstanceHash.clear();
@@ -1388,14 +1381,14 @@ void NodeInstanceServer::sendDebugOutput(DebugOutputCommand::Type type, const QS
     nodeInstanceClient()->debugOutput(command);
 }
 
-void NodeInstanceServer::removeInstanceRelationsipForDeletedObject(QObject *object)
+void NodeInstanceServer::removeInstanceRelationsipForDeletedObject(QObject *object, qint32 instanceId)
 {
     if (m_objectInstanceHash.contains(object)) {
         ServerNodeInstance instance = instanceForObject(object);
         m_objectInstanceHash.remove(object);
 
-        if (instance.instanceId() >= 0 && m_idInstances.size() > instance.instanceId())
-            m_idInstances[instance.instanceId()] = ServerNodeInstance{};
+        if (instanceId >= 0 && m_idInstances.size() > instanceId)
+            m_idInstances[instanceId] = {};
     }
 }
 
diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.h b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.h
index f44a0047fe..fef555c06c 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.h
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.h
@@ -204,7 +204,7 @@ public:
                          const QString &message,
                          const QVector<qint32> &instanceIds);
 
-    void removeInstanceRelationsipForDeletedObject(QObject *object);
+    void removeInstanceRelationsipForDeletedObject(QObject *object, qint32 instanceId);
 
     void incrementNeedsExtraRender();
     void decrementNeedsExtraRender();
diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.cpp b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.cpp
index d7fba0a8e7..0f56b809f4 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.cpp
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.cpp
@@ -65,12 +65,7 @@ ObjectNodeInstance::ObjectNodeInstance(QObject *object)
 {
     if (object)
         QObject::connect(m_object.data(), &QObject::destroyed, [=] {
-
-            /*This lambda is save because m_nodeInstanceServer
-            is a smartpointer and object is a dangling pointer anyway.*/
-
-            if (m_nodeInstanceServer)
-                m_nodeInstanceServer->removeInstanceRelationsipForDeletedObject(object);
+            handleObjectDeletion(object);
         });
 }
 
@@ -100,6 +95,14 @@ void ObjectNodeInstance::destroy()
     m_instanceId = -1;
 }
 
+void ObjectNodeInstance::handleObjectDeletion(QObject *object)
+{
+    // We must pass the m_instanceId here, because this instance is no longer
+    // valid, so the wrapper ServerNodeInstance will report -1 for id.
+    if (m_nodeInstanceServer)
+        m_nodeInstanceServer->removeInstanceRelationsipForDeletedObject(object, m_instanceId);
+}
+
 void ObjectNodeInstance::setInstanceId(qint32 id)
 {
     m_instanceId = id;
diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.h b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.h
index 01cb1c46fc..c66365d392 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.h
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.h
@@ -65,7 +65,7 @@ public:
 
     virtual ~ObjectNodeInstance();
     void destroy();
-    //void setModelNode(const ModelNode &node);
+    virtual void handleObjectDeletion(QObject *object);
 
     static Pointer create(QObject *objectToBeWrapped);
     static QObject *createPrimitive(const QString &typeName, int majorNumber, int minorNumber, QQmlContext *context);
diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5informationnodeinstanceserver.cpp b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5informationnodeinstanceserver.cpp
index 6879ddacdf..466bf97db8 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5informationnodeinstanceserver.cpp
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5informationnodeinstanceserver.cpp
@@ -915,6 +915,9 @@ Qt5InformationNodeInstanceServer::~Qt5InformationNodeInstanceServer()
     m_render3DEditViewTimer.stop();
     m_inputEventTimer.stop();
 
+    if (m_editView3DData.rootItem)
+        m_editView3DData.rootItem->disconnect(this);
+
     for (auto view : qAsConst(m_view3Ds))
         view->disconnect();
     for (auto node : qAsConst(m_3DSceneMap))
@@ -922,6 +925,15 @@ Qt5InformationNodeInstanceServer::~Qt5InformationNodeInstanceServer()
 
     if (m_editView3DData.rootItem)
         QMetaObject::invokeMethod(m_editView3DData.rootItem, "aboutToShutDown", Qt::DirectConnection);
+
+    if (!Internal::QuickItemNodeInstance::unifiedRenderPath()) {
+        if (m_editView3DData.contentItem)
+            designerSupport()->derefFromEffectItem(m_editView3DData.contentItem);
+        if (m_modelNode3DImageViewData.contentItem)
+            designerSupport()->derefFromEffectItem(m_modelNode3DImageViewData.contentItem);
+        if (m_modelNode2DImageViewData.contentItem)
+            designerSupport()->derefFromEffectItem(m_modelNode2DImageViewData.contentItem);
+    }
 }
 
 void Qt5InformationNodeInstanceServer::sendTokenBack()
diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5nodeinstanceserver.cpp b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5nodeinstanceserver.cpp
index ee1bacbc2e..a9dc087413 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5nodeinstanceserver.cpp
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5nodeinstanceserver.cpp
@@ -37,6 +37,7 @@
 #include <addimportcontainer.h>
 #include <createscenecommand.h>
 #include <reparentinstancescommand.h>
+#include <clearscenecommand.h>
 
 #include <QDebug>
 #include <QOpenGLContext>
@@ -59,7 +60,8 @@ Qt5NodeInstanceServer::Qt5NodeInstanceServer(NodeInstanceClientInterface *nodeIn
 
 Qt5NodeInstanceServer::~Qt5NodeInstanceServer()
 {
-    delete quickWindow();
+    NodeInstanceServer::clearScene({});
+    delete m_viewData.window.data();
 }
 
 QQuickView *Qt5NodeInstanceServer::quickView() const
diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.cpp b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.cpp
index 81607cbdd6..ee74ab856c 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.cpp
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.cpp
@@ -59,8 +59,15 @@ QuickItemNodeInstance::QuickItemNodeInstance(QQuickItem *item)
 
 QuickItemNodeInstance::~QuickItemNodeInstance()
 {
-    if (quickItem() && checkIfRefFromEffect(instanceId()))
-        designerSupport()->derefFromEffectItem(quickItem());
+}
+
+void QuickItemNodeInstance::handleObjectDeletion(QObject *object)
+{
+    auto item = qobject_cast<QQuickItem *>(object);
+    if (item && checkIfRefFromEffect(instanceId()))
+        designerSupport()->derefFromEffectItem(item);
+
+    ObjectNodeInstance::handleObjectDeletion(object);
 }
 
 static bool isContentItem(QQuickItem *item, NodeInstanceServer *nodeInstanceServer)
diff --git a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.h b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.h
index 2c68fe7974..b71b990986 100644
--- a/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.h
+++ b/share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.h
@@ -42,6 +42,7 @@ public:
     using WeakPointer = QWeakPointer<QuickItemNodeInstance>;
 
     ~QuickItemNodeInstance() override;
+    void handleObjectDeletion(QObject *object) override;
 
     static Pointer create(QObject *objectToBeWrapped);
     static void createEffectItem(bool createEffectItem);