qqmllistmodel: Fix QObjects getting garbage collected

Previously QObject's could get garbage collected while in use.
This change fixes this by making them indestructible while in use and then restoring their previous state.

[ChangeLog][QtQml][QQmlListModel][Important Behavior Changes] ListModels now take ownership of the objects added to them, only releasing them after the object has been removed again. This may break existing solutions which rely on the object not being owned by the model.

Fixes: QTBUG-91390
Change-Id: Ifd37c90e13fb0b6ad8a5a06e89f9fc9a429f6316
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit 16fc4cf366961e50b5bdf29af5c0546fe3c69495)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

3 files changed, 97 insertions, 17 deletions

diff --git a/src/qmlmodels/qqmllistmodel.cpp b/src/qmlmodels/qqmllistmodel.cpp
index 583f12164a..833f102e79 100644
--- a/src/qmlmodels/qqmllistmodel.cpp
+++ b/src/qmlmodels/qqmllistmodel.cpp
@@ -893,22 +893,24 @@ QJSValue *ListElement::getFunctionProperty(const ListLayout::Role &role)
     return f;
 }
 
-QPointer<QObject> *ListElement::getGuardProperty(const ListLayout::Role &role)
+QTaggedPointer<QObject, ListElement::ObjectIndestructible> *
+ListElement::getGuardProperty(const ListLayout::Role &role)
 {
     char *mem = getPropertyMemory(role);
 
     bool existingGuard = false;
-    for (size_t i=0 ; i < sizeof(QPointer<QObject>) ; ++i) {
+    for (size_t i = 0; i < sizeof(QTaggedPointer<QObject, ListElement::ObjectIndestructible>);
+         ++i) {
         if (mem[i] != 0) {
             existingGuard = true;
             break;
         }
     }
 
-    QPointer<QObject> *o = nullptr;
+    QTaggedPointer<QObject, ListElement::ObjectIndestructible> *o = nullptr;
 
     if (existingGuard)
-        o = reinterpret_cast<QPointer<QObject> *>(mem);
+        o = reinterpret_cast<QTaggedPointer<QObject, ListElement::ObjectIndestructible> *>(mem);
 
     return o;
 }
@@ -964,10 +966,12 @@ QVariant ListElement::getProperty(const ListLayout::Role &role, const QQmlListMo
             break;
         case ListLayout::Role::QObject:
             {
-                QPointer<QObject> *guard = reinterpret_cast<QPointer<QObject> *>(mem);
-                QObject *object = guard->data();
-                if (object)
-                    data = QVariant::fromValue(object);
+            QTaggedPointer<QObject, ListElement::ObjectIndestructible> *guard =
+                    reinterpret_cast<QTaggedPointer<QObject, ListElement::ObjectIndestructible> *>(
+                            mem);
+            QObject *object = guard->data();
+            if (object)
+                data = QVariant::fromValue(object);
             }
             break;
         case ListLayout::Role::VariantMap:
@@ -1079,15 +1083,48 @@ int ListElement::setListProperty(const ListLayout::Role &role, ListModel *m)
     return roleIndex;
 }
 
+static void
+restoreQObjectOwnership(QTaggedPointer<QObject, ListElement::ObjectIndestructible> *pointer)
+{
+    QQmlData *data = QQmlData::get(pointer->data(), 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);
+        data->explicitIndestructibleSet = (pointer->tag() & ListElement::ExplicitlySet);
+    }
+}
+
+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);
+
+    ddata->indestructible = true;
+    ddata->explicitIndestructibleSet = false;
+
+    new (mem) QTaggedPointer<QObject, ListElement::ObjectIndestructible>(
+            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);
-        QPointer<QObject> *g = reinterpret_cast<QPointer<QObject> *>(mem);
+        QTaggedPointer<QObject, ListElement::ObjectIndestructible> *g =
+                reinterpret_cast<QTaggedPointer<QObject, ListElement::ObjectIndestructible> *>(mem);
         bool existingGuard = false;
