DelegateModel: don't insert items using an invalidated iterator

The code in the while-loop in insert() has to allocate memory, and that
might trigger the garbage collector, which will clear the cache. This
invalidates the "before" iterator passed into insert(), resulting in
asserts when inserting new items into the cache.

To fix this we have to detect that the `before` iterator became invalid,
and then recompute the iterator. Change the return value of the insert()
function to provide more information than true/false, and use the logic
and information at the call-site to retry the insertion.

As a drive-by, skip computations of local values if they are not going
to be used.

Fixes: QTBUG-119181
Change-Id: I71543b8df776dbeaed5a275960ad34df1d12e19f
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
(cherry picked from commit 4c1cee7e11)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 9036e492dd)

src/qmlmodels/qqmldelegatemodel.cpp

@@ -2049,26 +2049,28 @@ QQmlDelegateModelAttached *QQmlDelegateModel::qmlAttachedProperties(QObject *obj
     return new QQmlDelegateModelAttached(obj);
 }
 
-bool QQmlDelegateModelPrivate::insert(Compositor::insert_iterator &before, const QV4::Value &object, int groups)
+QQmlDelegateModelPrivate::InsertionResult
+QQmlDelegateModelPrivate::insert(Compositor::insert_iterator &before, const QV4::Value &object, int groups)
 {
     if (!m_context || !m_context->isValid())
-        return false;
+        return InsertionResult::Error;
 
     QQmlDelegateModelItem *cacheItem = m_adaptorModel.createItem(m_cacheMetaType, -1);
     if (!cacheItem)
-        return false;
+        return InsertionResult::Error;
     if (!object.isObject())
-        return false;
+        return InsertionResult::Error;
 
     QV4::ExecutionEngine *v4 = object.as<QV4::Object>()->engine();
     QV4::Scope scope(v4);
     QV4::ScopedObject o(scope, object);
     if (!o)
-        return false;
+        return InsertionResult::Error;
 
     QV4::ObjectIterator it(scope, o, QV4::ObjectIterator::EnumerableOnly);
     QV4::ScopedValue propertyName(scope);
     QV4::ScopedValue v(scope);
+    const auto oldCache = m_cache;
     while (1) {
         propertyName = it.nextPropertyNameAsString(v);
         if (propertyName->isNull())
@@ -2077,6 +2079,9 @@ bool QQmlDelegateModelPrivate::insert(Compositor::insert_iterator &before, const
                     propertyName->toQStringNoThrow(),
                     QV4::ExecutionEngine::toVariant(v, QMetaType {}));
     }
+    const bool cacheModified = !m_cache.isSharedWith(oldCache);
+    if (cacheModified)
+        return InsertionResult::Retry;
 
     cacheItem->groups = groups | Compositor::UnresolvedFlag | Compositor::CacheFlag;
 
@@ -2086,7 +2091,7 @@ bool QQmlDelegateModelPrivate::insert(Compositor::insert_iterator &before, const
     m_cache.insert(before.cacheIndex(), cacheItem);
     m_compositor.insert(before, nullptr, 0, 1, cacheItem->groups);
 
-    return true;
+    return InsertionResult::Success;
 }
 
 //============================================================================
@@ -3087,9 +3092,8 @@ void QQmlDelegateModelGroup::insert(QQmlV4Function *args)
         v = (*args)[i];
     }
 
-    Compositor::insert_iterator before = index < model->m_compositor.count(group)
-            ? model->m_compositor.findInsertPosition(group, index)
-            : model->m_compositor.end();
+    if (v->as<QV4::ArrayObject>())
+        return;
 
     int groups = 1 << d->group;
     if (++i < args->length()) {
@@ -3097,11 +3101,16 @@ void QQmlDelegateModelGroup::insert(QQmlV4Function *args)
         groups |= model->m_cacheMetaType->parseGroups(val);
     }
 
-    if (v->as<QV4::ArrayObject>()) {
-        return;
-    } else if (v->as<QV4::Object>()) {
-        model->insert(before, v, groups);
-        model->emitChanges();
+    if (v->as<QV4::Object>()) {
+        auto insertionResult = QQmlDelegateModelPrivate::InsertionResult::Retry;
+        do {
+            Compositor::insert_iterator before = index < model->m_compositor.count(group)
+                    ? model->m_compositor.findInsertPosition(group, index)
+                    : model->m_compositor.end();
+            insertionResult = model->insert(before, v, groups);
+        } while (insertionResult == QQmlDelegateModelPrivate::InsertionResult::Retry);
+        if (insertionResult == QQmlDelegateModelPrivate::InsertionResult::Success)
+            model->emitChanges();
     }
 }
 
@@ -3151,16 +3160,19 @@ void QQmlDelegateModelGroup::create(QQmlV4Function *args)
                 groups |= model->m_cacheMetaType->parseGroups(val);
             }
 
