1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QProperty: Notify observers even when dependency is gone

Browse Source 
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
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>
(cherry picked from commit f1b1773d0ae636fa9afa36224ba17566484af3cc)
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
This commit is contained in:
Fabian Kosmale 2022-07-26 15:09:26 +02:00 committed by Ulf Hermann
parent 41eebd4778
commit 83bfe979c7
4 changed files with 235 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

144
src/corelib/kernel/qproperty.cpp
View File

@ -154,11 +154,13 @@ 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(qsizetype index, QBindingStatus *status) {
[[nodiscard]] PendingBindingObserverList evaluateBindings(
qsizetype 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));
@ -170,7 +172,8 @@ struct QPropertyDelayedNotifications
QPropertyBindingDataPointer bindingDataPointer{bindingData};
QPropertyObserverPointer observer = bindingDataPointer.firstObserver();
if (observer)
observer.evaluateBindings(status);
observer.evaluateBindings(bindingObservers, status);
return bindingObservers;
}
/*!
@ -252,8 +255,11 @@ void Qt::endPropertyUpdateGroup()
// update all delayed properties
auto start = data;
while (data) {
for (qsizetype i = 0; i < data->used; ++i)
data->evaluateBindings(i, status);
for (qsizetype i = 0; i < data->used; ++i) {
PendingBindingObserverList bindingObservers = data->evaluateBindings(i, status);
Q_UNUSED(bindingObservers);
// ### TODO: Use bindingObservers for notify
}
data = data->next;
}
// notify all delayed properties
@ -289,11 +295,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()
@ -312,6 +318,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.
@ -479,8 +510,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 {
@ -588,6 +620,7 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr
return;
QPropertyBindingDataPointer d{this};
PendingBindingObserverList bindingObservers;
if (QPropertyObserverPointer observer = d.firstObserver()) {
auto status = storage ? storage->bindingStatus : nullptr;
QPropertyDelayedNotifications *delay;
@ -602,14 +635,16 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr
delay->addProperty(this, propertyDataPtr);
return;
}
observer.evaluateBindings(status);
observer.evaluateBindings(bindingObservers, status);
} else {
return;
}
// 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();
}
int QPropertyBindingDataPointer::observerCount() const
@ -735,90 +770,18 @@ void QPropertyObserverPointer::setBindingToNotify_unsafe(QPropertyBindingPrivate
}
/*!
\class QPropertyObserverNodeProtector
\internal
QPropertyObserverNodeProtector is a RAII wrapper which takes care of the internal switching logic
for QPropertyObserverPointer::notify (described ibidem)
*/
struct [[nodiscard]] QPropertyObserverNodeProtector {
QPropertyObserverBase m_placeHolder;
QPropertyObserverNodeProtector(QPropertyObserver *observer)
{
// insert m_placeholder after observer into the linked list
QPropertyObserver *next = observer->next.data();
m_placeHolder.next = next;
observer->next = static_cast<QPropertyObserver *>(&m_placeHolder);
if (next)
next->prev = &m_placeHolder.next;
m_placeHolder.prev = &observer->next;
m_placeHolder.next.setTag(QPropertyObserver::ObserverIsPlaceholder);
}
QPropertyObserver *next() const { return m_placeHolder.next.data(); }
~QPropertyObserverNodeProtector() {
QPropertyObserverPointer d{static_cast<QPropertyObserver *>(&m_placeHolder)};
d.unlink_fast();
}
};
/*! \internal
/*!
\fn QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr)
\internal
\a propertyDataPtr is a pointer to the observed property's property data
*/
void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr)
{
auto observer = const_cast<QPropertyObserver*>(ptr);
/*
* The basic idea of the loop is as follows: We iterate over all observers in the linked list,
* and execute the functionality corresponding to their tag.
* However, complication arise due to the fact that the triggered operations might modify the list,
* which includes deletion and move of the current and next nodes.
* Therefore, we take a few safety precautions:
* 1. Before executing any action which might modify the list, we insert a placeholder node after the current node.
* As that one is stack allocated and owned by us, we can rest assured that it is
* still there after the action has executed, and placeHolder->next points to the actual next node in the list.
* Note that taking next at the beginning of the loop does not work, as the executed action might either move
* or delete that node.
* 2. After the triggered action has finished, we can use the next pointer in the placeholder node as a safe way to
* retrieve the next node.
* 3. Some care needs to be taken to avoid infinite recursion with change handlers, so we add an extra test there, that
* checks whether we're already have the same change handler in our call stack. This can be done by checking whether
* the node after the current one is a placeholder node.
*/
while (observer) {
QPropertyObserver *next = observer->next.data();
switch (QPropertyObserver::ObserverTag(observer->next.tag())) {
case QPropertyObserver::ObserverNotifiesChangeHandler:
{
auto handlerToCall = observer->changeHandler;
// prevent recursion
if (next && next->next.tag() == QPropertyObserver::ObserverIsPlaceholder) {
observer = next->next.data();
continue;
}
// handlerToCall might modify the list
QPropertyObserverNodeProtector protector(observer);
handlerToCall(observer, propertyDataPtr);
next = protector.next();
break;
}
case QPropertyObserver::ObserverNotifiesBinding:
{
auto bindingToNotify = observer->binding;
QPropertyObserverNodeProtector protector(observer);
bindingToNotify->notifyRecursive();
next = protector.next();
break;
}
case QPropertyObserver::ObserverIsPlaceholder:
// recursion is already properly handled somewhere else
break;
case QPropertyObserver::ObserverIsAlias:
break;
default: Q_UNREACHABLE();
}
observer = next;
}
}
#ifndef QT_NO_DEBUG
void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *binding)
@ -838,7 +801,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);
@ -847,9 +810,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();
}

