Address thread safety issues in QProperty classes

While we do not support cross-thread bindings, reading of properties
from a different thread when no bindings are involved must continue to
work.
However the check whether bindings are involved used the QBindingStatus
in the QObjectPrivate. That one contains the wrong value when the
QObject is accessed from a different thread than the one it has affinity
with. This patch reads from the thread_local directly instead to
sidetstep the issue.

Change-Id: I8ce2092f35e210566934e2439beb5d48fd8cf226
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
This commit is contained in:
Fabian Kosmale 2021-01-29 13:29:10 +01:00
parent cf42a0fe5e
commit 984bc7cc3e
3 changed files with 20 additions and 6 deletions

View File

@ -1830,11 +1830,14 @@ void QBindingStorage::maybeUpdateBindingAndRegister_helper(const QUntypedPropert
void QBindingStorage::registerDependency_helper(const QUntypedPropertyData *data) const void QBindingStorage::registerDependency_helper(const QUntypedPropertyData *data) const
{ {
Q_ASSERT(bindingStatus); Q_ASSERT(bindingStatus);
// Use ::bindingStatus to get the binding from TLS. This is required, so that reads from
// another thread do not register as dependencies
auto *currentBinding = QT_PREPEND_NAMESPACE(bindingStatus).currentlyEvaluatingBinding;
QUntypedPropertyData *dd = const_cast<QUntypedPropertyData *>(data); QUntypedPropertyData *dd = const_cast<QUntypedPropertyData *>(data);
auto storage = QBindingStoragePrivate(d).get(dd, /*create=*/ bindingStatus->currentlyEvaluatingBinding != nullptr); auto storage = QBindingStoragePrivate(d).get(dd, /*create=*/ currentBinding != nullptr);
if (!storage) if (!storage)
return; return;
storage->registerWithCurrentlyEvaluatingBinding(bindingStatus->currentlyEvaluatingBinding); storage->registerWithCurrentlyEvaluatingBinding(currentBinding);
} }
@ -1874,6 +1877,13 @@ bool isAnyBindingEvaluating()
{ {
return bindingStatus.currentlyEvaluatingBinding != nullptr; return bindingStatus.currentlyEvaluatingBinding != nullptr;
} }
bool isPropertyInBindingWrapper(const QUntypedPropertyData *property)
{
return bindingStatus.currentCompatProperty &&
bindingStatus.currentCompatProperty->property == property;
}
} // namespace QtPrivate end } // namespace QtPrivate end
QT_END_NAMESPACE QT_END_NAMESPACE

View File

@ -845,7 +845,7 @@ public:
void registerDependency(const QUntypedPropertyData *data) const void registerDependency(const QUntypedPropertyData *data) const
{ {
if (!d && !bindingStatus->currentlyEvaluatingBinding) if (!bindingStatus->currentlyEvaluatingBinding)
return; return;
registerDependency_helper(data); registerDependency_helper(data);
} }

View File

@ -368,6 +368,10 @@ inline QPropertyObserverPointer QPropertyBindingDataPointer::firstObserver() con
return { reinterpret_cast<QPropertyObserver *>(ptr->d_ptr) }; return { reinterpret_cast<QPropertyObserver *>(ptr->d_ptr) };
} }
namespace QtPrivate {
Q_CORE_EXPORT bool isPropertyInBindingWrapper(const QUntypedPropertyData *property);
}
template<typename Class, typename T, auto Offset, auto Setter, auto Signal=nullptr> template<typename Class, typename T, auto Offset, auto Setter, auto Signal=nullptr>
class QObjectCompatProperty : public QPropertyData<T> class QObjectCompatProperty : public QPropertyData<T>
{ {
@ -397,10 +401,10 @@ class QObjectCompatProperty : public QPropertyData<T>
(thisData->owner()->*Setter)(copy.valueBypassingBindings()); (thisData->owner()->*Setter)(copy.valueBypassingBindings());
return true; return true;
} }
inline bool inBindingWrapper(const QBindingStorage *storage) const bool inBindingWrapper(const QBindingStorage *storage) const
{ {
return storage->bindingStatus->currentCompatProperty && return storage->bindingStatus->currentCompatProperty
storage->bindingStatus->currentCompatProperty->property == this; && QtPrivate::isPropertyInBindingWrapper(this);
} }
public: public: