QThread/Unix: make QThreadPrivate::finish() be called much laterHEADdev

We need it to run after all the thread-local destructors have run, to
ensure that some user code hasn't run after QThreadPrivate::finish() has
finished. We achieve that by making it get called from a thread-local
destructor itself, in the form of a qScopeGuard.

This ought to have been done since C++11 thread_local with non-trivial
destructors became available. However, it only started showing up after
commit 4a93285b166ceceaea2e10c8fc6a254d2f7093b9 began using thread_local
inside Qt itself. The visible symptom was that QThreadPrivate::finish()
had already destroyed the thread's event dispatcher, but some user code
ran later and expected it to still exist (or, worse, recreated it, via
QEventLoop → QThreadData::ensureEventDispatcher).

Fixes: QTBUG-117996
Pick-to: 6.7
Change-Id: I8f3ce163ccc5408cac39fffd178d682e5bfa6955
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

2 files changed, 66 insertions, 22 deletions

diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp
index 617a5ebf28..556f05018f 100644
--- a/src/corelib/thread/qthread_unix.cpp
+++ b/src/corelib/thread/qthread_unix.cpp
@@ -282,8 +282,12 @@ void *QThreadPrivate::start(void *arg)
 #ifdef PTHREAD_CANCEL_DISABLE
     pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, nullptr);
 #endif
-    pthread_cleanup_push(QThreadPrivate::finish, arg);
-
+#if !defined(Q_OS_QNX)
+    // On QNX, calling finish() from a thread_local destructor causes the C
+    // library to hang.
+    static thread_local
+#endif
+            auto cleanup = qScopeGuard([=] { finish(arg); });
     terminate_on_exception([&] {
         QThread *thr = reinterpret_cast<QThread *>(arg);
         QThreadData *data = QThreadData::get2(thr);
@@ -328,11 +332,7 @@ void *QThreadPrivate::start(void *arg)
         thr->run();
     });
 
-    // This pop runs finish() below. It's outside the try/catch (and has its
-    // own try/catch) to prevent finish() to be run in case an exception is
-    // thrown.
-    pthread_cleanup_pop(1);
-
+    // The qScopeGuard above call runs finish() below.
     return nullptr;
 }
 
diff --git a/tests/auto/corelib/thread/qthread/tst_qthread.cpp b/tests/auto/corelib/thread/qthread/tst_qthread.cpp
index b163235d81..18c8d5fbd5 100644
--- a/tests/auto/corelib/thread/qthread/tst_qthread.cpp
+++ b/tests/auto/corelib/thread/qthread/tst_qthread.cpp
@@ -1299,8 +1299,16 @@ public:
     }
     void registerSocketNotifier(QSocketNotifier *) override {}
     void unregisterSocketNotifier(QSocketNotifier *) override {}
-    void registerTimer(Qt::TimerId, Duration, Qt::TimerType, QObject *) override {}
-    bool unregisterTimer(Qt::TimerId) override { return false; }
+    void registerTimer(Qt::TimerId id, Duration, Qt::TimerType, QObject *) override
+    {
+        if (registeredTimerId <= Qt::TimerId::Invalid)
+            registeredTimerId = id;
+    }
+    bool unregisterTimer(Qt::TimerId id) override
+    {
+        Qt::TimerId oldId = std::exchange(registeredTimerId, Qt::TimerId::Invalid);
+        return id == oldId;
+    }
     bool unregisterTimers(QObject *) override { return false; }
     QList<TimerInfoV2> timersForObject(QObject *) const override { return {}; }
     Duration remainingTime(Qt::TimerId) const override { return 0s; }
@@ -1313,25 +1321,47 @@ public:
 #endif
 
     QBasicAtomicInt visited; // bool
+    Qt::TimerId registeredTimerId = Qt::TimerId::Invalid;
 };
 
-class ThreadObj : public QObject
+struct ThreadLocalContent
 {
-    Q_OBJECT
-public slots:
-    void visit() {
-        emit visited();
+    static inline const QMetaObject *atStart;
+    static inline const QMetaObject *atEnd;
+    QSemaphore *sem;
+    QBasicTimer timer;
+
+    ThreadLocalContent(QObject *obj, QSemaphore *sem)
+        : sem(sem)
+    {
+        ensureEventDispatcher();
+        atStart = QAbstractEventDispatcher::instance()->metaObject();
+        timer.start(10s, obj);
+    }
+    ~ThreadLocalContent()
+    {
+        ensureEventDispatcher();
+        atEnd = QAbstractEventDispatcher::instance()->metaObject();
+        timer.stop();
+        sem->release();
+    }
+
+    void ensureEventDispatcher()
+    {
+        // QEventLoop's constructor has a call to QThreadData::ensureEventDispatcher()
+        QEventLoop dummy;
     }
-signals:
-    void visited();
 };
 
 void tst_QThread::customEventDispatcher()
 {
+    ThreadLocalContent::atStart = ThreadLocalContent::atEnd = nullptr;
+
     QThread thr;
     // there should be no ED yet
     QVERIFY(!thr.eventDispatcher());
     DummyEventDispatcher *ed = new DummyEventDispatcher;
+    QPointer<DummyEventDispatcher> weak_ed(ed);
     thr.setEventDispatcher(ed);
     // the new ED should be set
     QCOMPARE(thr.eventDispatcher(), ed);
@@ -1340,25 +1370,39 @@ void tst_QThread::customEventDispatcher()
     thr.start();
     // start() should not overwrite the ED
     QCOMPARE(thr.eventDispatcher(), ed);
+    QVERIFY(!weak_ed.isNull());
 
-    ThreadObj obj;
+    QObject obj;
     obj.moveToThread(&thr);
     // move was successful?
     QCOMPARE(obj.thread(), &thr);
-    QEventLoop loop;
-    connect(&obj, SIGNAL(visited()), &loop, SLOT(quit()), Qt::QueuedConnection);
-    QMetaObject::invokeMethod(&obj, "visit", Qt::QueuedConnection);
-    loop.exec();
+
+    QSemaphore threadLocalSemaphore;
+    QMetaObject::invokeMethod(&obj, [&]() {
+#ifndef Q_OS_WIN
+        // On Windows, the thread_locals are unsequenced between DLLs, so this
+        // could run after QThreadPrivate::finish()
+        static thread_local
+#endif
+                ThreadLocalContent d(&obj, &threadLocalSemaphore);
+    }, Qt::BlockingQueuedConnection);
+
     // test that the ED has really been used
     QVERIFY(ed->visited.loadRelaxed());
+    // and it's ours
+    QCOMPARE(ThreadLocalContent::atStart->className(), "DummyEventDispatcher");
 
-    QPointer<DummyEventDispatcher> weak_ed(ed);
     QVERIFY(!weak_ed.isNull());
     thr.quit();
+
     // wait for thread to be stopped
     QVERIFY(thr.wait(30000));
+    QVERIFY(threadLocalSemaphore.tryAcquire(1, 30s));
+
     // test that ED has been deleted
     QVERIFY(weak_ed.isNull());
+    // test that ED was ours
+    QCOMPARE(ThreadLocalContent::atEnd->className(), "DummyEventDispatcher");
 }
 
 class Job : public QObject