From 9e20db8ac9149766f6ab711d6dffbcd1a9d0b501 Mon Sep 17 00:00:00 2001 From: Jesper Alf Dam Date: Fri, 12 Jul 2024 22:48:53 +0200 Subject: [PATCH] Win32: Fix timers firing early when timer ID is reused It is possible for a timer to be stopped while a WM_TIMER event is in the event queue. In this case, we will process the event after the timer has been freed. If we're unlucky, the timer ID will have been reused, and then the new timer will erroneously fire. To fix this, quarantine timer IDs when the timer is stopped. In processEvents() we can release the quarantined IDs if we're sure there are no enqueued WM_TIMER events, if either: * we processed all events in the queue or, * we were interrupted, but a PeekMessage() call tells us no WM_TIMER events are enqueued. Because we only want this behavior on Windows, the static QAbstractEventDispatcher::releaseTimerId() function must delegate to a virtual method on the event dispatcher instance, allowing the Windows implementation to add the described behavior. --- .../kernel/qabstracteventdispatcher.cpp | 21 ++++++++++++++ .../kernel/qabstracteventdispatcher_p.h | 3 ++ src/corelib/kernel/qeventdispatcher_win.cpp | 29 ++++++++++++++++++- src/corelib/kernel/qeventdispatcher_win_p.h | 4 +++ 4 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/corelib/kernel/qabstracteventdispatcher.cpp b/src/corelib/kernel/qabstracteventdispatcher.cpp index 98493946d8a..d0527b3486e 100644 --- a/src/corelib/kernel/qabstracteventdispatcher.cpp +++ b/src/corelib/kernel/qabstracteventdispatcher.cpp @@ -77,6 +77,27 @@ int QAbstractEventDispatcherPrivate::allocateTimerId() } void QAbstractEventDispatcherPrivate::releaseTimerId(int timerId) +{ + // If the thread has an eventDispatcher, allow it to control the release of timer IDs + // Otherwise, just release the ID immediately + QAbstractEventDispatcher *eventDispatcher = QAbstractEventDispatcher::instance(); + + if (eventDispatcher) { + const auto d = eventDispatcher->d_func(); + d->releaseTimerIdImpl(timerId); + return; + } + + doReleaseTimerId(timerId); +} + +void QAbstractEventDispatcherPrivate::releaseTimerIdImpl(int timerId) +{ + // Default implementation just releases the ID, but subclasses may provide custom logic + doReleaseTimerId(timerId); +} + +void QAbstractEventDispatcherPrivate::doReleaseTimerId(int timerId) { // this function may be called by a global destructor after // timerIdFreeList() has been destructed diff --git a/src/corelib/kernel/qabstracteventdispatcher_p.h b/src/corelib/kernel/qabstracteventdispatcher_p.h index 7d57fd03604..fd071f1bc9c 100644 --- a/src/corelib/kernel/qabstracteventdispatcher_p.h +++ b/src/corelib/kernel/qabstracteventdispatcher_p.h @@ -33,6 +33,9 @@ public: static int allocateTimerId(); static void releaseTimerId(int id); +protected: + virtual void releaseTimerIdImpl(int id); + static void doReleaseTimerId(int id); }; QT_END_NAMESPACE diff --git a/src/corelib/kernel/qeventdispatcher_win.cpp b/src/corelib/kernel/qeventdispatcher_win.cpp index a7663b24813..ebd1b878564 100644 --- a/src/corelib/kernel/qeventdispatcher_win.cpp +++ b/src/corelib/kernel/qeventdispatcher_win.cpp @@ -408,6 +408,24 @@ void QEventDispatcherWin32Private::sendTimerEvent(int timerId) } } +void QEventDispatcherWin32Private::releaseTimerIdImpl(int timerId) +{ + // QTBUG-124496: On Windows, we may have pending WM_TIMER messages when we stop the timer, + // so we shouldn't immediately allow the timer ID to be reused. + // Instead, quarantine it so it can be released later, when it is safe + quarantinedTimers.push_back(timerId); +} + +void QEventDispatcherWin32Private::releaseQuarantinedTimers() +{ + // When this function is called we know the quarantined timers have no queued WM_TIMER events, + // so the IDs are safe to finally release + for (const auto &id : quarantinedTimers){ + QAbstractEventDispatcherPrivate::doReleaseTimerId(id); + } + quarantinedTimers.clear(); +} + void QEventDispatcherWin32Private::doWsaAsyncSelect(qintptr socket, long event) { Q_ASSERT(internalHwnd); @@ -539,9 +557,18 @@ bool QEventDispatcherWin32::processEvents(QEventLoop::ProcessEventsFlags flags) retVal = true; } + const auto interrupted = d->interrupt.loadRelaxed(); + MSG unused; + // If we weren't interrupted, that means there are no messages left in the queue + // If we were interrupted, there might be messages left, but we can check if there are timer events + if (!interrupted || !PeekMessage(&unused, nullptr, WM_TIMER, WM_TIMER, PM_NOREMOVE)) { + // If there are no timer events left, we can release the quarantined timer ids + d->releaseQuarantinedTimers(); + } + // wait for message canWait = (!retVal - && !d->interrupt.loadRelaxed() + && !interrupted && flags.testFlag(QEventLoop::WaitForMoreEvents) && threadData->canWaitLocked()); if (canWait) { diff --git a/src/corelib/kernel/qeventdispatcher_win_p.h b/src/corelib/kernel/qeventdispatcher_win_p.h index 558490a85e8..6128aae5cdf 100644 --- a/src/corelib/kernel/qeventdispatcher_win_p.h +++ b/src/corelib/kernel/qeventdispatcher_win_p.h @@ -19,6 +19,7 @@ #include "QtCore/qt_windows.h" #include "QtCore/qhash.h" #include "QtCore/qatomic.h" +#include "QtCore/qlist.h" #include "qabstracteventdispatcher_p.h" @@ -124,9 +125,12 @@ public: // timers WinTimerDict timerDict; + QList quarantinedTimers; void registerTimer(WinTimerInfo *t); void unregisterTimer(WinTimerInfo *t); void sendTimerEvent(int timerId); + void releaseTimerIdImpl(int timerId) override; + void releaseQuarantinedTimers(); // socket notifiers QSNDict sn_read; -- 2.45.2.windows.1