summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/corelib/kernel/qproperty.cpp142
-rw-r--r--src/corelib/kernel/qproperty.h11
-rw-r--r--src/corelib/kernel/qproperty_p.h7
-rw-r--r--tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp53
4 files changed, 188 insertions, 25 deletions
diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index 14ee881455..d25952c0bf 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -370,22 +370,27 @@ void QPropertyObserver::setSource(const QPropertyBindingData &property)
d.observeProperty(propPrivate);
}
-
QPropertyObserver::~QPropertyObserver()
{
+ if (next.tag() == ActivelyExecuting) {
+ if (nodeState)
+ *nodeState = nullptr;
+ }
QPropertyObserverPointer d{this};
d.unlink();
}
QPropertyObserver::QPropertyObserver(QPropertyObserver &&other)
{
- std::swap(bindingToMarkDirty, other.bindingToMarkDirty);
- std::swap(next, other.next);
- std::swap(prev, other.prev);
+ bindingToMarkDirty = std::exchange(other.bindingToMarkDirty, {});
+ next = std::exchange(other.next, {});
+ prev = std::exchange(other.prev, {});
if (next)
next->prev = &next;
if (prev)
prev.setPointer(this);
+ if (next.tag() == ActivelyExecuting)
+ *nodeState = this;
}
QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other)
@@ -397,20 +402,22 @@ QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other)
d.unlink();
bindingToMarkDirty = nullptr;
- std::swap(bindingToMarkDirty, other.bindingToMarkDirty);
- std::swap(next, other.next);
- std::swap(prev, other.prev);
+ bindingToMarkDirty = std::exchange(other.bindingToMarkDirty, {});
+ next = std::exchange(other.next, {});
+ prev = std::exchange(other.prev, {});
if (next)
next->prev = &next;
if (prev)
prev.setPointer(this);
+ if (next.tag() == ActivelyExecuting)
+ *nodeState = this;
return *this;
}
void QPropertyObserverPointer::unlink()
{
- if (ptr->next.tag() & QPropertyObserver::ObserverNotifiesAlias)
+ if (ptr->next.tag() == QPropertyObserver::ObserverNotifiesAlias)
ptr->aliasedPropertyData = nullptr;
if (ptr->next)
ptr->next->prev = ptr->prev;
@@ -422,22 +429,81 @@ void QPropertyObserverPointer::unlink()
void QPropertyObserverPointer::setChangeHandler(QPropertyObserver::ChangeHandler changeHandler)
{
+ Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
ptr->changeHandler = changeHandler;
ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
}
void QPropertyObserverPointer::setAliasedProperty(QUntypedPropertyData *property)
{
+ Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
ptr->aliasedPropertyData = property;
ptr->next.setTag(QPropertyObserver::ObserverNotifiesAlias);
}
void QPropertyObserverPointer::setBindingToMarkDirty(QPropertyBindingPrivate *binding)
{
+ Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
ptr->bindingToMarkDirty = binding;
ptr->next.setTag(QPropertyObserver::ObserverNotifiesBinding);
}
+/*!
+ \internal
+ QPropertyObserverNodeProtector is a RAII wrapper which takes care of the internal switching logic
+ for QPropertyObserverPointer::notify (described ibidem)
+*/
+template <QPropertyObserver::ObserverTag tag>
+struct QPropertyObserverNodeProtector {
+ QPropertyObserver m_placeHolder;
+ QPropertyObserver *&m_observer;
+ union {
+ QPropertyBindingPrivate *m_binding;
+ QPropertyObserver::ChangeHandler m_changeHandler;
+ };
+ Q_REQUIRED_RESULT QPropertyObserverNodeProtector(QPropertyObserver *&observer)
+ : m_observer(observer)
+ {
+ static_assert(tag == QPropertyObserver::ObserverNotifiesBinding ||
+ tag == QPropertyObserver::ObserverNotifiesChangeHandler);
+ if constexpr (tag == QPropertyObserver::ObserverNotifiesBinding)
+ m_binding = m_observer->bindingToMarkDirty;
+ else
+ m_changeHandler = m_observer->changeHandler;
+ switchNodes(m_placeHolder, m_observer);
+ m_observer->nodeState = &m_observer;
+ m_observer->next.setTag(QPropertyObserver::ActivelyExecuting);
+ m_placeHolder.next.setTag(QPropertyObserver::ActivelyExecuting);
+ }
+
+ ~QPropertyObserverNodeProtector() {
+ if (m_observer) {
+ if constexpr (tag == QPropertyObserver::ObserverNotifiesBinding)
+ m_observer->bindingToMarkDirty = m_binding;
+ else
+ m_observer->changeHandler = m_changeHandler;
+ switchNodes(*m_observer, &m_placeHolder);
+ m_observer->next.setTag(tag);
+ }
+ // set tag to a safer value where we don't execute anything in the dtor
+ m_placeHolder.next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
+ }
+
+ /*!
+ \internal
+ replaces a node \a observer in the list with another node \a placeholder which must not be in the list
+ */
+ static void switchNodes(QPropertyObserver &placeHolder, QPropertyObserver *observer) {
+ placeHolder.next = std::exchange(observer->next, {});
+ placeHolder.prev = std::exchange(observer->prev, {});
+ if (placeHolder.next) {
+ placeHolder.next->prev = &placeHolder.next;
+ }
+ if (placeHolder.prev)
+ placeHolder.prev.setPointer(&placeHolder);
+ };
+};
+
/*! \internal
\a propertyDataPtr is a pointer to the observed property's property data
In case that property has a binding, \a triggeringBinding points to the binding's QPropertyBindingPrivate
@@ -453,29 +519,62 @@ void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding
bool propertyChanged = true;
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 replace the actual node in the list with
+ * a placeholder 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 execuated action might either move
+ * or delete that node.
+ * 2. To properly handle deletion or moves of the real current node, we store a pointer to a pointer to itself in
+ * its nodeState. Whenever the node is reallocated and moved, we update that pointer to point to its new
+ * location. If the node is actually deleted, we set it to nullptr.
+ * 3. After the triggered action has finished, we can use that information to restore the list to contain the actual
+ * node again. We either switch the nodes with the real nodes current location, or, if the real node has been
+ * deleted, we simply unlink the temporary node.
+ */
while (observer) {
- auto * const next = observer->next.data();
+ QPropertyObserver *next = nullptr;
+ char preventBug[1] = {'\0'}; // QTBUG-87245
+ Q_UNUSED(preventBug);
switch (observer->next.tag()) {
case QPropertyObserver::ObserverNotifiesChangeHandler:
- if (!knownIfPropertyChanged && triggeringBinding) {
- knownIfPropertyChanged = true;
-
- propertyChanged = triggeringBinding->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
- }
- if (!propertyChanged)
- return;
-
- if (auto handlerToCall = std::exchange(observer->changeHandler, nullptr)) {
+ if (auto handlerToCall = observer->changeHandler) {
+ // both evaluateIfDirtyAndReturnTrueIfValueChanged and handlerToCall might modify the list
+ QPropertyObserverNodeProtector<QPropertyObserver::ObserverNotifiesChangeHandler> protector(observer);
+ if (!knownIfPropertyChanged && triggeringBinding) {
+ knownIfPropertyChanged = true;
+ propertyChanged = triggeringBinding->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
+ }
+ if (!propertyChanged)
+ return;
handlerToCall(observer, propertyDataPtr);
- observer->changeHandler = handlerToCall;
+ next = protector.m_placeHolder.next.data();
+ } else {
+ next = observer->next.data();
}
break;
case QPropertyObserver::ObserverNotifiesBinding:
- if (observer->bindingToMarkDirty)
- observer->bindingToMarkDirty->markDirtyAndNotifyObservers();
+ if (auto bindingToMarkDirty = observer->bindingToMarkDirty) {
+ QPropertyObserverNodeProtector<QPropertyObserver::ObserverNotifiesBinding> protector(observer);
+ bindingToMarkDirty->markDirtyAndNotifyObservers();
+ next = protector.m_placeHolder.next.data();
+ } else {
+ next = observer->next.data();
+ }
break;
case QPropertyObserver::ObserverNotifiesAlias:
+ next = observer->next.data();
break;
+ case QPropertyObserver::ActivelyExecuting:
+ // recursion is already properly handled somewhere else
+ return;
+ default:
+ Q_UNREACHABLE();
}
observer = next;
}
@@ -1484,5 +1583,4 @@ QPropertyBindingData *QBindingStorage::bindingData(QUntypedPropertyData *data, b
return storage;
}
-
QT_END_NAMESPACE
diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h
index 88745d6cde..0d9a34dcde 100644
--- a/src/corelib/kernel/qproperty.h
+++ b/src/corelib/kernel/qproperty.h
@@ -221,9 +221,11 @@ class Q_CORE_EXPORT QPropertyObserver
public:
// Internal
enum ObserverTag {
- ObserverNotifiesBinding,
- ObserverNotifiesChangeHandler,
- ObserverNotifiesAlias,
+ ObserverNotifiesBinding, // observer was installed to notify bindings that obsverved property changed
+ ObserverNotifiesChangeHandler, // observer is a change handler, which runs on every change
+ ObserverNotifiesAlias, // used for QPropertyAlias
+ ActivelyExecuting // the observer is currently evaluated in QPropertyObserver::notifyObservers or its
+ // placeholder. We only can store 4 different values, therefore those two conflate
};
QPropertyObserver() = default;
@@ -257,6 +259,7 @@ private:
QPropertyBindingPrivate *bindingToMarkDirty = nullptr;
ChangeHandler changeHandler;
QUntypedPropertyData *aliasedPropertyData;
+ QPropertyObserver **nodeState;
};
QPropertyObserver(const QPropertyObserver &) = delete;
@@ -265,6 +268,8 @@ private:
friend struct QPropertyObserverPointer;
friend struct QPropertyBindingDataPointer;
friend class QPropertyBindingPrivate;
+ template<ObserverTag>
+ friend struct QPropertyObserverNodeProtector;
};
template <typename Functor>
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index aa81aa9203..9185c939c6 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/src/corelib/kernel/qproperty_p.h
@@ -241,6 +241,13 @@ public:
void clearDependencyObservers() {
for (size_t i = 0; i < qMin(dependencyObserverCount, inlineDependencyObservers.size()); ++i) {
QPropertyObserverPointer p{&inlineDependencyObservers[i]};
+ if (p.ptr->next.tag() == QPropertyObserver::ActivelyExecuting) {
+ *(p.ptr->nodeState) = nullptr;
+ p.ptr->nodeState = nullptr;
+
+ // set tag to "safer" value, as we return the same observer pointer from allocateDependencyObserver
+ p.ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
+ }
p.unlink();
}
if (heapObservers)
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index 344ced9101..55ef3b12cb 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -76,6 +76,8 @@ private slots:
void compatBindings();
void metaProperty();
void aliasOnMetaProperty();
+
+ void modifyObserverListWhileIterating();
};
void tst_QProperty::functorBinding()
@@ -1248,6 +1250,57 @@ void tst_QProperty::aliasOnMetaProperty()
QCOMPARE(alias.value(), 100);
}
+void tst_QProperty::modifyObserverListWhileIterating()
+{
+ struct DestructingObserver : QPropertyObserver {
+ DestructingObserver() : QPropertyObserver([](QPropertyObserver *self, QUntypedPropertyData *) {
+ auto This = static_cast<DestructingObserver *>(self);
+ (*This)();
+ }), m_target(this){}
+ void operator()() {
+ (*counter)++;
+ std::destroy_at(m_target);
+ }
+ DestructingObserver *m_target;
+ int *counter = nullptr;
+ };
+ union ObserverOrUninit {
+ DestructingObserver observer = {};
+ char* memory;
+ ~ObserverOrUninit() {}
+ };
+ {
+ // observer deletes itself while running the notification
+ // while explicitly calling the destructor is rather unusual
+ // it is completely plausible for this to happen because the object to which a
+ // propertyobserver belongs has been destroyed
+ ObserverOrUninit data;
+ int counter = 0;
+ data.observer.counter = &counter;
+ QProperty<int> prop;
+ QUntypedBindable bindableProp(&prop);
+ bindableProp.observe(&data.observer);
+ prop = 42; // should not crash
+ QCOMPARE(counter, 1);
+ }
+ {
+ // observer deletes the next observer in the list
+ ObserverOrUninit data1;
+ ObserverOrUninit data2;
+ QProperty<int> prop;
+ QUntypedBindable bindableProp(&prop);
+ bindableProp.observe(&data1.observer);
+ bindableProp.observe(&data2.observer);
+ int counter = 0;
+ data1.observer.m_target = &data2.observer;
+ data1.observer.counter = &counter;
+ data2.observer.m_target = &data1.observer;
+ data2.observer.counter = &counter;
+ prop = 42; // should not crash
+ QCOMPARE(counter, 1); // only one trigger should run as the other has been deleted
+ }
+}
+
QTEST_MAIN(tst_QProperty);
#include "tst_qproperty.moc"
generated by cgit v1.2.3 (git 2.25.1) at 2024-06-11 20:19:47 +0000