[PATCH v2 0/3] 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(). -- v2: wineandroid.drv: Check for zero count in _MsgWaitForMultipleObjectsEx if events are present. winemac.drv: Check for zero count in _MsgWaitForMultipleObjectsEx if events are present. winex11.drv: Check for zero count in _MsgWaitForMultipleObjectsEx if events are present. https://gitlab.winehq.org/wine/wine/-/merge_requests/2242
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winex11.drv/event.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 86edf66b820..9f6553ff228 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -489,7 +489,7 @@ NTSTATUS X11DRV_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, if (data->current_event) mask = 0; /* don't process nested events */ - if (process_events( data->display, filter_event, mask )) ret = count - 1; + if (process_events( data->display, filter_event, mask )) ret = count ? count - 1 : 0; else if (count || !timeout || timeout->QuadPart) { ret = NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2242
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winemac.drv/event.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/winemac.drv/event.c b/dlls/winemac.drv/event.c index e4f76e2b0e1..b4f9cdd6de3 100644 --- a/dlls/winemac.drv/event.c +++ b/dlls/winemac.drv/event.c @@ -535,7 +535,7 @@ 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; + if (process_events(data->queue, event_mask)) ret = count ? count - 1 : 0; else if (count || !timeout || timeout->QuadPart) { ret = NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2242
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/wineandroid.drv/window.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/wineandroid.drv/window.c b/dlls/wineandroid.drv/window.c index a339c20ceda..bbb42c7865a 100644 --- a/dlls/wineandroid.drv/window.c +++ b/dlls/wineandroid.drv/window.c @@ -1210,7 +1210,7 @@ NTSTATUS ANDROID_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles { /* don't process nested events */ if (current_event) mask = 0; - if (process_events( mask )) return count - 1; + if (process_events( mask )) return count ? count - 1 : 0; } return NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), !!(flags & MWMO_ALERTABLE), timeout ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2242
v2: - check for 0 count in drivers instead of handling that in win32u:wait_message(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_24930
That's probably better, yeah. I'm still not a fan, because it makes the API meaning when count is 0 completely different from the API meaning when count is nonzero. If it were up to me, I'd rather make something closer to process_events() into the user driver API, and that'd allow us to factor out more common code into win32u as well. But I don't really want to argue about it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_24933
On Tue Feb 21 16:50:47 2023 +0000, Zebediah Figura wrote:
That's probably better, yeah. I'm still not a fan, because it makes the API meaning when count is 0 completely different from the API meaning when count is nonzero. If it were up to me, I'd rather make something closer to process_events() into the user driver API, and that'd allow us to factor out more common code into win32u as well. But I don't really want to argue about it. I like that idea. `MsgWaitForMultipleObjectsEx` gives drivers a bit more flexibility, but they don't use it anyway. It could be as simple as `BOOL (*pProcessEvents)(DWORD mask);`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25040
On Tue Feb 21 16:50:46 2023 +0000, Jacek Caban wrote:
I like that idea. `MsgWaitForMultipleObjectsEx` gives drivers a bit more flexibility, but they don't use it anyway. It could be as simple as `BOOL (*pProcessEvents)(DWORD mask);`. Do I understand correctly that you mean replacing wait_message() only for PeekMessage() usage and existing driver's pMsgWaitForMultipleObjectsEx stays the same?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25041
On Tue Feb 21 17:03:16 2023 +0000, Paul Gofman wrote:
Do I understand correctly that you mean replacing wait_message() only for PeekMessage() usage and existing driver's pMsgWaitForMultipleObjectsEx stays the same? I guess not and the idea is to move all the other logic to win32u, but current logic in wineandroid.drv is a bit different (there is an extra process_message after wait in winex11 / winemac). Do you know if that is on purpose or we can just use the same logic? Unfrotunately I don't have a setup to do any tests for wineandroid.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25043
On Tue Feb 21 17:09:43 2023 +0000, Paul Gofman wrote:
I guess not and the idea is to move all the other logic to win32u, but current logic in wineandroid.drv is a bit different (there is an extra process_message after wait in winex11 / winemac). Do you know if that is on purpose or we can just use the same logic? Unfrotunately I don't have a setup to do any tests for wineandroid. Yes, the idea is to move more `MsgWaitForMultipleObjectsEx` logic to win32u. I can't test it, but I think that extra event processing should be fine for wineandroid as well.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25044
On Tue Feb 21 17:15:08 2023 +0000, Jacek Caban wrote:
Yes, the idea is to move more `MsgWaitForMultipleObjectsEx` logic to win32u. I can't test it, but I think that extra event processing should be fine for wineandroid as well. Yeah, ok, I will redo it this way. Besides MsgWaitForMultipleObjectsEx implementation, all the other use cases of driver's pMsgWaitForMultipleObjectsEx are essentially ProcessEvents anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2242#note_25045
participants (4)
-
Jacek Caban (＠jacek) -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)