Avoid crash in QML app shutdown and actually send events in C++ app

The logic was such that in a C++ application the QNode::setParent()
function would bail out early in a C++ application when called from the
destructor of its parent object. This is because by the time the
child is being deleted the parent is in QObjectPrivate::deleteChildren
and therefore the QNode part of the object has already been destroyed.
This led to the cast in the parentNode() == parent to fail, thereby
exiting the functio early and never getting into
QNodePrivate::_q_setParentHelper().

In the case of a QML application, the parent has a dynamic metaobject
set by the QML engine. This resulted in the cast in QNode::setParent()
succeeding and we called into _q_setParentHelper(). The logic in here
resulted in a crash when called from a destructor because the child
had already been removed from its parent's list of children. Thus when
we called QObjectPrivate::setParentHelper(), this function ended up with
an index of -1 for the child in its child list (i.e. not found) and it
then tried to index into the children list with this index and we
then crashed.

The solution in this change is to not do the full logic in
QNode::setParent() and _q_setParentHelper(). Rather, we simply remove
the subtree at this node from the scene and we send node destruction
changes to the backend.

With this we avoid the crash of QML application shutdowns and we also
make sure to correctly send the node destruction changes even in the
case of a C++ Qt 3D application.

The backend does not yet get an opportunity to process these final
changes. This will be addressed in a follow up commit.

As a result of these changes many unit tests began crashing. This is
because the QNode dtor is now actually doing some work, rather than
bailing out of that work early when the parent is no longer a QNode.
This work involves mutating the QScene object which in the unit
tests did not live longer than the QNode's to which it was
associated with.

The unit tests have been adjusted to ensure that the arbiter and
scene objects remain alive longer than the QNodes they are being
used to test.

Task-number: QTBUG-42353
Change-Id: I197870f48fca30656bd85c4c51346d93403fba08
Reviewed-by: Kevin Ottens <kevin.ottens@kdab.com>

2 files changed, 56 insertions, 16 deletions

diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index e56d4bd81..6f19e7217 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -91,6 +91,7 @@ void QNodePrivate::init(QNode *parent)
     // in a deferred way when the object is fully constructed. This is delayed
     // until the object is fully constructed as it involves calling a virtual
     // function of QNode.
+    m_parentId = parent->id();
     const auto parentPrivate = get(parent);
     m_scene = parentPrivate->m_scene;
     Q_Q(QNode);
