QQmlAdaptorModel: Do not use reparenting for lifetime managemment

In QQmlAdaptorModel, we were using QQmlStrongJSQObjectReference to
ensure that a passed in model lives long enough. However,
QQmlAdaptorModel uses reparenting to keep objects alive. This is not
safe, as we can use QML singletons as models.

Reparenting singletons messes with the engine's lifetime handling once
their new parent gets deleted: The object will be marked as
queuedForDeletion by QQmlData::markAsDeleted; consequently wasDeleted
returns true for the object, and any ScopedObject or ObjectWrapper will
return nullptr when we try to retrieve their underlying QObject.

The actual object probaly does not get deleted, as it is not placed in
the QML heap. Consequently the gc will ignore it.

This leads to a crash when the singleton is accessed in a different
place: We see that the object is non-null, create a ScopedObject for it,
and then try to later access the ScopedObject's underlying object
(assuming that it must be non-null, because we already checked for the
actual object being non-null). However, due to the reasons outlined
above, we actually receive a null pointer, and thus encounter a crash.

To avoid he issue, we change the lifetime management strategy: Instead
of using the parent to keep the object alive, we now use a
QV4::PersistentValue.

Fixes: QTBUG-100260
Change-Id: I266e6ef94c4f079de3da2742d6fb8d61df5a64ce
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit 6901eacff40a7d8781e20fb5bcfd28d7526b589b)

5 files changed, 75 insertions, 10 deletions

diff --git a/src/qmlmodels/qqmladaptormodel.cpp b/src/qmlmodels/qqmladaptormodel.cpp
index dc3ea8019e..fe0ab6bcbc 100644
--- a/src/qmlmodels/qqmladaptormodel.cpp
+++ b/src/qmlmodels/qqmladaptormodel.cpp
@@ -962,30 +962,36 @@ QQmlAdaptorModel::~QQmlAdaptorModel()
     accessors->cleanup(*this);
 }
 
-void QQmlAdaptorModel::setModel(const QVariant &variant, QObject *parent)
+void QQmlAdaptorModel::setModel(const QVariant &variant, QObject *)
 {
     accessors->cleanup(*this);
 
     list.setList(variant);
+    modelStrongReference.clear();
 
     if (QObject *object = qvariant_cast<QObject *>(list.list())) {
-        setObject(object, parent);
+        if (QQmlData *ddata = QQmlData::get(object))
+            modelStrongReference = ddata->jsWrapper;
+        setObject(object);
         if (qobject_cast<QAbstractItemModel *>(object))
             accessors = new VDMAbstractItemModelDataType(this);
         else
             accessors = new VDMObjectDelegateDataType;
     } else if (list.type() == QQmlListAccessor::ListProperty) {
-        setObject(static_cast<const QQmlListReference *>(variant.constData())->object(), parent);
+        auto object = static_cast<const QQmlListReference *>(variant.constData())->object();
+        if (QQmlData *ddata = QQmlData::get(object))
+            modelStrongReference = ddata->jsWrapper;
+        setObject(object);
         accessors = new VDMObjectDelegateDataType;
     } else if (list.type() == QQmlListAccessor::ObjectList) {
-        setObject(nullptr, parent);
+        setObject(nullptr);
         accessors = new VDMObjectDelegateDataType;
     } else if (list.type() != QQmlListAccessor::Invalid
             && list.type() != QQmlListAccessor::Instance) { // Null QObject
-        setObject(nullptr, parent);
+        setObject(nullptr);
         accessors = new VDMListDelegateDataType;
     } else {
-        setObject(nullptr, parent);
+        setObject(nullptr);
         accessors = &qt_vdm_null_accessors;
     }
 }
diff --git a/src/qmlmodels/qqmladaptormodel_p.h b/src/qmlmodels/qqmladaptormodel_p.h
index 531e8d9105..8635762bc4 100644
--- a/src/qmlmodels/qqmladaptormodel_p.h
+++ b/src/qmlmodels/qqmladaptormodel_p.h
@@ -70,7 +70,7 @@ class QQmlDelegateModel;
 class QQmlDelegateModelItem;
 class QQmlDelegateModelItemMetaType;
 
