Fix storing QObject pointers in dynamic role models

Storing QObject pointers in a dynamic role model man end up crashing the
application once those objects get deleted.

As it looks impossible to make QVariant::fromValue<QObject*>(...) track
the life cycle in a binary compatible way, this patch fixes the crashes
by tracking with a QPointer at the level of QVariant storage. That's the
QQmlOpenMetaObject in the case of the dynamic role model. In principle a
similar approach is already in place for storage of QML declared
properties - this patch mirrors the approach.

Test cases provided by Ben Lau <xbenlau@gmail.com> and Chris Adams
<chris.adams@jollamobile.com>.

Task-number: QTBUG-35639
Change-Id: I0aee227cd2057654c2cb843c4ebb57ed2ec9a8bf
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

4 files changed, 166 insertions, 1 deletions

diff --git a/src/qml/qml/qqmlopenmetaobject.cpp b/src/qml/qml/qqmlopenmetaobject.cpp
index ff89278faf..ce5983e05c 100644
--- a/src/qml/qml/qqmlopenmetaobject.cpp
+++ b/src/qml/qml/qqmlopenmetaobject.cpp
@@ -187,14 +187,22 @@ public:
     struct Property {
     private:
         QVariant m_value;
+        QPointer<QObject> qobjectTracker;
     public:
         bool valueSet = false;
 
-        const QVariant &value() const { return m_value; }
+        QVariant value() const {
+            if (QMetaType::typeFlags(m_value.userType()) & QMetaType::PointerToQObject
+                && qobjectTracker.isNull())
+                return QVariant::fromValue<QObject*>(nullptr);
+            return m_value;
+        }
         QVariant &valueRef() { return m_value; }
         void setValue(const QVariant &v) {
             m_value = v;
             valueSet = true;
+            if (QMetaType::typeFlags(v.userType()) & QMetaType::PointerToQObject)
+                qobjectTracker = m_value.value<QObject*>();
         }
     };
 
diff --git a/tests/auto/qml/qqmllistmodel/data/dynamicroles.qml b/tests/auto/qml/qqmllistmodel/data/dynamicroles.qml
new file mode 100644
index 0000000000..7d3650c3b9
--- /dev/null
+++ b/tests/auto/qml/qqmllistmodel/data/dynamicroles.qml
@@ -0,0 +1,21 @@
+import QtQuick 2.0
+
+Item {
+    id: root
+
+    ListModel {
+        id: listModel
+        objectName: "listModel"
+        dynamicRoles: true
+
+        // have to add elements dynamically when dynamicRoles = true
+        function appendNewElement() {
+            listModel.append({"name": "test", "obj": null})
+        }
+
+        function setElementAgain() {
+            var element = listModel.get(0)
+            listModel.set(0, element)
+        }
+    }
+}
diff --git a/tests/auto/qml/qqmllistmodel/data/qtbug38907.qml b/tests/auto/qml/qqmllistmodel/data/qtbug38907.qml
new file mode 100644
index 0000000000..0abf221f60
--- /dev/null
+++ b/tests/auto/qml/qqmllistmodel/data/qtbug38907.qml
@@ -0,0 +1,25 @@
+import QtQuick 2.0
+import QtTest 1.0
+
+Item {
+
+    Item {
+        id : testItem
+        property string name : "testObject"
+        property var object : this
+        function testMethod() {
+            return -1;
+        }
+    }
+
+    ListModel {
+        id : listModel
+        dynamicRoles : true
+    }
+
+    function exec() {
+        listModel.append(testItem);
+        listModel.append({ item : testItem });
+        return true;
+    }
+}
diff --git a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
index 9fdc54f067..771f3e5c4e 100644
--- a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
+++ b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
@@ -106,6 +106,7 @@ private slots:
     void get_nested_data();
     void crash_model_with_multiple_roles();
     void crash_model_with_unknown_roles();
+    void crash_model_with_dynamic_roles();
     void set_model_cache();
     void property_changes();
     void property_changes_data();
@@ -126,6 +127,7 @@ private slots:
     void stringifyModelEntry();
     void qobjectTrackerForDynamicModelObjects();
     void crash_append_empty_array();
+    void dynamic_roles_crash_QTBUG_38907();
 };
 
 bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object)
@@ -984,6 +986,97 @@ void tst_qqmllistmodel::crash_model_with_unknown_roles()
     model->index(0, 0, QModelIndex()).data(Qt::UserRole);
 }
 
