Fix QQuickItem change listeners

All listeners should get invoked, even if they remove themselves
while iterating the listeners. An index-based loop would skip the
next listener in the list. This change replaces the QPODVector with
a QVector, so we can make a cheap copy before iterating the listeners.

Change-Id: I2430b3763184a40ad1c5c3a68d36fecafcadb3ee
Task-number: QTBUG-53453
Reviewed-by: Robin Burchell <robin.burchell@viroteck.net>

3 files changed, 243 insertions, 39 deletions

diff --git a/src/quick/items/qquickitem.cpp b/src/quick/items/qquickitem.cpp
index 3573788855..a8c9d34110 100644
--- a/src/quick/items/qquickitem.cpp
+++ b/src/quick/items/qquickitem.cpp
@@ -2336,8 +2336,9 @@ QQuickItem::~QQuickItem()
     while (!d->childItems.isEmpty())
         d->childItems.constFirst()->setParentItem(0);
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        QQuickAnchorsPrivate *anchor = d->changeListeners.at(ii).listener->anchorPrivate();
+    const auto listeners = d->changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
+        QQuickAnchorsPrivate *anchor = change.listener->anchorPrivate();
         if (anchor)
             anchor->clearItem(this);
     }
@@ -2346,14 +2347,13 @@ QQuickItem::~QQuickItem()
         update item anchors that depended on us unless they are our child (and will also be destroyed),
         or our sibling, and our parent is also being destroyed.
     */
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        QQuickAnchorsPrivate *anchor = d->changeListeners.at(ii).listener->anchorPrivate();
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
+        QQuickAnchorsPrivate *anchor = change.listener->anchorPrivate();
         if (anchor && anchor->item && anchor->item->parentItem() && anchor->item->parentItem() != this)
             anchor->update();
     }
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = d->changeListeners.at(ii);
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::Destroyed)
             change.listener->itemDestroyed(this);
     }
@@ -3595,8 +3595,8 @@ QQuickAnchors *QQuickItemPrivate::anchors() const
 void QQuickItemPrivate::siblingOrderChanged()
 {
     Q_Q(QQuickItem);
-    for (int ii = 0; ii < changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+    const auto listeners = changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::SiblingOrder) {
             change.listener->itemSiblingOrderChanged(q);
         }
@@ -3704,8 +3704,8 @@ void QQuickItem::geometryChanged(const QRectF &newGeometry, const QRectF &oldGeo
     bool widthChange = (newGeometry.width() != oldGeometry.width());
     bool heightChange = (newGeometry.height() != oldGeometry.height());
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = d->changeListeners.at(ii);
+    const auto listeners = d->changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::Geometry) {
             if (change.gTypes == QQuickItemPrivate::GeometryChange) {
                 change.listener->itemGeometryChanged(this, newGeometry, oldGeometry);
@@ -3841,7 +3841,7 @@ void QQuickItemPrivate::removeItemChangeListener(QQuickItemChangeListener *liste
 void QQuickItemPrivate::updateOrAddGeometryChangeListener(QQuickItemChangeListener *listener, GeometryChangeTypes types)
 {
     ChangeListener change(listener, types);
-    int index = changeListeners.find(change);
+    int index = changeListeners.indexOf(change);
     if (index > -1)
         changeListeners[index].gTypes = change.gTypes;  //we may have different GeometryChangeTypes
     else
@@ -3855,7 +3855,7 @@ void QQuickItemPrivate::updateOrRemoveGeometryChangeListener(QQuickItemChangeLis
     if (types == NoChange) {
         changeListeners.removeOne(change);
     } else {
-        int index = changeListeners.find(change);
+        int index = changeListeners.indexOf(change);
         if (index > -1)
             changeListeners[index].gTypes = change.gTypes;  //we may have different GeometryChangeTypes
     }
@@ -4264,8 +4264,8 @@ void QQuickItem::setBaselineOffset(qreal offset)
 
     d->baselineOffset = offset;
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = d->changeListeners.at(ii);
+    const auto listeners = d->changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::Geometry) {
             QQuickAnchorsPrivate *anchor = change.listener->anchorPrivate();
             if (anchor)
@@ -5936,66 +5936,72 @@ void QQuickItemPrivate::itemChange(QQuickItem::ItemChange change, const QQuickIt
 {
     Q_Q(QQuickItem);
     switch (change) {
-    case QQuickItem::ItemChildAddedChange:
+    case QQuickItem::ItemChildAddedChange: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Children) {
                 change.listener->itemChildAdded(q, data.item);
             }
         }
         break;
-    case QQuickItem::ItemChildRemovedChange:
+    }
+    case QQuickItem::ItemChildRemovedChange: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Children) {
                 change.listener->itemChildRemoved(q, data.item);
             }
         }
         break;
+    }
     case QQuickItem::ItemSceneChange:
         q->itemChange(change, data);
         break;
-    case QQuickItem::ItemVisibleHasChanged:
+    case QQuickItem::ItemVisibleHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Visibility) {
                 change.listener->itemVisibilityChanged(q);
             }
         }
         break;
-    case QQuickItem::ItemParentHasChanged:
+    }
+    case QQuickItem::ItemParentHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Parent) {
                 change.listener->itemParentChanged(q, data.item);
             }
         }
         break;
