ListModel: Use PersistentValue to keep track of objects

This is the simplest way to prevent the garbage collector from
destroying the object while at the same time allowing a manual
destruction.

Task-number: QTBUG-95895
Task-number: QTBUG-96167
Fixes: QTBUG-91390
Change-Id: Ic3f3146bc555991068ce3367971e050f745d235d
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
(cherry picked from commit 718f3469f6)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/qmlmodels/qqmllistmodel.cpp

@@ -98,7 +98,7 @@ const ListLayout::Role &ListLayout::createRole(const QString &key, ListLayout::R
         sizeof(double),
         sizeof(bool),
         sizeof(ListModel *),
-        sizeof(ListElement::GuardedQObjectPointer),
+        sizeof(QV4::PersistentValue),
         sizeof(QVariantMap),
         sizeof(QDateTime),
         sizeof(QUrl),
@@ -618,10 +618,9 @@ void ListModel::set(int elementIndex, QV4::Object *object, QVector<int> *roles)
             roleIndex = e->setFunctionProperty(r, jsv);
         } else if (QV4::Object *o = propertyValue->as<QV4::Object>()) {
             if (QV4::QObjectWrapper *wrapper = o->as<QV4::QObjectWrapper>()) {
-                QObject *o = wrapper->object();
                 const ListLayout::Role &role = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::QObject);
                 if (role.type == ListLayout::Role::QObject)
-                    roleIndex = e->setQObjectProperty(role, o);
+                    roleIndex = e->setQObjectProperty(role, wrapper);
             } else if (QVariant maybeUrl = QV4::ExecutionEngine::toVariant(
                            o->asReturnedValue(), QMetaType::fromType<QUrl>(), true);
                        maybeUrl.metaType() == QMetaType::fromType<QUrl>()) {
@@ -710,10 +709,9 @@ void ListModel::set(int elementIndex, QV4::Object *object, ListModel::SetElement
             }
         } else if (QV4::Object *o = propertyValue->as<QV4::Object>()) {
             if (QV4::QObjectWrapper *wrapper = o->as<QV4::QObjectWrapper>()) {
-                QObject *o = wrapper->object();
                 const ListLayout::Role &r = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::QObject);
                 if (r.type == ListLayout::Role::QObject)
-                    e->setQObjectPropertyFast(r, o);
+                    e->setQObjectPropertyFast(r, wrapper);
             } else {
                 QVariant maybeUrl = QV4::ExecutionEngine::toVariant(
                             o->asReturnedValue(), QMetaType::fromType<QUrl>(), true);
@@ -838,12 +836,11 @@ StringOrTranslation *ListElement::getStringProperty(const ListLayout::Role &role
     return s;
 }
 
-QObject *ListElement::getQObjectProperty(const ListLayout::Role &role)
+QV4::QObjectWrapper *ListElement::getQObjectProperty(const ListLayout::Role &role)
 {
     char *mem = getPropertyMemory(role);
-    GuardedQObjectPointer *o
+    QV4::PersistentValue *g = reinterpret_cast<QV4::PersistentValue *>(mem);
-            = reinterpret_cast<GuardedQObjectPointer *>(mem);
+    return g->as<QV4::QObjectWrapper>();
-    return o->data();
 }
 
 QVariantMap *ListElement::getVariantMapProperty(const ListLayout::Role &role)
@@ -890,13 +887,13 @@ QJSValue *ListElement::getFunctionProperty(const ListLayout::Role &role)
     return f;
 }
 
