From 4dfcaa7ee8273914ea8cd9fd232064ce95cb15d1 Mon Sep 17 00:00:00 2001
From: Ulf Hermann <ulf.hermann@qt.io>
Date: Mon, 2 Jan 2023 21:48:12 +0100
Subject: [PATCH] QQmlObjectCreator: Do not crash on read-only bindable

If the binding was not actually set (because the bindable is readonly)
then it's dead after the pop_front. We cannot examine it anymore, and we
don't have to.

Pick-to: 6.5 6.4 6.2
Fixes: QTBUG-109597
Change-Id: I3bf0ca501aa9ad45a64ad181b685ca6d9d325231
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
---
 src/qml/qml/qqmlobjectcreator.cpp             | 24 ++++++++++++-------
 .../qqmlecmascript/data/readOnlyBindable.qml  |  7 ++++++
 tests/auto/qml/qqmlecmascript/testtypes.cpp   |  1 +
 tests/auto/qml/qqmlecmascript/testtypes.h     | 19 +++++++++++++++
 .../qml/qqmlecmascript/tst_qqmlecmascript.cpp | 18 ++++++++++++++
 5 files changed, 61 insertions(+), 8 deletions(-)
 create mode 100644 tests/auto/qml/qqmlecmascript/data/readOnlyBindable.qml

diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index 78c248742e..c014c24d43 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -1442,16 +1442,24 @@ bool QQmlObjectCreator::finalize(QQmlInstantiationInterrupt &interrupt)
         void *argv[] = { &bindable };
         // allow interception
         target->metaObject()->metacall(target, QMetaObject::BindableProperty, index, argv);
-        bindable.setBinding(qmlBinding);
+        const bool success = bindable.setBinding(qmlBinding);
+
+        // Only pop_front after setting the binding as the bindings are refcounted.
         sharedState->allQPropertyBindings.pop_front();
-        if (auto priv = QPropertyBindingPrivate::get(qmlBinding); priv->hasCustomVTable()) {
-            auto qmlBindingPriv = static_cast<QQmlPropertyBinding *>(priv);
-            auto jsExpression = qmlBindingPriv->jsExpression();
-            const bool canRemove = !qmlBinding.error().hasError() && !qmlBindingPriv->hasDependencies()
-                    && !jsExpression->hasUnresolvedNames();
-            if (canRemove)
-                bindable.takeBinding();
+
+        // If the binding was actually not set, it's deleted now.
+        if (success) {
+            if (auto priv = QPropertyBindingPrivate::get(qmlBinding); priv->hasCustomVTable()) {
+                auto qmlBindingPriv = static_cast<QQmlPropertyBinding *>(priv);
+                auto jsExpression = qmlBindingPriv->jsExpression();
+                const bool canRemove = !qmlBinding.error().hasError()
+                        && !qmlBindingPriv->hasDependencies()
+                        && !jsExpression->hasUnresolvedNames();
+                if (canRemove)
+                    bindable.takeBinding();
+            }
         }
+
         if (watcher.hasRecursed() || interrupt.shouldInterrupt())
             return false;
     }
diff --git a/tests/auto/qml/qqmlecmascript/data/readOnlyBindable.qml b/tests/auto/qml/qqmlecmascript/data/readOnlyBindable.qml
new file mode 100644
index 0000000000..116036c9ff
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/readOnlyBindable.qml
@@ -0,0 +1,7 @@
+import Qt.test
+import QtQuick
+
+ReadOnlyBindable {
+    property int v: 12
+    x: v
+}
diff --git a/tests/auto/qml/qqmlecmascript/testtypes.cpp b/tests/auto/qml/qqmlecmascript/testtypes.cpp
index 0b26979c4d..40f5e5cf5c 100644
--- a/tests/auto/qml/qqmlecmascript/testtypes.cpp
+++ b/tests/auto/qml/qqmlecmascript/testtypes.cpp
@@ -541,6 +541,7 @@ void registerTypes()
 
     qmlRegisterType<Receiver>("Qt.test", 1,0, "Receiver");
     qmlRegisterType<Sender>("Qt.test", 1,0, "Sender");
+    qmlRegisterTypesAndRevisions<ReadOnlyBindable>("Qt.test", 1);
 }
 
 #include "testtypes.moc"
diff --git a/tests/auto/qml/qqmlecmascript/testtypes.h b/tests/auto/qml/qqmlecmascript/testtypes.h
index 0c8bf7f609..ff9dda36d1 100644
--- a/tests/auto/qml/qqmlecmascript/testtypes.h
+++ b/tests/auto/qml/qqmlecmascript/testtypes.h
@@ -1981,6 +1981,25 @@ public slots:
     int slot1(int i, int j, int k) {return i+j+k;}
 };
 
+class ReadOnlyBindable : public QObject
+{
+    Q_OBJECT
+    QML_ELEMENT
+    Q_PROPERTY(int x READ x WRITE setX BINDABLE bindableX)
+    Q_OBJECT_BINDABLE_PROPERTY(ReadOnlyBindable, int, _xProp)
+
+public:
+    ReadOnlyBindable(QObject *parent = nullptr)
+        : QObject(parent)
+    {
+        setX(7);
+    }
+
+    int x() const { return _xProp.value(); }
+    void setX(int x) { _xProp.setValue(x); }
+    QBindable<int> bindableX() const { return &_xProp; }
+};
+
 void registerTypes();
 
 #endif // TESTTYPES_H
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index 66d9dc7324..edb1e9ba80 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -414,6 +414,8 @@ private slots:
     void internalClassParentGc();
     void methodTypeMismatch();
 
+    void doNotCrashOnReadOnlyBindable();
+
 private:
 //    static void propertyVarWeakRefCallback(v8::Persistent<v8::Value> object, void* parameter);
     static void verifyContextLifetime(const QQmlRefPointer<QQmlContextData> &ctxt);
@@ -10355,6 +10357,22 @@ void tst_qqmlecmascript::methodTypeMismatch()
     QCOMPARE(object->actuals(), QVariantList() << QVariant::fromValue((QObject *)nullptr));
 }
 
+void tst_qqmlecmascript::doNotCrashOnReadOnlyBindable()
+{
+    QQmlEngine engine;
+    QQmlComponent c(&engine, testFileUrl("readOnlyBindable.qml"));
+    QVERIFY2(c.isReady(), qPrintable(c.errorString()));
+#ifndef QT_NO_DEBUG
+    QTest::ignoreMessage(
+                QtWarningMsg,
+                "setBinding: Could not set binding via bindable interface. "
+                "The QBindable is read-only.");
+#endif
+    QScopedPointer<QObject> o(c.create());
+    QVERIFY(o);
+    QCOMPARE(o->property("x").toInt(), 7);
+}
+
 QTEST_MAIN(tst_qqmlecmascript)
 
 #include "tst_qqmlecmascript.moc"