QThread/Unix: revert to pthread destruction instead of thread_local

Amends 65093a84c2b94b1543fd4593bc45d491951d28d4, which changed how we
destroyed the main thread's QThreadData. This merges the call to
destroy_current_thread_data() for both types of Unix systems: those with
broken thread_local destructors and those with working ones. It turns
out that the function got called too early for us in those working
systems (see updated comment).

The clean up of the QThreadData is split into two different mechanisms:
 * for any auxiliary thread, when it exits, PThread will call back to
   destroy_current_thread_data()
 * for the thread that called ::exit(), PThread won't, but ::exit() will
   invoke set_thread_data()::TlsKey's destructor

This is different from the situation that existed prior to commit
65093a84c2b94b1543fd4593bc45d491951d28d4: first, there's no code in
qcoreapplication.cpp for this (all in qthread_unix.cpp). Second one may
call ::exit() from any thread, whether that is the thread that called
main(), the thread Qt thinks is theMainThread, or any other.

This commit moves the tst_QCoreApplication check for no extant objects
to a new test. I've chosen to add a new test instead of running a helper
binary via QProcess because we do have a couple of !QT_CONFIG(process)
platforms in the CI, and this is too important.

Credit to OSS-Fuzz for finding this, though it is not itself a fuzzying
problem (all tests of a given structure were crashing on exit).

Fixes: QTBUG-132381
Task-number: QTBUG-130895
Task-number: QTBUG-129927
Task-number: QTBUG-129846
Task-number: QTBUG-130341
Task-number: QTBUG-117996
Pick-to: 6.9 6.8
Change-Id: Ie294dce7263b4189f89ffffd9155ec71d31b89d9
Reviewed-by: Robert Löhning <robert.loehning@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/corelib/thread/qthread_unix.cpp

@@ -116,14 +116,22 @@ int pthread_timedjoin_np(...) { return ENOSYS; }    // pretend
 // Moreover, this can be racy and having our own thread_local early in
 // QThreadPrivate::start() made it even more so. See QTBUG-129846 for analysis.
 //
-// For the platforms where this C++11 feature is not properly implemented yet,
-// we fall back to a pthread_setspecific() call and do not perform late
-// clean-up, because then the order of registration of those pthread_specific_t
-// keys matters and Glib uses them too.
+// There's a good correlation between this C++11 feature and our ability to
+// call QThreadPrivate::cleanup() from destroy_thread_data().
 //
 // https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/libsupc%2B%2B/atexit_thread.cc;hb=releases/gcc-14.2.0#l133
 // https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/libcxxabi/src/cxa_thread_atexit.cpp#L118-L120
-#endif // QT_CONFIG(broken_threadlocal_dtors)
+#endif
+//
+// However, we can't destroy the QThreadData for the thread that called
+// ::exit() that early, because a lot of existing content (including in Qt)
+// runs when the static destructors are run and they do depend on QThreadData
+// being extant. Likewise, we can't destroy it at global static destructor time
+// because it's too late: the event dispatcher is usually a class found in a
+// plugin and the plugin's destructors (as well as QtGui's) will have run. So
+// we strike a middle-ground and destroy at function-local static destructor
+// time (see set_thread_data()), because those run after the thread_local ones,
+// before the global ones, and in reverse order of creation.
 
 Q_CONSTINIT static thread_local QThreadData *currentThreadData = nullptr;
 
@@ -165,34 +173,21 @@ static QThreadData *get_thread_data()
     return currentThreadData;
 }
 
-#if QT_CONFIG(broken_threadlocal_dtors)
-// The destructors registered with pthread_key_create() below are NOT run from
-// exit(), so we must also use atexit().
-static void destroy_main_thread_data()
-{
-    if (QThreadData *d = get_thread_data())
-        destroy_current_thread_data(d);
-}
-Q_DESTRUCTOR_FUNCTION(destroy_main_thread_data)
-#endif
-
 static void set_thread_data(QThreadData *data) noexcept
 {
     if (data) {
-        if constexpr (QT_CONFIG(broken_threadlocal_dtors)) {
-            static pthread_key_t tls_key;
-            struct TlsKey {
-                TlsKey() noexcept { pthread_key_create(&tls_key, destroy_current_thread_data); }
-                ~TlsKey() { pthread_key_delete(tls_key); }
-            };
-            static TlsKey currentThreadCleanup;
-            pthread_setspecific(tls_key, data);
-        } else {
-            struct Cleanup {
-                ~Cleanup() { destroy_current_thread_data(currentThreadData); }
-            };
-            static thread_local Cleanup currentThreadCleanup;
-        }
+        static pthread_key_t tls_key;
+        struct TlsKey {
+            TlsKey() noexcept { pthread_key_create(&tls_key, destroy_current_thread_data); }
+            ~TlsKey()
+            {
+                if (QThreadData *data = currentThreadData)
+                    destroy_current_thread_data(data);
+                pthread_key_delete(tls_key);
+            }
+        };
+        static TlsKey currentThreadCleanup;
+        pthread_setspecific(tls_key, data);
     }
     currentThreadData = data;
 }

src/corelib/thread/qthread_win.cpp

@@ -7,6 +7,7 @@
 #include "qcoreapplication.h"
 #include <private/qcoreapplication_p.h>
 #include <private/qeventdispatcher_win_p.h>
+#include "qloggingcategory.h"
 #include "qmutex.h"
 #include "qthreadstorage.h"
 

tests/auto/corelib/kernel/qcoreapplication/CMakeLists.txt

@@ -37,3 +37,10 @@ endif()
 if (ANDROID)
     set_property(TARGET tst_qcoreapplication PROPERTY QT_ANDROID_VERSION_NAME ${target_version})
 endif()