-ListElement::GuardedQObjectPointer *
+QV4::PersistentValue *
 ListElement::getGuardProperty(const ListLayout::Role &role)
 {
     char *mem = getPropertyMemory(role);
 
     bool existingGuard = false;
-    for (size_t i = 0; i < sizeof(GuardedQObjectPointer);
+    for (size_t i = 0; i < sizeof(QV4::PersistentValue);
          ++i) {
         if (mem[i] != 0) {
             existingGuard = true;
@@ -904,12 +901,12 @@ ListElement::getGuardProperty(const ListLayout::Role &role)
         }
     }
 
-    GuardedQObjectPointer *o = nullptr;
+    QV4::PersistentValue *g = nullptr;
 
     if (existingGuard)
-        o = reinterpret_cast<GuardedQObjectPointer *>(mem);
+        g = reinterpret_cast<QV4::PersistentValue *>(mem);
 
-    return o;
+    return g;
 }
 
 ListModel *ListElement::getListProperty(const ListLayout::Role &role)
@@ -965,12 +962,8 @@ QVariant ListElement::getProperty(const ListLayout::Role &role, const QQmlListMo
             break;
         case ListLayout::Role::QObject:
             {
-            GuardedQObjectPointer *guard =
+                QV4::PersistentValue *guard = reinterpret_cast<QV4::PersistentValue *>(mem);
-                    reinterpret_cast<GuardedQObjectPointer *>(
+                data = QVariant::fromValue(guard->as<QV4::QObjectWrapper>()->object());
-                            mem);
-            QObject *object = guard->data();
-            if (object)
-                data = QVariant::fromValue(object);
             }
             break;
         case ListLayout::Role::VariantMap:
@@ -1082,67 +1075,17 @@ int ListElement::setListProperty(const ListLayout::Role &role, ListModel *m)
     return roleIndex;
 }
 
-static void
+int ListElement::setQObjectProperty(const ListLayout::Role &role, QV4::QObjectWrapper *o)
-restoreQObjectOwnership(ListElement::GuardedQObjectPointer *pointer)
-{
-    if (QObject *o = pointer->data()) {
-        QQmlData *data = QQmlData::get(o, false);
-        Q_ASSERT(data);
-
-        // Only restore the previous state if the object hasn't become explicitly
-        // owned
-        if (!data->explicitIndestructibleSet)
-            data->indestructible = (pointer->tag() & ListElement::Indestructible);
-    }
-}
-
-static void setQObjectOwnership(char *mem, QObject *o)
-{
-    QQmlData *ddata = QQmlData::get(o, false);
-    const int ownership = (!ddata || ddata->indestructible ? ListElement::Indestructible : 0)
-            | (ddata && ddata->explicitIndestructibleSet ? ListElement::ExplicitlySet : 0);
-
-    // If ddata didn't exist above, force its creation now
-    if (!ddata)
-        ddata = QQmlData::get(o, true);
-
-    if (!ddata->explicitIndestructibleSet)
-        ddata->indestructible = ownership != 0;
-
-    new (mem) ListElement::GuardedQObjectPointer(
-            o, static_cast<ListElement::ObjectIndestructible>(ownership));
-}
-
-int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o)
 {
     int roleIndex = -1;
 
     if (role.type == ListLayout::Role::QObject) {
         char *mem = getPropertyMemory(role);
-        GuardedQObjectPointer *g =
+        if (isMemoryUsed<QVariantMap>(mem))
-                reinterpret_cast<GuardedQObjectPointer *>(mem);
+            reinterpret_cast<QV4::PersistentValue *>(mem)->set(o->engine(), *o);
-        bool existingGuard = false;
+        else
-        for (size_t i = 0; i < sizeof(GuardedQObjectPointer);
+            new (mem) QV4::PersistentValue(o->engine(), o);
-             ++i) {
+        roleIndex = role.index;
-            if (mem[i] != 0) {
-                existingGuard = true;
-                break;
-            }
-        }
-        bool changed;
-        if (existingGuard) {
-            changed = g->data() != o;
-            if (changed)
-                restoreQObjectOwnership(g);
-            g->~GuardedQObjectPointer();
-        } else {
-            changed = true;
-        }
-
-        setQObjectOwnership(mem, o);
-
-        if (changed)
-            roleIndex = role.index;
     }
 
     return roleIndex;
@@ -1275,11 +1218,10 @@ void ListElement::setBoolPropertyFast(const ListLayout::Role &role, bool b)
     *value = b;
 }
 
