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.

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>
(cherry picked from commit 5ff83606a1c1365b95cd12ef9d45bc4e4eb13ecd)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_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 449552c9c3..f7f14a0e8f 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 50489fd55b..4e788e88d4 100644
--- a/src/qml/qml/qqmlobjectcreator_p.h
+++ b/src/qml/qml/qqmlobjectcreator_p.h
@@ -89,6 +89,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;
@@ -102,7 +108,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 976bf39ed2..7ecdcd35fd 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -309,6 +309,7 @@ private slots:
     void replaceBinding();
     void bindingBoundFunctions();
     void qpropertyAndQtBinding();
+    void qpropertyBindingReplacement();
     void deleteRootObjectInCreation();
     void onDestruction();
     void onDestructionViaGC();
@@ -7574,6 +7575,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;