From 2ce8a33347e827fa4a1579b937ff5bbbadc16451 Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Mon, 31 Jan 2022 14:14:26 +0100 Subject: 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 Reviewed-by: Ulf Hermann (cherry picked from commit 6901eacff40a7d8781e20fb5bcfd28d7526b589b) (cherry picked from commit ce8db0672557cef7942df061f52ccdf65fa250f6) Reviewed-by: Qt CI Bot --- src/qmlmodels/qqmladaptormodel.cpp | 18 ++++++++---- src/qmlmodels/qqmladaptormodel_p.h | 6 +++- src/qmlmodels/qqmldelegatemodel.cpp | 2 +- .../qquicklistview/data/singletonModelLifetime.qml | 32 ++++++++++++++++++++++ .../quick/qquicklistview/tst_qquicklistview.cpp | 24 ++++++++++++++++ 5 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 tests/auto/quick/qquicklistview/data/singletonModelLifetime.qml diff --git a/src/qmlmodels/qqmladaptormodel.cpp b/src/qmlmodels/qqmladaptormodel.cpp index f4dca6e518..b82a589788 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, QQmlEngine *engine) +void QQmlAdaptorModel::setModel(const QVariant &variant, QObject *, QQmlEngine *engine) { accessors->cleanup(*this); list.setList(variant, engine); + modelStrongReference.clear(); if (QObject *object = qvariant_cast(list.list())) { - setObject(object, parent); + if (QQmlData *ddata = QQmlData::get(object)) + modelStrongReference = ddata->jsWrapper; + setObject(object); if (qobject_cast(object)) accessors = new VDMAbstractItemModelDataType(this); else accessors = new VDMObjectDelegateDataType; } else if (list.type() == QQmlListAccessor::ListProperty) { - setObject(static_cast(variant.constData())->object(), parent); + auto object = static_cast(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 537ec65eed..4f01a37f0a 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 +class Q_QMLMODELS_PRIVATE_EXPORT QQmlAdaptorModel : public QQmlGuard { 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; int modelItemRevision = 0; diff --git a/src/qmlmodels/qqmldelegatemodel.cpp b/src/qmlmodels/qqmldelegatemodel.cpp index 751d9e301e..8e9ca0ce55 100644 --- a/src/qmlmodels/qqmldelegatemodel.cpp +++ b/src/qmlmodels/qqmldelegatemodel.cpp @@ -258,7 +258,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) { diff --git a/tests/auto/quick/qquicklistview/data/singletonModelLifetime.qml b/tests/auto/quick/qquicklistview/data/singletonModelLifetime.qml new file mode 100644 index 0000000000..f230786723 --- /dev/null +++ b/tests/auto/quick/qquicklistview/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/qquicklistview/tst_qquicklistview.cpp b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp index d3deb513d0..7b17c014d5 100644 --- a/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp +++ b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp @@ -33,6 +33,8 @@ #include #include #include +#include +#include #include #include #include @@ -302,6 +304,9 @@ private slots: void dragDelegateWithMouseArea(); void dragDelegateWithMouseArea_data(); + + void singletonModelLifetime(); + private: template void items(const QUrl &source); template void changed(const QUrl &source); @@ -10200,6 +10205,25 @@ void tst_QQuickListView::dragDelegateWithMouseArea_data() } } +class SingletonModel : public QStringListModel +{ + Q_OBJECT +public: + SingletonModel(QObject* parent = nullptr) : QStringListModel(parent) { } +}; + +void tst_QQuickListView::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("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_QQuickListView) #include "tst_qquicklistview.moc" -- cgit v1.2.1