@@ -119,6 +120,43 @@ void QNodePrivate::notifyCreationChange()
 /*!
  * \internal
  *
+ * Notify the backend that the parent lost this node as a child and
+ * that this node is being destroyed. We only send the node removed
+ * change for the parent's children property iff we have an id for
+ * a parent node. This is set/unset in the _q_addChild()/_q_removeChild()
+ * functions (and initialized in init() if there is a parent at
+ * construction time).
+ *
+ * Likewise, we only send the node destroyed change, iff we have
+ * previously sent a node created change. This is tracked via the
+ * m_hasBackendNode member.
+ */
+void QNodePrivate::notifyDestructionChangesAndRemoveFromScene()
+{
+    Q_Q(QNode);
+
+//    // We notify the backend that the parent lost us as a child
+    if (m_changeArbiter != nullptr && !m_parentId.isNull()) {
+        const auto change = QPropertyNodeRemovedChangePtr::create(m_parentId, q);
+        change->setPropertyName("children");
+        notifyObservers(change);
+    }
+
+    // Tell the backend we are about to be destroyed
+    if (m_hasBackendNode) {
+        const QDestructionIdAndTypeCollector collector(q);
+        const auto destroyedChange = QNodeDestroyedChangePtr::create(q, collector.subtreeIdsAndTypes());
+        notifyObservers(destroyedChange);
+    }
+
+    // We unset the scene from the node as its backend node was/is about to be destroyed
+    QNodeVisitor visitor;
+    visitor.traverse(q, this, &QNodePrivate::unsetSceneHelper);
+}
+
+/*!
+ * \internal
+ *
  * Sends a QNodeCreatedChange event to the aspects and then also notifies the
  * parent backend node of its new child. This is called in a deferred manner
  * by the QNodePrivate::init() method to notify the backend of newly created
@@ -152,6 +190,14 @@ void QNodePrivate::_q_addChild(QNode *childNode)
     Q_ASSERT(childNode);
     Q_ASSERT_X(childNode->parent() == q_func(), Q_FUNC_INFO,  "not a child of this node");
 
+    // Store our id as the parentId in the child so that even if the child gets
+    // removed from the scene as part of the destruction of the parent, when the
+    // parent's children are deleted in the QObject dtor, we still have access to
+    // the parentId. If we didn't store this, we wouldn't have access at that time
+    // because the parent woudl then only be a QObject, the QNode part would have
+    // been destroyed already.
+    QNodePrivate::get(childNode)->m_parentId = m_id;
+
     if (!m_scene)
         return;
 
@@ -180,6 +226,8 @@ void QNodePrivate::_q_removeChild(QNode *childNode)
     Q_ASSERT(childNode);
     Q_ASSERT_X(childNode->parent() == q_func(), Q_FUNC_INFO, "not a child of this node");
 
+    QNodePrivate::get(childNode)->m_parentId = QNodeId();
+
     // We notify the backend that we lost a child
     if (m_changeArbiter != nullptr) {
         const auto change = QPropertyNodeRemovedChangePtr::create(m_id, childNode);
@@ -220,18 +268,8 @@ void QNodePrivate::_q_setParentHelper(QNode *parent)
 
         // If we have an old parent but the new parent is null
         // the backend node needs to be destroyed
-        if (!parent) {
-            // Tell the backend we are about to be destroyed
-            if (m_hasBackendNode) {
-                const QDestructionIdAndTypeCollector collector(q);
-                const auto destroyedChange = QNodeDestroyedChangePtr::create(q, collector.subtreeIdsAndTypes());
-                notifyObservers(destroyedChange);
-            }
-
-            // We unset the scene from the node as its backend node was/is about to be destroyed
-            QNodeVisitor visitor;
-            visitor.traverse(q, oldParentNode->d_func(), &QNodePrivate::unsetSceneHelper);
-        }
+        if (!parent)
+            notifyDestructionChangesAndRemoveFromScene();
     }
 
     // Basically QObject::setParent but for QObjectPrivate
@@ -586,10 +624,10 @@ QNode::QNode(QNodePrivate &dd, QNode *parent)
 
 QNode::~QNode()
 {
-    // If we have a parent it makes sense to let it know we are about to be destroyed.
-    // This in turn triggers the deletion of the corresponding backend nodes for the
-    // subtree rooted at this QNode.
-    setParent(Q_NODE_NULLPTR);
+    // Notify the backend that the parent lost this node as a child and
+    // that this node is being destroyed.
+    Q_D(QNode);
+    d->notifyDestructionChangesAndRemoveFromScene();
 }
 
 /*!
diff --git a/src/core/nodes/qnode_p.h b/src/core/nodes/qnode_p.h
index 648bc4c34..e290ffe32 100644
--- a/src/core/nodes/qnode_p.h
+++ b/src/core/nodes/qnode_p.h
@@ -92,6 +92,7 @@ public:
     QMetaObject *m_typeInfo;
     QScene *m_scene;
     mutable QNodeId m_id;
+    QNodeId m_parentId; // Store this so we have it even in parent's QObject dtor
     bool m_blockNotifications;
     bool m_hasBackendNode;
     bool m_enabled;
@@ -101,6 +102,7 @@ public:
 
 private:
     void notifyCreationChange();
+    void notifyDestructionChangesAndRemoveFromScene();
     void _q_notifyCreationAndChildChanges();
     void _q_addChild(QNode *childNode);
     void _q_removeChild(QNode *childNode);