Fix itemGeometryChanged() listeners

We must iterate a (cheap) copy of changeListeners, because the
container might be indirectly modified during the loop. For example,
listeners may remove themselves from the list. In such scenario, the
copy gets detached and the loop remains safe.

I've added notes in comments, and extended the tests to prevent the
issue re-surfacing again.

Change-Id: Ib48d6e0765d45370d2fffa119a4c351e0119e40a
Task-number: QTBUG-54732
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>

1 files changed, 161 insertions, 102 deletions

diff --git a/tests/auto/quick/qquickitem2/tst_qquickitem.cpp b/tests/auto/quick/qquickitem2/tst_qquickitem.cpp
index 62133d6475..169ef1cbab 100644
--- a/tests/auto/quick/qquickitem2/tst_qquickitem.cpp
+++ b/tests/auto/quick/qquickitem2/tst_qquickitem.cpp
@@ -2698,117 +2698,122 @@ void tst_QQuickItem::childrenRectBottomRightCorner()
 
 struct TestListener : public QQuickItemChangeListener
 {
-    TestListener(bool remove = false) : remove(remove) { reset(); }
+    TestListener(bool remove = false) : remove(remove) { }
 
-    void itemGeometryChanged(QQuickItem *item, QQuickGeometryChange, const QRectF &) override
+    void itemGeometryChanged(QQuickItem *item, QQuickGeometryChange, const QRectF &diff) override
     {
-        ++itemGeometryChanges;
-        value = QRectF(item->x(), item->y(), item->width(), item->height());
+        record(item, QQuickItemPrivate::Geometry, diff);
+    }
+    void itemSiblingOrderChanged(QQuickItem *item) override
+    {
+        record(item, QQuickItemPrivate::SiblingOrder);
+    }
+    void itemVisibilityChanged(QQuickItem *item) override
+    {
+        record(item, QQuickItemPrivate::Visibility);
+    }
+    void itemOpacityChanged(QQuickItem *item) override
+    {
+        record(item, QQuickItemPrivate::Opacity);
+    }
+    void itemRotationChanged(QQuickItem *item) override
+    {
+        record(item, QQuickItemPrivate::Rotation);
+    }
+    void itemImplicitWidthChanged(QQuickItem *item) override
+    {
+        record(item, QQuickItemPrivate::ImplicitWidth);
+    }
+    void itemImplicitHeightChanged(QQuickItem *item) override
+    {
+        record(item, QQuickItemPrivate::ImplicitHeight);
     }
-    void itemSiblingOrderChanged(QQuickItem *) override { ++itemSiblingOrderChanges; }
-    void itemVisibilityChanged(QQuickItem *) override { ++itemVisibilityChanges; }
-    void itemOpacityChanged(QQuickItem *) override { ++itemOpacityChanges; }
-    void itemRotationChanged(QQuickItem *) override { ++itemRotationChanges; }
-    void itemImplicitWidthChanged(QQuickItem *) override { ++itemImplicitWidthChanges; }
-    void itemImplicitHeightChanged(QQuickItem *) override { ++itemImplicitHeightChanges; }
-
     void itemDestroyed(QQuickItem *item) override
     {
-        ++itemDestructions;
-        // QTBUG-53453
-        if (remove)
-            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Destroyed);
+        record(item, QQuickItemPrivate::Destroyed);
     }
     void itemChildAdded(QQuickItem *item, QQuickItem *child) override
     {
-        ++itemChildAdditions;
-        value = QVariant::fromValue(child);
-        // QTBUG-53453
-        if (remove)
-            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Children);
+        record(item, QQuickItemPrivate::Children, QVariant::fromValue(child));
     }
     void itemChildRemoved(QQuickItem *item, QQuickItem *child) override
     {
-        ++itemChildRemovals;
-        value = QVariant::fromValue(child);
-        // QTBUG-53453
-        if (remove)
-            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Children);
+        record(item, QQuickItemPrivate::Children, QVariant::fromValue(child));
     }
     void itemParentChanged(QQuickItem *item, QQuickItem *parent) override
     {
-        ++itemParentChanges;
-        value = QVariant::fromValue(parent);
-        // QTBUG-53453
-        if (remove)
-            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Parent);
+        record(item, QQuickItemPrivate::Parent, QVariant::fromValue(parent));
     }
 
     QQuickAnchorsPrivate *anchorPrivate() override { return nullptr; }
 
