From b21dba98e3e557eece0497aeea0f0beb70cc62da Mon Sep 17 00:00:00 2001
From: Fabian Kosmale <fabian.kosmale@qt.io>
Date: Tue, 1 Dec 2020 12:01:06 +0100
Subject: [PATCH] QProperty: Avoid spurious dependencies by suspending binding
 state

Avoid spurious bindings by resetting the binding state before calling
the setter of eager properties.

Fixes: QTBUG-88999
Pick-to: 6.0
Change-Id: I1e3b5662307d906598335a21d306be9c606529d4
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
---
 src/corelib/kernel/qproperty.cpp              | 19 +++++++++++++++-
 src/corelib/kernel/qproperty.h                |  4 ++--
 src/corelib/kernel/qproperty_p.h              | 22 ++++++++++++++-----
 .../kernel/qproperty/tst_qproperty.cpp        |  1 -
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index bbcd1bf0fa..e9256c7b42 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -303,7 +303,7 @@ BindingEvaluationState::BindingEvaluationState(QPropertyBindingPrivate *binding,
     binding->clearDependencyObservers();
 }
 
-CurrentCompatProperty::CurrentCompatProperty(QBindingStatus *status, QUntypedPropertyData *property)
+CompatPropertySafePoint::CompatPropertySafePoint(QBindingStatus *status, QUntypedPropertyData *property)
     : property(property)
 {
     // store a pointer to the currentBindingEvaluationState to avoid a TLS lookup in
@@ -311,6 +311,10 @@ CurrentCompatProperty::CurrentCompatProperty(QBindingStatus *status, QUntypedPro
     currentState = &status->currentCompatProperty;
     previousState = *currentState;
     *currentState = this;
+
+    currentlyEvaluatingBindingList = &bindingStatus.currentlyEvaluatingBinding;
+    bindingState = *currentlyEvaluatingBindingList;
+    *currentlyEvaluatingBindingList = nullptr;
 }
 
 QPropertyBindingPrivate *QPropertyBindingPrivate::currentlyEvaluatingBinding()
@@ -1486,4 +1490,17 @@ QPropertyBindingData *QBindingStorage::bindingData_helper(QUntypedPropertyData *
     return QBindingStoragePrivate(d).get(data, create);
 }
 
+
+BindingEvaluationState *suspendCurrentBindingStatus()
+{
+    auto ret = bindingStatus.currentlyEvaluatingBinding;
+    bindingStatus.currentlyEvaluatingBinding = nullptr;
+    return ret;
+}
+
+void restoreBindingStatus(BindingEvaluationState *status)
+{
+    bindingStatus.currentlyEvaluatingBinding = status;
+}
+
 QT_END_NAMESPACE
diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h
index e9e0e37f3c..946014dc6d 100644
--- a/src/corelib/kernel/qproperty.h
+++ b/src/corelib/kernel/qproperty.h
@@ -783,13 +783,13 @@ public:
 namespace QtPrivate {
 
 struct BindingEvaluationState;
-struct CurrentCompatProperty;
+struct CompatPropertySafePoint;
 }
 
 struct QBindingStatus
 {
     QtPrivate::BindingEvaluationState *currentlyEvaluatingBinding = nullptr;
-    QtPrivate::CurrentCompatProperty *currentCompatProperty = nullptr;
+    QtPrivate::CompatPropertySafePoint *currentCompatProperty = nullptr;
 };
 
 struct QBindingStorageData;
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index 67ce9a74f2..e88547569e 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/src/corelib/kernel/qproperty_p.h
@@ -134,16 +134,26 @@ struct BindingEvaluationState
     BindingEvaluationState **currentState = nullptr;
 };
 
-struct CurrentCompatProperty
+/*!
+ * \internal
+ * CompatPropertySafePoint needs to be constructed before the setter of
+ * a QObjectCompatProperty runs. It prevents spurious binding dependencies
+ * caused by reads of properties inside the compat property setter.
+ * Moreover, it ensures that we don't destroy bindings when using operator=
+ */
+struct CompatPropertySafePoint
 {
-    Q_CORE_EXPORT CurrentCompatProperty(QBindingStatus *status, QUntypedPropertyData *property);
-    ~CurrentCompatProperty()
+    Q_CORE_EXPORT CompatPropertySafePoint(QBindingStatus *status, QUntypedPropertyData *property);
+    ~CompatPropertySafePoint()
     {
         *currentState = previousState;
+        *currentlyEvaluatingBindingList = bindingState;
     }
     QUntypedPropertyData *property;
-    CurrentCompatProperty *previousState = nullptr;
-    CurrentCompatProperty **currentState = nullptr;
+    CompatPropertySafePoint *previousState = nullptr;
+    CompatPropertySafePoint **currentState = nullptr;
+    QtPrivate::BindingEvaluationState **currentlyEvaluatingBindingList = nullptr;
+    QtPrivate::BindingEvaluationState *bindingState = nullptr;
 };
 
 }
@@ -360,7 +370,7 @@ class QObjectCompatProperty : public QPropertyData<T>
                 return false;
         // ensure value and setValue know we're currently evaluating our binding
         QBindingStorage *storage = qGetBindingStorage(thisData->owner());
-        QtPrivate::CurrentCompatProperty guardThis(storage->bindingStatus, thisData);
+        QtPrivate::CompatPropertySafePoint guardThis(storage->bindingStatus, thisData);
         (thisData->owner()->*Setter)(copy.valueBypassingBindings());
         return true;
     }
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index d824d39941..8145b066da 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -1401,7 +1401,6 @@ void tst_QProperty::noFakeDependencies()
     QCOMPARE(slotCounter, 1); // sanity check
     int old = bindingFunctionCalled;
     fdc.setProp3(100);
-    QEXPECT_FAIL("", "Known to create a spurious dependency", Continue);
     QCOMPARE(old, bindingFunctionCalled);
 }