qiurui
/
qtbase
mirror of https://github.com/qt/qtbase.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

QProperty: Avoid referencing stale QBindingStatus 

Because we cache the QBindingStatus in QObjects, and the QObjects might
outlive the native thread, we can't bind the lifetime of the
QBindingStatus solely to the native thread.

Instead, keep it alive while the QThreadData associated with the QObject
still exists.
This is done by moving the BindingStatusOrList member for QThreadPrivate
to QThreadData, and by letting QThreadData own the binding status.

Pick-to: 6.10 6.9 6.8 6.5
Fixes: QTBUG-126134
Change-Id: I747ec1778f6b6f376c38d1c678dc5b2f62fcb7ef
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
This commit is contained in:
Fabian Kosmale 2025-08-04 16:28:53 +02:00
parent f5d936642b
commit 2d92ea0250
8 changed files with 159 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
src/corelib/kernel/qbindingstorage.h
View File

@ -32,6 +32,7 @@ struct QBindingStatus
namespace QtPrivate {
struct QBindingStatusAccessToken;
Q_AUTOTEST_EXPORT QBindingStatus *getBindingStatus(QBindingStatusAccessToken);
Q_AUTOTEST_EXPORT void setBindingStatus(QBindingStatus *, QBindingStatusAccessToken);
}

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

