QQuickPathViewPrivate: fix heap-use-after-free

The TabBar auto tests in Qt Quick Controls 2 repeats the following
process very quickly for several data rows:

1. Creates a TabBar (PathView, when using the Universal style)
2. Moves items in its QQmlObjectModel
3. Deletes the TabBar

When run with ASAN, this test would fail, because the TabButtons
(which are child items of the PathView) would try to access a deleted
QQuickItemChangeListener upon their destruction.

The underlying issue is that QQuickPathView::modelUpdated() is called,
and before a refill() can happen, the view is deleted.
QQuickPathView::refill() was the only execution path that was releasing
the cached items (QQuickPathViewPrivate::itemCache), and since part of
releasing an item involves removing the QQuickPathView as a change
listener from the item, the item would access the deleted view
(listener) when the item was being destroyed.

This patch fixes the issue by also releasing cached items in
QQuickPathViewPrivate::clear(), which is always called by the
destructor.

Task-number: QTBUG-68964
Change-Id: Ic5bf0943be79948c86bf7c07ef13ecd1a7b971ba
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
Reviewed-by: Robin Burchell <robin.burchell@crimson.no>

1 files changed, 48 insertions, 0 deletions

diff --git a/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp b/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp
index 4211d08393..e6c03d052c 100644
--- a/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp
+++ b/tests/auto/quick/qquickpathview/tst_qquickpathview.cpp
@@ -144,6 +144,7 @@ private slots:
     void movementDirection_data();
     void movementDirection();
     void removePath();
+    void objectModelMove();
 };
 
 class TestObject : public QObject
@@ -2522,6 +2523,53 @@ void tst_QQuickPathView::removePath()
     QVERIFY(QMetaObject::invokeMethod(pathview, "setPath"));
 }
 
+/*
+    Tests that moving items in an ObjectModel and then deleting the view
+    doesn't cause heap-use-after-free when run through ASAN.
+
+    The test case is based on a Qt Quick Controls 2 test where the issue was
+    discovered.
+*/
+void tst_QQuickPathView::objectModelMove()
+{
+    QScopedPointer<QQuickView> window(createView());
+    window->setSource(testFileUrl("objectModelMove.qml"));
+    window->show();
+
+    // Create the view.
+    QVERIFY(QMetaObject::invokeMethod(window->rootObject(), "newView"));
+    QPointer<QQuickPathView> pathView = window->rootObject()->property("pathViewItem").value<QQuickPathView*>();
+    QVERIFY(pathView);
+    QCOMPARE(pathView->count(), 3);
+    pathView->highlightItem()->setObjectName("highlight");
+
+    // Move an item from index 0 to 1.
+    QVERIFY(QMetaObject::invokeMethod(window->rootObject(), "move"));
+    QCOMPARE(pathView->count(), 3);
+
+    // Keep track of the amount of listeners
+    QVector<QString> itemObjectNames;
+    itemObjectNames << QLatin1String("red") << QLatin1String("green") << QLatin1String("blue");
+    QVector<QQuickItem*> childItems;
+    for (const QString itemObjectName : qAsConst(itemObjectNames)) {
+        QQuickItem *childItem = findItem<QQuickItem>(pathView, itemObjectName);
+        QVERIFY(childItem);
+        childItems.append(childItem);
+    }
+
+    // Destroy the view (via destroy()).
+    QVERIFY(QMetaObject::invokeMethod(window->rootObject(), "destroyView"));
+    // Ensure that the view has been destroyed. This check is also necessary in order for
+    // ASAN to complain (it will complain after the test function has finished).
+    QTRY_VERIFY(pathView.isNull());
+    // By this point, all of its cached items should have been released,
+    // which means none of the items should have any listeners.
+    for (const auto childItem : qAsConst(childItems)) {
+        const QQuickItemPrivate *childItemPrivate = QQuickItemPrivate::get(childItem);
+        QCOMPARE(childItemPrivate->changeListeners.size(), 0);
+    }
+}
+
 QTEST_MAIN(tst_QQuickPathView)
 
 #include "tst_qquickpathview.moc"