-        for (size_t i=0 ; i < sizeof(QPointer<QObject>) ; ++i) {
+        for (size_t i = 0; i < sizeof(QTaggedPointer<QObject, ListElement::ObjectIndestructible>);
+             ++i) {
             if (mem[i] != 0) {
                 existingGuard = true;
                 break;
@@ -1096,11 +1133,15 @@ int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o)
         bool changed;
         if (existingGuard) {
             changed = g->data() != o;
-            g->~QPointer();
+            if (changed)
+                restoreQObjectOwnership(g);
+            g->~QTaggedPointer();
         } else {
             changed = true;
         }
-        new (mem) QPointer<QObject>(o);
+
+        setQObjectOwnership(mem, o);
+
         if (changed)
             roleIndex = role.index;
     }
@@ -1238,7 +1279,8 @@ void ListElement::setBoolPropertyFast(const ListLayout::Role &role, bool b)
 void ListElement::setQObjectPropertyFast(const ListLayout::Role &role, QObject *o)
 {
     char *mem = getPropertyMemory(role);
-    new (mem) QPointer<QObject>(o);
+
+    setQObjectOwnership(mem, o);
 }
 
 void ListElement::setListPropertyFast(const ListLayout::Role &role, ListModel *m)
@@ -1410,9 +1452,14 @@ void ListElement::destroy(ListLayout *layout)
                     break;
                 case ListLayout::Role::QObject:
                     {
-                        QPointer<QObject> *guard = getGuardProperty(r);
-                        if (guard)
-                            guard->~QPointer();
+                    QTaggedPointer<QObject, ListElement::ObjectIndestructible> *guard =
+                            getGuardProperty(r);
+
+                    if (guard) {
+                        restoreQObjectOwnership(guard);
+
+                        guard->~QTaggedPointer();
+                    }
                     }
                     break;
                 case ListLayout::Role::VariantMap:
diff --git a/src/qmlmodels/qqmllistmodel_p_p.h b/src/qmlmodels/qqmllistmodel_p_p.h
index 4435fdfc7f..7f41960ae3 100644
--- a/src/qmlmodels/qqmllistmodel_p_p.h
+++ b/src/qmlmodels/qqmllistmodel_p_p.h
@@ -290,6 +290,8 @@ public:
         BLOCK_SIZE = 64 - sizeof(int) - sizeof(ListElement *) - sizeof(ModelNodeMetaObject *)
     };
 
+    enum ObjectIndestructible { Indestructible = 1, ExplicitlySet = 2 };
+
 private:
 
     void destroy(ListLayout *layout);
@@ -326,7 +328,7 @@ private:
     ListModel *getListProperty(const ListLayout::Role &role);
     StringOrTranslation *getStringProperty(const ListLayout::Role &role);
     QObject *getQObjectProperty(const ListLayout::Role &role);
-    QPointer<QObject> *getGuardProperty(const ListLayout::Role &role);
+    QTaggedPointer<QObject, ObjectIndestructible> *getGuardProperty(const ListLayout::Role &role);
     QVariantMap *getVariantMapProperty(const ListLayout::Role &role);
     QDateTime *getDateTimeProperty(const ListLayout::Role &role);
     QUrl *getUrlProperty(const ListLayout::Role &role);
diff --git a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
index bdefc85770..812f37e7dd 100644
--- a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
+++ b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
@@ -132,6 +132,7 @@ private slots:
     void nestedListModelIteration();
     void undefinedAppendShouldCauseError();
     void nullPropertyCrash();
+    void objectDestroyed();
 };
 
 bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object)
@@ -1757,6 +1758,36 @@ void tst_qqmllistmodel::nullPropertyCrash()
     QScopedPointer<QObject>(component.create());
 }
 
+// QTBUG-91390
+void tst_qqmllistmodel::objectDestroyed()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine);
+    component.setData(
+            R"(import QtQuick
+                   ListModel {
+                       id: model
+                       Component.onCompleted: { model.append({"a": contextObject}); }
+                   })",
+            QUrl());
+
+    QObject *obj = new QObject;
+    bool destroyed = false;
+    connect(obj, &QObject::destroyed, [&]() { destroyed = true; });
+
+    engine.rootContext()->setContextProperty(u"contextObject"_qs, obj);
+    engine.setObjectOwnership(obj, QJSEngine::JavaScriptOwnership);
+
+    QScopedPointer<QObject>(component.create());
+    QVERIFY(!destroyed);
+    engine.collectGarbage();
+    QTest::qSleep(250);
+    QVERIFY(!destroyed);
+    engine.evaluate(u"model.clear();"_qs);
+    engine.collectGarbage();
+    QTRY_VERIFY(destroyed);
+}
+
 QTEST_MAIN(tst_qqmllistmodel)
 
 #include "tst_qqmllistmodel.moc"