From 80fd26ff2f5dd1e3d06ba3e4f250ca6b02a62d41 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Fri, 17 Nov 2023 15:16:41 +0100 Subject: [PATCH] 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 Reviewed-by: Qt CI Bot (cherry picked from commit 4c1cee7e116012ed34ec1e76331736b89f1bb374) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 9036e492ddd96565767679bf5ea30717b7e39331) --- src/qmlmodels/qqmldelegatemodel.cpp | 54 ++++--- src/qmlmodels/qqmldelegatemodel_p_p.h | 7 +- .../data/clearCacheDuringInsertion.qml | 138 ++++++++++++++++++ .../tst_qqmldelegatemodel.cpp | 11 ++ 4 files changed, 188 insertions(+), 22 deletions(-) create mode 100644 tests/auto/qml/qqmldelegatemodel/data/clearCacheDuringInsertion.qml diff --git a/src/qmlmodels/qqmldelegatemodel.cpp b/src/qmlmodels/qqmldelegatemodel.cpp index 6a763431a9..7af77969bd 100644 --- a/src/qmlmodels/qqmldelegatemodel.cpp +++ b/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()->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()) + 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()) { - return; - } else if (v->as()) { - model->insert(before, v, groups); - model->emitChanges(); + if (v->as()) { + 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)) { diff --git a/src/qmlmodels/qqmldelegatemodel_p_p.h b/src/qmlmodels/qqmldelegatemodel_p_p.h index 0252f1c6d4..cfff082d2c 100644 --- a/src/qmlmodels/qqmldelegatemodel_p_p.h +++ b/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; diff --git a/tests/auto/qml/qqmldelegatemodel/data/clearCacheDuringInsertion.qml b/tests/auto/qml/qqmldelegatemodel/data/clearCacheDuringInsertion.qml new file mode 100644 index 0000000000..da69e55a17 --- /dev/null +++ b/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) + } + } + } + } + } + } + } +} diff --git a/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp b/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp index 7b20449227..1f85f68891 100644 --- a/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp +++ b/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 object(component.create()); + QVERIFY(object); + QTRY_COMPARE(object->property("testModel").toInt(), 0); +} + QTEST_MAIN(tst_QQmlDelegateModel) #include "tst_qqmldelegatemodel.moc"