+
+qt_internal_add_test(tst_static_qcoreapplication
+    SOURCES
+        tst_static_qcoreapplication.cpp
+    LIBRARIES
+        Qt::CorePrivate
+)

tests/auto/corelib/kernel/qcoreapplication/tst_qcoreapplication.cpp

@@ -1065,27 +1065,6 @@ static void createQObjectOnDestruction()
     // QThread) after the last QObject has been destroyed (especially after
     // QCoreApplication has).
 
-#if defined(QT_QGUIAPPLICATIONTEST)
-    // If we've linked to QtGui, we make no representations about there being
-    // global static (not Q_GLOBAL_STATIC) variables that are QObject.
-#elif QT_CONFIG(broken_threadlocal_dtors)
-    // With broken thread-local destructors, we cannot guarantee the ordering
-    // between thread_local destructors and static-lifetime destructors (hence
-    // why they're broken).
-    //
-    // On Unix systems, we use a Q_DESTRUCTOR_FUNCTION in qthread_unix.cpp to
-    // work around the issue, but that means it cannot have run yet.
-    //
-    // On Windows, given the nature of how destructors are run in DLLs, they're
-    // always considered broken. In fact, we know the destructor in
-    // qthread_win.cpp hasn't run yet.
-#else
-    // The thread_local destructor in qthread_unix.cpp has run so the
-    // QAdoptedThread must have been cleaned up.
-    if (theMainThreadIsSet())
-        qFatal("theMainThreadIsSet() returned true; some QObject must have leaked");
-#endif
-
     // Before the fixes, this would cause a dangling pointer dereference. If
     // the problem comes back, it's possible that the following causes no
     // effect.

tests/auto/corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp

@@ -0,0 +1,81 @@
+// Copyright (C) 2024 Intel Corporation.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only
+
+#include <QtTest/QTest>
+#include <private/qcoreapplication_p.h>
+#include <private/qhooks_p.h>
+
+#ifdef QT_WIDGETS_LIB
+#  define tst_Static_QCoreApplication tst_Static_QApplication
+using App = QApplication;
+#elif defined(QT_GUI_LIB)
+#  define tst_Static_QCoreApplication tst_Static_QGuiApplication
+using App = QGuiApplication;
+#else
+using App = QCoreApplication;
+#endif
+
+class tst_Static_QCoreApplication : public QObject
+{
+    Q_OBJECT
+public:
+    ~tst_Static_QCoreApplication() {}
+private Q_SLOTS:
+    void staticApplication();
+};
+
+void tst_Static_QCoreApplication::staticApplication()
+{
+    // This test only verifies that the destructor, when run inside of exit(),
+    // will not crash.
+    static int argc = 1;
+    const char *argv[] = { staticMetaObject.className(), nullptr };
+    static App app(argc, const_cast<char **>(argv));
+}
+
+struct HookManager
+{
+    static inline QBasicAtomicInt objectCount = {};
+    static void addObject(QObject *o)
+    {
+        // printf("+ %p = %s\n", o, o->metaObject()->className());
+        objectCount.ref();
+        Q_UNUSED(o);
+    }
+    static void removeObject(QObject *o)
+    {
+        objectCount.deref();
+        // printf("- %p\n", o);
+        Q_UNUSED(o);
+    }
+
+    HookManager()
+    {
+        qtHookData[QHooks::AddQObject] = reinterpret_cast<quintptr>(&addObject);
+        qtHookData[QHooks::RemoveQObject] = reinterpret_cast<quintptr>(&removeObject);
+    }
+
+    ~HookManager()
+    {
+        qtHookData[QHooks::AddQObject] = 0;
+        qtHookData[QHooks::RemoveQObject] = 0;
+
+        auto app = qApp;
+        if (app)
+            qFatal("qApp was not destroyed = %p", app);
+#if !defined(QT_GUI_LIB) && !defined(Q_OS_WIN)
+        // Only tested for QtCore/QCoreApplication on Unix. QtGui has statics
+        // with QObject that haven't been cleaned up.
+        if (int c = objectCount.loadRelaxed())
+            qFatal("%d objects still alive", c);
+
+        if (void *id = QCoreApplicationPrivate::theMainThreadId.loadRelaxed())
+            qFatal("QCoreApplicationPrivate::theMainThreadId is still set - %p", id);
+#endif
+    }
+};
+static HookManager hookManager;
+
+QTEST_APPLESS_MAIN(tst_Static_QCoreApplication)
+#include "tst_static_qcoreapplication.moc"
+

tests/auto/gui/kernel/qguiapplication/CMakeLists.txt

@@ -46,3 +46,11 @@ if (APPLE)
     set_property(TARGET tst_qguiapplication PROPERTY MACOSX_BUNDLE_INFO_PLIST "${CMAKE_CURRENT_SOURCE_DIR}/Info.plist")
     set_property(TARGET tst_qguiapplication PROPERTY PROPERTY MACOSX_BUNDLE TRUE)
 endif()
+
+qt_internal_add_test(tst_static_qguiapplication
+    SOURCES
+        ../../../corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp
+    LIBRARIES
+        Qt::CorePrivate
+        Qt::Gui
+)

tests/auto/widgets/kernel/qapplication/CMakeLists.txt

@@ -15,3 +15,11 @@ add_dependencies(tst_qapplication
     desktopsettingsaware_helper
     modal_helper
 )
+
+qt_internal_add_test(tst_static_qapplication
+    SOURCES
+        ../../../corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp
+    LIBRARIES
+        Qt::CorePrivate
+        Qt::Widgets
+)