[PATCH v3 0/1] MR2242: win32u: Do not set last error in wait_message() for zero count.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54405 Also fixes Assassin Creed hanging on start (for a similar reason: it doesn't expect a spurious -1 last error). There is already a fix attempt sent by Ivan Chikish in !2067 (https://gitlab.winehq.org/wine/wine/-/merge_requests/2067). But I suppose it may be fixed simpler like in this patch. wait_message() with zero count is only called from NtUserPeekMessage(). I think even if for some reason driver's MsgWaitForMultipleObjectsEx returns a real error code we still don't want to set last error here when called from PeekMessage(). -- v3: win32u: Expose and use ProcessEvents from drivers instead of MsgWaitForMultipleObjectsEx. https://gitlab.winehq.org/wine/wine/-/merge_requests/2242
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/win32u/dce.c | 3 +-- dlls/win32u/driver.c | 12 ++++-------- dlls/win32u/input.c | 3 +-- dlls/win32u/message.c | 20 +++++++++++++------- dlls/wineandroid.drv/android.h | 4 +--- dlls/wineandroid.drv/init.c | 2 +- dlls/wineandroid.drv/window.c | 11 ++++------- dlls/winemac.drv/event.c | 27 +++++---------------------- dlls/winemac.drv/gdi.c | 2 +- dlls/winemac.drv/macdrv.h | 4 +--- dlls/winex11.drv/event.c | 25 ++++--------------------- dlls/winex11.drv/init.c | 2 +- dlls/winex11.drv/x11drv.h | 4 +--- include/wine/gdi_driver.h | 2 +- 14 files changed, 39 insertions(+), 82 deletions(-) diff --git a/dlls/win32u/dce.c b/dlls/win32u/dce.c index af117fe1a23..5df30550cae 100644 --- a/dlls/win32u/dce.c +++ b/dlls/win32u/dce.c @@ -1449,7 +1449,6 @@ static void update_now( HWND hwnd, UINT rdw_flags ) */ BOOL WINAPI NtUserRedrawWindow( HWND hwnd, const RECT *rect, HRGN hrgn, UINT flags ) { - LARGE_INTEGER zero = { .QuadPart = 0 }; static const RECT empty; BOOL ret; @@ -1470,7 +1469,7 @@ BOOL WINAPI NtUserRedrawWindow( HWND hwnd, const RECT *rect, HRGN hrgn, UINT fla } /* process pending expose events before painting */ - if (flags & RDW_UPDATENOW) user_driver->pMsgWaitForMultipleObjectsEx( 0, NULL, &zero, QS_PAINT, 0 ); + if (flags & RDW_UPDATENOW) user_driver->pProcessEvents( QS_PAINT ); if (rect && !hrgn) { diff --git a/dlls/win32u/driver.c b/dlls/win32u/driver.c index e2c9ba04efd..1c0708461a1 100644 --- a/dlls/win32u/driver.c +++ b/dlls/win32u/driver.c @@ -806,13 +806,9 @@ static void nulldrv_GetDC( HDC hdc, HWND hwnd, HWND top_win, const RECT *win_rec { } -static NTSTATUS nulldrv_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, - const LARGE_INTEGER *timeout, - DWORD mask, DWORD flags ) +static BOOL nulldrv_ProcessEvents( DWORD mask ) { - if (!count && timeout && !timeout->QuadPart) return WAIT_TIMEOUT; - return NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), - !!(flags & MWMO_ALERTABLE), timeout ); + return FALSE; } static void nulldrv_ReleaseDC( HWND hwnd, HDC hdc ) @@ -1193,7 +1189,7 @@ static const struct user_driver_funcs lazy_load_driver = nulldrv_DestroyWindow, loaderdrv_FlashWindowEx, loaderdrv_GetDC, - nulldrv_MsgWaitForMultipleObjectsEx, + nulldrv_ProcessEvents, nulldrv_ReleaseDC, nulldrv_ScrollDC, nulldrv_SetCapture, @@ -1268,7 +1264,7 @@ void __wine_set_user_driver( const struct user_driver_funcs *funcs, UINT version SET_USER_FUNC(DestroyWindow); SET_USER_FUNC(FlashWindowEx); SET_USER_FUNC(GetDC); - SET_USER_FUNC(MsgWaitForMultipleObjectsEx); + SET_USER_FUNC(ProcessEvents); SET_USER_FUNC(ReleaseDC); SET_USER_FUNC(ScrollDC); SET_USER_FUNC(SetCapture); diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index 36f2a45f4ef..f768453594f 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -750,8 +750,7 @@ BOOL WINAPI NtUserGetCursorInfo( CURSORINFO *info ) static void check_for_events( UINT flags ) { - LARGE_INTEGER zero = { .QuadPart = 0 }; - if (user_driver->pMsgWaitForMultipleObjectsEx( 0, NULL, &zero, flags, 0 ) == WAIT_TIMEOUT) + if (!user_driver->pProcessEvents( flags )) flush_window_surfaces( TRUE ); } diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index ed4198991cf..de1ca8cefbc 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -2086,9 +2086,8 @@ static inline void check_for_driver_events( UINT msg ) { if (get_user_thread_info()->message_count > 200) { - LARGE_INTEGER zero = { .QuadPart = 0 }; flush_window_surfaces( FALSE ); - user_driver->pMsgWaitForMultipleObjectsEx( 0, NULL, &zero, QS_ALLINPUT, 0 ); + user_driver->pProcessEvents( QS_ALLINPUT ); } else if (msg == WM_TIMER || msg == WM_SYSTIMER) { @@ -2117,13 +2116,20 @@ static DWORD wait_message( DWORD count, const HANDLE *handles, DWORD timeout, DW if (enable_thunk_lock) lock = KeUserModeCallback( NtUserThunkLock, NULL, 0, &ret_ptr, &ret_len ); - ret = user_driver->pMsgWaitForMultipleObjectsEx( count, handles, get_nt_timeout( &time, timeout ), - mask, flags ); - if (HIWORD(ret)) /* is it an error code? */ + if (user_driver->pProcessEvents( mask )) ret = count ? count - 1 : 0; + else if (count) { - RtlSetLastWin32Error( RtlNtStatusToDosError(ret) ); - ret = WAIT_FAILED; + ret = NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), + !!(flags & MWMO_ALERTABLE), get_nt_timeout( &time, timeout )); + if (ret == count - 1) user_driver->pProcessEvents( mask ); + else if (HIWORD(ret)) /* is it an error code? */ + { + RtlSetLastWin32Error( RtlNtStatusToDosError(ret) ); + ret = WAIT_FAILED; + } } + else ret = WAIT_TIMEOUT; + if (ret == WAIT_TIMEOUT && !count && !timeout) NtYieldExecution(); if ((mask & QS_INPUT) == QS_INPUT) get_user_thread_info()->message_count = 0; diff --git a/dlls/wineandroid.drv/android.h b/dlls/wineandroid.drv/android.h index 22652f11d27..0d073a63bcc 100644 --- a/dlls/wineandroid.drv/android.h +++ b/dlls/wineandroid.drv/android.h @@ -89,9 +89,7 @@ extern SHORT ANDROID_VkKeyScanEx( WCHAR ch, HKL hkl ) DECLSPEC_HIDDEN; extern void ANDROID_SetCursor( HCURSOR handle ) DECLSPEC_HIDDEN; extern BOOL ANDROID_CreateWindow( HWND hwnd ) DECLSPEC_HIDDEN; extern void ANDROID_DestroyWindow( HWND hwnd ) DECLSPEC_HIDDEN; -extern NTSTATUS ANDROID_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, - const LARGE_INTEGER *timeout, - DWORD mask, DWORD flags ) DECLSPEC_HIDDEN; +extern BOOL ANDROID_ProcessEvents( DWORD mask ) DECLSPEC_HIDDEN; extern LRESULT ANDROID_DesktopWindowProc( HWND hwnd, UINT msg, WPARAM wp, LPARAM lp ) DECLSPEC_HIDDEN; extern void ANDROID_SetCursor( HCURSOR handle ) DECLSPEC_HIDDEN; extern void ANDROID_SetLayeredWindowAttributes( HWND hwnd, COLORREF key, BYTE alpha, diff --git a/dlls/wineandroid.drv/init.c b/dlls/wineandroid.drv/init.c index acf6b9abbbe..074aa6c6257 100644 --- a/dlls/wineandroid.drv/init.c +++ b/dlls/wineandroid.drv/init.c @@ -352,7 +352,7 @@ static const struct user_driver_funcs android_drv_funcs = .pCreateWindow = ANDROID_CreateWindow, .pDesktopWindowProc = ANDROID_DesktopWindowProc, .pDestroyWindow = ANDROID_DestroyWindow, - .pMsgWaitForMultipleObjectsEx = ANDROID_MsgWaitForMultipleObjectsEx, + .pProcessEvents = ANDROID_ProcessEvents, .pSetCapture = ANDROID_SetCapture, .pSetLayeredWindowAttributes = ANDROID_SetLayeredWindowAttributes, .pSetParent = ANDROID_SetParent, diff --git a/dlls/wineandroid.drv/window.c b/dlls/wineandroid.drv/window.c index a339c20ceda..f01df37a400 100644 --- a/dlls/wineandroid.drv/window.c +++ b/dlls/wineandroid.drv/window.c @@ -1200,20 +1200,17 @@ LRESULT ANDROID_DesktopWindowProc( HWND hwnd, UINT msg, WPARAM wp, LPARAM lp ) /*********************************************************************** - * ANDROID_MsgWaitForMultipleObjectsEx + * ANDROID_ProcessEvents */ -NTSTATUS ANDROID_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, - const LARGE_INTEGER *timeout, - DWORD mask, DWORD flags ) +BOOL ANDROID_ProcessEvents( DWORD mask ) { if (GetCurrentThreadId() == desktop_tid) { /* don't process nested events */ if (current_event) mask = 0; - if (process_events( mask )) return count - 1; + return process_events( mask ); } - return NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), - !!(flags & MWMO_ALERTABLE), timeout ); + return FALSE; } /********************************************************************** diff --git a/dlls/winemac.drv/event.c b/dlls/winemac.drv/event.c index e4f76e2b0e1..5b717b4b730 100644 --- a/dlls/winemac.drv/event.c +++ b/dlls/winemac.drv/event.c @@ -510,24 +510,16 @@ static int process_events(macdrv_event_queue queue, macdrv_event_mask mask) /*********************************************************************** - * MsgWaitForMultipleObjectsEx (MACDRV.@) + * ProcessEvents (MACDRV.@) */ -NTSTATUS macdrv_MsgWaitForMultipleObjectsEx(DWORD count, const HANDLE *handles, - const LARGE_INTEGER *timeout, DWORD mask, DWORD flags) +NTSTATUS macdrv_ProcessEvents(DWORD mask) { - DWORD ret; struct macdrv_thread_data *data = macdrv_thread_data(); macdrv_event_mask event_mask = get_event_mask(mask); - TRACE("count %d, handles %p, timeout %p, mask %x, flags %x\n", (unsigned int)count, - handles, timeout, (unsigned int)mask, (unsigned int)flags); + TRACE("mask %x\n", (unsigned int)mask); - if (!data) - { - if (!count && timeout && !timeout->QuadPart) return WAIT_TIMEOUT; - return NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), - !!(flags & MWMO_ALERTABLE), timeout ); - } + if (!data) return FALSE; if (data->current_event && data->current_event->type != QUERY_EVENT && data->current_event->type != QUERY_EVENT_NO_PREEMPT_WAIT && @@ -535,14 +527,5 @@ NTSTATUS macdrv_MsgWaitForMultipleObjectsEx(DWORD count, const HANDLE *handles, data->current_event->type != WINDOW_DRAG_BEGIN) event_mask = 0; /* don't process nested events */ - if (process_events(data->queue, event_mask)) ret = count - 1; - else if (count || !timeout || timeout->QuadPart) - { - ret = NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), - !!(flags & MWMO_ALERTABLE), timeout ); - if (ret == count - 1) process_events(data->queue, event_mask); - } - else ret = WAIT_TIMEOUT; - - return ret; + return process_events(data->queue, event_mask); } diff --git a/dlls/winemac.drv/gdi.c b/dlls/winemac.drv/gdi.c index fd1da722061..d22532fd3b7 100644 --- a/dlls/winemac.drv/gdi.c +++ b/dlls/winemac.drv/gdi.c @@ -282,7 +282,7 @@ static const struct user_driver_funcs macdrv_funcs = .pGetKeyboardLayoutList = macdrv_GetKeyboardLayoutList, .pGetKeyNameText = macdrv_GetKeyNameText, .pMapVirtualKeyEx = macdrv_MapVirtualKeyEx, - .pMsgWaitForMultipleObjectsEx = macdrv_MsgWaitForMultipleObjectsEx, + .pProcessEvents = macdrv_ProcessEvents, .pRegisterHotKey = macdrv_RegisterHotKey, .pSetCapture = macdrv_SetCapture, .pSetCursor = macdrv_SetCursor, diff --git a/dlls/winemac.drv/macdrv.h b/dlls/winemac.drv/macdrv.h index 40f70e55094..281d49c1e9a 100644 --- a/dlls/winemac.drv/macdrv.h +++ b/dlls/winemac.drv/macdrv.h @@ -169,9 +169,7 @@ extern UINT macdrv_GetKeyboardLayoutList(INT size, HKL *list) DECLSPEC_HIDDEN; extern INT macdrv_GetKeyNameText(LONG lparam, LPWSTR buffer, INT size) DECLSPEC_HIDDEN; extern BOOL macdrv_SystemParametersInfo(UINT action, UINT int_param, void *ptr_param, UINT flags) DECLSPEC_HIDDEN; -extern NTSTATUS macdrv_MsgWaitForMultipleObjectsEx(DWORD count, const HANDLE *handles, - const LARGE_INTEGER *timeout, DWORD mask, - DWORD flags) DECLSPEC_HIDDEN; +extern BOOL macdrv_ProcessEvents(DWORD mask) DECLSPEC_HIDDEN; extern void macdrv_ThreadDetach(void) DECLSPEC_HIDDEN; diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 86edf66b820..1ae39eb9edf 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -472,33 +472,16 @@ static BOOL process_events( Display *display, Bool (*filter)(Display*, XEvent*,X /*********************************************************************** - * MsgWaitForMultipleObjectsEx (X11DRV.@) + * ProcessEvents (X11DRV.@) */ -NTSTATUS X11DRV_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, - const LARGE_INTEGER *timeout, DWORD mask, DWORD flags ) +BOOL X11DRV_ProcessEvents( DWORD mask ) { struct x11drv_thread_data *data = x11drv_thread_data(); - NTSTATUS ret; - - if (!data) - { - if (!count && timeout && !timeout->QuadPart) return WAIT_TIMEOUT; - return NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), - !!(flags & MWMO_ALERTABLE), timeout ); - } + if (!data) return FALSE; if (data->current_event) mask = 0; /* don't process nested events */ - if (process_events( data->display, filter_event, mask )) ret = count - 1; - else if (count || !timeout || timeout->QuadPart) - { - ret = NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), - !!(flags & MWMO_ALERTABLE), timeout ); - if (ret == count - 1) process_events( data->display, filter_event, mask ); - } - else ret = WAIT_TIMEOUT; - - return ret; + return process_events( data->display, filter_event, mask ); } /*********************************************************************** diff --git a/dlls/winex11.drv/init.c b/dlls/winex11.drv/init.c index 7e343591413..e26cfb5a789 100644 --- a/dlls/winex11.drv/init.c +++ b/dlls/winex11.drv/init.c @@ -413,7 +413,7 @@ static const struct user_driver_funcs x11drv_funcs = .pDestroyWindow = X11DRV_DestroyWindow, .pFlashWindowEx = X11DRV_FlashWindowEx, .pGetDC = X11DRV_GetDC, - .pMsgWaitForMultipleObjectsEx = X11DRV_MsgWaitForMultipleObjectsEx, + .pProcessEvents = X11DRV_ProcessEvents, .pReleaseDC = X11DRV_ReleaseDC, .pScrollDC = X11DRV_ScrollDC, .pSetCapture = X11DRV_SetCapture, diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index f4f6ba07e07..2b0b79e1665 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -691,9 +691,7 @@ extern void retry_grab_clipping_window(void) DECLSPEC_HIDDEN; extern BOOL clip_fullscreen_window( HWND hwnd, BOOL reset ) DECLSPEC_HIDDEN; extern void move_resize_window( HWND hwnd, int dir ) DECLSPEC_HIDDEN; extern void X11DRV_InitKeyboard( Display *display ) DECLSPEC_HIDDEN; -extern NTSTATUS X11DRV_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, - const LARGE_INTEGER *timeout, - DWORD mask, DWORD flags ) DECLSPEC_HIDDEN; +extern BOOL X11DRV_ProcessEvents( DWORD mask ) DECLSPEC_HIDDEN; extern HWND *build_hwnd_list(void) DECLSPEC_HIDDEN; typedef int (*x11drv_error_callback)( Display *display, XErrorEvent *event, void *arg ); diff --git a/include/wine/gdi_driver.h b/include/wine/gdi_driver.h index 3de7e20af9a..69854b11818 100644 --- a/include/wine/gdi_driver.h +++ b/include/wine/gdi_driver.h @@ -307,7 +307,7 @@ struct user_driver_funcs void (*pDestroyWindow)(HWND); void (*pFlashWindowEx)(FLASHWINFO*); void (*pGetDC)(HDC,HWND,HWND,const RECT *,const RECT *,DWORD); - NTSTATUS (*pMsgWaitForMultipleObjectsEx)(DWORD,const HANDLE*,const LARGE_INTEGER*,DWORD,DWORD); + BOOL (*pProcessEvents)(DWORD); void (*pReleaseDC)(HWND,HDC); BOOL (*pScrollDC)(HDC,INT,INT,HRGN); void (*pSetCapture)(HWND,UINT); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2242
v3: - expose ProcessEvents from drivers and move the other logic to win32u:wait_message(). Note that I changed the condition for performing NtWaitForMultipleObjects. That was done if either count is nonzero or there is a timeout, but NtWaitForMultipleObjects always fails with zero count with STATUS_INVALID_PARAMETER_1, I believe that's not what we want. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25057
Shouldn't wait_message() always have a nonzero count passed to it in v3? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25059
We can't simply replace wait_message() for PeekMessage with driver's ProcessEvents, there are also side effects which are the same currently for all the wait_message() usages. Although it is possible to include more redesign in this patch and do it some other way at once. Do you think it is needed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25061
I like the idea of duplicating the thunk lock / yield more than I like overloading the return value of wait_message(), but that's my personal preference I guess. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25063
Fwiw it seems to me that it is not exactly overloading of the return value. That is rather input ambiguity which, unlike winapi MsgWaitForMultipleObjects, takes explicit queue handle as the last wait object which may also be absent. Then if that is really an issue it would probably be more clear if we'd pass a queue handle to wait_message() instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25065
Fwiw it seems to me that it is not exactly overloading of the return value. That is rather input ambiguity which, unlike winapi MsgWaitForMultipleObjects, takes explicit queue handle as the last wait object which may also be absent.
wait_message(), as it is, has an API convention that matches WaitForMultipleObjects; i.e. it returns the index of whatever object was signaled, which is relatively straightforward. The "zero count" case breaks this convention, either before or after this patch.
Then if that is really an issue it would probably be more clear if we'd pass a queue handle to wait_message() instead.
That'd be another straightforward solution to this problem, but I don't know if that's correct to do in PeekMessage(). I can only assume it isn't for one reason or another. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25083
I think that when using 0 timeout like PeekMessage() does, we don't need to worry about thunk locks, so calling driver directly from there wouldn't need much duplication. Anyway, that's a separate change and this patch looks good enough to me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25128
This merge request was approved by Jacek Caban. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242
participants (4)
-
Jacek Caban (＠jacek) -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)