QQmlListModel: Invalidate ModelObject when necessary

Both the object and the listmodel may be deleted during the life time of
ModelObject. Don't crash when that happens.

Also, fix QV4QPointer to actually name the type of the pointer it
stores. Apparently this is the first time we add a QV4QPointer of
something that's not a plain QObject.

Pick-to: 6.8 6.7 6.5 6.2 5.15
Task-number: QTBUG-118024
Change-Id: I208d8749bcd67970f7bfbe569eed7a472f909508
Reviewed-by: Fabian Kosmale <[email protected]>

Author: ulfhermannqt

src/qml/memory/qv4heap_p.h

@@ -215,7 +215,7 @@ struct QV4QPointer {
 
 private:
     QtSharedPointer::ExternalRefCountData *d;
-    QObject *qObject;
+    T *qObject;
 };
 Q_STATIC_ASSERT(std::is_trivial_v<QV4QPointer<QObject>>);
 #endif

src/qmlmodels/qqmllistmodel.cpp

@@ -1665,9 +1665,12 @@ bool ModelObject::virtualPut(Managed *m, PropertyKey id, const Value &value, Val
 
     ExecutionEngine *eng = that->engine();
     const int elementIndex = that->d()->elementIndex();
-    int roleIndex = that->d()->m_model->m_listModel->setExistingProperty(elementIndex, propName, value, eng);
-    if (roleIndex != -1)
-        that->d()->m_model->emitItemsChanged(elementIndex, 1, QVector<int>(1, roleIndex));
+    if (QQmlListModel *model = that->d()->m_model) {
+        const int roleIndex
+                = model->listModel()->setExistingProperty(elementIndex, propName, value, eng);
+        if (roleIndex != -1)
+            model->emitItemsChanged(elementIndex, 1, QVector<int>(1, roleIndex));
+    }
 
     ModelNodeMetaObject *mo = ModelNodeMetaObject::get(that->object());
     if (mo->initialized())
@@ -1683,7 +1686,11 @@ ReturnedValue ModelObject::virtualGet(const Managed *m, PropertyKey id, const Va
     const ModelObject *that = static_cast<const ModelObject*>(m);
     Scope scope(that);
     ScopedString name(scope, id.asStringOrSymbol());
-    const ListLayout::Role *role = that->d()->m_model->m_listModel->getExistingRole(name);
+    QQmlListModel *model = that->d()->m_model;
+    if (!model)
+        return QObjectWrapper::virtualGet(m, id, receiver, hasProperty);
+
+    const ListLayout::Role *role = model->listModel()->getExistingRole(name);
     if (!role)
         return QObjectWrapper::virtualGet(m, id, receiver, hasProperty);
     if (hasProperty)
@@ -1696,7 +1703,7 @@ ReturnedValue ModelObject::virtualGet(const Managed *m, PropertyKey id, const Va
     }
 
     const int elementIndex = that->d()->elementIndex();
-    QVariant value = that->d()->m_model->data(elementIndex, role->index);
+    QVariant value = model->data(elementIndex, role->index);
     return that->engine()->fromVariant(value);
 }
 
@@ -1719,16 +1726,19 @@ PropertyKey ModelObjectOwnPropertyKeyIterator::next(const Object *o, Property *p
     const ModelObject *that = static_cast<const ModelObject *>(o);
 
     ExecutionEngine *v4 = that->engine();
-    if (roleNameIndex < that->listModel()->roleCount()) {
+
+    QQmlListModel *model = that->d()->m_model;
+    ListModel *listModel = model ? model->listModel() : nullptr;
+    if (listModel && roleNameIndex < listModel->roleCount()) {
         Scope scope(that->engine());
-        const ListLayout::Role &role = that->listModel()->getExistingRole(roleNameIndex);
+        const ListLayout::Role &role = listModel->getExistingRole(roleNameIndex);
         ++roleNameIndex;
         ScopedString roleName(scope, v4->newString(role.name));
         if (attrs)
             *attrs = QV4::Attr_Data;
         if (pd) {
 
-            QVariant value = that->d()->m_model->data(that->d()->elementIndex(), role.index);
+            QVariant value = model->data(that->d()->elementIndex(), role.index);
             if (auto recursiveListModel = qvariant_cast<QQmlListModel*>(value)) {
                 auto size = recursiveListModel->count();
                 auto array = ScopedArrayObject{scope, v4->newArrayObject(size)};

src/qmlmodels/qqmllistmodel_p.h

@@ -79,6 +79,8 @@ class Q_QMLMODELS_EXPORT QQmlListModel : public QAbstractListModel
     bool dynamicRoles() const { return m_dynamicRoles; }
     void setDynamicRoles(bool enableDynamicRoles);
 
+    ListModel *listModel() const { return m_listModel; }
+
 Q_SIGNALS:
     void countChanged();
 

src/qmlmodels/qqmllistmodel_p_p.h

@@ -125,13 +125,23 @@ struct ModelObject : public QObjectWrapper {
     {
         QObjectWrapper::init(object);
         m_model = model;
-        QObjectPrivate *op = QObjectPrivate::get(object);
-        m_nodeModelMetaObject = static_cast<ModelNodeMetaObject *>(op->metaObject);
     }
-    void destroy() { QObjectWrapper::destroy(); }
-    int elementIndex() const { return m_nodeModelMetaObject->m_elementIndex; }
-    QQmlListModel *m_model;
-    ModelNodeMetaObject *m_nodeModelMetaObject;
+
+    void destroy()
+    {
+        m_model.destroy();
+        QObjectWrapper::destroy();
+    }
+
+    int elementIndex() const {
+        if (const QObject *o = object()) {
+            const QObjectPrivate *op = QObjectPrivate::get(o);
+            return static_cast<ModelNodeMetaObject *>(op->metaObject)->m_elementIndex;
+        }
+        return -1;
+    }
+
+    QV4QPointer<QQmlListModel> m_model;
 };
 
 }
@@ -141,8 +151,6 @@ struct ModelObject : public QObjectWrapper
     V4_OBJECT2(ModelObject, QObjectWrapper)
     V4_NEEDS_DESTROY
 
-    ListModel *listModel() const { return d()->m_model->m_listModel; }
-
 protected:
     static bool virtualPut(Managed *m, PropertyKey id, const Value& value, Value *receiver);
     static ReturnedValue virtualGet(const Managed *m, PropertyKey id, const Value *receiver, bool *hasProperty);

tests/auto/qml/qqmllistmodel/data/deadModelData.qml

@@ -0,0 +1,45 @@
+import QtQml
+
+QtObject {
+    function swapCorpses() {
+        const lhsData = getModelData(lhsButtonListModel);
+        const rhsData = getModelData(rhsButtonListModel);
+
+        lhsButtonListModel.clear();
+        rhsButtonListModel.clear();
+
+        addToModel(lhsButtonListModel, rhsData);
+        addToModel(rhsButtonListModel, lhsData);
+    }
+
+    property ListModel l1: ListModel {
+        id: lhsButtonListModel
+    }
+
+    property ListModel l2: ListModel {
+        id: rhsButtonListModel
+    }
+
+    Component.onCompleted: {
+        lhsButtonListModel.append({ "ident": 1,  "buttonText": "B 1"});
+        lhsButtonListModel.append({ "ident": 2,  "buttonText": "B 2"});
+        lhsButtonListModel.append({ "ident": 3,  "buttonText": "B 3"});
+
+        rhsButtonListModel.append({ "ident": 4,  "buttonText": "B 4"});
+        rhsButtonListModel.append({ "ident": 5,  "buttonText": "B 5"});
+        rhsButtonListModel.append({ "ident": 6,  "buttonText": "B 6"});
+    }
+
+    function getModelData(model) {
+        var dataList = []
+        for (var i = 0; i < model.count; ++i)
+            dataList.push(model.get(i));
+
+        return dataList;
+    }
+
+    function addToModel(model, buttonData) {
+        for (var i = 0; i < buttonData.length; ++i)
+            model.append(buttonData[i]);
+    }
+}

tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp

@@ -122,6 +122,7 @@ private slots:
     void enumsInListElement();
     void protectQObjectFromGC();
     void nestedLists();
+    void deadModelData();
 };
 
 bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object)
@@ -2056,6 +2057,67 @@ void tst_qqmllistmodel::nestedLists()
     verifyLists(list, topLevel);
 }
 
+void tst_qqmllistmodel::deadModelData()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("deadModelData.qml"));
+    QVERIFY2(component.isReady(), qPrintable(component.errorString()));
+    QScopedPointer<QObject> o(component.create());
+    QVERIFY(!o.isNull());
+
+    QQmlListModel *l1 = o->property("l1").value<QQmlListModel *>();
+    QVERIFY(l1);
+    QQmlListModel *l2 = o->property("l2").value<QQmlListModel *>();
+    QVERIFY(l2);
+
+    QCOMPARE(l1->count(), 3);
+    QCOMPARE(l2->count(), 3);
+
+    for (int i = 0; i < 3; ++i) {
+        QObject *i1 = qjsvalue_cast<QObject *>(l1->get(i));
+        QVERIFY(i1);
+        QCOMPARE(i1->property("ident").value<double>(), i + 1);
+        QCOMPARE(i1->property("buttonText").value<QString>(),
+                 QLatin1String("B %1").arg(QLatin1Char('0' + i + 1)));
+
+        QObject *i2 = qjsvalue_cast<QObject *>(l2->get(i));
+        QVERIFY(i2);
+        QCOMPARE(i2->property("ident").value<double>(), i + 4);
+        QCOMPARE(i2->property("buttonText").value<QString>(),
+                 QLatin1String("B %1").arg(QLatin1Char('0' + i + 4)));
+    }
+
+    for (int i = 0; i < 6; ++i) {
+        QTest::ignoreMessage(
+                QtWarningMsg,
+                QRegularExpression(".*: ident is undefined. Adding an object with a undefined "
+                                   "member does not create a role for it."));
+        QTest::ignoreMessage(
+                QtWarningMsg,
+                QRegularExpression(".*: buttonText is undefined. Adding an object with a undefined "
+                                   "member does not create a role for it."));
+    }
+
+    QMetaObject::invokeMethod(o.data(), "swapCorpses");
+
+    // We get default-created values for all the roles now.
+
+    QCOMPARE(l1->count(), 3);
+    QCOMPARE(l2->count(), 3);
+
+    for (int i = 0; i < 3; ++i) {
+        QObject *i1 = qjsvalue_cast<QObject *>(l1->get(i));
+        QVERIFY(i1);
+        QCOMPARE(i1->property("ident").value<double>(), double());
+        QCOMPARE(i1->property("buttonText").value<QString>(), QString());
+
+        QObject *i2 = qjsvalue_cast<QObject *>(l2->get(i));
+        QVERIFY(i2);
+        QCOMPARE(i2->property("ident").value<double>(), double());
+        QCOMPARE(i2->property("buttonText").value<QString>(), QString());
+    }
+}
+
 QTEST_MAIN(tst_qqmllistmodel)
 
 #include "tst_qqmllistmodel.moc"