158
src/corelib/kernel/qproperty_p.h
View File

@ -57,6 +57,7 @@
#include <qscopedpointer.h>
#include <qscopedvaluerollback.h>
#include <vector>
#include <QtCore/QVarLengthArray>
QT_BEGIN_NAMESPACE
@ -65,6 +66,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.
@ -98,6 +127,25 @@ struct Q_AUTOTEST_EXPORT QPropertyBindingDataPointer
}
};
struct [[nodiscard]] QPropertyObserverNodeProtector {
QPropertyObserverBase m_placeHolder;
QPropertyObserverNodeProtector(QPropertyObserver *observer)
{
// insert m_placeholder after observer into the linked list
QPropertyObserver *next = observer->next.data();
m_placeHolder.next = next;
observer->next = static_cast<QPropertyObserver *>(&m_placeHolder);
if (next)
next->prev = &m_placeHolder.next;
m_placeHolder.prev = &observer->next;
m_placeHolder.next.setTag(QPropertyObserver::ObserverIsPlaceholder);
}
QPropertyObserver *next() const { return m_placeHolder.next.data(); }
~QPropertyObserverNodeProtector();
};
// This is a helper "namespace"
struct QPropertyObserverPointer
{
@ -110,18 +158,28 @@ 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;
};
};
class QPropertyBindingErrorPrivate : public QSharedData
@ -329,10 +387,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()); }
@ -700,7 +769,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);
@ -739,9 +808,88 @@ inline void QPropertyBindingPrivate::evaluateRecursive_inline(QBindingStatus *st
return;
firstObserver.noSelfDependencies(this);
firstObserver.evaluateBindings(status);
firstObserver.evaluateBindings(bindingObservers, status);
}
template<QPropertyObserverPointer::Notify notifyPolicy>
void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr)
{
auto observer = const_cast<QPropertyObserver*>(ptr);
/*
* The basic idea of the loop is as follows: We iterate over all observers in the linked list,
* and execute the functionality corresponding to their tag.
* However, complication arise due to the fact that the triggered operations might modify the list,
* which includes deletion and move of the current and next nodes.
* Therefore, we take a few safety precautions:
* 1. Before executing any action which might modify the list, we insert a placeholder node after the current node.
* As that one is stack allocated and owned by us, we can rest assured that it is
* still there after the action has executed, and placeHolder->next points to the actual next node in the list.
* Note that taking next at the beginning of the loop does not work, as the executed action might either move
* or delete that node.
* 2. After the triggered action has finished, we can use the next pointer in the placeholder node as a safe way to
* retrieve the next node.
* 3. Some care needs to be taken to avoid infinite recursion with change handlers, so we add an extra test there, that
* checks whether we're already have the same change handler in our call stack. This can be done by checking whether
* the node after the current one is a placeholder node.
*/
while (observer) {
QPropertyObserver *next = observer->next.data();
switch (QPropertyObserver::ObserverTag(observer->next.tag())) {
case QPropertyObserver::ObserverNotifiesChangeHandler:
{
auto handlerToCall = observer->changeHandler;
// prevent recursion
if (next && next->next.tag() == QPropertyObserver::ObserverIsPlaceholder) {
observer = next->next.data();
continue;
}
// handlerToCall might modify the list
QPropertyObserverNodeProtector protector(observer);
handlerToCall(observer, propertyDataPtr);
next = protector.next();
break;
}
case QPropertyObserver::ObserverNotifiesBinding:
{
if constexpr (notifyPolicy == Notify::Everything) {
auto bindingToNotify = observer->binding;
QPropertyObserverNodeProtector protector(observer);
bindingToNotify->notifyRecursive();
next = protector.next();
}
break;
}
case QPropertyObserver::ObserverIsPlaceholder:
// recursion is already properly handled somewhere else
break;
case QPropertyObserver::ObserverIsAlias:
break;
default: Q_UNREACHABLE();
}
observer = next;
}
}
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

4
src/corelib/kernel/qpropertyprivate.h
View File

@ -54,6 +54,7 @@
#include <QtCore/qglobal.h>
#include <QtCore/qtaggedpointer.h>
#include <QtCore/qmetatype.h>
#include <QtCore/qcontainerfwd.h>
#include <functional>
@ -64,6 +65,9 @@ class QBindingStorage;
template<typename Class, typename T, auto Offset, auto Setter, auto Signal>
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

24
tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
View File

@ -121,6 +121,8 @@ private slots:
void qpropertyAlias();
void scheduleNotify();
void notifyAfterAllDepsGone();
};
void tst_QProperty::functorBinding()
@ -2029,6 +2031,28 @@ void tst_QProperty::scheduleNotify()
QCOMPARE(p.value(), 0);
}
void tst_QProperty::notifyAfterAllDepsGone()
{
bool b = true;
QProperty<int> iprop;
QProperty<int> jprop(42);
iprop.setBinding([&](){
if (b)
return jprop.value();
return 13;
});
int changeCounter = 0;
auto keepAlive = iprop.onValueChanged([&](){ changeCounter++; });
QCOMPARE(iprop.value(), 42);
jprop = 44;
QCOMPARE(iprop.value(), 44);
QCOMPARE(changeCounter, 1);
b = false;
jprop = 43;
QCOMPARE(iprop.value(), 13);
QCOMPARE(changeCounter, 2);
}
QTEST_MAIN(tst_QProperty);
#undef QT_SOURCE_LOCATION_NAMESPACE