-    void reset()
+    void record(QQuickItem *item, QQuickItemPrivate::ChangeType change, const QVariant &value = QVariant())
+    {
+        changes += change;
+        values[change] = value;
+        // QTBUG-54732
+        if (remove)
+            QQuickItemPrivate::get(item)->removeItemChangeListener(this, change);
+    }
+
+    int count(QQuickItemPrivate::ChangeType change) const
     {
-        value = QVariant();
-        itemGeometryChanges = 0;
-        itemSiblingOrderChanges = 0;
-        itemVisibilityChanges = 0;
-        itemOpacityChanges = 0;
-        itemDestructions = 0;
-        itemChildAdditions = 0;
-        itemChildRemovals = 0;
-        itemParentChanges = 0;
-        itemRotationChanges = 0;
-        itemImplicitWidthChanges = 0;
-        itemImplicitHeightChanges = 0;
+        return changes.count(change);
+    }
+
+    QVariant value(QQuickItemPrivate::ChangeType change) const
+    {
+        return values.value(change);
     }
 
     bool remove;
-    QVariant value;
-    int itemGeometryChanges;
-    int itemSiblingOrderChanges;
-    int itemVisibilityChanges;
-    int itemOpacityChanges;
-    int itemDestructions;
-    int itemChildAdditions;
-    int itemChildRemovals;
-    int itemParentChanges;
-    int itemRotationChanges;
-    int itemImplicitWidthChanges;
-    int itemImplicitHeightChanges;
+    QList<QQuickItemPrivate::ChangeType> changes;
+    QHash<QQuickItemPrivate::ChangeType, QVariant> values;
 };
 
 void tst_QQuickItem::changeListener()
 {
-    QQuickItem item;
+    QQuickWindow window;
+    window.show();
+    QTest::qWaitForWindowExposed(&window);
+
+    QQuickItem *item = new QQuickItem;
     TestListener itemListener;
-    QQuickItemPrivate::get(&item)->addItemChangeListener(&itemListener, QQuickItemPrivate::Geometry | QQuickItemPrivate::ImplicitWidth | QQuickItemPrivate::ImplicitHeight |
-                                                                        QQuickItemPrivate::Opacity | QQuickItemPrivate::Rotation);
+    QQuickItemPrivate::get(item)->addItemChangeListener(&itemListener, QQuickItemPrivate::ChangeTypes(0xffff));
+
+    item->setImplicitWidth(10);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::ImplicitWidth), 1);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Geometry), 1);
+    QCOMPARE(itemListener.value(QQuickItemPrivate::Geometry), QVariant(QRectF(0,0,10,0)));
 
-    item.setImplicitWidth(50);
-    QCOMPARE(itemListener.itemImplicitWidthChanges, 1);
-    QCOMPARE(itemListener.itemGeometryChanges, 1);
-    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,50,0)));
+    item->setImplicitHeight(20);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::ImplicitHeight), 1);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Geometry), 2);
+    QCOMPARE(itemListener.value(QQuickItemPrivate::Geometry), QVariant(QRectF(0,0,0,20)));
 
-    item.setImplicitHeight(50);
-    QCOMPARE(itemListener.itemImplicitHeightChanges, 1);
-    QCOMPARE(itemListener.itemGeometryChanges, 2);
-    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,50,50)));
+    item->setWidth(item->width() + 30);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Geometry), 3);
+    QCOMPARE(itemListener.value(QQuickItemPrivate::Geometry), QVariant(QRectF(0,0,30,0)));
 
-    item.setWidth(100);
-    QCOMPARE(itemListener.itemGeometryChanges, 3);
-    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,100,50)));
+    item->setHeight(item->height() + 40);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Geometry), 4);
+    QCOMPARE(itemListener.value(QQuickItemPrivate::Geometry), QVariant(QRectF(0,0,0,40)));
 
-    item.setHeight(100);
-    QCOMPARE(itemListener.itemGeometryChanges, 4);
-    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,100,100)));
+    item->setOpacity(0.5);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Opacity), 1);
 
