From 1e883cf9b56376084f3329811aa91f4887ef6fcb Mon Sep 17 00:00:00 2001
From: Fabian Kosmale <fabian.kosmale@qt.io>
Date: Thu, 24 Sep 2020 17:23:55 +0200
Subject: [PATCH] Prevent endless markDirtyAndNotifyObservers <-> notify loop

Before we had the option of eager evaluation, we were able to use the
dirty flag to detect whether we are recursing. However, eager properties
will lead to a evaluateIfDirtyAndReturnTrueIfValueChanged call, and that
in turn will clear the dirty flag.
Introduce a new member to detect that situation, and set the bindings
error state to BindingLoop if we detect that kind of loop.

Change-Id: If40b93221848bd9e9422502318d992fad95b0b74
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
---
 src/corelib/kernel/qproperty.cpp              |  8 +++++
 src/corelib/kernel/qproperty_p.h              | 11 +++++--
 .../kernel/qproperty/tst_qproperty.cpp        | 32 +++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index 72f74ac1e82..81871587e8f 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -41,6 +41,7 @@
 #include "qproperty_p.h"
 
 #include <qscopedvaluerollback.h>
+#include <QScopeGuard>
 
 QT_BEGIN_NAMESPACE
 
@@ -82,6 +83,13 @@ void QPropertyBindingPrivate::markDirtyAndNotifyObservers()
     if (dirty)
         return;
     dirty = true;
+    if (eagerlyUpdating) {
+        error = QPropertyBindingError(QPropertyBindingError::BindingLoop);
+        return;
+    }
+
+    eagerlyUpdating = true;
+    QScopeGuard guard([&](){eagerlyUpdating = false;});
     if (requiresEagerEvaluation()) {
         // these are compat properties that we will need to evaluate eagerly
         evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index c5c74147c14..999760ec862 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/src/corelib/kernel/qproperty_p.h
@@ -163,10 +163,15 @@ private:
     using ObserverArray = std::array<QPropertyObserver, 4>;
 
     // QSharedData is 4 bytes. Use the padding for the bools as we need 8 byte alignment below.
+
+    // a dependent property has changed, and the binding needs to be reevaluated on access
     bool dirty = false;
+    // used to detect binding loops for lazy evaluated properties
     bool updating = false;
     bool hasStaticObserver = false;
-    bool hasBindingWrapper = false;
+    bool hasBindingWrapper:1;
+    // used to detect binding loops for eagerly evaluated properties
+    bool eagerlyUpdating:1;
 
     QUntypedPropertyBinding::BindingEvaluationFunction evaluationFunction;
 
@@ -192,7 +197,9 @@ public:
 
     QPropertyBindingPrivate(QMetaType metaType, QUntypedPropertyBinding::BindingEvaluationFunction evaluationFunction,
                             const QPropertyBindingSourceLocation &location)
-        : evaluationFunction(std::move(evaluationFunction))
+        : hasBindingWrapper(false)
+        , eagerlyUpdating(false)
+        , evaluationFunction(std::move(evaluationFunction))
         , inlineDependencyObservers() // Explicit initialization required because of union
         , location(location)
         , metaType(metaType)
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index 9bc2ce26b64..0394e9ccf7f 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -514,6 +514,31 @@ void tst_QProperty::bindingError()
     QCOMPARE(prop.binding().error().description(), QString("my error"));
 }
 
+
+
+class BindingLoopTester : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(int eagerProp READ eagerProp WRITE setEagerProp BINDABLE bindableEagerProp)
+    Q_PROPERTY(int eagerProp2 READ eagerProp2 WRITE setEagerProp2 BINDABLE bindableEagerProp2)
+    public:
+    BindingLoopTester(QProperty<int> *i, QObject *parent = nullptr) : QObject(parent) {
+        eagerData.setBinding(Qt::makePropertyBinding([&](){ return eagerData2.value() + i->value(); }  ) );
+        eagerData2.setBinding(Qt::makePropertyBinding([&](){ return eagerData.value(); }  ) );
+        i->setValue(42);
+    }
+
+    int eagerProp() {return eagerData.value();}
+    void setEagerProp(int i) { eagerData = i; }
+    QBindable<int> bindableEagerProp() {return QBindable<int>(&eagerData);}
+    Q_OBJECT_COMPAT_PROPERTY(BindingLoopTester, int, eagerData, &BindingLoopTester::setEagerProp)
+
+    int eagerProp2() {return eagerData2.value();}
+    void setEagerProp2(int i) { eagerData2 = i; }
+    QBindable<int> bindableEagerProp2() {return QBindable<int>(&eagerData2);}
+    Q_OBJECT_COMPAT_PROPERTY(BindingLoopTester, int, eagerData2, &BindingLoopTester::setEagerProp2)
+};
+
 void tst_QProperty::bindingLoop()
 {
     QScopedPointer<QProperty<int>> firstProp;
@@ -533,6 +558,13 @@ void tst_QProperty::bindingLoop()
 
     QCOMPARE(thirdProp.value(), 0);
     QCOMPARE(secondProp.binding().error().type(), QPropertyBindingError::BindingLoop);
+
+
+    QProperty<int> i;
+    BindingLoopTester tester(&i);
+    QCOMPARE(tester.bindableEagerProp().binding().error().type(), QPropertyBindingError::BindingLoop);
+    QEXPECT_FAIL("", "Only the first property in a dependency cycle is set to the error state", Continue);
+    QCOMPARE(tester.bindableEagerProp2().binding().error().type(), QPropertyBindingError::BindingLoop);
 }
 
 void tst_QProperty::changePropertyFromWithinChangeHandler()