@ -8,7 +8,7 @@
#include <QScopeGuard>
#include <QtCore/qalloc.h>
#include <QtCore/qloggingcategory.h>
#include <QThread>
#include <QtCore/private/qthread_p.h>
#include <QtCore/qmetaobject.h>
#include "qobject_p.h"
@ -166,7 +166,49 @@ struct QPropertyDelayedNotifications
}
};
Q_CONSTINIT static thread_local QBindingStatus bindingStatus;
/*
The binding status needs some care: Conceptually, it is a thread-local. However, we cache it
in QObjects via their QBindingStorage. Those QObjects might outlive the thread in which the
binding status was initially created (e.g. when their QThread is stopped).
If they are not migrated to another (running) QThread, they would have a stale pointer if
a plain thread local were used for the QBindingStatus.
So instead of a normal thread_local, we use the following scheme:
- On first access, the QBindingStatus gets allocated on the heap, and stored in QThreadData
- It also gets cached in in a thread_local variable for faster access
- The QThreadData takes care of deleting the QBindingStatus in its destructor
- Moreover, if a QThread is restarted, the native thread's thread local gets initialized with
the QBindingStatus of the QThreadData. Otherwise, we'd somehow need to update all objects
whose affinity is pointing to that thread, which we can't easily do. Moreover, this avoids
freeing and reallocating a QBindingStatus instance.
Note that the lifetime is coupled to the QThreadData, which is kept alive by QObjects, even if
the corresponding QThread is gone.
The draw-back here is that even plain QProperty will cause the creation of QThreadData if used
in a thread which doesn't already have it; however that should be rare in practice.
*/
Q_CONSTINIT thread_local QBindingStatus *tl_status = nullptr;
Q_NEVER_INLINE static void initBindingStatus()
{
auto status = new QBindingStatus {};
/* needs to happen before setting it on the thread-data, as
setStatusAndClearList might need to actually update the objectt list, which would
end up calling into bindingStatus again
*/
tl_status = status;
QThreadData::current()->m_statusOrPendingObjects.setStatusAndClearList(status);
}
static QBindingStatus &bindingStatus()
{
if (!tl_status)
initBindingStatus();
return *tl_status;
}
/*!
\since 6.2
@ -190,7 +232,7 @@ Q_CONSTINIT static thread_local QBindingStatus bindingStatus;
*/
void Qt::beginPropertyUpdateGroup()
{
QPropertyDelayedNotifications *& groupUpdateData = bindingStatus.groupUpdateData;
QPropertyDelayedNotifications *& groupUpdateData = bindingStatus().groupUpdateData;
if (!groupUpdateData)
groupUpdateData = new QPropertyDelayedNotifications;
++groupUpdateData->ref;
@ -210,7 +252,7 @@ void Qt::beginPropertyUpdateGroup()
*/
void Qt::endPropertyUpdateGroup()
{
auto status = &bindingStatus;
auto status = &bindingStatus();
QPropertyDelayedNotifications *& groupUpdateData = status->groupUpdateData;
auto *data = groupUpdateData;
Q_ASSERT(data->ref);
@ -318,7 +360,7 @@ void QPropertyBindingPrivate::unlinkAndDeref()
bool QPropertyBindingPrivate::evaluateRecursive(PendingBindingObserverList &bindingObservers, QBindingStatus *status)
{
if (!status)
status = &bindingStatus;
status = &bindingStatus();
return evaluateRecursive_inline(bindingObservers, status);
}
@ -559,14 +601,14 @@ CompatPropertySafePoint::CompatPropertySafePoint(QBindingStatus *status, QUntype
previousState = *currentState;
*currentState = this;
currentlyEvaluatingBindingList = &bindingStatus.currentlyEvaluatingBinding;
currentlyEvaluatingBindingList = &bindingStatus().currentlyEvaluatingBinding;
bindingState = *currentlyEvaluatingBindingList;
*currentlyEvaluatingBindingList = nullptr;
}
QPropertyBindingPrivate *QPropertyBindingPrivate::currentlyEvaluatingBinding()
{
auto currentState = bindingStatus.currentlyEvaluatingBinding ;
auto currentState = bindingStatus().currentlyEvaluatingBinding ;
return currentState ? currentState->binding : nullptr;
}
@ -594,7 +636,7 @@ void QPropertyBindingData::removeBinding_helper()
void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding() const
{
auto currentState = bindingStatus.currentlyEvaluatingBinding;
auto currentState = bindingStatus().currentlyEvaluatingBinding;
if (!currentState)
return;
registerWithCurrentlyEvaluatingBinding_helper(currentState);
@ -658,10 +700,10 @@ QPropertyBindingData::NotificationResult QPropertyBindingData::notifyObserver_he
#ifdef QT_HAS_FAST_CURRENT_THREAD_ID
QBindingStatus *status = storage ? storage->bindingStatus : nullptr;
if (!status || status->threadId != QThread::currentThreadId())
status = &bindingStatus;
status = &bindingStatus();
#else
Q_UNUSED(storage);
QBindingStatus *status = &bindingStatus;
QBindingStatus *status = &bindingStatus();
#endif
if (QPropertyDelayedNotifications *delay = status->groupUpdateData) {
delay->addProperty(this, propertyDataPtr);
@ -2303,7 +2345,7 @@ struct QBindingStoragePrivate
QBindingStorage::QBindingStorage()
{
bindingStatus = &QT_PREPEND_NAMESPACE(bindingStatus);
bindingStatus = &QT_PREPEND_NAMESPACE(bindingStatus)();
Q_ASSERT(bindingStatus);
}
@ -2314,7 +2356,7 @@ QBindingStorage::~QBindingStorage()
void QBindingStorage::reinitAfterThreadMove()
{
bindingStatus = &QT_PREPEND_NAMESPACE(bindingStatus);
bindingStatus = &QT_PREPEND_NAMESPACE(bindingStatus)();
Q_ASSERT(bindingStatus);
}
@ -2336,9 +2378,9 @@ void QBindingStorage::registerDependency_helper(const QUntypedPropertyData *data
if (Q_LIKELY(threadMatches))
currentBinding = bindingStatus->currentlyEvaluatingBinding;
else
currentBinding = QT_PREPEND_NAMESPACE(bindingStatus).currentlyEvaluatingBinding;
currentBinding = QT_PREPEND_NAMESPACE(bindingStatus)().currentlyEvaluatingBinding;
#else
currentBinding = QT_PREPEND_NAMESPACE(bindingStatus).currentlyEvaluatingBinding;
currentBinding = QT_PREPEND_NAMESPACE(bindingStatus)().currentlyEvaluatingBinding;
#endif
QUntypedPropertyData *dd = const_cast<QUntypedPropertyData *>(data);
if (!currentBinding)
@ -2371,19 +2413,19 @@ namespace QtPrivate {
void initBindingStatusThreadId()
{
bindingStatus.threadId = QThread::currentThreadId();
bindingStatus().threadId = QThread::currentThreadId();
}
BindingEvaluationState *suspendCurrentBindingStatus()
{
auto ret = bindingStatus.currentlyEvaluatingBinding;
bindingStatus.currentlyEvaluatingBinding = nullptr;
auto ret = bindingStatus().currentlyEvaluatingBinding;
bindingStatus().currentlyEvaluatingBinding = nullptr;
return ret;
}
void restoreBindingStatus(BindingEvaluationState *status)
{
bindingStatus.currentlyEvaluatingBinding = status;
bindingStatus().currentlyEvaluatingBinding = status;
}
/*!
@ -2396,13 +2438,13 @@ void restoreBindingStatus(BindingEvaluationState *status)
*/
bool isAnyBindingEvaluating()
{
return bindingStatus.currentlyEvaluatingBinding != nullptr;
return bindingStatus().currentlyEvaluatingBinding != nullptr;
}
bool isPropertyInBindingWrapper(const QUntypedPropertyData *property)
{
// Accessing bindingStatus is expensive because it's thread-local. Do it only once.
if (const auto current = bindingStatus.currentCompatProperty)
if (const auto current = bindingStatus().currentCompatProperty)
return current->property == property;
return false;
}
@ -2441,7 +2483,16 @@ void printMetaTypeMismatch(QMetaType actual, QMetaType expected)
\internal
Returns the binding statusof the current thread.
*/
QBindingStatus* getBindingStatus(QtPrivate::QBindingStatusAccessToken) { return &QT_PREPEND_NAMESPACE(bindingStatus); }
QBindingStatus* getBindingStatus(QtPrivate::QBindingStatusAccessToken)
{
return &QT_PREPEND_NAMESPACE(bindingStatus)();
}
void setBindingStatus(QBindingStatus *status, QBindingStatusAccessToken)
{
Q_ASSERT(!tl_status);
Q_ASSERT(status);
tl_status = status;
}
namespace PropertyAdaptorSlotObjectHelpers {
void getter(const QUntypedPropertyData *d, void *value)

15
src/corelib/thread/qthread.cpp
View File

@ -118,7 +118,7 @@ QAdoptedThread::QAdoptedThread(QThreadData *data)
#if QT_CONFIG(thread)
d_func()->threadState = QThreadPrivate::Running;
init();
d_func()->m_statusOrPendingObjects.setStatusAndClearList(
d_func()->data->m_statusOrPendingObjects.setStatusAndClearList(
QtPrivate::getBindingStatus({}));
#endif
// fprintf(stderr, "new QAdoptedThread = %p\n", this);
@ -173,7 +173,7 @@ QThreadPrivate::~QThreadPrivate()
// access to m_statusOrPendingObjects cannot race with anything
// unless there is already a potential use-after-free bug, as the
// thread is in the process of being destroyed
delete m_statusOrPendingObjects.list();
delete data->m_statusOrPendingObjects.list();
data->clearEvents();
data->deref();
}
@ -625,7 +625,6 @@ QThread::QualityOfService QThread::serviceLevel() const
*/
void QtPrivate::BindingStatusOrList::setStatusAndClearList(QBindingStatus *status) noexcept
{
if (auto pendingObjects = list()) {
for (auto obj: *pendingObjects)
QObjectPrivate::get(obj)->reinitBindingStorageAfterThreadMove();
@ -654,7 +653,7 @@ int QThread::exec()
const auto status = QtPrivate::getBindingStatus(QtPrivate::QBindingStatusAccessToken{});
QMutexLocker locker(&d->mutex);
d->m_statusOrPendingObjects.setStatusAndClearList(status);
d->data->m_statusOrPendingObjects.setStatusAndClearList(status);
d->data->quitNow = false;
if (d->exited) {
d->exited = false;
@ -708,18 +707,18 @@ void QtPrivate::BindingStatusOrList::removeObject(QObject *object)
QBindingStatus *QThreadPrivate::addObjectWithPendingBindingStatusChange(QObject *obj)
{
if (auto status = m_statusOrPendingObjects.bindingStatus())
if (auto status = data->m_statusOrPendingObjects.bindingStatus())
return status;
QMutexLocker lock(&mutex);
return m_statusOrPendingObjects.addObjectUnlessAlreadyStatus(obj);
return data->m_statusOrPendingObjects.addObjectUnlessAlreadyStatus(obj);
}
void QThreadPrivate::removeObjectWithPendingBindingStatusChange(QObject *obj)
{
if (m_statusOrPendingObjects.bindingStatus())
if (data->m_statusOrPendingObjects.bindingStatus())
return;
QMutexLocker lock(&mutex);
m_statusOrPendingObjects.removeObject(obj);
data->m_statusOrPendingObjects.removeObject(obj);
}

28
src/corelib/thread/qthread_p.h
View File

@ -109,6 +109,11 @@ public:
explicit BindingStatusOrList(QBindingStatus *status) noexcept :
data(encodeBindingStatus(status)) {}
explicit BindingStatusOrList(List *list) noexcept : data(encodeList(list)) {}
~BindingStatusOrList()
{
auto status = bindingStatus();
delete status;
}
// requires external synchronization:
QBindingStatus *addObjectUnlessAlreadyStatus(QObject *object);
@ -252,10 +257,7 @@ public:
}
}
QBindingStatus *bindingStatus()
{
return m_statusOrPendingObjects.bindingStatus();
}
QBindingStatus *bindingStatus();
/* Returns nullptr if the object has been added, or the binding status
if that one has been set in the meantime
@ -263,8 +265,6 @@ public:
QBindingStatus *addObjectWithPendingBindingStatusChange(QObject *obj);
void removeObjectWithPendingBindingStatusChange(QObject *obj);
// manipulating m_statusOrPendingObjects requires mutex to be locked
QtPrivate::BindingStatusOrList m_statusOrPendingObjects = {};
#ifndef Q_OS_INTEGRITY
private:
// Used in QThread(Private)::start to avoid racy access to QObject::objectName,
@ -355,12 +355,21 @@ public:
void clearEvents();
void reuseBindingStatusForNewNativeThread()
{
auto status = m_statusOrPendingObjects.bindingStatus();
if (status)
QtPrivate::setBindingStatus(status, {});
}
QStack<QEventLoop *> eventLoops;
QPostEventList postEventList;
QAtomicPointer<QThread> thread;
QAtomicPointer<void> threadId;
QAtomicPointer<QAbstractEventDispatcher> eventDispatcher;
QList<void *> tls;
// manipulating m_statusOrPendingObjects requires QTreadPrivate's mutex to be locked
QtPrivate::BindingStatusOrList m_statusOrPendingObjects = {};
private:
QAtomicInt _ref;
@ -408,6 +417,13 @@ private:
#endif
};
#if QT_CONFIG(thread)
inline QBindingStatus *QThreadPrivate::bindingStatus()
{
return data->m_statusOrPendingObjects.bindingStatus();
}
#endif
QT_END_NAMESPACE
#endif // QTHREAD_P_H

2
src/corelib/thread/qthread_unix.cpp
View File

@ -387,6 +387,8 @@ void *QThreadPrivate::start(void *arg)
#endif
QThread *thr = reinterpret_cast<QThread *>(arg);
QThreadData *data = QThreadData::get2(thr);
// If a QThread is restarted, reuse the QBindingStatus, too
data->reuseBindingStatusForNewNativeThread();
// this ensures the thread-local is created as early as possible
set_thread_data(data);

2
src/corelib/thread/qthread_win.cpp
View File

@ -145,6 +145,8 @@ unsigned int __stdcall QT_ENSURE_STACK_ALIGNED_FOR_SSE QThreadPrivate::start(voi
{
QThread *thr = reinterpret_cast<QThread *>(arg);
QThreadData *data = QThreadData::get2(thr);
// If a QThread is restarted, reuse the QBindingStatus, too
data->reuseBindingStatusForNewNativeThread();
data->ref();
set_thread_data(data);

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

@ -72,6 +72,7 @@ private slots:
void qobjectBindableManualNotify();
void qobjectBindableReallocatedBindingStorage();
void qobjectBindableSignalTakingNewValue();
void bindableStateAfterThreadRestart();
void testNewStuff();
void qobjectObservers();
@ -1358,6 +1359,56 @@ void tst_QProperty::qobjectBindableSignalTakingNewValue()
QCOMPARE(newValue, 3);
}
class TestWorker : public QObject
{
Q_OBJECT
public:
Q_INVOKABLE int work() {
// calling value will access the bindingStatus to see if we are in a binding
int old = testProp.value();
testProp.setValue(old+1);
return old;
}
private:
Q_OBJECT_BINDABLE_PROPERTY_WITH_ARGS(TestWorker, int, testProp, 0);
};
void tst_QProperty::bindableStateAfterThreadRestart()
{
auto workerThread = new QThread(this);
auto worker = std::unique_ptr<TestWorker, QScopedPointerDeleteLater>(new TestWorker);
worker->moveToThread(workerThread);
workerThread->start();
int returnValue = -1;
QMetaObject::invokeMethod(worker.get(), &TestWorker::work, Qt::BlockingQueuedConnection,
qReturnArg(returnValue));
QCOMPARE(returnValue, 0);
workerThread->quit();
workerThread->wait(); // the native thread is gone now
// accessing a property should work even if there is no actively running native thread for its QThread
returnValue = worker->work();
QCOMPARE(returnValue, 1);
// it should also work if we restart the thread
workerThread->start();
QVERIFY(workerThread->isRunning());
QMetaObject::invokeMethod(worker.get(), &TestWorker::work, Qt::BlockingQueuedConnection,
qReturnArg(returnValue));
QCOMPARE(returnValue, 2);
// accessing a property should work even if the thread is gone completely
workerThread->quit();
workerThread->wait();
delete workerThread;
returnValue = worker->work();
QCOMPARE(returnValue, 3);
// deleteLater no longer works, because the thread+eventloop are gone
delete worker.release();
}
void tst_QProperty::testNewStuff()
{
MyQObject testReadOnly;

4
tests/auto/corelib/thread/qthread/tst_qthread.cpp
View File

@ -1010,7 +1010,7 @@ void tst_QThread::adoptedThreadBindingStatus()
nativeThread.startAndWait();
QVERIFY(nativeThread.qthread);
auto privThread = static_cast<QThreadPrivate *>(QObjectPrivate::get(nativeThread.qthread));
QVERIFY(privThread->m_statusOrPendingObjects.bindingStatus());
QVERIFY(privThread->data->m_statusOrPendingObjects.bindingStatus());
nativeThread.stop();
nativeThread.join();
@ -2116,7 +2116,7 @@ void tst_QThread::bindingListCleanupAfterDelete()
auto optr = std::make_unique<QObject>();
optr->moveToThread(&t);
auto threadPriv = static_cast<QThreadPrivate *>(QObjectPrivate::get(&t));
auto list = threadPriv->m_statusOrPendingObjects.list();
auto list = threadPriv->data->m_statusOrPendingObjects.list();
QVERIFY(list);
optr.reset();
QVERIFY(list->empty());