Add unit test and fix for broken order of event delivery

Ensure a backend node is always created before it is used in a
property of any other node. This avoids a race between sending
the creation, add child and property update changes and the
start of a new Qt 3D frame. The race is caused by the use of the
event loop to trigger the node created and child added changes.

Also be careful not to repeat the node creation.

Task-number: QTBUG-65829
Change-Id: I6ca5eb269ce657f8d42d855550fb4f898e3bd420
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>
Reviewed-by: Volker Krause <volker.krause@kdab.com>

2 files changed, 96 insertions, 1 deletions

diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index ce5e76a55..09bd597e6 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -173,11 +173,21 @@ void QNodePrivate::_q_postConstructorInit()
 {
     Q_Q(QNode);
 
+    // If we've already done the work then bail out. This can happen if the
+    // user creates a QNode subclass with an explicit parent, then immediately
+    // sets the new QNode as a property on another node. In this case, the
+    // property setter will call this function directly, but as we can't
+    // un-schedule a deferred invocation, this function will be called again
+    // the next time the event loop spins. So, catch this case and abort.
+    if (m_hasBackendNode)
+        return;
+
     // Check that the parent hasn't been unset since this call was enqueued
     auto parentNode = q->parentNode();
     if (!parentNode)
         return;
 
+
     if (m_scene)
         m_scene->addObservable(q); // Sets the m_changeArbiter to that of the scene
 
@@ -369,7 +379,18 @@ void QNodePrivate::propertyChanged(int propertyIndex)
 
     const QVariant data = property.read(q);
     if (data.canConvert<QNode*>()) {
-        const QNode * const node = data.value<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
+        // 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.
+        if (node)
+            QNodePrivate::get(node)->_q_postConstructorInit();
+
         const QNodeId id = node ? node->id() : QNodeId();
         notifyPropertyChange(property.name(), QVariant::fromValue(id));
     } else {
diff --git a/tests/auto/core/nodes/tst_nodes.cpp b/tests/auto/core/nodes/tst_nodes.cpp
index 25c2a6dba..49618821c 100644
--- a/tests/auto/core/nodes/tst_nodes.cpp
+++ b/tests/auto/core/nodes/tst_nodes.cpp
@@ -80,6 +80,7 @@ private slots:
     void removingChildEntitiesFromNode();
 
     void checkConstructionSetParentMix(); // QTBUG-60612
+    void checkConstructionWithParent();
 
     void appendingComponentToEntity();
     void appendingParentlessComponentToEntity();
@@ -169,13 +170,17 @@ void SimplePostman::notifyBackend(const Qt3DCore::QSceneChangePtr &change)
     m_spy->sceneChangeEventWithLock(change);
 }
 
+
+
 class MyQNode : public Qt3DCore::QNode
 {
     Q_OBJECT
     Q_PROPERTY(QString customProperty READ customProperty WRITE setCustomProperty NOTIFY customPropertyChanged)
+    Q_PROPERTY(MyQNode *nodeProperty READ nodeProperty WRITE setNodeProperty NOTIFY nodePropertyChanged)
 public:
     explicit MyQNode(Qt3DCore::QNode *parent = 0)
         : QNode(parent)
+        , m_nodeProperty(nullptr)
     {}
 
     ~MyQNode()
@@ -211,11 +216,37 @@ public:
         Qt3DCore::QNodePrivate::get(this)->m_hasBackendNode = created;
     }
 
+    MyQNode *nodeProperty() const { return m_nodeProperty; }
+
+public slots:
+    void setNodeProperty(MyQNode *node)
+    {
+        Qt3DCore::QNodePrivate *d = Qt3DCore::QNodePrivate::get(this);
+        if (m_nodeProperty == node)
+            return;
+
+        if (m_nodeProperty)
+            d->unregisterDestructionHelper(m_nodeProperty);
+
+        if (node && !node->parent())
+            node->setParent(this);
+
+        m_nodeProperty = node;
+
+        // Ensures proper bookkeeping
+        if (m_nodeProperty)
+            d->registerDestructionHelper(m_nodeProperty, &MyQNode::setNodeProperty, m_nodeProperty);
+
+        emit nodePropertyChanged(node);
+    }
+
 signals:
     void customPropertyChanged();
+    void nodePropertyChanged(MyQNode *node);
 
 protected:
     QString m_customProperty;
+    MyQNode *m_nodeProperty;
 };
 
 class MyQEntity : public Qt3DCore::QEntity
@@ -847,6 +878,49 @@ void tst_Nodes::checkConstructionSetParentMix()
     QCOMPARE(lastEvent->addedNodeId(), subTreeRoot->id());
 }
 
+void tst_Nodes::checkConstructionWithParent()
+{
+    // GIVEN
+    ObserverSpy spy;
+    Qt3DCore::QScene scene;
+    QScopedPointer<MyQNode> root(new MyQNode());
+
+    // WHEN
+    root->setArbiterAndScene(&spy, &scene);
+    root->setSimulateBackendCreated(true);
+
+    // THEN
+    QVERIFY(Qt3DCore::QNodePrivate::get(root.data())->scene() != nullptr);
+
+    // WHEN we create a child and then set it as a Node* property
+    auto *node = new MyQNode(root.data());
+    root->setNodeProperty(node);
+
+    // THEN we should get one creation change, one child added change
+    // and one property change event, in that order.
+    QCoreApplication::processEvents();
+    QCOMPARE(root->children().count(), 1);
+    QCOMPARE(spy.events.size(), 3); // 1 creation change, 1 child added change, 1 property change
+
+    // Ensure first event is child node's creation change
+    const auto creationEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!creationEvent.isNull());
+    QCOMPARE(creationEvent->subjectId(), node->id());
+
+    const auto newChildEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyNodeAddedChange>();
+    QVERIFY(!newChildEvent.isNull());
+    QCOMPARE(newChildEvent->subjectId(), root->id());
+    QCOMPARE(newChildEvent->propertyName(), "children");
+    QCOMPARE(newChildEvent->addedNodeId(), node->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>(), node->id());
+}
+
 void tst_Nodes::appendingParentlessComponentToEntity()
 {
     // GIVEN