-    item.setOpacity(0.5);
-    QCOMPARE(itemListener.itemOpacityChanges, 1);
+    item->setRotation(90);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Rotation), 1);
+
+    item->setParentItem(window.contentItem());
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Parent), 1);
+
+    item->setVisible(false);
+    QCOMPARE(itemListener.count(QQuickItemPrivate::Visibility), 1);
 
-    item.setRotation(90);
-    QCOMPARE(itemListener.itemRotationChanges, 1);
+    QQuickItemPrivate::get(item)->removeItemChangeListener(&itemListener, QQuickItemPrivate::ChangeTypes(0xffff));
 
-    QQuickItem *parent = new QQuickItem;
+    QQuickItem *parent = new QQuickItem(window.contentItem());
     TestListener parentListener;
     QQuickItemPrivate::get(parent)->addItemChangeListener(&parentListener, QQuickItemPrivate::Children);
 
@@ -2820,52 +2825,79 @@ void tst_QQuickItem::changeListener()
     QQuickItemPrivate::get(child2)->addItemChangeListener(&child2Listener, QQuickItemPrivate::Parent | QQuickItemPrivate::SiblingOrder | QQuickItemPrivate::Destroyed);
 
     child1->setParentItem(parent);
-    QCOMPARE(parentListener.itemChildAdditions, 1);
-    QCOMPARE(parentListener.value, QVariant::fromValue(child1));
-    QCOMPARE(child1Listener.itemParentChanges, 1);
-    QCOMPARE(child1Listener.value, QVariant::fromValue(parent));
+    QCOMPARE(parentListener.count(QQuickItemPrivate::Children), 1);
+    QCOMPARE(parentListener.value(QQuickItemPrivate::Children), QVariant::fromValue(child1));
+    QCOMPARE(child1Listener.count(QQuickItemPrivate::Parent), 1);
+    QCOMPARE(child1Listener.value(QQuickItemPrivate::Parent), QVariant::fromValue(parent));
 
     child2->setParentItem(parent);
-    QCOMPARE(parentListener.itemChildAdditions, 2);
-    QCOMPARE(parentListener.value, QVariant::fromValue(child2));
-    QCOMPARE(child2Listener.itemParentChanges, 1);
-    QCOMPARE(child2Listener.value, QVariant::fromValue(parent));
+    QCOMPARE(parentListener.count(QQuickItemPrivate::Children), 2);
+    QCOMPARE(parentListener.value(QQuickItemPrivate::Children), QVariant::fromValue(child2));
+    QCOMPARE(child2Listener.count(QQuickItemPrivate::Parent), 1);
+    QCOMPARE(child2Listener.value(QQuickItemPrivate::Parent), QVariant::fromValue(parent));
 
     child2->stackBefore(child1);
-    QCOMPARE(child1Listener.itemSiblingOrderChanges, 1);
-    QCOMPARE(child2Listener.itemSiblingOrderChanges, 1);
+    QCOMPARE(child1Listener.count(QQuickItemPrivate::SiblingOrder), 1);
+    QCOMPARE(child2Listener.count(QQuickItemPrivate::SiblingOrder), 1);
 
     child1->setParentItem(nullptr);
-    QCOMPARE(parentListener.itemChildRemovals, 1);
-    QCOMPARE(parentListener.value, QVariant::fromValue(child1));
-    QCOMPARE(child1Listener.itemParentChanges, 2);
-    QCOMPARE(child1Listener.value, QVariant::fromValue<QQuickItem *>(nullptr));
+    QCOMPARE(parentListener.count(QQuickItemPrivate::Children), 3);
+    QCOMPARE(parentListener.value(QQuickItemPrivate::Children), QVariant::fromValue(child1));
+    QCOMPARE(child1Listener.count(QQuickItemPrivate::Parent), 2);
+    QCOMPARE(child1Listener.value(QQuickItemPrivate::Parent), QVariant::fromValue<QQuickItem *>(nullptr));
 
     delete child1;
-    QCOMPARE(child1Listener.itemDestructions, 1);
+    QCOMPARE(child1Listener.count(QQuickItemPrivate::Destroyed), 1);
 
     delete child2;
