Fix heap-use-after-free in Container when using ListView transitionsHEADdev

Container's removeItem function removes and deletes items immediately.
This has caused issues in the past, like QTBUG-46798. QTBUG-133256 is
similar to that, except it also involves transitions.

This patch fixes the crash by listening for the destruction of items
created within an ObjectModel and removing them so that views don't
have dangling pointers to them.

Add removeAndDestroyObjectModelItem to tst_qquicklistview2, which is
similar to the test added for 1e3924d8f585dd9099eb74ffbc17950c693d14da
but adds more thorough checks and transitions.

Fixes: QTBUG-133256
Task-number: QTBUG-46798
Pick-to: 6.5 6.8 6.9 6.10
Change-Id: I9777713edfc6f82a4e9edefe2fd6c9831f03c261
Reviewed-by: Shawn Rutledge <[email protected]>
Reviewed-by: Richard Moe Gustavsen <[email protected]>

6 files changed, 275 insertions, 16 deletions

diff --git a/src/quick/items/qquickitemview.cpp b/src/quick/items/qquickitemview.cpp
index 653d4f4b15..d5b870ad26 100644
--- a/src/quick/items/qquickitemview.cpp
+++ b/src/quick/items/qquickitemview.cpp
@@ -1643,6 +1643,8 @@ QQuickItemViewPrivate::QQuickItemViewPrivate()
     bufferPause.setDuration(16);
 }
 
+static const QQuickItemPrivate::ChangeTypes itemChangeListenerTypes = QQuickItemPrivate::Destroyed;
+
 QQuickItemViewPrivate::~QQuickItemViewPrivate()
 {
 #if QT_CONFIG(quick_viewtransitions)
@@ -2517,6 +2519,12 @@ FxViewItem *QQuickItemViewPrivate::createItem(int modelIndex, QQmlIncubator::Inc
         inRequest = false;
         return nullptr;
     } else {
+        // Container removes and instantly deletes items created within ObjectModels.
+        // We need to account for this to avoid having references to deleted items.
+        // itemDestroyed is called as a result of adding this listener.
+        if (qobject_cast<QQmlObjectModel *>(model))
+            QQuickItemPrivate::get(item)->updateOrAddItemChangeListener(this, itemChangeListenerTypes);
+
         item->setParentItem(q->contentItem());
         if (requestedIndex == modelIndex)
             requestedIndex = -1;
@@ -2563,6 +2571,7 @@ void QQuickItemView::initItem(int, QObject *object)
     }
 }
 
+// This is called when the model (if it's a QQmlInstanceModel) emits destroyingItem.
 void QQuickItemView::destroyingItem(QObject *object)
 {
     Q_D(QQuickItemView);
@@ -2570,6 +2579,7 @@ void QQuickItemView::destroyingItem(QObject *object)
     if (item) {
         item->setParentItem(nullptr);
         d->unrequestedItems.remove(item);
+        QQuickItemPrivate::get(item)->removeItemChangeListener(d, itemChangeListenerTypes);
     }
 }
 
