QProperty: fix threading issues

QObject's cache the binding status pointer to avoid TLS lookups.
However, when an object is moved to a different thread, we need to
update the cached pointer (as the original thread might stop and thus no
longer exist, and to correctly allow setting up bindings in the object's
thread).
Fix this by also storing the binding status in QThreadPrivate and
updating the object's binding status when moved. This does only work
when the thread is already running, though. If it is not running, we
instead treat the QThreadPrivate's status pointer as a pointer to a
vector of pending objects. Once the QThread has been started, we check
if there are pending objects, and update them at this point.

Pick-to: 6.2 6.3
Fixes: QTBUG-101177
Change-Id: I0490bbbdc1a17cb5f85044ad6eb2e1a8c759d4b7
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

9 files changed, 184 insertions, 5 deletions

diff --git a/src/corelib/kernel/qbindingstorage.h b/src/corelib/kernel/qbindingstorage.h
index 2ffaf18adf..4f8b7bfc5e 100644
--- a/src/corelib/kernel/qbindingstorage.h
+++ b/src/corelib/kernel/qbindingstorage.h
@@ -65,6 +65,11 @@ struct QBindingStatus
     QPropertyDelayedNotifications *groupUpdateData = nullptr;
 };
 
+namespace QtPrivate {
+struct QBindingStatusAccessToken;
+Q_AUTOTEST_EXPORT QBindingStatus *getBindingStatus(QBindingStatusAccessToken);
+}
+
 
 struct QBindingStorageData;
 class Q_CORE_EXPORT QBindingStorage
@@ -82,6 +87,8 @@ public:
 
     bool isEmpty() { return !d; }
 
+    const QBindingStatus *status(QtPrivate::QBindingStatusAccessToken) const;
+
     void registerDependency(const QUntypedPropertyData *data) const
     {
         if (!bindingStatus->currentlyEvaluatingBinding)
@@ -106,6 +113,7 @@ public:
         return bindingData_helper(data, create);
     }
 private:
+    void reinitAfterThreadMove();
     void clear();
     void registerDependency_helper(const QUntypedPropertyData *data) const;
 #if QT_CORE_REMOVED_SINCE(6, 2)
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 13708be391..5275fc4bf7 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -505,6 +505,13 @@ bool QObjectPrivate::maybeSignalConnected(uint signalIndex) const
     return false;
 }
 
+void QObjectPrivate::reinitBindingStorageAfterThreadMove()
+{
+    bindingStorage.reinitAfterThreadMove();
+    for (int i = 0; i < children.size(); ++i)
+        children[i]->d_func()->reinitBindingStorageAfterThreadMove();
+}
+
 /*!
     \internal
  */
@@ -1641,7 +1648,16 @@ void QObject::moveToThread(QThread *targetThread)
     currentData->ref();
 
     // move the object
-    d_func()->setThreadData_helper(currentData, targetData);
+    auto threadPrivate =  targetThread
+        ? static_cast<QThreadPrivate *>(QThreadPrivate::get(targetThread))
+        : nullptr;
+    QBindingStatus *bindingStatus = threadPrivate
+        ? threadPrivate->bindingStatus()
+        : nullptr;
+    if (threadPrivate && !bindingStatus) {
+        threadPrivate->addObjectWithPendingBindingStatusChange(this);
+    }
+    d_func()->setThreadData_helper(currentData, targetData, bindingStatus);
 
     locker.unlock();
 
@@ -1656,14 +1672,20 @@ void QObjectPrivate::moveToThread_helper()
     QCoreApplication::sendEvent(q, &e);
     for (int i = 0; i < children.size(); ++i) {
         QObject *child = children.at(i);
+        child->d_func()->bindingStorage.clear();
         child->d_func()->moveToThread_helper();
     }
 }
 
