From 91aed00288ec61275576121f957a35d7b98ad57f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 13 Jan 2018 15:23:30 +0100 Subject: [PATCH 1/4] Revert "Revert using an event object for waking up event loop in wxMSW" This reverts commit ebb3a791b96bcb26bcad9a4c97e8e62e2d6aa6ed, effectively reapplying 6c40531fb763fcb850bfe275bdcac018b147be7c once again. This breaks wake up when not running our own event loop once again (see #17579), but this will be fixed in the next commit. --- include/wx/msw/evtloop.h | 1 - include/wx/msw/evtloopconsole.h | 19 ++++++- src/msw/app.cpp | 63 ++++++----------------- src/msw/evtloop.cpp | 5 -- src/msw/evtloopconsole.cpp | 90 +++++++++++++++++++-------------- 5 files changed, 85 insertions(+), 93 deletions(-) diff --git a/include/wx/msw/evtloop.h b/include/wx/msw/evtloop.h index 43ef271a87..911d77d385 100644 --- a/include/wx/msw/evtloop.h +++ b/include/wx/msw/evtloop.h @@ -53,7 +53,6 @@ public: // override/implement base class virtuals virtual bool Dispatch() wxOVERRIDE; virtual int DispatchTimeout(unsigned long timeout) wxOVERRIDE; - virtual void WakeUp() wxOVERRIDE; protected: virtual void OnNextIteration() wxOVERRIDE; diff --git a/include/wx/msw/evtloopconsole.h b/include/wx/msw/evtloopconsole.h index 32c411a50a..17df0a4e7a 100644 --- a/include/wx/msw/evtloopconsole.h +++ b/include/wx/msw/evtloopconsole.h @@ -15,9 +15,20 @@ class WXDLLIMPEXP_BASE wxMSWEventLoopBase : public wxEventLoopManual { public: wxMSWEventLoopBase(); + virtual ~wxMSWEventLoopBase(); // implement base class pure virtuals virtual bool Pending() const wxOVERRIDE; + virtual void WakeUp() wxOVERRIDE; + +#if wxUSE_THREADS + // MSW-specific method to wait for the termination of the specified (by its + // native handle) thread or any input message arriving (in GUI case). + // + // Return value is WAIT_OBJECT_0 if the thread terminated, WAIT_OBJECT_0+1 + // if a message arrived with anything else indicating an error. + WXDWORD MSWWaitForThread(WXHANDLE hThread); +#endif // wxUSE_THREADS protected: // get the next message from queue and return true or return false if we @@ -25,8 +36,13 @@ protected: bool GetNextMessage(WXMSG *msg); // same as above but with a timeout and return value can be -1 meaning that - // time out expired in addition to + // time out expired in addition to true/false int GetNextMessageTimeout(WXMSG *msg, unsigned long timeout); + +private: + // An auto-reset Win32 event which is signalled when we need to wake up the + // main thread waiting in GetNextMessage[Timeout](). + WXHANDLE m_heventWake; }; #if wxUSE_CONSOLE_EVENTLOOP @@ -39,7 +55,6 @@ public: // override/implement base class virtuals virtual bool Dispatch() wxOVERRIDE; virtual int DispatchTimeout(unsigned long timeout) wxOVERRIDE; - virtual void WakeUp() wxOVERRIDE; // Windows-specific function to process a single message virtual void ProcessMessage(WXMSG *msg); diff --git a/src/msw/app.cpp b/src/msw/app.cpp index 27a4ca23e7..866078a53a 100644 --- a/src/msw/app.cpp +++ b/src/msw/app.cpp @@ -248,23 +248,16 @@ WXDWORD wxGUIAppTraits::WaitForThread(WXHANDLE hThread, int flags) // have a running event loop as we would never remove them from the message // queue then and so we would enter an infinite loop as // MsgWaitForMultipleObjects() keeps returning WAIT_OBJECT_0 + 1. - if ( flags == wxTHREAD_WAIT_BLOCK || - !wxIsMainThread() || - !wxEventLoop::GetActive() ) + if ( flags == wxTHREAD_WAIT_YIELD && wxIsMainThread() ) { - // Simple blocking wait. - return DoSimpleWaitForThread(hThread); + wxMSWEventLoopBase* const + evtLoop = static_cast(wxEventLoop::GetActive()); + if ( evtLoop ) + return evtLoop->MSWWaitForThread(hThread); } - return ::MsgWaitForMultipleObjects - ( - 1, // number of objects to wait for - (HANDLE *)&hThread, // the objects - false, // wait for any objects, not all - INFINITE, // no timeout - QS_ALLINPUT | // return as soon as there are any events - QS_ALLPOSTMESSAGE - ); + // Simple blocking wait. + return DoSimpleWaitForThread(hThread); } #endif // wxUSE_THREADS @@ -785,42 +778,16 @@ void wxApp::OnIdle(wxIdleEvent& WXUNUSED(event)) void wxApp::WakeUpIdle() { - // Send the top window a dummy message so idle handler processing will - // start up again. Doing it this way ensures that the idle handler - // wakes up in the right thread (see also wxWakeUpMainThread() which does - // the same for the main app thread only) - wxWindow * const topWindow = wxTheApp->GetTopWindow(); - if ( topWindow ) + wxEventLoopBase * const evtLoop = wxEventLoop::GetActive(); + if ( !evtLoop ) { - HWND hwndTop = GetHwndOf(topWindow); - - // Do not post WM_NULL if there's already a pending WM_NULL to avoid - // overflowing the message queue. - // - // Notice that due to a limitation of PeekMessage() API (which handles - // 0,0 range specially), we have to check the range from 0-1 instead. - // This still makes it possible to overflow the queue with WM_NULLs by - // interspersing the calles to WakeUpIdle() with windows creation but - // it should be rather hard to do it accidentally. - MSG msg; - if ( !::PeekMessage(&msg, hwndTop, 0, 1, PM_NOREMOVE) || - ::PeekMessage(&msg, hwndTop, 1, 1, PM_NOREMOVE) ) - { - // If this fails too, there is really not much we can do, but then - // neither do we need to, as it normally indicates that the window - // queue is full to the brim with the messages and so the main loop - // is running and doesn't need to be woken up. - // - // Notice that we especially should not try use wxLogLastError() - // here as this would lead to another call to wxWakeUpIdle() from - // inside wxLog and stack overflow due to the resulting recursion. - ::PostMessage(hwndTop, WM_NULL, 0, 0); - } + // We can't wake up the event loop if there is none and there is just + // no need to do anything in this case, any pending events will be + // handled when the event loop starts. + return; } -#if wxUSE_THREADS - else - wxWakeUpMainThread(); -#endif // wxUSE_THREADS + + evtLoop->WakeUp(); } // ---------------------------------------------------------------------------- diff --git a/src/msw/evtloop.cpp b/src/msw/evtloop.cpp index bdfab07471..47b7972bc3 100644 --- a/src/msw/evtloop.cpp +++ b/src/msw/evtloop.cpp @@ -248,11 +248,6 @@ void wxGUIEventLoop::OnNextIteration() #endif // wxUSE_THREADS } -void wxGUIEventLoop::WakeUp() -{ - ::PostMessage(NULL, WM_NULL, 0, 0); -} - // ---------------------------------------------------------------------------- // Yield to incoming messages diff --git a/src/msw/evtloopconsole.cpp b/src/msw/evtloopconsole.cpp index a746343609..c0ba2716fa 100644 --- a/src/msw/evtloopconsole.cpp +++ b/src/msw/evtloopconsole.cpp @@ -42,6 +42,17 @@ wxMSWEventLoopBase::wxMSWEventLoopBase() { m_shouldExit = false; m_exitcode = 0; + + // Create initially not signalled auto-reset event object. + m_heventWake = ::CreateEvent(NULL, FALSE, FALSE, NULL); + if ( !m_heventWake ) + wxLogLastError(wxS("CreateEvent(wake)")); +} + +wxMSWEventLoopBase::~wxMSWEventLoopBase() +{ + if ( m_heventWake && !::CloseHandle(m_heventWake) ) + wxLogLastError(wxS("CloseHandle(wake)")); } // ---------------------------------------------------------------------------- @@ -54,26 +65,36 @@ bool wxMSWEventLoopBase::Pending() const return ::PeekMessage(&msg, 0, 0, 0, PM_NOREMOVE) != 0; } +void wxMSWEventLoopBase::WakeUp() +{ + if ( !::SetEvent(m_heventWake) ) + wxLogLastError(wxS("SetEvent(wake)")); +} + +#if wxUSE_THREADS + +WXDWORD wxMSWEventLoopBase::MSWWaitForThread(WXHANDLE hThread) +{ + // The order is important here, the code using this function assumes that + // WAIT_OBJECT_0 indicates the thread termination and anything else -- the + // availability of an input event. So the thread handle must come first. + HANDLE handles[2] = { hThread, m_heventWake }; + return ::MsgWaitForMultipleObjects + ( + WXSIZEOF(handles), // number of objects to wait for + handles, // the objects + false, // wait for any objects, not all + INFINITE, // no timeout + QS_ALLINPUT | // return as soon as there are any events + QS_ALLPOSTMESSAGE + ); +} + +#endif // wxUSE_THREADS + bool wxMSWEventLoopBase::GetNextMessage(WXMSG* msg) { - const BOOL rc = ::GetMessage(msg, NULL, 0, 0); - - if ( rc == 0 ) - { - // got WM_QUIT - return false; - } - - if ( rc == -1 ) - { - // should never happen, but let's test for it nevertheless - wxLogLastError(wxT("GetMessage")); - - // still break from the loop - return false; - } - - return true; + return GetNextMessageTimeout(msg, INFINITE) == TRUE; } int wxMSWEventLoopBase::GetNextMessageTimeout(WXMSG *msg, unsigned long timeout) @@ -81,16 +102,14 @@ int wxMSWEventLoopBase::GetNextMessageTimeout(WXMSG *msg, unsigned long timeout) // MsgWaitForMultipleObjects() won't notice any input which was already // examined (e.g. using PeekMessage()) but not yet removed from the queue // so we need to remove any immediately messages manually - if ( !::PeekMessage(msg, 0, 0, 0, PM_REMOVE) ) + while ( !::PeekMessage(msg, 0, 0, 0, PM_REMOVE) ) { - // we use this function just in order to not block longer than the - // given timeout, so we don't pass any handles to it at all DWORD rc = ::MsgWaitForMultipleObjects ( - 0, NULL, + 1, &m_heventWake, FALSE, timeout, - QS_ALLINPUT + QS_ALLINPUT | QS_ALLPOSTMESSAGE ); switch ( rc ) @@ -104,13 +123,17 @@ int wxMSWEventLoopBase::GetNextMessageTimeout(WXMSG *msg, unsigned long timeout) return -1; case WAIT_OBJECT_0: - if ( !::PeekMessage(msg, 0, 0, 0, PM_REMOVE) ) - { - // somehow it may happen that MsgWaitForMultipleObjects() - // returns true but there are no messages -- just treat it - // the same as timeout then - return -1; - } + // We were woken up by a background thread, which means there + // is no actual input message available, but we should still + // return to the event loop, so pretend there was WM_NULL in + // the queue. + wxZeroMemory(*msg); + return TRUE; + + case WAIT_OBJECT_0 + 1: + // Some message is supposed to be available, but spurious + // wake ups are also possible, so just return to the loop: + // either we'll get the message or start waiting again. break; } } @@ -124,13 +147,6 @@ int wxMSWEventLoopBase::GetNextMessageTimeout(WXMSG *msg, unsigned long timeout) #if wxUSE_CONSOLE_EVENTLOOP -void wxConsoleEventLoop::WakeUp() -{ -#if wxUSE_THREADS - wxWakeUpMainThread(); -#endif -} - void wxConsoleEventLoop::ProcessMessage(WXMSG *msg) { ::DispatchMessage(msg); From 9b4759d7b6dfc1ea016db543405c09ae2f0d49db Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 13 Jan 2018 17:33:09 +0100 Subject: [PATCH 2/4] Don't rely on getting WM_NULL messages in wxIdleWakeUpModule Since the switch to using an event object for idle handling wakeup, WM_NULLs are not being sent any longer, so wxIdleWakeUpModule didn't do anything, resulting in pending event dispatching being paused while our event loop was not running, as it happened, for example, while the window was resized. See #17579. --- include/wx/msw/app.h | 4 ++++ include/wx/msw/evtloopconsole.h | 4 ++++ src/msw/app.cpp | 10 ++++++++++ src/msw/evtloopconsole.cpp | 5 +++++ src/msw/window.cpp | 21 ++++++++------------- 5 files changed, 31 insertions(+), 13 deletions(-) diff --git a/include/wx/msw/app.h b/include/wx/msw/app.h index cbd4c09a46..6a8c8baf0d 100644 --- a/include/wx/msw/app.h +++ b/include/wx/msw/app.h @@ -103,6 +103,10 @@ public: // use it. static wxLayoutDirection MSWGetDefaultLayout(wxWindow* parent = NULL); + // Call ProcessPendingEvents() but only if we need to do it, i.e. there was + // a recent call to WakeUpIdle(). + void MSWProcessPendingEventsIfNeeded(); + protected: int m_printMode; // wxPRINT_WINDOWS, wxPRINT_POSTSCRIPT diff --git a/include/wx/msw/evtloopconsole.h b/include/wx/msw/evtloopconsole.h index 17df0a4e7a..87217bc64b 100644 --- a/include/wx/msw/evtloopconsole.h +++ b/include/wx/msw/evtloopconsole.h @@ -30,6 +30,10 @@ public: WXDWORD MSWWaitForThread(WXHANDLE hThread); #endif // wxUSE_THREADS + // Return true if wake up was requested and not handled yet, i.e. if + // m_heventWake is signaled. + bool MSWIsWakeUpRequested(); + protected: // get the next message from queue and return true or return false if we // got WM_QUIT or an error occurred diff --git a/src/msw/app.cpp b/src/msw/app.cpp index 866078a53a..2c3d14273b 100644 --- a/src/msw/app.cpp +++ b/src/msw/app.cpp @@ -790,6 +790,16 @@ void wxApp::WakeUpIdle() evtLoop->WakeUp(); } +void wxApp::MSWProcessPendingEventsIfNeeded() +{ + // The cast below is safe as wxEventLoop derives from wxMSWEventLoopBase in + // both console and GUI applications. + wxMSWEventLoopBase * const evtLoop + = static_cast(wxEventLoop::GetActive()); + if ( evtLoop && evtLoop->MSWIsWakeUpRequested() ) + ProcessPendingEvents(); +} + // ---------------------------------------------------------------------------- // other wxApp event handlers // ---------------------------------------------------------------------------- diff --git a/src/msw/evtloopconsole.cpp b/src/msw/evtloopconsole.cpp index c0ba2716fa..869ea888fd 100644 --- a/src/msw/evtloopconsole.cpp +++ b/src/msw/evtloopconsole.cpp @@ -71,6 +71,11 @@ void wxMSWEventLoopBase::WakeUp() wxLogLastError(wxS("SetEvent(wake)")); } +bool wxMSWEventLoopBase::MSWIsWakeUpRequested() +{ + return ::WaitForSingleObject(m_heventWake, 0) == WAIT_OBJECT_0; +} + #if wxUSE_THREADS WXDWORD wxMSWEventLoopBase::MSWWaitForThread(WXHANDLE hThread) diff --git a/src/msw/window.cpp b/src/msw/window.cpp index ea92c52ed0..41e84b5559 100644 --- a/src/msw/window.cpp +++ b/src/msw/window.cpp @@ -7511,10 +7511,9 @@ bool wxWindowMSW::HandleHotKey(WXWPARAM wParam, WXLPARAM lParam) #endif // wxUSE_HOTKEY // this class installs a message hook which really wakes up our idle processing -// each time a WM_NULL is received (wxWakeUpIdle does this), even if we're -// sitting inside a local modal loop (e.g. a menu is opened or scrollbar is -// being dragged or even inside ::MessageBox()) and so don't control message -// dispatching otherwise +// each time a message is handled, even if we're sitting inside a local modal +// loop (e.g. a menu is opened or scrollbar is being dragged or even inside +// ::MessageBox()) and so don't control message dispatching otherwise class wxIdleWakeUpModule : public wxModule { public: @@ -7545,15 +7544,11 @@ public: static LRESULT CALLBACK MsgHookProc(int nCode, WPARAM wParam, LPARAM lParam) { - MSG *msg = (MSG*)lParam; - - // only process the message if it is actually going to be removed from - // the message queue, this prevents that the same event from being - // processed multiple times if now someone just called PeekMessage() - if ( msg->message == WM_NULL && wParam == PM_REMOVE ) - { - wxTheApp->ProcessPendingEvents(); - } + // Don't process idle events unless the message is going to be really + // handled, i.e. removed from the queue, as it seems wrong to do it + // just because someone called PeekMessage(PM_NOREMOVE). + if ( wParam == PM_REMOVE ) + wxTheApp->MSWProcessPendingEventsIfNeeded(); return CallNextHookEx(ms_hMsgHookProc, nCode, wParam, lParam); } From d617834eb96b3c879d68f731c60231fdd49c2afe Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 13 Jan 2018 17:39:40 +0100 Subject: [PATCH 3/4] Don't make wxEventLoop::WakeUpIdle() virtual It just forwards to (virtual) WakeUp() and there should be no need to ever override this method itself (nor even to keep it, except for backwards compatibility). No real changes. --- include/wx/evtloop.h | 5 +++-- src/common/evtloopcmn.cpp | 5 ----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/include/wx/evtloop.h b/include/wx/evtloop.h index 00e6d097d5..bd50999509 100644 --- a/include/wx/evtloop.h +++ b/include/wx/evtloop.h @@ -128,8 +128,9 @@ public: // idle handling // ------------- - // make sure that idle events are sent again - virtual void WakeUpIdle(); + // make sure that idle events are sent again: this is just an obsolete + // synonym for WakeUp() + void WakeUpIdle() { WakeUp(); } // this virtual function is called when the application // becomes idle and by default it forwards to wxApp::ProcessIdle() and diff --git a/src/common/evtloopcmn.cpp b/src/common/evtloopcmn.cpp index e8123f3caa..572c29626a 100644 --- a/src/common/evtloopcmn.cpp +++ b/src/common/evtloopcmn.cpp @@ -88,11 +88,6 @@ void wxEventLoopBase::OnExit() wxTheApp->OnEventLoopExit(this); } -void wxEventLoopBase::WakeUpIdle() -{ - WakeUp(); -} - bool wxEventLoopBase::ProcessIdle() { return wxTheApp && wxTheApp->ProcessIdle(); From d1e57312c2be1637c8c52feff1b9f072d828ca48 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 13 Jan 2018 17:41:16 +0100 Subject: [PATCH 4/4] Revert "Don't call wxWakeUpIdle() with a lock in wxProgressDialog" This reverts commit d26758044c55c0111ccd5e2c4d161f74c06d1771 which was a workaround for the problem of wxWakeUpIdle() dispatching events, as this is not the case any more after the latest changes. --- src/msw/progdlg.cpp | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/msw/progdlg.cpp b/src/msw/progdlg.cpp index 2f1131a5dc..46416402c5 100644 --- a/src/msw/progdlg.cpp +++ b/src/msw/progdlg.cpp @@ -1133,18 +1133,6 @@ wxProgressDialogTaskRunner::TaskDialogCallbackProc LONG_PTR dwRefData ) { - if ( uNotification == TDN_CREATED ) - { - // The main thread may be sitting in an event dispatching loop waiting - // for this dialog to be shown, so make sure it does wake up now that - // it is. Notice that we must do it from here and not from inside the - // block below in which sharedData is locked as otherwise we could - // deadlock if wxWakeUpIdle() dispatched some event which tried to call - // any of wxProgressDialog methods, which also lock this data, from the - // main thread. - wxWakeUpIdle(); - } - bool endDialog = false; // Block for shared data critical section. @@ -1160,6 +1148,10 @@ wxProgressDialogTaskRunner::TaskDialogCallbackProc // Store the HWND for the main thread use. sharedData->m_hwnd = hwnd; + // The main thread is sitting in an event dispatching loop waiting + // for this dialog to be shown, so make sure it does get an event. + wxWakeUpIdle(); + // Set the maximum value and disable Close button. ::SendMessage( hwnd, TDM_SET_PROGRESS_BAR_RANGE,