-class Q_QMLMODELS_PRIVATE_EXPORT QQmlAdaptorModel : public QQmlStrongJSQObjectReference<QObject>
+class Q_QMLMODELS_PRIVATE_EXPORT QQmlAdaptorModel : public QQmlGuard<QObject>
 {
 public:
     class Accessors
@@ -114,6 +114,10 @@ public:
     const Accessors *accessors;
     QPersistentModelIndex rootIndex;
     QQmlListAccessor list;
+    // we need to ensure that a JS created model does not get gced, but cannot
+    // arbitrarily set the parent  (using QQmlStrongJSQObjectReference)  of QObject based models,
+    // as that causes issues with singletons
+    QV4::PersistentValue modelStrongReference;
 
     QTypeRevision modelItemRevision = QTypeRevision::zero();
 
@@ -121,7 +125,7 @@ public:
     ~QQmlAdaptorModel();
 
     inline QVariant model() const { return list.list(); }
-    void setModel(const QVariant &variant, QObject *parent);
+    void setModel(const QVariant &variant, QObject *parent = nullptr);
     void invalidateModel();
 
     bool isValid() const;
diff --git a/src/qmlmodels/qqmldelegatemodel.cpp b/src/qmlmodels/qqmldelegatemodel.cpp
index bbaad6d878..5ace1809d1 100644
--- a/src/qmlmodels/qqmldelegatemodel.cpp
+++ b/src/qmlmodels/qqmldelegatemodel.cpp
@@ -259,7 +259,7 @@ QQmlDelegateModel::~QQmlDelegateModel()
 {
     Q_D(QQmlDelegateModel);
     d->disconnectFromAbstractItemModel();
-    d->m_adaptorModel.setObject(nullptr, this);
+    d->m_adaptorModel.setObject(nullptr);
 
     for (QQmlDelegateModelItem *cacheItem : qAsConst(d->m_cache)) {
         if (cacheItem->object) {
@@ -443,7 +443,7 @@ void QQmlDelegateModel::setModel(const QVariant &model)
         _q_itemsRemoved(0, d->m_count);
 
     d->disconnectFromAbstractItemModel();
-    d->m_adaptorModel.setModel(model, this);
+    d->m_adaptorModel.setModel(model);
     d->connectToAbstractItemModel();
 
     d->m_adaptorModel.replaceWatchedRoles(QList<QByteArray>(), d->m_watchedRoles);
diff --git a/tests/auto/quick/qquicklistview2/data/singletonModelLifetime.qml b/tests/auto/quick/qquicklistview2/data/singletonModelLifetime.qml
new file mode 100644
index 0000000000..f230786723
--- /dev/null
+++ b/tests/auto/quick/qquicklistview2/data/singletonModelLifetime.qml
@@ -0,0 +1,32 @@
+import QtQuick 2.15
+import QtQuick.Window 2.15
+import test 1.0
+
+Window {
+    id: root
+    visible: true
+    width: 800
+    height: 680
+    property bool alive: false
+
+    Component {
+        id: view
+        ListView {
+            model: SingletonModel
+        }
+    }
+    function compare(a,b) {
+        root.alive = (a === b)
+    }
+
+    function test_singletonModelCrash() {
+        SingletonModel.objectName = "model"
+        var o = view.createObject(root)
+        o.destroy()
+        Qt.callLater(function() {
+            compare(SingletonModel.objectName, "model")
+        })
+    }
+
+    Component.onCompleted: root.test_singletonModelCrash()
+}
diff --git a/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp b/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
index b177b69602..af563c6f7b 100644
--- a/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
+++ b/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
@@ -31,6 +31,8 @@
 #include <QtQuick/private/qquickitemview_p_p.h>
 #include <QtQuick/private/qquicklistview_p.h>
 #include <QtQuickTest/QtQuickTest>
+#include <QStringListModel>
+#include <QQmlApplicationEngine>
 
 #include <QtQuickTestUtils/private/viewtestutils_p.h>
 #include <QtQuickTestUtils/private/visualtestutils_p.h>
@@ -52,6 +54,7 @@ private slots:
     void delegateChooserEnumRole();
     void QTBUG_92809();
     void footerUpdate();
+    void singletonModelLifetime();
 };
 
 tst_QQuickListView2::tst_QQuickListView2()
@@ -223,6 +226,26 @@ void tst_QQuickListView2::footerUpdate()
     QTRY_COMPARE(footer->y(), 0);
 }
 
+
+class SingletonModel : public QStringListModel
+{
+    Q_OBJECT
+public:
+    SingletonModel(QObject* parent = nullptr) : QStringListModel(parent) { }
+};
+
+void tst_QQuickListView2::singletonModelLifetime()
+{
+    // this does not really test any functionality of listview, but we do not have a good way
+    // to unit test QQmlAdaptorModel in isolation.
+    qmlRegisterSingletonType<SingletonModel>("test", 1, 0, "SingletonModel",
+            [](QQmlEngine* , QJSEngine*) -> QObject* { return new SingletonModel; });
+
+    QQmlApplicationEngine engine(testFile("singletonModelLifetime.qml"));
+    // needs event loop iteration for callLater to execute
+    QTRY_VERIFY(engine.rootObjects().first()->property("alive").toBool());
+}
+
 QTEST_MAIN(tst_QQuickListView2)
 
 #include "tst_qquicklistview2.moc"