-void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData *targetData)
+void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData *targetData, QBindingStatus *status)
 {
     Q_Q(QObject);
 
+    if (status) {
+        // the new thread is already running
+        this->bindingStorage.bindingStatus = status;
+    }
+
     // move posted events
     int eventsMoved = 0;
     for (int i = 0; i < currentData->postEventList.size(); ++i) {
@@ -1718,7 +1740,7 @@ void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData
 
     for (int i = 0; i < children.size(); ++i) {
         QObject *child = children.at(i);
-        child->d_func()->setThreadData_helper(currentData, targetData);
+        child->d_func()->setThreadData_helper(currentData, targetData, status);
     }
 }
 
diff --git a/src/corelib/kernel/qobject.h b/src/corelib/kernel/qobject.h
index ef785ddc24..67b751935e 100644
--- a/src/corelib/kernel/qobject.h
+++ b/src/corelib/kernel/qobject.h
@@ -111,7 +111,7 @@ public:
     uint willBeWidget : 1; // for handling widget-specific bits in QObject's ctor
     uint wasWidget : 1; // for properly cleaning up in QObject's dtor
     uint unused : 21;
-    int postedEvents;
+    QAtomicInt postedEvents;
     QDynamicMetaObjectData *metaObject;
     QBindingStorage bindingStorage;
 
diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h
index ac77d898da..1b0706e356 100644
--- a/src/corelib/kernel/qobject_p.h
+++ b/src/corelib/kernel/qobject_p.h
@@ -350,7 +350,7 @@ public:
 
     void setParent_helper(QObject *);
     void moveToThread_helper();
-    void setThreadData_helper(QThreadData *currentData, QThreadData *targetData);
+    void setThreadData_helper(QThreadData *currentData, QThreadData *targetData, QBindingStatus *status);
     void _q_reregisterTimers(void *pointer);
 
     bool isSender(const QObject *receiver, const char *signal) const;
@@ -372,6 +372,8 @@ public:
     inline void connectNotify(const QMetaMethod &signal);
     inline void disconnectNotify(const QMetaMethod &signal);
 
+    void reinitBindingStorageAfterThreadMove();
+
     template <typename Func1, typename Func2>
     static inline QMetaObject::Connection connect(const typename QtPrivate::FunctionPointer<Func1>::Object *sender, Func1 signal,
                                                   const typename QtPrivate::FunctionPointer<Func2>::Object *receiverPrivate, Func2 slot,
diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index 984d207204..b2ef09db2f 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -2201,6 +2201,12 @@ QBindingStorage::~QBindingStorage()
     QBindingStoragePrivate(d).destroy();
 }
 
+void QBindingStorage::reinitAfterThreadMove()
+{
+    bindingStatus = &QT_PREPEND_NAMESPACE(bindingStatus);
+    Q_ASSERT(bindingStatus);
+}
+
 void QBindingStorage::clear()
 {
     QBindingStoragePrivate(d).destroy();
@@ -2237,6 +2243,11 @@ QPropertyBindingData *QBindingStorage::bindingData_helper(const QUntypedProperty
     return QBindingStoragePrivate(d).get(data);
 }
 
+const QBindingStatus *QBindingStorage::status(QtPrivate::QBindingStatusAccessToken) const
+{
+    return bindingStatus;
+}
+
 QPropertyBindingData *QBindingStorage::bindingData_helper(QUntypedPropertyData *data, bool create)
 {
     return QBindingStoragePrivate(d).get(data, create);
@@ -2314,6 +2325,12 @@ void printMetaTypeMismatch(QMetaType actual, QMetaType expected)
 
 } // namespace BindableWarnings end
 
+/*!
+    \internal
+    Returns the binding statusof the current thread.
+ */
+QBindingStatus* getBindingStatus(QtPrivate::QBindingStatusAccessToken) { return &QT_PREPEND_NAMESPACE(bindingStatus); }
+
 } // namespace QtPrivate end
 
 QT_END_NAMESPACE
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index 21f9478835..a3a9072793 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/src/corelib/kernel/qproperty_p.h
@@ -62,6 +62,7 @@ QT_BEGIN_NAMESPACE
 
 namespace QtPrivate {
     Q_CORE_EXPORT bool isAnyBindingEvaluating();
+    struct QBindingStatusAccessToken {};
 }
 
 // Keep all classes related to QProperty in one compilation unit. Performance of this code is crucial and
diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp
index 4cfcab2258..994d77926c 100644
--- a/src/corelib/thread/qthread.cpp
+++ b/src/corelib/thread/qthread.cpp
@@ -43,6 +43,7 @@
 #include "qmutex.h"
 #include "qreadwritelock.h"
 #include "qabstracteventdispatcher.h"
+#include "qbindingstorage.h"
 
 #include <qeventloop.h>
 
@@ -195,6 +196,7 @@ QThreadPrivate::QThreadPrivate(QThreadData *d)
 
 QThreadPrivate::~QThreadPrivate()
 {
+    delete pendingObjectsWithBindingStatusChange();
     data->deref();
 }
 
@@ -552,6 +554,13 @@ uint QThread::stackSize() const
 int QThread::exec()
 {
     Q_D(QThread);
+    const auto status = QtPrivate::getBindingStatus(QtPrivate::QBindingStatusAccessToken{});
+    if (auto pendingObjects = d->pendingObjectsWithBindingStatusChange()) {
+        for (auto obj: *pendingObjects)
+            QObjectPrivate::get(obj)->reinitBindingStorageAfterThreadMove();
+        delete pendingObjects;
+    }
+    d->m_statusOrPendingObjects.storeRelease(quintptr(status));
     QMutexLocker locker(&d->mutex);
     d->data->quitNow = false;
     if (d->exited) {
@@ -953,6 +962,7 @@ QThreadPrivate::QThreadPrivate(QThreadData *d) : data(d ? d : new QThreadData)
 
 QThreadPrivate::~QThreadPrivate()
 {
+    delete pendingObjectsWithBindingStatusChange();
     data->thread.storeRelease(nullptr); // prevent QThreadData from deleting the QThreadPrivate (again).
     delete data;
 }
diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h
index aa6183f7e2..a895b37b29 100644
--- a/src/corelib/thread/qthread_p.h
+++ b/src/corelib/thread/qthread_p.h
@@ -202,6 +202,34 @@ public:
         }
     }
 
+    QBindingStatus *bindingStatus()
+    {
+        auto statusOrPendingObjects = m_statusOrPendingObjects.loadAcquire();
+        if (!(statusOrPendingObjects & 1))
+            return (QBindingStatus *) statusOrPendingObjects;
+        return nullptr;
+    }
+
+    void addObjectWithPendingBindingStatusChange(QObject *obj)
+    {
+        Q_ASSERT(!bindingStatus());
+        auto pendingObjects = pendingObjectsWithBindingStatusChange();
+        if (!pendingObjects) {
+            pendingObjects = new std::vector<QObject *>();
+            m_statusOrPendingObjects = quintptr(pendingObjects) | 1;
+        }
+        pendingObjects->push_back(obj);
+    }
+
+    std::vector<QObject *> *pendingObjectsWithBindingStatusChange()
+    {
+        auto statusOrPendingObjects = m_statusOrPendingObjects.loadAcquire();
+        if (statusOrPendingObjects & 1)
+            return reinterpret_cast<std::vector<QObject *> *>(statusOrPendingObjects - 1);
+        return nullptr;
+    }
+
+    QAtomicInteger<quintptr> m_statusOrPendingObjects = 0;
 #ifndef Q_OS_INTEGRITY
 private:
     // Used in QThread(Private)::start to avoid racy access to QObject::objectName,
@@ -220,8 +248,13 @@ public:
 
     mutable QMutex mutex;
     QThreadData *data;
+    QBindingStatus* m_bindingStatus;
     bool running = false;
 
+    QBindingStatus* bindingStatus() { return m_bindingStatus; }
+    void addObjectWithPendingBindingStatusChange(QObject *) {}
+    std::vector<QObject *> * pendingObjectsWithBindingStatusChange() { return nullptr; }
+
     static void setCurrentThread(QThread *) { }
     static QAbstractEventDispatcher *createEventDispatcher(QThreadData *data);
 
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index ef4c6868cf..877896407a 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -103,6 +103,7 @@ private slots:
     void compatPropertySignals();
 
     void noFakeDependencies();
+    void threadSafety();
 
     void bindablePropertyWithInitialization();
     void noDoubleNotification();
@@ -1681,6 +1682,91 @@ void tst_QProperty::noFakeDependencies()
     QCOMPARE(old, bindingFunctionCalled);
 }
 