-            Compositor::insert_iterator before = index < model->m_compositor.count(group)
-                    ? model->m_compositor.findInsertPosition(group, index)
-                    : model->m_compositor.end();
+            auto insertionResult = QQmlDelegateModelPrivate::InsertionResult::Retry;
+            do {
+                Compositor::insert_iterator before = index < model->m_compositor.count(group)
+                        ? model->m_compositor.findInsertPosition(group, index)
+                        : model->m_compositor.end();
 
-            index = before.index[d->group];
-            group = d->group;
+                index = before.index[d->group];
+                group = d->group;
 
-            if (!model->insert(before, v, groups)) {
+                insertionResult = model->insert(before, v, groups);
+            } while (insertionResult == QQmlDelegateModelPrivate::InsertionResult::Retry);
+            if (insertionResult == QQmlDelegateModelPrivate::InsertionResult::Error)
                 return;
-            }
         }
     }
     if (index < 0 || index >= model->m_compositor.count(group)) {

src/qmlmodels/qqmldelegatemodel_p_p.h

@@ -301,7 +301,12 @@ public:
     void emitModelUpdated(const QQmlChangeSet &changeSet, bool reset) override;
     void delegateChanged(bool add = true, bool remove = true);
 
-    bool insert(Compositor::insert_iterator &before, const QV4::Value &object, int groups);
+    enum class InsertionResult {
+        Success,
+        Error,
+        Retry
+    };
+    InsertionResult insert(Compositor::insert_iterator &before, const QV4::Value &object, int groups);
 
     int adaptorModelCount() const;
 

tests/auto/qml/qqmldelegatemodel/data/clearCacheDuringInsertion.qml

@@ -0,0 +1,138 @@
+// Copyright (C) 2023 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+import QtQuick
+import QtQuick.Window
+import QtQuick.Controls
+import QtQuick.Layouts
+import QtQml.Models
+
+Window {
+    id: root
+    width: 640
+    height: 480
+    visible: true
+    color: "#111111"
+
+    Column {
+        spacing: 1
+        Repeater {
+            model: 1000
+
+            Rectangle {
+                width: 100
+                height: 100
+                color: "grey"
+                DelegateModel {
+                    id: delegateModel
+                    delegate: Rectangle {
+                        width: 100
+                        height: 20
+                        color: "black"
+                        Text {
+                            anchors.centerIn: parent
+                            text: "Name: " + model.name
+                            color: "white"
+                        }
+                    }
+
+                    property int length: 0
+                    property var filterAcceptsItem: function(item) { return true; }
+
+                    model: ListModel {
+                        id: myModel
+
+                        ListElement {
+                            name: "tomato"
+                            classifications: [
+                                ListElement { classification: "fruit" },
+                                ListElement { classification: "veg" }
+                            ]
+                            nutritionFacts: [
+                                ListElement {
+                                    calories:  "45"
+                                }
+                            ]
+                        }
+                    
+                        ListElement {
+                            name: "apple"
+                            classifications: [
+                                ListElement { classification: "fruit" }
+                            ]
+                            nutritionFacts: [
+                                ListElement {
+                                    calories:  "87"
+                                }
+                            ]
+                        }
+
+                        ListElement {
+                            name: "broccoli"
+                            classifications: [
+                                ListElement { classification: "veg" }
+                            ]
+                            nutritionFacts: [
+                                ListElement {
+                                    calories:  "12"
+                                }
+                            ]
+                        }
+
+                        ListElement {
+                            name: "squash"
+                            classifications: [
+                                ListElement { classification: "veg" }
+                            ]
+                            nutritionFacts: [
+                                ListElement {
+                                    calories:  "112"
+                                }
+                            ]
+                        }
+                    }
+
+                    groups: [
+                        DelegateModelGroup { id: visibleItems; name: "visible" },
+                        DelegateModelGroup { name: "veg" },
+                        DelegateModelGroup { name: "fruit"; includeByDefault: true }
+                    ]
+
+                    function update() {
+                        
+                        // Step 1: Filter items
+                        var visible = [];
+                        for (var i = 0; i < items.count; ++i) {
+                            var item = items.get(i);
+                            if (filterAcceptsItem(item.model)) {
+                                visible.push(item);
+                            }
+                        }
+
+                        // Step 2: Add all items to the visible group:
+                        for (i = 0; i < visible.length; ++i) {
+                            items.insert(visible[i], delegateModel.filterOnGroup)
+                        }
+                        delegateModel.length = visible.length
+                    }
+
+                    items.onChanged: update()
+                    onFilterAcceptsItemChanged: update()
+
+                    filterOnGroup: "visible"
+                    Component.onCompleted: {
+                        for(var i = 0; i < myModel.count; i++) {
+                            var temp = 0;
+                            var entry = myModel.get(i);
+
+                            for (var j = 0; j < entry.classifications.count; j++) {
+                                temp = entry.classifications.get(j)
+                                items.insert(entry, temp.classification)
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp

@@ -33,6 +33,7 @@ private slots:
     void deleteRace();
     void persistedItemsStayInCache();
     void doNotUnrefObjectUnderConstruction();
+    void clearCacheDuringInsertion();
 };
 
 class AbstractItemModel : public QAbstractItemModel
@@ -374,6 +375,16 @@ void tst_QQmlDelegateModel::doNotUnrefObjectUnderConstruction()
     QTRY_COMPARE(object->property("testModel").toInt(), 0);
 }
 
+void tst_QQmlDelegateModel::clearCacheDuringInsertion()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("clearCacheDuringInsertion.qml"));
+    QVERIFY2(component.isReady(), qPrintable(component.errorString()));
+    std::unique_ptr<QObject> object(component.create());
+    QVERIFY(object);
+    QTRY_COMPARE(object->property("testModel").toInt(), 0);
+}
+
 QTEST_MAIN(tst_QQmlDelegateModel)
 
 #include "tst_qqmldelegatemodel.moc"