Make the QEventDispatcherWin32Private::interrupt flag atomic

Not entirely sure that this solves the problem reported in the bug
report, but here's the theory: the loop

 633        while (!d->interrupt) {
 ...
 710        }

has few calls that recurse back, so the compiler optimizer can assume
that the variable remains unchanged (not interrupted) in most of the
branches. Additionally, it can assume the variable did not change from
there to

 712        // still nothing - wait for message or signalled objects
 713        canWait = (!retVal
 714                   && !d->interrupt
 715                   && (flags & QEventLoop::WaitForMoreEvents));

Which causes canWait to be true, despite having been interrupted by
another thread.

Changing to an atomic does not force the reloading of the variable
(strictly speaking, would need volatile, but all atomic implementations
do reload now), but it solves the problem of data race, which was UB.

The equivalent variable in the Unix event dispatcher is atomic (commit
49d7e71f77 from 2013). I've reordered the
bool members so they're all together and reduce the amount of padding in
this class.

Fixes: QTBUG-72438
Change-Id: I4ac1156702324f0fb814fffd156f290df94dc4c7
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
This commit is contained in:
Thiago Macieira 2018-12-10 19:18:57 -08:00
parent 1500d2283e
commit 197029b3d2
2 changed files with 9 additions and 8 deletions

View File

@ -95,7 +95,7 @@ class QEventDispatcherWin32Private;
LRESULT QT_WIN_CALLBACK qt_internal_proc(HWND hwnd, UINT message, WPARAM wp, LPARAM lp);
QEventDispatcherWin32Private::QEventDispatcherWin32Private()
: threadId(GetCurrentThreadId()), interrupt(false), closingDown(false), internalHwnd(0),
: threadId(GetCurrentThreadId()), interrupt(false), internalHwnd(0),
getMessageHook(0), serialNumber(0), lastSerialNumber(0), sendPostedEventsWindowsTimerId(0),
wakeUps(0), activateNotifiersPosted(false), winEventNotifierActivatedEvent(NULL)
{
@ -552,7 +552,7 @@ bool QEventDispatcherWin32::processEvents(QEventLoop::ProcessEventsFlags flags)
wakeUp(); // trigger a call to sendPostedEvents()
}
d->interrupt = false;
d->interrupt.store(false);
emit awake();
bool canWait;
@ -568,7 +568,7 @@ bool QEventDispatcherWin32::processEvents(QEventLoop::ProcessEventsFlags flags)
pHandles = &d->winEventNotifierActivatedEvent;
}
QVarLengthArray<MSG> processedTimers;
while (!d->interrupt) {
while (!d->interrupt.load()) {
MSG msg;
bool haveMessage;
@ -649,7 +649,7 @@ bool QEventDispatcherWin32::processEvents(QEventLoop::ProcessEventsFlags flags)
// still nothing - wait for message or signalled objects
canWait = (!retVal
&& !d->interrupt
&& !d->interrupt.load()
&& (flags & QEventLoop::WaitForMoreEvents));
if (canWait) {
emit aboutToBlock();
@ -1016,7 +1016,7 @@ void QEventDispatcherWin32::wakeUp()
void QEventDispatcherWin32::interrupt()
{
Q_D(QEventDispatcherWin32);
d->interrupt = true;
d->interrupt.store(true);
wakeUp();
}

View File

@ -165,8 +165,7 @@ public:
DWORD threadId;
bool interrupt;
bool closingDown;
QAtomicInt interrupt;
// internal window handle used for socketnotifiers/timers/etc
HWND internalHwnd;
@ -193,9 +192,11 @@ public:
void postActivateSocketNotifiers();
void doWsaAsyncSelect(int socket, long event);
bool closingDown = false;
bool winEventNotifierListModified = false;
HANDLE winEventNotifierActivatedEvent;
QList<QWinEventNotifier *> winEventNotifierList;
bool winEventNotifierListModified = false;
void activateEventNotifier(QWinEventNotifier * wen);
QList<MSG> queuedUserInputEvents;