QProperty: Notify observers even when dependency is gone

Problem description:
--------------------
Assume we have two properties, P1 and P2. Assume further that we assign
a binding to P2, so that it depends on P1. Let the binding additionally
capture some (non-QProperty) boolean, and only create the dependency to
P1 if the boolean is true.

The state afterwards is
P1:[p1vaue|firstObserver]
                      |
                      |
                      v
                ---[p2binding]
	       /
P2:[p2value|binding]

If the boolean is set to false, and P1 changes its value, we still
correctly re-evaluate the binding and update P2's value. However, during
binding evaluation we will notice that there is no further dependency
from P2 on P1, and remove its observer.

The state afterwards is
P1:[p1vaue|firstObserver=nullptr]

                ---[p2binding]
	       /
P2:[p2value|binding]

Then, during the notify phase, we traverse the observer's again,
starting from P1's firstObserver. Given that it is nullptr now, we never
reach P2's binding, and thus won't send a notification from it.

Fix:
----

We store a list of all visited binding-observers (in a QVarLengthArray,
to avoid allocations as long as possible). After the binding evaluation
phase, we then use that list to send notifications from every binding
that we visited. As we already have a list of all bindings, we no longer
need to recurse on binding-observes during the notification process;
instead, we only need to deal with static callbacks and ChangeHandlers.

The pre-existing notification logic is still kept for the grouped update
case, where we already have a list of all delayed properties, and should
therefore not encounter the same issue. Unifying its codepath with the
existing logic is left as an exercise for a later patch.

Fixes: QTBUG-105204
Task-number: QTBUG-104982
Pick-to: 6.4 6.3 6.2
Change-Id: I2951f7d9597f4da0b8560a64dfb834f7ad86e757
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Sami Shalayel <sami.shalayel@qt.io>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

3 files changed, 146 insertions, 30 deletions

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index a635cb8afc..bade48042d 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -118,11 +118,12 @@ struct QPropertyDelayedNotifications
         Change notifications are sent later with notify (following the logic of separating
         binding updates and notifications used in non-deferred updates).
      */