+struct ThreadSafetyTester : public QObject
+{
+    Q_OBJECT
+
+public:
+    ThreadSafetyTester(QObject *parent = nullptr) : QObject(parent) {}
+
+    Q_INVOKABLE bool hasCorrectStatus() const
+    {
+        return qGetBindingStorage(this)->status({}) == QtPrivate::getBindingStatus({});
+    }
+
+    Q_INVOKABLE bool bindingTest()
+    {
+        QProperty<QString> name(u"inThread"_qs);
+        bindableObjectName().setBinding([&]() -> QString { return name; });
+        name = u"inThreadChanged"_qs;
+        const bool nameChangedCorrectly = objectName() == name;
+        bindableObjectName().takeBinding();
+        return nameChangedCorrectly;
+    }
+};
+
+
+void tst_QProperty::threadSafety()
+{
+    QThread workerThread;
+    auto cleanup = qScopeGuard([&](){
+        QMetaObject::invokeMethod(&workerThread, "quit");
+        workerThread.wait();
+    });
+    QScopedPointer<ThreadSafetyTester> scopedObj1(new ThreadSafetyTester);
+    auto obj1 = scopedObj1.data();
+    auto child1 = new ThreadSafetyTester(obj1);
+    obj1->moveToThread(&workerThread);
+    const auto mainThreadBindingStatus = QtPrivate::getBindingStatus({});
+    QCOMPARE(qGetBindingStorage(child1)->status({}), mainThreadBindingStatus);
+    workerThread.start();
+
+    bool correctStatus = false;
+    bool ok = QMetaObject::invokeMethod(obj1, "hasCorrectStatus", Qt::BlockingQueuedConnection,
+            Q_RETURN_ARG(bool, correctStatus));
+    QVERIFY(ok);
+    QVERIFY(correctStatus);
+
+    bool bindingWorks = false;
+    ok = QMetaObject::invokeMethod(obj1, "bindingTest", Qt::BlockingQueuedConnection,
+            Q_RETURN_ARG(bool, bindingWorks));
+    QVERIFY(ok);
+    QVERIFY(bindingWorks);
+
+    correctStatus = false;
+    ok = QMetaObject::invokeMethod(child1, "hasCorrectStatus", Qt::BlockingQueuedConnection,
+            Q_RETURN_ARG(bool, correctStatus));
+    QVERIFY(ok);
+    QVERIFY(correctStatus);
+
+    QScopedPointer scopedObj2(new ThreadSafetyTester);
+    auto obj2 = scopedObj2.data();
+    QCOMPARE(qGetBindingStorage(obj2)->status({}), mainThreadBindingStatus);
+
+    obj2->setObjectName("moved");
+    QCOMPARE(obj2->objectName(), "moved");
+
+    obj2->moveToThread(&workerThread);
+    correctStatus = false;
+    ok = QMetaObject::invokeMethod(obj2, "hasCorrectStatus", Qt::BlockingQueuedConnection,
+            Q_RETURN_ARG(bool, correctStatus));
+
+    QVERIFY(ok);
+    QVERIFY(correctStatus);
+    // potentially unsafe, but should still work (no writes in owning thread)
+    QCOMPARE(obj2->objectName(), "moved");
+
+
+    QScopedPointer scopedObj3(new ThreadSafetyTester);
+    auto obj3 = scopedObj3.data();
+    obj3->setObjectName("moved");
+    QCOMPARE(obj3->objectName(), "moved");
+    obj3->moveToThread(nullptr);
+    QCOMPARE(obj2->objectName(), "moved");
+    obj3->setObjectName("moved again");
+    QCOMPARE(obj3->objectName(), "moved again");
+}
+
 struct CustomType
 {
     CustomType() = default;