QQmlListModel: Guard QObject pointers in the list

If a QObject in the list is deleted from elsewhere, we need to zero it.

Amends commit 3edac1d0a70961e6f86564cc0a98339df6ac5964.

Fixes: QTBUG-94833
Change-Id: Ic33a5a0baa160824a34f353806dbfd876fa7123a
Reviewed-by: Maximilian Goldstein <max.goldstein@qt.io>
Reviewed-by: Paul Olav Tvete <paul.tvete@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
(cherry picked from commit b2ee7364be2f11dc16b80a85141cfdd54e801b10)
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

3 files changed, 93 insertions, 31 deletions

diff --git a/src/qmlmodels/qqmllistmodel.cpp b/src/qmlmodels/qqmllistmodel.cpp
index 833f102e79..d983d7f88e 100644
--- a/src/qmlmodels/qqmllistmodel.cpp
+++ b/src/qmlmodels/qqmllistmodel.cpp
@@ -129,7 +129,7 @@ const ListLayout::Role &ListLayout::getRoleOrCreate(QV4::String *key, Role::Data
 
 const ListLayout::Role &ListLayout::createRole(const QString &key, ListLayout::Role::DataType type)
 {
-    const int dataSizes[] = { sizeof(StringOrTranslation), sizeof(double), sizeof(bool), sizeof(ListModel *), sizeof(QPointer<QObject>), sizeof(QVariantMap), sizeof(QDateTime), sizeof(QUrl), sizeof(QJSValue) };
+    const int dataSizes[] = { sizeof(StringOrTranslation), sizeof(double), sizeof(bool), sizeof(ListModel *), sizeof(ListElement::GuardedQObjectPointer), sizeof(QVariantMap), sizeof(QDateTime), sizeof(QUrl), sizeof(QJSValue) };
     const int dataAlignments[] = { alignof(StringOrTranslation), alignof(double), alignof(bool), alignof(ListModel *), alignof(QObject *), alignof(QVariantMap), alignof(QDateTime), alignof(QUrl), alignof(QJSValue) };
 
     Role *r = new Role;
@@ -845,7 +845,8 @@ StringOrTranslation *ListElement::getStringProperty(const ListLayout::Role &role
 QObject *ListElement::getQObjectProperty(const ListLayout::Role &role)
 {
     char *mem = getPropertyMemory(role);
-    QPointer<QObject> *o = reinterpret_cast<QPointer<QObject> *>(mem);
+    GuardedQObjectPointer *o
+            = reinterpret_cast<GuardedQObjectPointer *>(mem);
     return o->data();
 }
 
@@ -893,13 +894,13 @@ QJSValue *ListElement::getFunctionProperty(const ListLayout::Role &role)
     return f;
 }
 
-QTaggedPointer<QObject, ListElement::ObjectIndestructible> *
+ListElement::GuardedQObjectPointer *
 ListElement::getGuardProperty(const ListLayout::Role &role)
 {
     char *mem = getPropertyMemory(role);
 
     bool existingGuard = false;
-    for (size_t i = 0; i < sizeof(QTaggedPointer<QObject, ListElement::ObjectIndestructible>);
+    for (size_t i = 0; i < sizeof(GuardedQObjectPointer);
          ++i) {
         if (mem[i] != 0) {
             existingGuard = true;
@@ -907,10 +908,10 @@ ListElement::getGuardProperty(const ListLayout::Role &role)
         }
     }
 
-    QTaggedPointer<QObject, ListElement::ObjectIndestructible> *o = nullptr;
+    GuardedQObjectPointer *o = nullptr;
 
     if (existingGuard)
-        o = reinterpret_cast<QTaggedPointer<QObject, ListElement::ObjectIndestructible> *>(mem);
+        o = reinterpret_cast<GuardedQObjectPointer *>(mem);
 
     return o;
 }
@@ -966,8 +967,8 @@ QVariant ListElement::getProperty(const ListLayout::Role &role, const QQmlListMo
             break;
         case ListLayout::Role::QObject:
             {
-            QTaggedPointer<QObject, ListElement::ObjectIndestructible> *guard =
-                    reinterpret_cast<QTaggedPointer<QObject, ListElement::ObjectIndestructible> *>(
+            GuardedQObjectPointer *guard =
+                    reinterpret_cast<GuardedQObjectPointer *>(
                             mem);
             QObject *object = guard->data();
             if (object)
@@ -1084,16 +1085,18 @@ int ListElement::setListProperty(const ListLayout::Role &role, ListModel *m)
 }
 
 static void
-restoreQObjectOwnership(QTaggedPointer<QObject, ListElement::ObjectIndestructible> *pointer)
+restoreQObjectOwnership(ListElement::GuardedQObjectPointer *pointer)
 {
-    QQmlData *data = QQmlData::get(pointer->data(), false);
-    Q_ASSERT(data);
+    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);
-        data->explicitIndestructibleSet = (pointer->tag() & ListElement::ExplicitlySet);
+        // 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);
+        }
     }
 }
 
@@ -1110,7 +1113,7 @@ static void setQObjectOwnership(char *mem, QObject *o)
     ddata->indestructible = true;
     ddata->explicitIndestructibleSet = false;
 
-    new (mem) QTaggedPointer<QObject, ListElement::ObjectIndestructible>(
+    new (mem) ListElement::GuardedQObjectPointer(
             o, static_cast<ListElement::ObjectIndestructible>(ownership));
 }
 
@@ -1120,10 +1123,10 @@ int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o)
 
     if (role.type == ListLayout::Role::QObject) {
         char *mem = getPropertyMemory(role);
-        QTaggedPointer<QObject, ListElement::ObjectIndestructible> *g =
-                reinterpret_cast<QTaggedPointer<QObject, ListElement::ObjectIndestructible> *>(mem);
+        GuardedQObjectPointer *g =
+                reinterpret_cast<GuardedQObjectPointer *>(mem);
         bool existingGuard = false;
-        for (size_t i = 0; i < sizeof(QTaggedPointer<QObject, ListElement::ObjectIndestructible>);
+        for (size_t i = 0; i < sizeof(GuardedQObjectPointer);
              ++i) {
             if (mem[i] != 0) {
                 existingGuard = true;
@@ -1135,7 +1138,7 @@ int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o)
             changed = g->data() != o;
             if (changed)
                 restoreQObjectOwnership(g);
-            g->~QTaggedPointer();
+            g->~GuardedQObjectPointer();
         } else {
             changed = true;
         }
@@ -1452,13 +1455,13 @@ void ListElement::destroy(ListLayout *layout)
                     break;
                 case ListLayout::Role::QObject:
                     {
-                    QTaggedPointer<QObject, ListElement::ObjectIndestructible> *guard =
+                    GuardedQObjectPointer *guard =
                             getGuardProperty(r);
 
                     if (guard) {
                         restoreQObjectOwnership(guard);
 
-                        guard->~QTaggedPointer();
+                        guard->~GuardedQObjectPointer();
                     }
                     }
                     break;
diff --git a/src/qmlmodels/qqmllistmodel_p_p.h b/src/qmlmodels/qqmllistmodel_p_p.h
index 7f41960ae3..6c40934c04 100644
--- a/src/qmlmodels/qqmllistmodel_p_p.h
+++ b/src/qmlmodels/qqmllistmodel_p_p.h
@@ -278,6 +278,45 @@ private:
 class ListElement
 {
 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);
@@ -285,13 +324,6 @@ public:
 
     static QVector<int> sync(ListElement *src, ListLayout *srcLayout, ListElement *target, ListLayout *targetLayout);
 
-    enum
-    {
-        BLOCK_SIZE = 64 - sizeof(int) - sizeof(ListElement *) - sizeof(ModelNodeMetaObject *)
-    };
-
-    enum ObjectIndestructible { Indestructible = 1, ExplicitlySet = 2 };
-
 private:
 
     void destroy(ListLayout *layout);
@@ -328,7 +360,7 @@ private:
     ListModel *getListProperty(const ListLayout::Role &role);
     StringOrTranslation *getStringProperty(const ListLayout::Role &role);
     QObject *getQObjectProperty(const ListLayout::Role &role);
-    QTaggedPointer<QObject, ObjectIndestructible> *getGuardProperty(const ListLayout::Role &role);
+    GuardedQObjectPointer *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 812f37e7dd..e372db271e 100644
--- a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
+++ b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
@@ -133,6 +133,7 @@ private slots:
     void undefinedAppendShouldCauseError();
     void nullPropertyCrash();
     void objectDestroyed();
+    void destroyObject();
 };
 
 bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object)
@@ -1788,6 +1789,32 @@ void tst_qqmllistmodel::objectDestroyed()
     QTRY_VERIFY(destroyed);
 }
 
+void tst_qqmllistmodel::destroyObject()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine);
+    component.setData(
+                R"(import QtQuick
+                   ListModel {
+                       id: model
+                       Component.onCompleted: { model.append({"a": contextObject}); }
+                   })",
+                QUrl());
+    QVERIFY2(component.isReady(), qPrintable(component.errorString()));
+    QScopedPointer<QObject> element(new QObject);
+    engine.rootContext()->setContextProperty(u"contextObject"_qs, element.data());
+
+    QScopedPointer<QObject> o(component.create());
+    QVERIFY(!o.isNull());
+
+    QQmlListModel *model = qobject_cast<QQmlListModel *>(o.data());
+    QVERIFY(model);
+    QCOMPARE(model->count(), 1);
+    QCOMPARE(model->get(0).property("a").toQObject(), element.data());
+    element.reset();
+    QCOMPARE(model->get(0).property("a").toQObject(), nullptr);
+}
+
 QTEST_MAIN(tst_qqmllistmodel)
 
 #include "tst_qqmllistmodel.moc"