-    void evaluateBindings(int index, QBindingStatus *status) {
+     [[nodiscard]] PendingBindingObserverList evaluateBindings(int index, QBindingStatus *status) {
+        PendingBindingObserverList bindingObservers;
         auto *delayed = delayedProperties + index;
         auto *bindingData = delayed->originalBindingData;
         if (!bindingData)
-            return;
+            return bindingObservers;
 
         bindingData->d_ptr = delayed->d_ptr;
         Q_ASSERT(!(bindingData->d_ptr & QPropertyBindingData::DelayedNotificationBit));
@@ -134,7 +135,8 @@ struct QPropertyDelayedNotifications
         QPropertyBindingDataPointer bindingDataPointer{bindingData};
         QPropertyObserverPointer observer = bindingDataPointer.firstObserver();
         if (observer)
-            observer.evaluateBindings(status);
+            observer.evaluateBindings(bindingObservers, status);
+        return bindingObservers;
     }
 
     /*!
@@ -216,8 +218,11 @@ void Qt::endPropertyUpdateGroup()
     // update all delayed properties
     auto start = data;
     while (data) {
-        for (int i = 0; i < data->used; ++i)
-            data->evaluateBindings(i, status);
+        for (int i = 0; i < data->used; ++i) {
+            PendingBindingObserverList bindingObserves = data->evaluateBindings(i, status);
+            Q_UNUSED(bindingObserves);
+            // ### TODO: Use bindingObservers for notify
+        }
         data = data->next;
     }
     // notify all delayed properties
@@ -271,11 +276,11 @@ void QPropertyBindingPrivate::unlinkAndDeref()
         destroyAndFreeMemory(this);
 }
 
-void QPropertyBindingPrivate::evaluateRecursive(QBindingStatus *status)
+void QPropertyBindingPrivate::evaluateRecursive(PendingBindingObserverList &bindingObservers, QBindingStatus *status)
 {
     if (!status)
         status = &bindingStatus;
-    return evaluateRecursive_inline(status);
+    return evaluateRecursive_inline(bindingObservers, status);
 }
 
 void QPropertyBindingPrivate::notifyRecursive()
@@ -294,6 +299,31 @@ void QPropertyBindingPrivate::notifyRecursive()
     updating = false;
 }
 
+void QPropertyBindingPrivate::notifyNonRecursive(const PendingBindingObserverList &bindingObservers)
+{
+    notifyNonRecursive();
+    for (auto &&bindingObserver: bindingObservers) {
+        bindingObserver.binding()->notifyNonRecursive();
+    }
+}
+
+QPropertyBindingPrivate::NotificationState QPropertyBindingPrivate::notifyNonRecursive()
+{
+    if (!pendingNotify)
+        return Delayed;
+    pendingNotify = false;
+    Q_ASSERT(!updating);
+    updating = true;
+    if (firstObserver) {
+        firstObserver.noSelfDependencies(this);
+        firstObserver.notifyOnlyChangeHandler(propertyDataPtr);
+    }
+    if (hasStaticObserver)
+        staticObserverCallback(propertyDataPtr);
+    updating = false;
+    return Sent;
+}
+
 /*!
   Constructs a null QUntypedPropertyBinding.
 
@@ -461,8 +491,9 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB
             newBindingRaw->prependObserver(observer);
         newBindingRaw->setStaticObserver(staticObserverCallback, guardCallback);
 
-        newBindingRaw->evaluateRecursive();
-        newBindingRaw->notifyRecursive();
+        PendingBindingObserverList bindingObservers;
+        newBindingRaw->evaluateRecursive(bindingObservers);
+        newBindingRaw->notifyNonRecursive(bindingObservers);
     } else if (observer) {
         d.setObservers(observer.ptr);
     } else {
@@ -565,18 +596,23 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr
         return;
     QPropertyBindingDataPointer d{this};
 
+    PendingBindingObserverList bindingObservers;
     if (QPropertyObserverPointer observer = d.firstObserver()) {
-        if (notifyObserver_helper(propertyDataPtr, observer, storage) == Evaluated) {
+        if (notifyObserver_helper(propertyDataPtr, storage, observer, bindingObservers) == Evaluated) {
             // evaluateBindings() can trash the observers. We need to re-fetch here.
             if (QPropertyObserverPointer observer = d.firstObserver())
-                observer.notify(propertyDataPtr);
+                observer.notifyOnlyChangeHandler(propertyDataPtr);
+            for (auto &&bindingObserver: bindingObservers)
+                bindingObserver.binding()->notifyNonRecursive();
         }
     }
 }
 
-QPropertyBindingData::NotificationResult QPropertyBindingData::notifyObserver_helper(
-        QUntypedPropertyData *propertyDataPtr, QPropertyObserverPointer observer,
-        QBindingStorage *storage) const
+QPropertyBindingData::NotificationResult QPropertyBindingData::notifyObserver_helper
+(
+            QUntypedPropertyData *propertyDataPtr, QBindingStorage *storage,
+            QPropertyObserverPointer observer,
+            PendingBindingObserverList &bindingObservers) const
 {
 #ifdef QT_HAS_FAST_CURRENT_THREAD_ID
     QBindingStatus *status = storage ? storage->bindingStatus : nullptr;
@@ -591,7 +627,7 @@ QPropertyBindingData::NotificationResult QPropertyBindingData::notifyObserver_he
         return Delayed;
     }
 
-    observer.evaluateBindings(status);
+    observer.evaluateBindings(bindingObservers, status);
     return Evaluated;
 }
 
@@ -724,7 +760,7 @@ void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *bindi
 }
 #endif
 
-void QPropertyObserverPointer::evaluateBindings(QBindingStatus *status)
+void QPropertyObserverPointer::evaluateBindings(PendingBindingObserverList &bindingObservers, QBindingStatus *status)
 {
     Q_ASSERT(status);
     auto observer = const_cast<QPropertyObserver*>(ptr);
@@ -733,9 +769,10 @@ void QPropertyObserverPointer::evaluateBindings(QBindingStatus *status)
         QPropertyObserver *next = observer->next.data();
 
         if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) {
+            bindingObservers.push_back(observer);
             auto bindingToEvaluate = observer->binding;
             QPropertyObserverNodeProtector protector(observer);
-            bindingToEvaluate->evaluateRecursive_inline(status);
+            bindingToEvaluate->evaluateRecursive_inline(bindingObservers, status);
             next = protector.next();
         }
 
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index a569c172c5..9c32a63fc3 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/src/corelib/kernel/qproperty_p.h
@@ -21,6 +21,7 @@
 #include <qscopedpointer.h>
 #include <qscopedvaluerollback.h>
 #include <vector>
+#include <QtCore/QVarLengthArray>
 
 QT_BEGIN_NAMESPACE
 
@@ -29,6 +30,34 @@ namespace QtPrivate {
     struct QBindingStatusAccessToken {};
 }
 
+
+/*!
+    \internal
+    Similar to \c QPropertyBindingPrivatePtr, but stores a
+    \c QPropertyObserver * linking to the QPropertyBindingPrivate*
+    instead of the QPropertyBindingPrivate* itself
+ */
+struct QBindingObserverPtr
+{
+private:
+    QPropertyObserver *d = nullptr;
+public:
+    QBindingObserverPtr() = default;
+    Q_DISABLE_COPY(QBindingObserverPtr);
+    void swap(QBindingObserverPtr &other) noexcept
+    { qt_ptr_swap(d, other.d); }
+    QBindingObserverPtr(QBindingObserverPtr &&other) : d(std::exchange(other.d, nullptr)) {}
+    QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QBindingObserverPtr);
+
+
+    inline QBindingObserverPtr(QPropertyObserver *observer);
+    inline ~QBindingObserverPtr();
+    inline QPropertyBindingPrivate *binding() const;
+    inline QPropertyObserver *operator ->();
+};
+
+using PendingBindingObserverList = QVarLengthArray<QBindingObserverPtr>;
+
 // Keep all classes related to QProperty in one compilation unit. Performance of this code is crucial and
 // we need to allow the compiler to inline where it makes sense.
 