-    case QQuickItem::ItemOpacityHasChanged:
+    }
+    case QQuickItem::ItemOpacityHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Opacity) {
                 change.listener->itemOpacityChanged(q);
             }
         }
         break;
+    }
     case QQuickItem::ItemActiveFocusHasChanged:
         q->itemChange(change, data);
         break;
-    case QQuickItem::ItemRotationHasChanged:
+    case QQuickItem::ItemRotationHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Rotation) {
                 change.listener->itemRotationChanged(q);
             }
         }
         break;
+    }
     case QQuickItem::ItemAntialiasingHasChanged:
         // fall through
     case QQuickItem::ItemDevicePixelRatioHasChanged:
@@ -6350,8 +6356,8 @@ void QQuickItem::resetWidth()
 void QQuickItemPrivate::implicitWidthChanged()
 {
     Q_Q(QQuickItem);
-    for (int ii = 0; ii < changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+    const auto listeners = changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::ImplicitWidth) {
             change.listener->itemImplicitWidthChanged(q);
         }
@@ -6514,8 +6520,8 @@ void QQuickItem::resetHeight()
 void QQuickItemPrivate::implicitHeightChanged()
 {
     Q_Q(QQuickItem);
-    for (int ii = 0; ii < changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+    const auto listeners = changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::ImplicitHeight) {
             change.listener->itemImplicitHeightChanged(q);
         }
diff --git a/src/quick/items/qquickitem_p.h b/src/quick/items/qquickitem_p.h
index ad649e5b5f..fed3e88b68 100644
--- a/src/quick/items/qquickitem_p.h
+++ b/src/quick/items/qquickitem_p.h
@@ -62,7 +62,6 @@
 #include <QtQuick/qsgnode.h>
 #include "qquickclipnode_p.h"
 
-#include <private/qpodvector_p.h>
 #include <QtQuick/private/qquickstate_p.h>
 #include <private/qqmlnullablevalue_p.h>
 #include <private/qqmlnotifier_p.h>
@@ -332,7 +331,7 @@ public:
     Q_DECLARE_FLAGS(GeometryChangeTypes, GeometryChangeType)
 
     struct ChangeListener {
-        ChangeListener(QQuickItemChangeListener *l, QQuickItemPrivate::ChangeTypes t) : listener(l), types(t), gTypes(GeometryChange) {}
+        ChangeListener(QQuickItemChangeListener *l = Q_NULLPTR, QQuickItemPrivate::ChangeTypes t = 0) : listener(l), types(t), gTypes(GeometryChange) {}
         ChangeListener(QQuickItemChangeListener *l, QQuickItemPrivate::GeometryChangeTypes gt) : listener(l), types(Geometry), gTypes(gt) {}
         QQuickItemChangeListener *listener;
         QQuickItemPrivate::ChangeTypes types;
@@ -386,7 +385,7 @@ public:
 
     inline Qt::MouseButtons acceptedMouseButtons() const;
 
-    QPODVector<QQuickItemPrivate::ChangeListener,4> changeListeners;
+    QVector<QQuickItemPrivate::ChangeListener> changeListeners;
 
     void addItemChangeListener(QQuickItemChangeListener *listener, ChangeTypes types);
     void removeItemChangeListener(QQuickItemChangeListener *, ChangeTypes types);
@@ -932,6 +931,7 @@ QSGNode *QQuickItemPrivate::childContainerNode()
 }
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(QQuickItemPrivate::ChangeTypes)
+Q_DECLARE_TYPEINFO(QQuickItemPrivate::ChangeListener, Q_PRIMITIVE_TYPE);
 
 QT_END_NAMESPACE
 
diff --git a/tests/auto/quick/qquickitem2/tst_qquickitem.cpp b/tests/auto/quick/qquickitem2/tst_qquickitem.cpp
index 62ff09e698..a087efd6b8 100644
--- a/tests/auto/quick/qquickitem2/tst_qquickitem.cpp
+++ b/tests/auto/quick/qquickitem2/tst_qquickitem.cpp
@@ -35,6 +35,7 @@
 #include <QtGui/private/qinputmethod_p.h>
 #include <QtQuick/private/qquickrectangle_p.h>
 #include <QtQuick/private/qquicktextinput_p.h>
+#include <QtQuick/private/qquickitemchangelistener_p.h>
 #include <QtGui/qstylehints.h>
 #include <private/qquickitem_p.h>
 #include "../../shared/util.h"
@@ -107,6 +108,7 @@ private slots:
     void childrenProperty();
     void resourcesProperty();
 
+    void changeListener();
     void transformCrash();
     void implicitSize();
     void qtbug_16871();
@@ -2694,6 +2696,202 @@ void tst_QQuickItem::childrenRectBottomRightCorner()
     delete window;
 }
 
+struct TestListener : public QQuickItemChangeListener
+{
+    TestListener(bool remove = false) : remove(remove) { reset(); }
+
+    void itemGeometryChanged(QQuickItem *, const QRectF &newGeometry, const QRectF &) override { ++itemGeometryChanges; value = newGeometry; }
+    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);
+    }
+    void itemChildAdded(QQuickItem *item, QQuickItem *child) override
+    {
+        ++itemChildAdditions;
+        value = QVariant::fromValue(child);
+        // QTBUG-53453
+        if (remove)
+            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Children);
+    }
+    void itemChildRemoved(QQuickItem *item, QQuickItem *child) override
+    {
+        ++itemChildRemovals;
+        value = QVariant::fromValue(child);
+        // QTBUG-53453
+        if (remove)
+            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Children);
+    }
+    void itemParentChanged(QQuickItem *item, QQuickItem *parent) override
+    {
+        ++itemParentChanges;
+        value = QVariant::fromValue(parent);
+        // QTBUG-53453
+        if (remove)
+            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Parent);
+    }
+
+    QQuickAnchorsPrivate *anchorPrivate() override { return nullptr; }
+
+    void reset()
+    {
+        value = QVariant();
+        itemGeometryChanges = 0;
+        itemSiblingOrderChanges = 0;
+        itemVisibilityChanges = 0;
+        itemOpacityChanges = 0;
+        itemDestructions = 0;
+        itemChildAdditions = 0;
+        itemChildRemovals = 0;
+        itemParentChanges = 0;
+        itemRotationChanges = 0;
+        itemImplicitWidthChanges = 0;
+        itemImplicitHeightChanges = 0;
+    }
+
+    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;
+};
+
+void tst_QQuickItem::changeListener()
+{
+    QQuickItem item;
+    TestListener itemListener;
+    QQuickItemPrivate::get(&item)->addItemChangeListener(&itemListener, QQuickItemPrivate::Geometry | QQuickItemPrivate::ImplicitWidth | QQuickItemPrivate::ImplicitHeight |
+                                                                        QQuickItemPrivate::Opacity | QQuickItemPrivate::Rotation);
+
+    item.setImplicitWidth(50);
+    QCOMPARE(itemListener.itemImplicitWidthChanges, 1);
+    QCOMPARE(itemListener.itemGeometryChanges, 1);
+    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,50,0)));
+
+    item.setImplicitHeight(50);
+    QCOMPARE(itemListener.itemImplicitHeightChanges, 1);
+    QCOMPARE(itemListener.itemGeometryChanges, 2);
+    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,50,50)));
+
+    item.setWidth(100);
+    QCOMPARE(itemListener.itemGeometryChanges, 3);
+    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,100,50)));
+
+    item.setHeight(100);
+    QCOMPARE(itemListener.itemGeometryChanges, 4);
+    QCOMPARE(itemListener.value, QVariant(QRectF(0,0,100,100)));
+
+    item.setOpacity(0.5);
+    QCOMPARE(itemListener.itemOpacityChanges, 1);
+
+    item.setRotation(90);
+    QCOMPARE(itemListener.itemRotationChanges, 1);
+
+    QQuickItem *parent = new QQuickItem;
+    TestListener parentListener;
+    QQuickItemPrivate::get(parent)->addItemChangeListener(&parentListener, QQuickItemPrivate::Children);
+
+    QQuickItem *child1 = new QQuickItem;
+    QQuickItem *child2 = new QQuickItem;
+    TestListener child1Listener;
+    TestListener child2Listener;
+    QQuickItemPrivate::get(child1)->addItemChangeListener(&child1Listener, QQuickItemPrivate::Parent | QQuickItemPrivate::SiblingOrder | QQuickItemPrivate::Destroyed);
+    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));
+
+    child2->setParentItem(parent);
+    QCOMPARE(parentListener.itemChildAdditions, 2);
+    QCOMPARE(parentListener.value, QVariant::fromValue(child2));
+    QCOMPARE(child2Listener.itemParentChanges, 1);
+    QCOMPARE(child2Listener.value, QVariant::fromValue(parent));
+
+    child2->stackBefore(child1);
+    QCOMPARE(child1Listener.itemSiblingOrderChanges, 1);
+    QCOMPARE(child2Listener.itemSiblingOrderChanges, 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));
+
+    delete child1;
+    QCOMPARE(child1Listener.itemDestructions, 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);
+
+    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
+    QList<TestListener *> listeners;
+    for (int i = 0; i < 5; ++i)
+        listeners << new TestListener(true);
+
+    // 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(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
+    // itemParentChanged() x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(child1)->addItemChangeListener(listener, QQuickItemPrivate::Parent);
+    QCOMPARE(QQuickItemPrivate::get(child1)->changeListeners.count(), listeners.count());
+    child1->setParentItem(nullptr);
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->itemParentChanges, 1);
+    QCOMPARE(QQuickItemPrivate::get(child1)->changeListeners.count(), 0);
+
+    // itemChildRemoved() x 5
+    child1->setParentItem(parent);
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::Children);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    delete child1;
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->itemChildRemovals, 1);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), 0);
+
+    // itemDestroyed() x 5
+    foreach (TestListener *listener, listeners)
+        QQuickItemPrivate::get(parent)->addItemChangeListener(listener, QQuickItemPrivate::Destroyed);
+    QCOMPARE(QQuickItemPrivate::get(parent)->changeListeners.count(), listeners.count());
+    delete parent;
+    foreach (TestListener *listener, listeners)
+        QCOMPARE(listener->itemDestructions, 1);
+}
+
 // QTBUG-13893
 void tst_QQuickItem::transformCrash()
 {