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>

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
+    for (ServerNodeInstance &instance : m_objectInstanceHash) {
-
-    foreach (ServerNodeInstance instance, m_objectInstanceHash) {
         if (instance.isValid())
-            instance.setId(QString());
+            instance.setId({});
     }
 
-    //first  the root object
+    // First the root object
-    if (rootNodeInstance().internalObject())
+    // This also cleans up all objects that have root object as ancestor
-        rootNodeInstance().internalObject()->disconnect();
-
     rootNodeInstance().makeInvalid();
 
-
+    // Invalidate any remaining objects
-    foreach (ServerNodeInstance instance, m_objectInstanceHash) {
+    for (ServerNodeInstance &instance : m_objectInstanceHash)
-        if (instance.internalObject())
-            instance.internalObject()->disconnect();
         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())
+        if (instanceId >= 0 && m_idInstances.size() > instanceId)
-            m_idInstances[instance.instanceId()] = ServerNodeInstance{};
+            m_idInstances[instanceId] = {};
     }
 }
 

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();

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, [=] {
-
+            handleObjectDeletion(object);
-            /*This lambda is save because m_nodeInstanceServer
-            is a smartpointer and object is a dangling pointer anyway.*/
-
-            if (m_nodeInstanceServer)
-                m_nodeInstanceServer->removeInstanceRelationsipForDeletedObject(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;

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);

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()

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

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)

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);