@@ -106,19 +135,29 @@ struct QPropertyObserverPointer
     void setBindingToNotify_unsafe(QPropertyBindingPrivate *binding);
     void setChangeHandler(QPropertyObserver::ChangeHandler changeHandler);
 
+    enum class Notify {Everything, OnlyChangeHandlers};
+
+    template<Notify notifyPolicy = Notify::Everything>
     void notify(QUntypedPropertyData *propertyDataPtr);
+    void notifyOnlyChangeHandler(QUntypedPropertyData *propertyDataPtr);
 #ifndef QT_NO_DEBUG
     void noSelfDependencies(QPropertyBindingPrivate *binding);
 #else
     void noSelfDependencies(QPropertyBindingPrivate *) {}
 #endif
-    void evaluateBindings(QBindingStatus *status);
+    void evaluateBindings(PendingBindingObserverList &bindingObservers, QBindingStatus *status);
     void observeProperty(QPropertyBindingDataPointer property);
 
     explicit operator bool() const { return ptr != nullptr; }
 
     QPropertyObserverPointer nextObserver() const { return {ptr->next.data()}; }
 
+    QPropertyBindingPrivate *binding() const
+    {
+        Q_ASSERT(ptr->next.tag() == QPropertyObserver::ObserverNotifiesBinding);
+        return ptr->binding;
+    };
+
 private:
     void unlink_common()
     {
@@ -321,10 +360,21 @@ public:
 
     void unlinkAndDeref();
 
-    void evaluateRecursive(QBindingStatus *status = nullptr);
-    void Q_ALWAYS_INLINE evaluateRecursive_inline(QBindingStatus *status);
+    void evaluateRecursive(PendingBindingObserverList &bindingObservers, QBindingStatus *status = nullptr);
+
+    // ### TODO: remove as soon as declarative no longer needs this overload
+    void evaluateRecursive()
+    {
+        PendingBindingObserverList bindingObservers;
+        evaluateRecursive(bindingObservers);
+    }
+
+    void Q_ALWAYS_INLINE evaluateRecursive_inline(PendingBindingObserverList &bindingObservers, QBindingStatus *status);
 
     void notifyRecursive();
+    void notifyNonRecursive(const PendingBindingObserverList &bindingObservers);
+    enum NotificationState : bool { Delayed, Sent };
+    NotificationState notifyNonRecursive();
 
     static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding)
     { return static_cast<QPropertyBindingPrivate *>(binding.d.data()); }
@@ -566,11 +616,14 @@ public:
                 QPropertyBindingDataPointer d{bd};
                 if (QPropertyObserverPointer observer = d.firstObserver()) {
                     if (!inBindingWrapper(storage)) {
-                        if (bd->notifyObserver_helper(this, observer, storage)
+                        PendingBindingObserverList bindingObservers;
+                        if (bd->notifyObserver_helper(this, storage, observer, bindingObservers)
                                 == QtPrivate::QPropertyBindingData::Evaluated) {
                             // evaluateBindings() can trash the observers. We need to re-fetch here.
                             if (QPropertyObserverPointer observer = d.firstObserver())
-                                observer.notify(this);
+                                observer.notifyOnlyChangeHandler(this);
+                            for (auto&& bindingObserver: bindingObservers)
+                                bindingObserver.binding()->notifyNonRecursive();
                         }
                     }
                 }
@@ -727,7 +780,7 @@ struct QUntypedBindablePrivate
     }
 };
 
