QQmlObjectCreator: Correctly remove overwritten bindings

For QProperty bindings, we would not explicitly remove bindings that were
added and later overwritten.
For "complex" bindings, that would cause some unnecessary binding
evaluation during object creation.
Worse, in the case of a literal binding overwriting a complex binding,
we would acutally end up with the complex binding still being there,
because the literal binding would result in an immediate write of the
value, whereas the binding was only installed at a later point.
We fix this by removing the binding from the list of to-be-installed
bindings when it gets overwritten.
We also now take the IsPropertyObserver flag into consideration when
checking whether existing bindings should be removed: IsPropertyObserver
is basically the same as IsSignalHandlerExpression, but for QProperty
bindings instead of "old-style" bindings.

Pick-to: 6.2
Fixes: QTBUG-96668
Change-Id: I2ef00d5b62af4f6fcc71960c38e1f0568b3b9c40
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

5 files changed, 48 insertions, 5 deletions

diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index 1ead5e2866..ee8346a5e5 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -873,10 +873,20 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *bindingProper
         }
     }
 
-    if (_ddata->hasBindingBit(bindingProperty->coreIndex()) && !(binding->flags & QV4::CompiledData::Binding::IsSignalHandlerExpression)
-        && !(binding->flags & QV4::CompiledData::Binding::IsOnAssignment)
-        && !_valueTypeProperty)
+    const bool allowedToRemoveBinding = !(binding->flags & QV4::CompiledData::Binding::IsSignalHandlerExpression)
+            && !(binding->flags & QV4::CompiledData::Binding::IsOnAssignment)
+            && !(binding->flags & QV4::CompiledData::Binding::IsPropertyObserver)
+            && !_valueTypeProperty;
+
+    if (_ddata->hasBindingBit(bindingProperty->coreIndex()) && allowedToRemoveBinding) {
         QQmlPropertyPrivate::removeBinding(_bindingTarget, QQmlPropertyIndex(bindingProperty->coreIndex()));
+    } else if (bindingProperty->isBindable() && allowedToRemoveBinding) {
+        QList<DeferredQPropertyBinding> &pendingBindings = sharedState.data()->allQPropertyBindings;
+        auto it = std::remove_if(pendingBindings.begin(), pendingBindings.end(), [&](const DeferredQPropertyBinding &deferred) {
+            return deferred.properyIndex == bindingProperty->coreIndex() && deferred.target == _bindingTarget;
+        });
+        pendingBindings.erase(it, pendingBindings.end());
+    }
 
     if (binding->type == QV4::CompiledData::Binding::Type_Script || binding->isTranslationBinding()) {
         if (binding->flags & QV4::CompiledData::Binding::IsSignalHandlerExpression
@@ -920,7 +930,7 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *bindingProper
                 QQmlPropertyIndex index(bindingProperty->coreIndex(), -1);
                 qmlBinding = QQmlPropertyBinding::create(bindingProperty, runtimeFunction, _scopeObject, context, currentQmlContext(), _bindingTarget, index);
             }
-            sharedState.data()->allQPropertyBindings.emplaceBack(_bindingTarget, bindingProperty->coreIndex(), qmlBinding);
+            sharedState.data()->allQPropertyBindings.push_back(DeferredQPropertyBinding {_bindingTarget, bindingProperty->coreIndex(), qmlBinding });
         } else {
             // When writing bindings to grouped properties implemented as value types,
             // such as point.x: { someExpression; }, then the binding is installed on
diff --git a/src/qml/qml/qqmlobjectcreator_p.h b/src/qml/qml/qqmlobjectcreator_p.h
index e3b848624c..03e1ad7169 100644
--- a/src/qml/qml/qqmlobjectcreator_p.h
+++ b/src/qml/qml/qqmlobjectcreator_p.h
@@ -90,6 +90,12 @@ struct RequiredPropertyInfo
 
 class RequiredProperties : public QHash<QQmlPropertyData*, RequiredPropertyInfo> {};
 
+struct DeferredQPropertyBinding {
+    QObject *target = nullptr;
+    int properyIndex = -1;
+    QUntypedPropertyBinding binding;
+};
+
 struct QQmlObjectCreatorSharedState : QQmlRefCount
 {
     QQmlRefPointer<QQmlContextData> rootContext;
@@ -103,7 +109,7 @@ struct QQmlObjectCreatorSharedState : QQmlRefCount
     QQmlVmeProfiler profiler;
     QRecursionNode recursionNode;
     RequiredProperties requiredProperties;
-    QList<std::tuple<QObject *, int, QUntypedPropertyBinding>> allQPropertyBindings;
+    QList<DeferredQPropertyBinding> allQPropertyBindings;
     bool hadRequiredProperties;
 };
 
diff --git a/tests/auto/qml/qqmlecmascript/data/PreNamed.qml b/tests/auto/qml/qqmlecmascript/data/PreNamed.qml
new file mode 100644
index 0000000000..dc991e674f
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/PreNamed.qml
@@ -0,0 +1,11 @@
+import QtQml
+
+QtObject {
+    property QtObject other: QtObject {
+        objectName: "original"
+    }
+    objectName: other.objectName
+    function updateOriginal() {
+        other.objectName = "updated"
+    }
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/qpropertyBindingReplacement.qml b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingReplacement.qml
new file mode 100644
index 0000000000..478a88678b
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingReplacement.qml
@@ -0,0 +1,6 @@
+import QtQml
+
+PreNamed {
+    objectName: "overwritten"
+    Component.onCompleted: updateOriginal()
+}
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index 83d7a5ee03..25c0166fa0 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -312,6 +312,7 @@ private slots:
     void replaceBinding();
     void bindingBoundFunctions();
     void qpropertyAndQtBinding();
+    void qpropertyBindingReplacement();
     void deleteRootObjectInCreation();
     void onDestruction();
     void onDestructionViaGC();
@@ -7615,6 +7616,15 @@ void tst_qqmlecmascript::qpropertyAndQtBinding()
     QCOMPARE(root->complex.value(), 150);
 }
 
+void tst_qqmlecmascript::qpropertyBindingReplacement()
+{
+    QQmlEngine engine;
+    QQmlComponent c(&engine, testFileUrl("qpropertyBindingReplacement.qml"));
+    QScopedPointer<QObject> root(c.create());
+    QVERIFY(root);
+    QCOMPARE(root->objectName(), u"overwritten"_qs);
+}
+
 void tst_qqmlecmascript::deleteRootObjectInCreation()
 {
     QQmlEngine engine;