From c8d3d7a7af73d97e0415b99df8d8378edb3117f5 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 22 May 2025 17:27:31 -0700 Subject: [PATCH] QAbstractEventDispatcher: prevent too late unregistering of timers Timers can't unregister when the dispatcher object is no longer of a dispatcher type. It's too late even at ~QAbstractEventDispatcher, because unregisterTimer() is a pure virtual. To prevent their attempting to unregister, we set the thread's dispatcher to nullptr if it is this object. This has been a latent bug, so it's worth fixing. This started happening for me with an un-pushed change that changed the order of how QCoreApplication and QGuiApplication destroy the main thread event dispatcher (namely, in their destructors, not waiting for ~QObject to deleteChildren()). Drive-by relax the store in QThread::setEventDispatcher(). Fixes: QTBUG-137130 Pick-to: 6.9 6.8 Change-Id: I8845736c38a931af62e3fffdfd3554874df89e8e Reviewed-by: Volker Hilsheimer --- .../kernel/qabstracteventdispatcher.cpp | 6 +++++- src/corelib/thread/qthread.cpp | 2 +- .../auto/corelib/kernel/qtimer/tst_qtimer.cpp | 21 +++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/corelib/kernel/qabstracteventdispatcher.cpp b/src/corelib/kernel/qabstracteventdispatcher.cpp index f022b3b463b..997d6f0c98e 100644 --- a/src/corelib/kernel/qabstracteventdispatcher.cpp +++ b/src/corelib/kernel/qabstracteventdispatcher.cpp @@ -175,7 +175,11 @@ QAbstractEventDispatcher::QAbstractEventDispatcher(QAbstractEventDispatcherPriva Destroys the event dispatcher. */ QAbstractEventDispatcher::~QAbstractEventDispatcher() -{ } +{ + QThreadData *data = QThreadData::current(); + if (data->eventDispatcher.loadRelaxed() == this) + data->eventDispatcher.storeRelaxed(nullptr); +} /*! Returns a pointer to the event dispatcher object for the specified diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp index f94e5db1538..ea3d0769877 100644 --- a/src/corelib/thread/qthread.cpp +++ b/src/corelib/thread/qthread.cpp @@ -1236,7 +1236,7 @@ void QThread::setEventDispatcher(QAbstractEventDispatcher *eventDispatcher) } else { eventDispatcher->moveToThread(this); if (eventDispatcher->thread() == this) // was the move successful? - d->data->eventDispatcher = eventDispatcher; + d->data->eventDispatcher.storeRelaxed(eventDispatcher); else qWarning("QThread::setEventDispatcher: Could not move event dispatcher to target thread"); } diff --git a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp index 0a009d5ca8a..adff512e6a3 100644 --- a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp +++ b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp @@ -83,6 +83,7 @@ private slots: void singleShotToFunctors(); void singleShot_chrono(); void singleShot_static(); + void singleShotDestructionBeforeEventDispatcher(); void crossThreadSingleShotToFunctor_data(); void crossThreadSingleShotToFunctor(); #ifdef QT_BUILD_INTERNAL @@ -1193,6 +1194,22 @@ void tst_QTimer::postedEventsShouldNotStarveTimers() QTRY_VERIFY_WITH_TIMEOUT(timeoutSpy.size() > 5, 100); } +void tst_QTimer::singleShotDestructionBeforeEventDispatcher() +{ + // This makes sure the QSingleShotTimer doesn't cause a crash when the + // event dispatcher is deleted. As of the time of this test's writing, the + // QSST was parented to the dispatcher, so if it hadn't yet expired, it + // would be deleted by the QObject destructor, which is too late to + // unregister the timer. + + auto thr = QThread::create([] { + QObject o; + QTimer::singleShot(300s, &o, [] {}); + }); + thr->start(); + thr->wait(); +} + struct DummyFunctor { static QThread *callThread; void operator()() { @@ -1673,6 +1690,10 @@ void tst_QTimer::initMain() void tst_QTimer::cleanupTestCase() { delete s_staticSingleShotUser; + + // Same as singleShotDestructionBeforeEventDispatcher() above, but for the + // main thread. + QTimer::singleShot(300s, this, [] {}); } void tst_QTimer::singleShot_static()