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>

src/qml/qml/qqmlengine_p.h

@@ -81,9 +81,16 @@ public:
 };
 
 struct QPropertyChangeTrigger : QPropertyObserver {
-    QPropertyChangeTrigger(QQmlJavaScriptExpression *expression) : QPropertyObserver(&QPropertyChangeTrigger::trigger), m_expression(expression) {}
+    Q_DISABLE_COPY_MOVE(QPropertyChangeTrigger)
-    QQmlJavaScriptExpression * m_expression;
+
-    QObject *target = nullptr;
+    QPropertyChangeTrigger(QQmlJavaScriptExpression *expression)
+        : QPropertyObserver(&QPropertyChangeTrigger::trigger)
+        , m_expression(expression)
+    {
+    }
+
+    QPointer<QObject> target;
+    QQmlJavaScriptExpression *m_expression;
     int propertyIndex = 0;
     static void trigger(QPropertyObserver *, QUntypedPropertyData *);
 

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 = &current->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;
+    if (expression->needsPropertyChangeTrigger(engine, invalidIndex)) {
-         trigger = trigger->next) {
-        if (trigger->target == engine && trigger->propertyIndex == invalidIndex)
-            return; // already installed
-    }
         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 (expression->needsPropertyChangeTrigger(o, c)) {
-        if (trigger->target == o && trigger->propertyIndex == c)
-            return; // already installed
-    }
         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)

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:

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"
+        }
+    }
+}

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"