QmlModels: Split QQmlDelegateModelItem::m_objectRef

We need strong references for the cases where we place guards on the stack to prevent the deletion of the objects, and weak references we hand out to the view. The views don't systematically track the objects they hold and generally use a QPointer as second line of defence. In certain cases we cannot avoid deleting objects that are in fact still referenced as the clearObjectWeakReferences() call shows. If the QQmlDelegateModel itself is deleted while some view still holds a weak reference to an object, we cannot avoid deleting that object, no matter if the view agrees. Also, we can do with 16bit numbers for the references. The guards on the stack are rather few and you'll overflow the stack before you hit the 16bit maximum. The references handed out to the views are generally one or two per item. Task-number: QTBUG-141963 Change-Id: Ifb79e993abcaf4c169b08641ebc2a6378e0a4776 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
author: Ulf Hermann <ulf.hermann@qt.io> 2025-11-18 10:53:09 +0100
committer: Ulf Hermann <ulf.hermann@qt.io> 2025-11-28 15:41:49 +0100
commit: e4db77906b3f8bdcbe2ee20ff62ad818c6817e24 (patch)
tree: e924c17f3aa8e9fa07c168e50553e013740c0848 /src
parent: 985ff60882d8c1e461f387464ac3c29083088750 (diff)
3 files changed, 81 insertions, 50 deletions
diff --git a/src/qmlmodels/qqmldelegatemodel.cpp b/src/qmlmodels/qqmldelegatemodel.cpp
index 61171ba8a9..49a05fe304 100644
--- a/src/qmlmodels/qqmldelegatemodel.cpp
+++ b/src/qmlmodels/qqmldelegatemodel.cpp
@@ -236,7 +236,7 @@ QQmlDelegateModel::~QQmlDelegateModel()
     for (QQmlDelegateModelItem *cacheItem : std::as_const(d->m_cache)) {
         cacheItem->destroyObject();
         cacheItem->removeGroups(Compositor::UnresolvedFlag);
-        cacheItem->clearObjectReferences();
+        cacheItem->clearObjectWeakReferences();
 
         if (QQDMIncubationTask *incubationTask = cacheItem->incubationTask()) {
             d->releaseIncubator(incubationTask);
@@ -621,7 +621,7 @@ QQmlDelegateModel::ReleaseFlags QQmlDelegateModelPrivate::release(QObject *objec
     if (!cacheItem)
         return QQmlDelegateModel::ReleaseFlags{};
 
-    if (!cacheItem->releaseObject())
+    if (!cacheItem->releaseObjectWeak())
         return QQmlDelegateModel::Referenced;
 
     if (reusableFlag == QQmlInstanceModel::Reusable && m_reusableItemsPool.insertItem(cacheItem)) {
@@ -1275,11 +1275,10 @@ QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, QQ
                 // has already been incubated, otherwise it wouldn't be in the pool).
                 addCacheItem(cacheItem, it);
                 reuseItem(cacheItem, index, flags);
-                cacheItem->referenceObject();
 
                 if (index == m_compositor.count(group) - 1)
                     requestMoreIfNecessary();
-                return cacheItem->object();
+                return cacheItem->referenceObjectWeak();
             }
 
             // Since we could't find an available item in the pool, we create a new one
@@ -1298,7 +1297,7 @@ QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, QQ
         // Bump the reference counts temporarily so neither the content data or the delegate object
         // are deleted if incubatorStatusChanged() is called synchronously.
         QQmlDelegateModelItem::ScriptReference scriptRef(cacheItem);
-        cacheItem->referenceObject();
+        QQmlDelegateModelItem::ObjectReference objectRef(cacheItem);
 
         if (QQDMIncubationTask *incubationTask = cacheItem->incubationTask()) {
             bool sync = (incubationMode == QQmlIncubator::Synchronous
@@ -1356,18 +1355,14 @@ QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, QQ
 
         if (index == m_compositor.count(group) - 1)
             requestMoreIfNecessary();
-    }
 
-    if (QObject *object = cacheItem->object()) {
-        QQDMIncubationTask *incubationTask = cacheItem->incubationTask();
-        if (!incubationTask || isDoneIncubating(incubationTask->status()))
-            return object;
-    }
 
-    // TODO: We may not be able to releaseObject() here because incubateObject() above may have
-    //       released as a side effect. This needs cleanup.
-    if (cacheItem->objectRef() > 0)
-        cacheItem->releaseObject();
+        if (cacheItem->object()) {
+            QQDMIncubationTask *incubationTask = cacheItem->incubationTask();
+            if (!incubationTask || isDoneIncubating(incubationTask->status()))
+                return cacheItem->referenceObjectWeak();
+        }
+    }
 
     if (!cacheItem->isScriptReferenced()) {
 
@@ -1744,7 +1739,9 @@ void QQmlDelegateModelPrivate::itemsRemoved(
         } else {
             for (; cacheIndex < remove.cacheIndex() + remove.count - removedCache; ++cacheIndex) {
                 QQmlDelegateModelItem *cacheItem = m_cache.at(cacheIndex);
-                if (remove.inGroup(Compositor::Persisted) && cacheItem->objectRef() == 0
+                if (remove.inGroup(Compositor::Persisted)
+                        && cacheItem->objectStrongRef() == 0
+                        && cacheItem->objectWeakRef() == 0
                         && cacheItem->object()) {
                     QObject *object = cacheItem->object();
                     cacheItem->destroyObjectLater();
@@ -2524,7 +2521,7 @@ QQmlDelegateModelItem::QQmlDelegateModelItem(
 QQmlDelegateModelItem::~QQmlDelegateModelItem()
 {
     Q_ASSERT(m_scriptRef == 0);
-    Q_ASSERT(m_objectRef == 0);
+    Q_ASSERT(m_objectStrongRef == 0);
     Q_ASSERT(!m_object);
 
     if (m_incubationTask) {
@@ -3351,7 +3348,7 @@ void QQmlDelegateModelGroup::create(QQmlV4FunctionPtr args)
         Compositor::iterator it = model->m_compositor.find(group, index);
         model->m_compositor.setFlags(it, 1, d->group, Compositor::PersistedFlag, &inserts);
         model->itemsInserted(inserts);
-        model->m_cache.at(it.cacheIndex())->releaseObject();
+        model->m_cache.at(it.cacheIndex())->releaseObjectWeak();
     }
 
     args->setReturnValue(QV4::QObjectWrapper::wrap(args->v4engine(), object));
diff --git a/src/qmlmodels/qqmldelegatemodel_p_p.h b/src/qmlmodels/qqmldelegatemodel_p_p.h
index 579064f6d9..89476875a5 100644
--- a/src/qmlmodels/qqmldelegatemodel_p_p.h
+++ b/src/qmlmodels/qqmldelegatemodel_p_p.h
@@ -98,12 +98,13 @@ public:
 
         ObjectReference(QQmlDelegateModelItem *item) : item(item)
         {
-            item->referenceObject();
+            ++item->m_objectStrongRef;
         }
 
         ~ObjectReference()
         {
-            item->releaseObject();
+            Q_ASSERT(item->m_objectStrongRef > 0);
+            --item->m_objectStrongRef;
         }
     private:
         QQmlDelegateModelItem *item = nullptr;
@@ -116,13 +117,15 @@ public:
         ObjectSpanReference(QSpan<QQmlDelegateModelItem *const> span) : items(std::move(span))
         {
             for (QQmlDelegateModelItem *item : items)
-                item->referenceObject();
+                ++item->m_objectStrongRef;
         }
 
         ~ObjectSpanReference()
         {
-            for (QQmlDelegateModelItem *item : items)
-                item->releaseObject();
+            for (QQmlDelegateModelItem *item : items) {
+                Q_ASSERT(item->m_objectStrongRef > 0);
+                --item->m_objectStrongRef;
+            }
         }
 
     private:
@@ -146,20 +149,30 @@ public:
                           int row, int column);
     ~QQmlDelegateModelItem();
 
-    void referenceObject() { ++m_objectRef; }
-    bool releaseObject()
+    [[nodiscard]] QObject *referenceObjectWeak()
+    {
+        Q_ASSERT(m_object);
+        ++m_objectWeakRef;
+        return m_object;
+    }
+
+    bool releaseObjectWeak()
     {
-        Q_ASSERT(m_objectRef > 0);
-        return --m_objectRef == 0 && !(m_groups & Compositor::PersistedFlag);
+        if (m_objectWeakRef > 0)
+            --m_objectWeakRef;
+        return m_objectWeakRef == 0 && !(m_groups & Compositor::PersistedFlag);
     }
-    void clearObjectReferences()
+    void clearObjectWeakReferences()
     {
-        // TODO: This can be OK if we regard object references as merely advisory.
-        //       We need to remove all the Q_ASSERTs about them then.
-        m_objectRef = 0;
+        m_objectWeakRef = 0;
     }
 
-    bool isObjectReferenced() const { return m_objectRef != 0 || (m_groups & Compositor::PersistedFlag); }
+    bool isObjectReferenced() const
+    {
+        return m_objectWeakRef != 0
+                || m_objectStrongRef != 0
+                || (m_groups & Compositor::PersistedFlag);
+    }
     void childContextObjectDestroyed(QObject *childContextObject);
 
     bool isScriptReferenced() const {
@@ -231,8 +244,9 @@ public:
         m_incubationTask = incubationTask;
     }
 
-    int objectRef() const { return m_objectRef; }
-    int scriptRef() const { return m_scriptRef; }
+    quint16 objectStrongRef() const { return m_objectStrongRef; }
+    quint16 objectWeakRef() const { return m_objectWeakRef; }
+    quint16 scriptRef() const { return m_scriptRef; }
 
     QObject *object() const { return m_object; }
     void setObject(QObject *object) {
@@ -278,12 +292,35 @@ private:
     QPointer<QObject> m_object;
     QQDMIncubationTask *m_incubationTask = nullptr;
     QQmlComponent *m_delegate = nullptr;
-    int m_objectRef = 0;
-    int m_scriptRef = 0;
     int m_groups = 0;
     int m_index = -1;
     int m_row = -1;
     int m_column = -1;
+
+    // A strong reference to the object prevents the deletion of the object. We use them as
+    // temporary guards with ObjectReference or ObjectSpanReference to make sure the objects we're
+    // currently manipulating don't disappear.
+    quint16 m_objectStrongRef = 0;
+
+    // A weak reference to the object is added when handing the object out to the view via object().
+    // It is dropped when the view calls release(). Weak references are advisory. We try not to
+    // delete an object while weak references are still in place. However, during destruction, the
+    // weak refernces may be ignored. Views are expected to use QPointer or similar guards in
+    // addition.
+    quint16 m_objectWeakRef = 0;
+
+    // A script reference is a strong reference to the QQmlDelegateModelItem itself. We don't delete
+    // the QQmlDelegateModelItem while script references are still in place. We can use them as
+    // temporary guards to hold on to an item while working on it, or we can attach them to
+    // Heap::QQmlDelegateModelItemObject in order to extend the life time of a QQmlDelegateModelItem
+    // to the life time of its heap object.
+    // The latter case is particular in that it contradicts our usual mechanism of only holding weak
+    // references to QObjects in e.g. QV4::QObjectWrapper. The trick here is that the actual
+    // QQmlDelegateModelItem is never exposed to the user, so that the user has no chance of
+    // manually deleting it.
+    // TODO: This is brittle.
+    quint16 m_scriptRef = 0;
+
     bool m_useStructuredModelData = true;
 };
 
diff --git a/src/qmlmodels/qqmltableinstancemodel.cpp b/src/qmlmodels/qqmltableinstancemodel.cpp
index 5cd5d4be15..7c38ff4116 100644
--- a/src/qmlmodels/qqmltableinstancemodel.cpp
+++ b/src/qmlmodels/qqmltableinstancemodel.cpp
@@ -59,7 +59,8 @@ QQmlTableInstanceModel::~QQmlTableInstanceModel()
         // No item in m_modelItems should be referenced at this point. The view
         // should release all its items before it deletes this model. Only model items
         // that are still being incubated should be left for us to delete.
-        Q_ASSERT(modelItem->objectRef() == 0);
+        // We can't rely on that, though. So we only check the strong ref.
+        Q_ASSERT(modelItem->objectStrongRef() == 0);
         Q_ASSERT(modelItem->incubationTask());
         // Check that we are not being deleted while we're
         // in the process of e.g emitting a created signal.
@@ -130,12 +131,10 @@ QObject *QQmlTableInstanceModel::object(int index, QQmlIncubator::IncubationMode
     if (!modelItem)
         return nullptr;
 
-    if (QObject *object = modelItem->object()) {
-        // The model item has already been incubated. So
-        // just bump the ref-count and return it.
-        modelItem->referenceObject();
-        return object;
-    }
+    // The model item has already been incubated. So
+    // just bump the ref-count and return it.
+    if (modelItem->object())
+        return modelItem->referenceObjectWeak();
 
     // The object is not ready, and needs to be incubated
     incubateModelItem(modelItem, incubationMode);
@@ -145,11 +144,9 @@ QObject *QQmlTableInstanceModel::object(int index, QQmlIncubator::IncubationMode
     // Incubation is done, so the task should be removed
     Q_ASSERT(!modelItem->incubationTask());
 
-    if (QObject *object = modelItem->object()) {
-        // Incubation was completed sync and successful
-        modelItem->referenceObject();
-        return object;
-    }
+    // Incubation was completed sync and successful
+    if (modelItem->object())
+        return modelItem->referenceObjectWeak();
 
     // The object was incubated synchronously (otherwise we would return above). But since
     // we have no object, the incubation must have failed. And when we have no object, there
@@ -171,7 +168,7 @@ QQmlInstanceModel::ReleaseFlags QQmlTableInstanceModel::release(QObject *object,
     Q_ASSERT(m_modelItems.contains(modelItem->modelIndex()));
     Q_ASSERT(m_modelItems[modelItem->modelIndex()]->object() == object);
 
-    if (!modelItem->releaseObject())
+    if (!modelItem->releaseObjectWeak())
         return QQmlDelegateModel::Referenced;
 
     if (modelItem->isScriptReferenced()) {
@@ -213,7 +210,7 @@ void QQmlTableInstanceModel::dispose(QObject *object)
     auto modelItem = qvariant_cast<QQmlDelegateModelItem *>(object->property(kModelItemTag));
     Q_ASSERT(modelItem);
 
-    modelItem->releaseObject();
+    modelItem->releaseObjectWeak();
 
     // The item is not referenced by anyone
     Q_ASSERT(!modelItem->isObjectReferenced());
author	Ulf Hermann <ulf.hermann@qt.io>	2025-11-18 10:53:09 +0100
committer	Ulf Hermann <ulf.hermann@qt.io>	2025-11-28 15:41:49 +0100
commit	e4db77906b3f8bdcbe2ee20ff62ad818c6817e24 (patch)
tree	e924c17f3aa8e9fa07c168e50553e013740c0848 /src
parent	985ff60882d8c1e461f387464ac3c29083088750 (diff)