-inline void QPropertyBindingPrivate::evaluateRecursive_inline(QBindingStatus *status)
+inline void QPropertyBindingPrivate::evaluateRecursive_inline(PendingBindingObserverList &bindingObservers, QBindingStatus *status)
 {
     if (updating) {
         error = QPropertyBindingError(QPropertyBindingError::BindingLoop);
@@ -766,9 +819,10 @@ inline void QPropertyBindingPrivate::evaluateRecursive_inline(QBindingStatus *st
         return;
 
     firstObserver.noSelfDependencies(this);
-    firstObserver.evaluateBindings(status);
+    firstObserver.evaluateBindings(bindingObservers, status);
 }
 
+template<QPropertyObserverPointer::Notify notifyPolicy>
 inline void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr)
 {
     auto observer = const_cast<QPropertyObserver*>(ptr);
@@ -808,10 +862,12 @@ inline void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataP
         }
         case QPropertyObserver::ObserverNotifiesBinding:
         {
-            auto bindingToNotify =  observer->binding;
-            QPropertyObserverNodeProtector protector(observer);
-            bindingToNotify->notifyRecursive();
-            next = protector.next();
+            if constexpr (notifyPolicy == Notify::Everything) {
+                auto bindingToNotify =  observer->binding;
+                QPropertyObserverNodeProtector protector(observer);
+                bindingToNotify->notifyRecursive();
+                next = protector.next();
+            }
             break;
         }
         case QPropertyObserver::ObserverIsPlaceholder:
@@ -825,12 +881,29 @@ inline void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataP
     }
 }
 
+inline void QPropertyObserverPointer::notifyOnlyChangeHandler(QUntypedPropertyData *propertyDataPtr)
+{
+    notify<Notify::OnlyChangeHandlers>(propertyDataPtr);
+}
+
 inline QPropertyObserverNodeProtector::~QPropertyObserverNodeProtector()
 {
     QPropertyObserverPointer d{static_cast<QPropertyObserver *>(&m_placeHolder)};
     d.unlink_fast();
 }
 
+QBindingObserverPtr::QBindingObserverPtr(QPropertyObserver *observer) : d(observer)
+{
+    Q_ASSERT(d);
+    QPropertyObserverPointer{d}.binding()->addRef();
+}
+
+QBindingObserverPtr::~QBindingObserverPtr() { if (d)  QPropertyObserverPointer{d}.binding()->deref(); }
+
+QPropertyBindingPrivate *QBindingObserverPtr::binding() const { return QPropertyObserverPointer{d}.binding(); }
+
+QPropertyObserver *QBindingObserverPtr::operator->() { return d; }
+
 QT_END_NAMESPACE
 
 #endif // QPROPERTY_P_H
diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h
index ab69e966cf..1356a3702e 100644
--- a/src/corelib/kernel/qpropertyprivate.h
+++ b/src/corelib/kernel/qpropertyprivate.h
@@ -18,6 +18,7 @@
 #include <QtCore/qglobal.h>
 #include <QtCore/qtaggedpointer.h>
 #include <QtCore/qmetatype.h>
+#include <QtCore/qcontainerfwd.h>
 
 #include <functional>
 
@@ -28,6 +29,9 @@ class QBindingStorage;
 template<typename Class, typename T, auto Offset, auto Setter, auto Signal, auto Getter>
 class QObjectCompatProperty;
 
+struct QBindingObserverPtr;
+using PendingBindingObserverList = QVarLengthArray<QBindingObserverPtr>;
+
 namespace QtPrivate {
 // QPropertyBindingPrivatePtr operates on a RefCountingMixin solely so that we can inline
 // the constructor and copy constructor
@@ -115,6 +119,7 @@ private:
 class QUntypedPropertyBinding;
 class QPropertyBindingPrivate;
 struct QPropertyBindingDataPointer;
+class QPropertyObserver;
 struct QPropertyObserverPointer;
 
 class QUntypedPropertyData
@@ -308,8 +313,9 @@ private:
 
     enum NotificationResult { Delayed, Evaluated };
     NotificationResult notifyObserver_helper(
-            QUntypedPropertyData *propertyDataPtr, QPropertyObserverPointer observer,
-            QBindingStorage *storage) const;
+            QUntypedPropertyData *propertyDataPtr, QBindingStorage *storage,
+            QPropertyObserverPointer observer,
+            PendingBindingObserverList &bindingObservers) const;
 };
 
 template <typename T, typename Tag>