diff --git a/src/corelib/kernel/qbindingstorage.h b/src/corelib/kernel/qbindingstorage.h index 1c03b23bfae..27fba0e4b9e 100644 --- a/src/corelib/kernel/qbindingstorage.h +++ b/src/corelib/kernel/qbindingstorage.h @@ -32,6 +32,7 @@ struct QBindingStatus namespace QtPrivate { struct QBindingStatusAccessToken; Q_AUTOTEST_EXPORT QBindingStatus *getBindingStatus(QBindingStatusAccessToken); +Q_AUTOTEST_EXPORT void setBindingStatus(QBindingStatus *, QBindingStatusAccessToken); } diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 1da1704619f..9a8e20e0c45 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -8,7 +8,7 @@ #include #include #include -#include +#include #include #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(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) diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp index 3160ee27c37..553b38fec8f 100644 --- a/src/corelib/thread/qthread.cpp +++ b/src/corelib/thread/qthread.cpp @@ -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); } diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h index 1ea9268f5ca..3661599ca00 100644 --- a/src/corelib/thread/qthread_p.h +++ b/src/corelib/thread/qthread_p.h @@ -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 eventLoops; QPostEventList postEventList; QAtomicPointer thread; QAtomicPointer threadId; QAtomicPointer eventDispatcher; QList 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 diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp index 0b0b0a51883..1ba73b49b45 100644 --- a/src/corelib/thread/qthread_unix.cpp +++ b/src/corelib/thread/qthread_unix.cpp @@ -387,6 +387,8 @@ void *QThreadPrivate::start(void *arg) #endif QThread *thr = reinterpret_cast(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); diff --git a/src/corelib/thread/qthread_win.cpp b/src/corelib/thread/qthread_win.cpp index ac44a757e00..f53e488a8d7 100644 --- a/src/corelib/thread/qthread_win.cpp +++ b/src/corelib/thread/qthread_win.cpp @@ -145,6 +145,8 @@ unsigned int __stdcall QT_ENSURE_STACK_ALIGNED_FOR_SSE QThreadPrivate::start(voi { QThread *thr = reinterpret_cast(arg); QThreadData *data = QThreadData::get2(thr); + // If a QThread is restarted, reuse the QBindingStatus, too + data->reuseBindingStatusForNewNativeThread(); data->ref(); set_thread_data(data); diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 9801fd2db9d..dbfee3ded80 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -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(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; diff --git a/tests/auto/corelib/thread/qthread/tst_qthread.cpp b/tests/auto/corelib/thread/qthread/tst_qthread.cpp index c5837133d34..c2f0152e0c7 100644 --- a/tests/auto/corelib/thread/qthread/tst_qthread.cpp +++ b/tests/auto/corelib/thread/qthread/tst_qthread.cpp @@ -1010,7 +1010,7 @@ void tst_QThread::adoptedThreadBindingStatus() nativeThread.startAndWait(); QVERIFY(nativeThread.qthread); auto privThread = static_cast(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(); optr->moveToThread(&t); auto threadPriv = static_cast(QObjectPrivate::get(&t)); - auto list = threadPriv->m_statusOrPendingObjects.list(); + auto list = threadPriv->data->m_statusOrPendingObjects.list(); QVERIFY(list); optr.reset(); QVERIFY(list->empty());