From 9f5bd5ab102325b03c2afaae39c6927fa89a956c Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Sun, 29 Jun 2014 15:47:04 +0800 Subject: 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(...) 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 and Chris Adams . Task-number: QTBUG-35639 Change-Id: I0aee227cd2057654c2cb843c4ebb57ed2ec9a8bf Reviewed-by: Lars Knoll --- src/qml/qml/qqmlopenmetaobject.cpp | 10 +- tests/auto/qml/qqmllistmodel/data/dynamicroles.qml | 21 ++++ tests/auto/qml/qqmllistmodel/data/qtbug38907.qml | 25 +++++ tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp | 111 +++++++++++++++++++++ 4 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 tests/auto/qml/qqmllistmodel/data/dynamicroles.qml create mode 100644 tests/auto/qml/qqmllistmodel/data/qtbug38907.qml 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 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(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(); } }; 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("listModel"); + QVERIFY(model != 0); + + QMetaObject::invokeMethod(model, "appendNewElement"); + + QObject *testObj = new QObject; + model->setProperty(0, "obj", QVariant::fromValue(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("listModel"); + QVERIFY(model != 0); + + QMetaObject::invokeMethod(model, "appendNewElement"); + + QObject *testObj = new QObject; + model->setProperty(0, "obj", QVariant::fromValue(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("listModel"); + QVERIFY(model != 0); + + QMetaObject::invokeMethod(model, "appendNewElement"); + + QObject *testObj = new QObject; + model->setProperty(0, "obj", QVariant::fromValue(testObj)); + delete testObj; + QObject *testObj2 = new QObject; + model->setProperty(0, "obj", QVariant::fromValue(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 item(qobject_cast(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" -- cgit v1.2.3