@@ -2599,29 +2609,82 @@ bool QQuickItemViewPrivate::releaseItem(FxViewItem *item, QQmlInstanceModel::Reu
     item->trackGeometry(false);
 
     QQmlInstanceModel::ReleaseFlags flags = {};
-    if (model && item->item) {
-        flags = model->release(item->item, reusableFlag);
-        if (!flags) {
-            // item was not destroyed, and we no longer reference it.
-            if (item->item->parentItem() == contentItem) {
-                // Only cull the item if its parent item is still our contentItem.
-                // One case where this can happen is moving an item out of one ObjectModel and into another.
-                QQuickItemPrivate::get(item->item)->setCulled(true);
+    if (QPointer<QQuickItem> quickItem = item->item) {
+        if (model) {
+            flags = model->release(quickItem, reusableFlag);
+            if (!flags) {
+                // item was not destroyed, and we no longer reference it.
+                if (quickItem->parentItem() == contentItem) {
+                    // Only cull the item if its parent item is still our contentItem.
+                    // One case where this can happen is moving an item out of one ObjectModel and into another.
+                    QQuickItemPrivate::get(quickItem)->setCulled(true);
+                }
+                // If deleteLater was called, the item isn't long for this world and so we shouldn't store references to it.
+                // This can happen when a Repeater is used to populate items in SwipeView's ListView contentItem.
+                if (!isClearing && !QObjectPrivate::get(quickItem)->deleteLaterCalled)
+                    unrequestedItems.insert(quickItem, model->indexOf(quickItem, q));
+            } else if (flags & QQmlInstanceModel::Destroyed) {
+                quickItem->setParentItem(nullptr);
+            } else if (flags & QQmlInstanceModel::Pooled) {
+                item->setVisible(false);
             }
-            // If deleteLater was called, the item isn't long for this world and so we shouldn't store references to it.
-            // This can happen when a Repeater is used to populate items in SwipeView's ListView contentItem.
-            if (!isClearing && !QObjectPrivate::get(item->item)->deleteLaterCalled)
-                unrequestedItems.insert(item->item, model->indexOf(item->item, q));
-        } else if (flags & QQmlInstanceModel::Destroyed) {
-            item->item->setParentItem(nullptr);
-        } else if (flags & QQmlInstanceModel::Pooled) {
-            item->setVisible(false);
         }
+
+        QQuickItemPrivate::get(quickItem)->removeItemChangeListener(this, itemChangeListenerTypes);
+        delete item->transitionableItem;
+        item->transitionableItem = nullptr;
     }
+
     delete item;
     return flags != QQmlInstanceModel::Referenced;
 }
 
+/*!
+    \internal
+
+    Called when an item created in an ObjectModel is deleted rather than
+    removing it via the model.
+
+    Similar in what it does to destroyRemoved except that it intentionally
+    doesn't account for delayRemove.
+*/
+void QQuickItemViewPrivate::itemDestroyed(QQuickItem *item)
+{
+    // We can't check model->indexOf(item, q_func()) here, because the item
+    // may not exist there, so we instead check visibleItems.
+    FxViewItem *visibleFxItem = nullptr;
+    const int indexOfItem = -1;
+    for (auto *fxItem : std::as_const(visibleItems)) {
+        if (fxItem->item == item) {
+            visibleFxItem = fxItem;
+            break;
+        }
+    }
+
+    // Make sure that we don't try to clean up the same FxViewItem twice,
+    // as apparently there can be two FxViewItems for the same QQuickItem.
+    if (currentItem && visibleFxItem)
+        Q_ASSERT(currentItem != visibleFxItem);
+
+    if (visibleFxItem) {
+        qCDebug(lcItemViewDelegateLifecycle) << "removing deleted item"
+            << item << visibleFxItem << "at index" << indexOfItem << "without running transitions";
+        // We need to remove it from visibleItems manually, as we don't want to call
+        // removeNonVisibleItems since it won't remove items with transitions.
+        const bool removedVisibleFxItem = visibleItems.removeOne(visibleFxItem);
+        Q_ASSERT(removedVisibleFxItem);
+        releaseItem(visibleFxItem, QQmlDelegateModel::NotReusable);
+    }
+
+    if (currentItem && currentItem->item == item) {
+        releaseItem(currentItem, QQmlDelegateModel::NotReusable);
+        currentItem = nullptr;
+    }
+
+    // Update the positioning of the items.
+    forceLayoutPolish();
+}
+
 QQuickItem *QQuickItemViewPrivate::createHighlightItem()
 {
     QQuickItem *item = nullptr;
diff --git a/src/quick/items/qquickitemview_p_p.h b/src/quick/items/qquickitemview_p_p.h
index 77326aeddf..daeb7e0fdb 100644
--- a/src/quick/items/qquickitemview_p_p.h
+++ b/src/quick/items/qquickitemview_p_p.h
@@ -151,6 +151,7 @@ public:
         return releaseItem(oldCurrentItem, reusableFlag);
     }
     virtual bool releaseItem(FxViewItem *item, QQmlInstanceModel::ReusableFlag reusableFlag);
+    void itemDestroyed(QQuickItem *item) override;
 
     QQuickItem *createHighlightItem();
     QQuickItem *createComponentItem(QQmlComponent *component, qreal zValue, bool createDefault = false) const;
diff --git a/tests/auto/quick/qquicklistview2/data/removeAndDestroyObjectModelItem.qml b/tests/auto/quick/qquicklistview2/data/removeAndDestroyObjectModelItem.qml
new file mode 100644
index 0000000000..994bd8400b
--- /dev/null
+++ b/tests/auto/quick/qquicklistview2/data/removeAndDestroyObjectModelItem.qml
@@ -0,0 +1,30 @@
+// Copyright (C) 2025 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only
+
+import QtQml.Models
+import QtQuick
+
+ListView {
+    width: 200
+    height: 200
+
+    model: ObjectModel {
+        id: objectModel
+
+        Rectangle {
+            width: 200
+            height: 200
+            color: "red"
+        }
+        Rectangle {
+            width: 200
+            height: 200
+            color: "green"
+        }
+        Rectangle {
+            width: 200
+            height: 200
+            color: "blue"
+        }
+    }
+}
diff --git a/tests/auto/quick/qquicklistview2/data/removeAndDestroyObjectModelItemWithTransitions.qml b/tests/auto/quick/qquicklistview2/data/removeAndDestroyObjectModelItemWithTransitions.qml
new file mode 100644
index 0000000000..b979bfd118
--- /dev/null
+++ b/tests/auto/quick/qquicklistview2/data/removeAndDestroyObjectModelItemWithTransitions.qml
@@ -0,0 +1,38 @@
+// Copyright (C) 2025 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only
+
+import QtQml.Models
+import QtQuick
+
+ListView {
+    width: 200
+    height: 200
+
+    displaced: Transition {
+        NumberAnimation { properties: "x,y"; duration: 400; easing.type: Easing.OutBounce }
+    }
+
+    remove: Transition {
+        NumberAnimation { properties: "scale"; from: 1; to: 0; duration: 400 }
+    }
+
+    model: ObjectModel {
+        id: objectModel
+
+        Rectangle {
+            width: 200
+            height: 200
+            color: "red"
+        }
+        Rectangle {
+            width: 200
+            height: 200
+            color: "green"
+        }
+        Rectangle {
+            width: 200
+            height: 200
+            color: "blue"
+        }
+    }
+}
diff --git a/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp b/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
index ed95f06f26..94feee92f3 100644
--- a/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
+++ b/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
@@ -4,6 +4,7 @@
 #include "delegatemodelkinds.h"
 
 #include <QtTest/QTest>
+#include <QtQmlModels/private/qqmlobjectmodel_p.h>
 #include <QtQuick/qquickview.h>
 #include <QtQuick/private/qquickitemview_p_p.h>
 #include <QtQuick/private/qquicklistview_p.h>
@@ -80,6 +81,8 @@ private slots:
 
     void delegateModelAccess_data();
     void delegateModelAccess();
+    void removeAndDestroyObjectModelItem_data();
+    void removeAndDestroyObjectModelItem();
 
 private:
     void flickWithTouch(QQuickWindow *window, const QPoint &from, const QPoint &to);
@@ -1480,6 +1483,56 @@ void tst_QQuickListView2::delegateModelAccess()
     QCOMPARE(delegate->property("modelX").toDouble(), expected);
 }
 
+enum RemovalPolicy {
+    RemoveAndDestroy,
+    OnlyDestroy
+};
+
+void tst_QQuickListView2::removeAndDestroyObjectModelItem_data()
+{
+    QTest::addColumn<QString>("qmlFilePath");
+    QTest::addColumn<RemovalPolicy>("removalPolicy");
+
+    QTest::addRow("remove and destroy, no transitions")
+        << "removeAndDestroyObjectModelItem.qml" << RemoveAndDestroy;
+    QTest::addRow("destroy, no transitions")
+        << "removeAndDestroyObjectModelItem.qml" << OnlyDestroy;
+    QTest::addRow("remove and destroy, transitions")
+        << "removeAndDestroyObjectModelItemWithTransitions.qml" << RemoveAndDestroy;
+    QTest::addRow("destroy, transitions")
+        << "removeAndDestroyObjectModelItemWithTransitions.qml" << OnlyDestroy;
+}
+
+// QTBUG-46798, QTBUG-133256
+void tst_QQuickListView2::removeAndDestroyObjectModelItem()
+{
+    QFETCH(QString, qmlFilePath);
+    QFETCH(RemovalPolicy, removalPolicy);
+
+    QQuickView window;
+    QVERIFY(QQuickTest::showView(window, testFileUrl(qmlFilePath)));
+
+    QQuickListView *listView = qobject_cast<QQuickListView*>(window.rootObject());
+    QVERIFY(listView);
+
+    auto *objectModel = listView->model().value<QQmlObjectModel *>();
+    QVERIFY(objectModel);
+    QPointer<QObject> firstItem = objectModel->get(0);
+    QVERIFY(firstItem);
+    // Shouldn't crash.
+    if (removalPolicy == RemoveAndDestroy)
+        objectModel->remove(0);
+    firstItem->deleteLater();
+    QTRY_VERIFY(!firstItem);
+
+    // Now try moving the view. It also shouldn't crash.
+    if (removalPolicy == RemoveAndDestroy) {
+        if (QQuickTest::qIsPolishScheduled(listView))
+            QVERIFY(QQuickTest::qWaitForPolish(listView));
+    }
+    listView->positionViewAtEnd();
+}
+
 QTEST_MAIN(tst_QQuickListView2)
 
 #include "tst_qquicklistview2.moc"
diff --git a/tests/auto/quickcontrols/controls/data/tst_container.qml b/tests/auto/quickcontrols/controls/data/tst_container.qml
index a829404a21..f6c50699e2 100644
--- a/tests/auto/quickcontrols/controls/data/tst_container.qml
+++ b/tests/auto/quickcontrols/controls/data/tst_container.qml
@@ -293,4 +293,78 @@ TestCase {
         compare(control.implicitWidth, 100)
         compare(control.implicitHeight, 30)
     }
+
+    Transition {
+        id: yTransition
+
+        NumberAnimation {
+            properties: "y"
+            duration: 1000
+        }
+    }
+
+    Transition {
+        id: scaleTransition
+
+        NumberAnimation {
+            properties: "scale"
+            from: 1
+            to: 0
+            duration: 400
+        }
+    }
+
+    Component {
+        id: listViewContainerComponent
+
+        Container {
+            id: container
+            width: 200
+            height: 200
+
+            property alias addDisplaced: view.addDisplaced
+            property alias moveDisplaced: view.moveDisplaced
+            property alias removeDisplaced: view.removeDisplaced
+            property alias displaced: view.displaced
+            property alias remove: view.remove
+
+            contentItem: ListView {
+                id: view
+                objectName: "containerListView"
+                model: container.contentModel
+                spacing: 5
+            }
+        }
+    }
+
+    function test_listViewTransitions_data() {
+        return [
+            { tag: "addDisplaced", addDisplaced: yTransition },
+            { tag: "moveDisplaced", moveDisplaced: yTransition },
+            { tag: "removeDisplaced", removeDisplaced: yTransition },
+            { tag: "displaced", displaced: yTransition },
+            { tag: "remove", remove: scaleTransition }
+        ]
+    }
+
+    function test_listViewTransitions(data) {
+        let control = createTemporaryObject(listViewContainerComponent, testCase, {
+            addDisplaced: data.addDisplaced ?? null,
+            moveDisplaced: data.moveDisplaced ?? null,
+            removeDisplaced: data.removeDisplaced ?? null,
+            displaced: data.displaced ?? null,
+            remove: data.remove ?? null
+        })
+        verify(control)
+
+        let rect = rectangle.createObject(control,
+            { width: 200, height: 40, color: "tomato", objectName: "tomatoRect" })
+        control.addItem(rect)
+        verify(isPolishScheduled(testCase.Window.window))
+        verify(waitForPolish(testCase.Window.window))
+
+        control.removeItem(rect)
+        verify(isPolishScheduled(testCase.Window.window))
+        verify(waitForPolish(testCase.Window.window))
+    }
 }