diff options
author | Ulf Hermann <ulf.hermann@qt.io> | 2023-06-16 11:33:30 +0200 |
---|---|---|
committer | Ulf Hermann <ulf.hermann@qt.io> | 2023-06-16 23:43:13 +0200 |
commit | 46842ec7c67ce16ade08076c0fd7e78e96f33887 (patch) | |
tree | 8820dfcfc3cb750993678d947c26e013b1fc7519 | |
parent | 74fac24a2785af1fe7a3252b1c804c762ede400b (diff) |
QML: Guard QProperty change triggers against deletion of target
Previously, we relied on QObject* pointers being unique even after
deletion of the objects. That's not good.
Pick-to: 6.6 6.5 6.2
Fixes: QTBUG-114329
Change-Id: Ia0a2c1d2cb5d8a0d47ec00e73424c959c59c09bc
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
-rw-r--r-- | src/qml/qml/qqmlengine_p.h | 13 | ||||
-rw-r--r-- | src/qml/qml/qqmljavascriptexpression.cpp | 46 | ||||
-rw-r--r-- | src/qml/qml/qqmljavascriptexpression_p.h | 2 | ||||
-rw-r--r-- | tests/auto/qml/qqmlproperty/data/invalidateQPropertyChangeTriggers.qml | 50 | ||||
-rw-r--r-- | tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp | 29 |
5 files changed, 121 insertions, 19 deletions
diff --git a/src/qml/qml/qqmlengine_p.h b/src/qml/qml/qqmlengine_p.h index a948df3b43..f7da8d5c72 100644 --- a/src/qml/qml/qqmlengine_p.h +++ b/src/qml/qml/qqmlengine_p.h @@ -81,9 +81,16 @@ public: }; struct QPropertyChangeTrigger : QPropertyObserver { - QPropertyChangeTrigger(QQmlJavaScriptExpression *expression) : QPropertyObserver(&QPropertyChangeTrigger::trigger), m_expression(expression) {} - QQmlJavaScriptExpression * m_expression; - QObject *target = nullptr; + Q_DISABLE_COPY_MOVE(QPropertyChangeTrigger) + + QPropertyChangeTrigger(QQmlJavaScriptExpression *expression) + : QPropertyObserver(&QPropertyChangeTrigger::trigger) + , m_expression(expression) + { + } + + QPointer<QObject> target; + QQmlJavaScriptExpression *m_expression; int propertyIndex = 0; static void trigger(QPropertyObserver *, QUntypedPropertyData *); diff --git a/src/qml/qml/qqmljavascriptexpression.cpp b/src/qml/qml/qqmljavascriptexpression.cpp index 304c5da29a..d7cf38984b 100644 --- a/src/qml/qml/qqmljavascriptexpression.cpp +++ b/src/qml/qml/qqmljavascriptexpression.cpp @@ -350,19 +350,35 @@ void QQmlPropertyCapture::captureProperty( captureNonBindableProperty(o, propertyData->notifyIndex(), propertyData->coreIndex(), doNotify); } +bool QQmlJavaScriptExpression::needsPropertyChangeTrigger(QObject *target, int propertyIndex) +{ + TriggerList **prev = &qpropertyChangeTriggers; + TriggerList *current = qpropertyChangeTriggers; + while (current) { + if (!current->target) { + *prev = current->next; + QRecyclePool<TriggerList>::Delete(current); + current = *prev; + } else if (current->target == target && current->propertyIndex == propertyIndex) { + return false; // already installed + } else { + prev = ¤t->next; + current = current->next; + } + } + + return true; +} + void QQmlPropertyCapture::captureTranslation() { // use a unique invalid index to avoid needlessly querying the metaobject for // the correct index of of the translationLanguage property int const invalidIndex = -2; - for (auto trigger = expression->qpropertyChangeTriggers; trigger; - trigger = trigger->next) { - if (trigger->target == engine && trigger->propertyIndex == invalidIndex) - return; // already installed + if (expression->needsPropertyChangeTrigger(engine, invalidIndex)) { + auto trigger = expression->allocatePropertyChangeTrigger(engine, invalidIndex); + trigger->setSource(QQmlEnginePrivate::get(engine)->translationLanguage); } - auto trigger = expression->allocatePropertyChangeTrigger(engine, invalidIndex); - - trigger->setSource(QQmlEnginePrivate::get(engine)->translationLanguage); } void QQmlPropertyCapture::captureBindableProperty( @@ -372,16 +388,14 @@ void QQmlPropertyCapture::captureBindableProperty( // the automatic capturing process already takes care of everything if (!expression->mustCaptureBindableProperty()) return; - for (auto trigger = expression->qpropertyChangeTriggers; trigger; - trigger = trigger->next) { - if (trigger->target == o && trigger->propertyIndex == c) - return; // already installed + + if (expression->needsPropertyChangeTrigger(o, c)) { + auto trigger = expression->allocatePropertyChangeTrigger(o, c); + QUntypedBindable bindable; + void *argv[] = { &bindable }; + metaObjectForBindable->metacall(o, QMetaObject::BindableProperty, c, argv); + bindable.observe(trigger); } - auto trigger = expression->allocatePropertyChangeTrigger(o, c); - QUntypedBindable bindable; - void *argv[] = { &bindable }; - metaObjectForBindable->metacall(o, QMetaObject::BindableProperty, c, argv); - bindable.observe(trigger); } void QQmlPropertyCapture::captureNonBindableProperty(QObject *o, int n, int c, bool doNotify) diff --git a/src/qml/qml/qqmljavascriptexpression_p.h b/src/qml/qml/qqmljavascriptexpression_p.h index e8e4bb4990..3a56cbe423 100644 --- a/src/qml/qml/qqmljavascriptexpression_p.h +++ b/src/qml/qml/qqmljavascriptexpression_p.h @@ -132,6 +132,8 @@ public: QQmlEngine *engine() const { return m_context ? m_context->engine() : nullptr; } bool hasUnresolvedNames() const { return m_context && m_context->hasUnresolvedNames(); } + + bool needsPropertyChangeTrigger(QObject *target, int propertyIndex); QPropertyChangeTrigger *allocatePropertyChangeTrigger(QObject *target, int propertyIndex); protected: diff --git a/tests/auto/qml/qqmlproperty/data/invalidateQPropertyChangeTriggers.qml b/tests/auto/qml/qqmlproperty/data/invalidateQPropertyChangeTriggers.qml new file mode 100644 index 0000000000..bfa832c1c8 --- /dev/null +++ b/tests/auto/qml/qqmlproperty/data/invalidateQPropertyChangeTriggers.qml @@ -0,0 +1,50 @@ +import QtQml + +QtObject { + id: root + objectName: column.text + + property Component c: Component { + id: comp + QtObject { } + } + + property QtObject rectItem: null + + property bool running: false + + property Timer t: Timer { + id: column + interval: 200 + running: root.running + repeat: true + + property string text: { + let item = root.rectItem + let result = rectItem ? rectItem.objectName : "Create Object" + return result + } + + onTriggered: { + let rectItem = root.rectItem + + // If rectItem exists destory it. + if (rectItem) { + rectItem.destroy() + return + } + + // Otherwise create a new object + let newRectItem = comp.createObject(column, {}) + + + // Setting the objectName before setting root.rectItem seems to work. + // newRectItem.width = 1200 + root.rectItem = newRectItem + + // But setting the objectName after setting root.rectItem seems to + // cause the issue. + newRectItem.objectName = "1300" + } + } +} diff --git a/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp b/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp index a636fe2292..bb335e5ad3 100644 --- a/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp +++ b/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp @@ -220,6 +220,8 @@ private slots: void listAssignmentSignals(); + void invalidateQPropertyChangeTriggers(); + private: QQmlEngine engine; }; @@ -2560,6 +2562,33 @@ void tst_qqmlproperty::listAssignmentSignals() QCOMPARE(root->property("signalCounter").toInt(), 2); } +void tst_qqmlproperty::invalidateQPropertyChangeTriggers() +{ + QQmlEngine engine; + QQmlComponent component(&engine, testFileUrl("invalidateQPropertyChangeTriggers.qml")); + QVERIFY2(component.isReady(), qPrintable(component.errorString())); + QScopedPointer<QObject> root(component.create()); + QVERIFY(!root.isNull()); + + QStringList names; + QObject::connect(root.data(), &QObject::objectNameChanged, [&](const QString &name) { + if (names.length() == 10) + root->setProperty("running", false); + else + names.append(name); + }); + + root->setProperty("running", true); + QTRY_VERIFY(!root->property("running").toBool()); + + QCOMPARE(names, (QStringList { + u""_s, u"1300"_s, u"Create Object"_s, + u""_s, u"1300"_s, u"Create Object"_s, + u""_s, u"1300"_s, u"Create Object"_s, + u""_s + })); +} + QTEST_MAIN(tst_qqmlproperty) #include "tst_qqmlproperty.moc" |