-    QCOMPARE(parentListener.itemChildRemovals, 2);
-    QCOMPARE(parentListener.value, QVariant::fromValue(child2));
-    QCOMPARE(child2Listener.itemParentChanges, 2);
-    QCOMPARE(child2Listener.value, QVariant::fromValue<QQuickItem *>(nullptr));
-    QCOMPARE(child2Listener.itemDestructions, 1);
+    QCOMPARE(parentListener.count(QQuickItemPrivate::Children), 4);
+    QCOMPARE(parentListener.value(QQuickItemPrivate::Children), QVariant::fromValue(child2));
+    QCOMPARE(child2Listener.count(QQuickItemPrivate::Parent), 2);
+    QCOMPARE(child2Listener.value(QQuickItemPrivate::Parent), QVariant::fromValue<QQuickItem *>(nullptr));
+    QCOMPARE(child2Listener.count(QQuickItemPrivate::Destroyed), 1);
 
     QQuickItemPrivate::get(parent)->removeItemChangeListener(&parentListener, QQuickItemPrivate::Children);
     QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
 
-    // QTBUG-53453: all listeners should get invoked even if they remove themselves while iterating the listeners
+    // QTBUG-54732: all listeners should get invoked even if they remove themselves while iterating the listeners
     QList<TestListener *> listeners;
     for (int i = 0; i < 5; ++i)
         listeners << new TestListener(true);
 
+    // itemVisibilityChanged x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::Visibility);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    parent->setVisible(false);
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->count(QQuickItemPrivate::Visibility), 1);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
+    // itemRotationChanged x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::Rotation);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    parent->setRotation(90);
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->count(QQuickItemPrivate::Rotation), 1);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
+    // itemOpacityChanged x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::Opacity);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    parent->setOpacity(0.5);
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->count(QQuickItemPrivate::Opacity), 1);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
     // itemChildAdded() x 5
     foreach (TestListener *listener, listeners)
         QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::Children);
     QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
     child1 = new QQuickItem(parent);
     foreach (TestListener *listener, listeners)
-        QCOMPARE(listener->itemChildAdditions, 1);
+        QCOMPARE(listener->count(QQuickItemPrivate::Children), 1);
     QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
 
     // itemParentChanged() x 5
@@ -2874,9 +2906,36 @@ void tst_QQuickItem::changeListener()
     QCOMPARE(QQuickItemPrivate::get(child1)->changeListeners.count(), listeners.count());
     child1->setParentItem(nullptr);
     foreach (TestListener *listener, listeners)
-        QCOMPARE(listener->itemParentChanges, 1);
+        QCOMPARE(listener->count(QQuickItemPrivate::Parent), 1);
     QCOMPARE(QQuickItemPrivate::get(child1)->changeListeners.count(), 0);
 
+    // itemImplicitWidthChanged() x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::ImplicitWidth);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    parent->setImplicitWidth(parent->implicitWidth() + 1);
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->count(QQuickItemPrivate::ImplicitWidth), 1);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
+    // itemImplicitHeightChanged() x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::ImplicitHeight);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    parent->setImplicitHeight(parent->implicitHeight() + 1);
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->count(QQuickItemPrivate::ImplicitHeight), 1);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
+    // itemGeometryChanged() x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::Geometry);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    parent->setWidth(parent->width() + 1);
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->count(QQuickItemPrivate::Geometry), 1);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
     // itemChildRemoved() x 5
     child1->setParentItem(parent);
     foreach (TestListener *listener, listeners)
@@ -2884,7 +2943,7 @@ void tst_QQuickItem::changeListener()
     QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
     delete child1;
     foreach (TestListener *listener, listeners)
-        QCOMPARE(listener->itemChildRemovals, 1);
+        QCOMPARE(listener->count(QQuickItemPrivate::Children), 2);
     QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
 
     // itemDestroyed() x 5
@@ -2893,7 +2952,7 @@ void tst_QQuickItem::changeListener()
     QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
     delete parent;
     foreach (TestListener *listener, listeners)
-        QCOMPARE(listener->itemDestructions, 1);
+        QCOMPARE(listener->count(QQuickItemPrivate::Destroyed), 1);
 }
 
 // QTBUG-13893