From e5b256effa320fb7806205f5a37a4dc1943175f0 Mon Sep 17 00:00:00 2001
From: Svenn-Arne Dragly <s@dragly.com>
Date: Thu, 21 Feb 2019 15:54:02 +0100
Subject: Fix broken creation order for nodes used as properties

Otherwise, child nodes may be constructed before their parents and their
node creation changes will arrive at the backend out-of-order.
This could result in a child node referencing a parent that we have not
yet received a creation change for.
Also add a unit test to take this case into account.

Change-Id: I26b29e63863d1686e7b9239c63297c7e6c341f4e
Task-number: QTBUG-73986
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>
---
 src/core/nodes/qnode.cpp            | 12 ++++----
 tests/auto/core/nodes/tst_nodes.cpp | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index 83ac49471..8143ccc75 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -394,15 +394,15 @@ void QNodePrivate::propertyChanged(int propertyIndex)
         if (data.canConvert<QNode*>()) {
             QNode *node = data.value<QNode*>();
 
-            // Ensure the node has issued a node creation change. We can end
-            // up here if a newly created node with a parent is immediately set
+            // Ensure the node and all ancestors have issued their node creation changes.
+            // We can end up here if a newly created node with a parent is immediately set
             // as a property on another node. In this case the deferred call to
             // _q_postConstructorInit() will not have happened yet as the event
-            // loop will still be blocked. So force it here and we catch this
-            // eventuality in the _q_postConstructorInit() function so that we
-            // do not repeat the creation and new child scene change events.
+            // loop will still be blocked. We need to do this for all ancestors,
+            // since the subtree of this node otherwise can end up on the backend
+            // with a reference to a non-existent parent.
             if (node)
-                QNodePrivate::get(node)->_q_postConstructorInit();
+                QNodePrivate::get(node)->_q_ensureBackendNodeCreated();
 
             const QNodeId id = node ? node->id() : QNodeId();
             return QVariant::fromValue(id);
diff --git a/tests/auto/core/nodes/tst_nodes.cpp b/tests/auto/core/nodes/tst_nodes.cpp
index 75d7a7799..accd20ceb 100644
--- a/tests/auto/core/nodes/tst_nodes.cpp
+++ b/tests/auto/core/nodes/tst_nodes.cpp
@@ -82,6 +82,7 @@ private slots:
 
     void checkConstructionSetParentMix(); // QTBUG-60612
     void checkConstructionWithParent();
+    void checkConstructionWithNonRootParent(); // QTBUG-73986
     void checkConstructionAsListElement();
     void checkSceneIsSetOnConstructionWithParent(); // QTBUG-69352
 
@@ -1119,6 +1120,61 @@ void tst_Nodes::checkConstructionWithParent()
     QCOMPARE(propertyEvent->value().value<Qt3DCore::QNodeId>(), node->id());
 }
 
+void tst_Nodes::checkConstructionWithNonRootParent()
+{
+    // GIVEN
+    ObserverSpy spy;
+    Qt3DCore::QScene scene;
+    QScopedPointer<MyQNode> root(new MyQNode());
+
+    // WHEN
+    root->setArbiterAndScene(&spy, &scene);
+    root->setSimulateBackendCreated(true);
+    QScopedPointer<MyQNode> parent(new MyQNode(root.data()));
+
+    // THEN
+    QVERIFY(Qt3DCore::QNodePrivate::get(root.data())->scene() != nullptr);
+    QVERIFY(Qt3DCore::QNodePrivate::get(parent.data())->scene() != nullptr);
+
+    // WHEN we create a child and then set it as a Node* property
+    auto *child = new MyQNode(parent.data());
+    root->setNodeProperty(child);
+
+    // THEN we should get
+    // - one creation change for parent,
+    // - one creation change for child,
+    // - one child added change for root->parent,
+    // - and one property change event,
+    // in that order.
+    QCoreApplication::processEvents();
+    QCOMPARE(root->children().count(), 1);
+    QCOMPARE(parent->children().count(), 1);
+
+    QCOMPARE(spy.events.size(), 4); // 2 creation changes, 1 child added changes, 1 property change
+
+    // Ensure first event is parent node's creation change
+    const auto parentCreationEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!parentCreationEvent.isNull());
+    QCOMPARE(parentCreationEvent->subjectId(), parent->id());
+
+    const auto childCreationEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!childCreationEvent.isNull());
+    QCOMPARE(childCreationEvent->subjectId(), child->id());
+
+    const auto parentNewChildEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyNodeAddedChange>();
+    QVERIFY(!parentNewChildEvent.isNull());
+    QCOMPARE(parentNewChildEvent->subjectId(), root->id());
+    QCOMPARE(parentNewChildEvent->propertyName(), "children");
+    QCOMPARE(parentNewChildEvent->addedNodeId(), parent->id());
+
+    // Ensure second and last event is property set change
+    const auto propertyEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyUpdatedChange>();
+    QVERIFY(!propertyEvent.isNull());
+    QCOMPARE(propertyEvent->subjectId(), root->id());
+    QCOMPARE(propertyEvent->propertyName(), "nodeProperty");
+    QCOMPARE(propertyEvent->value().value<Qt3DCore::QNodeId>(), child->id());
+}
+
 void tst_Nodes::checkConstructionAsListElement()
 {
     // GIVEN
-- 
cgit v1.2.3