+//QTBUG-35639
+void tst_qqmllistmodel::crash_model_with_dynamic_roles()
+{
+    {
+        // setting a dynamic role to a QObject value, then triggering dtor
+        QQmlEngine eng;
+        QQmlComponent component(&eng, testFileUrl("dynamicroles.qml"));
+        QObject *rootItem = component.create();
+        qWarning() << component.errorString();
+        QVERIFY(component.errorString().isEmpty());
+        QVERIFY(rootItem != 0);
+        QQmlListModel *model = rootItem->findChild<QQmlListModel*>("listModel");
+        QVERIFY(model != 0);
+
+        QMetaObject::invokeMethod(model, "appendNewElement");
+
+        QObject *testObj = new QObject;
+        model->setProperty(0, "obj", QVariant::fromValue<QObject*>(testObj));
+        delete testObj;
+
+        // delete the root item, which will cause the model dtor to run
+        // previously, this would crash as it attempted to delete testObj.
+        delete rootItem;
+    }
+
+    {
+        // setting a dynamic role to a QObject value, then triggering
+        // DynamicRoleModelNode::updateValues() to trigger unsafe qobject_cast
+        QQmlEngine eng;
+        QQmlComponent component(&eng, testFileUrl("dynamicroles.qml"));
+        QObject *rootItem = component.create();
+        qWarning() << component.errorString();
+        QVERIFY(component.errorString().isEmpty());
+        QVERIFY(rootItem != 0);
+        QQmlListModel *model = rootItem->findChild<QQmlListModel*>("listModel");
+        QVERIFY(model != 0);
+
+        QMetaObject::invokeMethod(model, "appendNewElement");
+
+        QObject *testObj = new QObject;
+        model->setProperty(0, "obj", QVariant::fromValue<QObject*>(testObj));
+        delete testObj;
+
+        QMetaObject::invokeMethod(model, "setElementAgain");
+
+        delete rootItem;
+    }
+
+    {
+        // setting a dynamic role to a QObject value, then triggering
+        // DynamicRoleModelNodeMetaObject::propertyWrite()
+
+        /*
+           XXX TODO: I couldn't reproduce this one simply - I think it
+           requires a WorkerScript sync() call, and that's non-trivial.
+           I thought I could do it simply via:
+
+        QQmlEngine eng;
+        QQmlComponent component(&eng, testFileUrl("dynamicroles.qml"));
+        QObject *rootItem = component.create();
+        qWarning() << component.errorString();
+        QVERIFY(component.errorString().isEmpty());
+        QVERIFY(rootItem != 0);
+        QQmlListModel *model = rootItem->findChild<QQmlListModel*>("listModel");
+        QVERIFY(model != 0);
+
+        QMetaObject::invokeMethod(model, "appendNewElement");
+
+        QObject *testObj = new QObject;
+        model->setProperty(0, "obj", QVariant::fromValue<QObject*>(testObj));
+        delete testObj;
+        QObject *testObj2 = new QObject;
+        model->setProperty(0, "obj", QVariant::fromValue<QObject*>(testObj2));
+
+           But it turns out that that doesn't work (the setValue() call within
+           setProperty() doesn't seem to trigger the right codepath, for some
+           reason), and you can't trigger it manually via:
+
+        QObject *testObj2 = new QObject;
+        void *a[] = { testObj2, 0 };
+        QMetaObject::metacall(dynamicNodeModel, QMetaObject::WriteProperty, 0, a);
+
+           because the dynamicNodeModel for that index cannot be retrieved
+           using the public API.
+
+           But, anyway, I think the above two test cases are sufficient to
+           show that QObject* values should be guarded internally.
+        */
+    }
+}
+
 //QTBUG-15190
 void tst_qqmllistmodel::set_model_cache()
 {
@@ -1556,6 +1649,24 @@ void tst_qqmllistmodel::crash_append_empty_array()
     QCOMPARE(spy.count(), 0);
 }
 
+void tst_qqmllistmodel::dynamic_roles_crash_QTBUG_38907()
+{
+    QQmlEngine eng;
+    QQmlComponent component(&eng, testFileUrl("qtbug38907.qml"));
+    QVERIFY(!component.isError());
+    QScopedPointer<QQuickItem> item(qobject_cast<QQuickItem*>(component.create()));
+    QVERIFY(item != 0);
+
+    QVariant retVal;
+
+    QMetaObject::invokeMethod(item.data(),
+                              "exec",
+                              Qt::DirectConnection,
+                              Q_RETURN_ARG(QVariant, retVal));
+
+    QVERIFY(retVal.toBool());
+}
+
 QTEST_MAIN(tst_qqmllistmodel)
 
 #include "tst_qqmllistmodel.moc"