From 73ad6e87bbeceea5830ab3a6b3dc66fa99e30f45 Mon Sep 17 00:00:00 2001
From: Fabian Kosmale <fabian.kosmale@qt.io>
Date: Mon, 28 Oct 2019 13:41:11 +0100
Subject: [PATCH] QQuickItem::setParentItem: add child earlier

Calling (de)refWindow can trigger QQuickItem::windowChanged, which in turn
can call a user defined windowChanged handler. If that signal handler
were to call setParentItem, we would encounter an inconsistent state:
The item already has its parent set, but that parent would lack the item
in its children list (as we would only call refWindow at a later point).

Fixes: QTBUG-79573
Fixes: QTBUG-73439
Change-Id: I46adaa54a0521b5cd7f37810b3dd1a206e6a09c6
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>
---
 src/quick/items/qquickitem.cpp                      | 21 +++++++++++++++++----
 .../qquickitem/data/setParentInWindowChange.qml     | 12 ++++++++++++
 tests/auto/quick/qquickitem/tst_qquickitem.cpp      |  8 ++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 tests/auto/quick/qquickitem/data/setParentInWindowChange.qml

diff --git a/src/quick/items/qquickitem.cpp b/src/quick/items/qquickitem.cpp
index 396012e1e67..26f02aeed7f 100644
--- a/src/quick/items/qquickitem.cpp
+++ b/src/quick/items/qquickitem.cpp
@@ -2748,22 +2748,35 @@ void QQuickItem::setParentItem(QQuickItem *parentItem)
     }
 
     QQuickWindow *parentWindow = parentItem ? QQuickItemPrivate::get(parentItem)->window : nullptr;
+    bool alreadyAddedChild = false;
     if (d->window == parentWindow) {
         // Avoid freeing and reallocating resources if the window stays the same.
         d->parentItem = parentItem;
     } else {
-        if (d->window)
-            d->derefWindow();
+        auto oldParentItem = d->parentItem;
         d->parentItem = parentItem;
+        if (d->parentItem) {
+            QQuickItemPrivate::get(d->parentItem)->addChild(this);
+            alreadyAddedChild = true;
+        }
+        if (d->window) {
+            d->derefWindow();
+            // as we potentially changed d->parentWindow above
+            // the check in derefWindow could not work
+            // thus, we redo it here with the old parent
+            if (!oldParentItem) {
+                QQuickWindowPrivate::get(d->window)->parentlessItems.remove(this);
+            }
+        }
         if (parentWindow)
             d->refWindow(parentWindow);
     }
 
     d->dirty(QQuickItemPrivate::ParentChanged);
 
-    if (d->parentItem)
+    if (d->parentItem && !alreadyAddedChild)
         QQuickItemPrivate::get(d->parentItem)->addChild(this);
-    else if (d->window)
+    else if (d->window && !alreadyAddedChild)
         QQuickWindowPrivate::get(d->window)->parentlessItems.insert(this);
 
     d->setEffectiveVisibleRecur(d->calcEffectiveVisible());
diff --git a/tests/auto/quick/qquickitem/data/setParentInWindowChange.qml b/tests/auto/quick/qquickitem/data/setParentInWindowChange.qml
new file mode 100644
index 00000000000..d68b7adb72a
--- /dev/null
+++ b/tests/auto/quick/qquickitem/data/setParentInWindowChange.qml
@@ -0,0 +1,12 @@
+import QtQuick 2.12
+
+Rectangle {
+    width: 800
+    height: 600
+    Item {
+        id: it
+        onWindowChanged: () => it.parent = newParent
+    }
+
+    Item { id: newParent }
+}
diff --git a/tests/auto/quick/qquickitem/tst_qquickitem.cpp b/tests/auto/quick/qquickitem/tst_qquickitem.cpp
index 7e132f97b67..9ce9766c925 100644
--- a/tests/auto/quick/qquickitem/tst_qquickitem.cpp
+++ b/tests/auto/quick/qquickitem/tst_qquickitem.cpp
@@ -197,6 +197,8 @@ private slots:
     void qtBug60123();
 #endif
 
+    void setParentCalledInOnWindowChanged();
+
 private:
 
     enum PaintOrderOp {
@@ -2145,6 +2147,12 @@ void tst_qquickitem::qtBug60123()
     activateWindowAndTestPress(&window);
 }
 #endif
+void tst_qquickitem::setParentCalledInOnWindowChanged()
+{
+    QQuickView view;
+    view.setSource(testFileUrl("setParentInWindowChange.qml"));
+    QVERIFY(ensureFocus(&view)); // should not crash
+}
 
 QTEST_MAIN(tst_qquickitem)
 
-- 
2.16.3