-void ListElement::setQObjectPropertyFast(const ListLayout::Role &role, QObject *o)
+void ListElement::setQObjectPropertyFast(const ListLayout::Role &role, QV4::QObjectWrapper *o)
 {
     char *mem = getPropertyMemory(role);
-
+    new (mem) QV4::PersistentValue(o->engine(), o);
-    setQObjectOwnership(mem, o);
 }
 
 void ListElement::setListPropertyFast(const ListLayout::Role &role, ListModel *m)
@@ -1396,7 +1338,7 @@ QVector<int> ListElement::sync(ListElement *src, ListLayout *srcLayout, ListElem
                 break;
             case ListLayout::Role::QObject:
                 {
-                    QObject *object = src->getQObjectProperty(srcRole);
+                    QV4::QObjectWrapper *object = src->getQObjectProperty(srcRole);
                     roleIndex = target->setQObjectProperty(targetRole, object);
                 }
                 break;
@@ -1451,14 +1393,8 @@ void ListElement::destroy(ListLayout *layout)
                     break;
                 case ListLayout::Role::QObject:
                     {
-                    GuardedQObjectPointer *guard =
+                        if (QV4::PersistentValue *guard = getGuardProperty(r))
-                            getGuardProperty(r);
+                            guard->~PersistentValue();
-
-                    if (guard) {
-                        restoreQObjectOwnership(guard);
-
-                        guard->~GuardedQObjectPointer();
-                    }
                     }
                     break;
                 case ListLayout::Role::VariantMap:
@@ -1595,8 +1531,7 @@ int ListElement::setJsProperty(const ListLayout::Role &role, const QV4::Value &d
         QV4::ScopedObject o(scope, d);
         QV4::QObjectWrapper *wrapper = o->as<QV4::QObjectWrapper>();
         if (role.type == ListLayout::Role::QObject && wrapper) {
-            QObject *o = wrapper->object();
+            roleIndex = setQObjectProperty(role, wrapper);
-            roleIndex = setQObjectProperty(role, o);
         } else if (role.type == ListLayout::Role::VariantMap) {
             roleIndex = setVariantMapProperty(role, o);
         } else if (role.type == ListLayout::Role::Url) {

src/qmlmodels/qqmllistmodel_p_p.h

@@ -245,43 +245,6 @@ public:
     enum ObjectIndestructible { Indestructible = 1, ExplicitlySet = 2 };
     enum { BLOCK_SIZE = 64 - sizeof(int) - sizeof(ListElement *) - sizeof(ModelNodeMetaObject *) };
 
-    // This is a basic guarded QObject pointer, with tag. It cannot be copied or moved.
-    class GuardedQObjectPointer
-    {
-        Q_DISABLE_COPY_MOVE(GuardedQObjectPointer)
-
-        using RefCountData = QtSharedPointer::ExternalRefCountData;
-        using Storage = QTaggedPointer<QObject, ObjectIndestructible>;
-
-    public:
-        GuardedQObjectPointer(QObject *o, ObjectIndestructible ownership)
-            : storage(o, ownership)
-            , refCount(o ? RefCountData::getAndRef(o) : nullptr)
-        {}
-
-        ~GuardedQObjectPointer()
-        {
-            if (refCount && !refCount->weakref.deref())
-                delete refCount;
-        }
-
-        QObject *data() const
-        {
-            return (refCount == nullptr || refCount->strongref.loadRelaxed() == 0)
-                    ? nullptr
-                    : storage.data();
-        }
-
-        ObjectIndestructible tag() const
-        {
-            return storage.tag();
-        }
-
-    private:
-        Storage storage;
-        RefCountData *refCount = nullptr;
-    };
-
     ListElement();
     ListElement(int existingUid);
     ~ListElement();
@@ -300,7 +263,7 @@ private:
     int setDoubleProperty(const ListLayout::Role &role, double n);
     int setBoolProperty(const ListLayout::Role &role, bool b);
     int setListProperty(const ListLayout::Role &role, ListModel *m);
-    int setQObjectProperty(const ListLayout::Role &role, QObject *o);
+    int setQObjectProperty(const ListLayout::Role &role, QV4::QObjectWrapper *o);
     int setVariantMapProperty(const ListLayout::Role &role, QV4::Object *o);
     int setVariantMapProperty(const ListLayout::Role &role, QVariantMap *m);
     int setDateTimeProperty(const ListLayout::Role &role, const QDateTime &dt);
@@ -311,7 +274,7 @@ private:
     void setStringPropertyFast(const ListLayout::Role &role, const QString &s);
     void setDoublePropertyFast(const ListLayout::Role &role, double n);
     void setBoolPropertyFast(const ListLayout::Role &role, bool b);
-    void setQObjectPropertyFast(const ListLayout::Role &role, QObject *o);
+    void setQObjectPropertyFast(const ListLayout::Role &role, QV4::QObjectWrapper *o);
     void setListPropertyFast(const ListLayout::Role &role, ListModel *m);
     void setVariantMapFast(const ListLayout::Role &role, QV4::Object *o);
     void setDateTimePropertyFast(const ListLayout::Role &role, const QDateTime &dt);
@@ -323,8 +286,8 @@ private:
     QVariant getProperty(const ListLayout::Role &role, const QQmlListModel *owner, QV4::ExecutionEngine *eng);
     ListModel *getListProperty(const ListLayout::Role &role);
     StringOrTranslation *getStringProperty(const ListLayout::Role &role);
-    QObject *getQObjectProperty(const ListLayout::Role &role);
+    QV4::QObjectWrapper *getQObjectProperty(const ListLayout::Role &role);
-    GuardedQObjectPointer *getGuardProperty(const ListLayout::Role &role);
+    QV4::PersistentValue *getGuardProperty(const ListLayout::Role &role);
     QVariantMap *getVariantMapProperty(const ListLayout::Role &role);
     QDateTime *getDateTimeProperty(const ListLayout::Role &role);
     QUrl *getUrlProperty(const ListLayout::Role &role);

tests/auto/qml/qqmllistmodel/data/protectQObjectFromGC.qml

@@ -0,0 +1,21 @@
+import QtQml
+import QtQml.Models
+
+ListModel {
+    id: filesModel
+    property Component testComponent: Component {
+        id: testComponent
+        QtObject {
+            required property string name
+        }
+    }
+    Component.onCompleted: {
+        filesModel.clear()
+        for(let i = 0; i < 10; i++) {
+            filesModel.append({
+                path: testComponent.createObject(null, { name: "" + i })
+            })
+        }
+        gc()
+    }
+}

tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp

@@ -118,6 +118,7 @@ private slots:
     void destroyComponentObject();
     void objectOwnershipFlip();
     void enumsInListElement();
+    void protectQObjectFromGC();
 };
 
 bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object)
@@ -1855,7 +1856,7 @@ void tst_qqmllistmodel::destroyComponentObject()
                                Q_RETURN_ARG(QVariant, retVal));
     QVERIFY(retVal.toBool());
     QTRY_VERIFY(created.isNull());
-    QTRY_VERIFY(list->get(0).property("obj").isUndefined());
+    QTRY_VERIFY(list->get(0).property("obj").isNull());
     QCOMPARE(list->count(), 1);
 }
 
@@ -1909,6 +1910,25 @@ void tst_qqmllistmodel::enumsInListElement()
     }
 }
 
+void tst_qqmllistmodel::protectQObjectFromGC()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("protectQObjectFromGC.qml"));
+    QVERIFY2(component.isReady(), qPrintable(component.errorString()));
+    QScopedPointer<QObject> root(component.create());
+    QVERIFY(!root.isNull());
+
+    QQmlListModel *listModel = qobject_cast<QQmlListModel *>(root.data());
+    QVERIFY(listModel);
+    QCOMPARE(listModel->count(), 10);
+
+    for (int i = 0; i < 10; ++i) {
+        QObject *element = qjsvalue_cast<QObject *>(listModel->get(i).property("path"));
+        QVERIFY(element);
+        QCOMPARE(element->property("name").toString(), QString::number(i));
+    }
+}
+
 QTEST_MAIN(tst_qqmllistmodel)
 
 #include "tst_qqmllistmodel.moc"