[PATCH v2 0/1] MR2153: win32u: invalidate all cached keys after input
This patch addresses an issue in Second Life and potentially other multi-threaded applications which process WM_KEYDOWN in one thread and then verify that the key is "still down" with GetAsyncKeyState from another thread. Wine uses a per-thread key cache, resulting in inconsistent views of key status. Caches are now invalidated when an input event is injected by the driver or via SendInput. -- v2: win32u: invalidate all cached keys after input https://gitlab.winehq.org/wine/wine/-/merge_requests/2153
From: Henry Goffin <hgoffin(a)amazon.com> This patch addresses an issue in Second Life and potentially other multi-threaded applications which process WM_KEYDOWN in one thread and then verify that the key is "still down" with GetAsyncKeyState from another thread. Wine uses a per-thread key cache, resulting in inconsistent views of key status. Caches are now invalidated when an input event is injected by the driver or via SendInput. --- dlls/win32u/message.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index de1ca8cefbc..c7617980b7b 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -2588,7 +2588,7 @@ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *r struct user_key_state_info *key_state_info = get_user_thread_info()->key_state; struct send_message_info info; int prev_x, prev_y, new_x, new_y; - INT counter = global_key_state_counter; + ULONG counter = global_key_state_counter; USAGE hid_usage_page, hid_usage; NTSTATUS ret; BOOL wait; @@ -2677,6 +2677,17 @@ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *r if (!ret) { +#define BTN_MASK (MOUSEEVENTF_LEFTDOWN|MOUSEEVENTF_LEFTUP|MOUSEEVENTF_RIGHTDOWN|MOUSEEVENTF_RIGHTUP| \ + MOUSEEVENTF_MIDDLEDOWN|MOUSEEVENTF_MIDDLEUP|MOUSEEVENTF_XDOWN|MOUSEEVENTF_XUP) + if (input->type == INPUT_KEYBOARD || + (input->type == INPUT_MOUSE && 0 != (input->mi.dwFlags & BTN_MASK))) + { + /* always invalidate cross-thread key caches after sending key-related input; + cache data from reply is only fresh if no other threads also incremented */ + if ((ULONG)InterlockedIncrement(&global_key_state_counter) == counter+1) + ++counter; + } +#undef BTN_MASK if (key_state_info) { key_state_info->time = NtGetTickCount(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2153
I don't know what the process is for finding a reviewer :), but this is an important correctness change in upstream Wine to preserve the intent of a GetAsyncKeyState fix originally authored for Photoshop CS5. The keydown cache is important for performance in mainstream Wine (it was deleted in Proton due to faster server IPC access), but the cache needs to be invalidated on edge detection whenever keydown status changes. The original fix from 10 years ago was to add an edge trigger that forces keydown status to be refreshed across all threads whenever one thread detects a key change. This change covers an additional edge which was missed at the time - input message handling can update a single thread's key status without running edge detection logic. It manifests in some open-source game clients, such as Second Life, as an occasional inability to turn properly while running forward (due to inconsistent keydown state across threads). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_25681
Rémi Bernon (@rbernon) commented about dlls/win32u/message.c:
+#define BTN_MASK (MOUSEEVENTF_LEFTDOWN|MOUSEEVENTF_LEFTUP|MOUSEEVENTF_RIGHTDOWN|MOUSEEVENTF_RIGHTUP| \ + MOUSEEVENTF_MIDDLEDOWN|MOUSEEVENTF_MIDDLEUP|MOUSEEVENTF_XDOWN|MOUSEEVENTF_XUP) + if (input->type == INPUT_KEYBOARD || + (input->type == INPUT_MOUSE && 0 != (input->mi.dwFlags & BTN_MASK))) + { + /* always invalidate cross-thread key caches after sending key-related input; + cache data from reply is only fresh if no other threads also incremented */ + if ((ULONG)InterlockedIncrement(&global_key_state_counter) == counter+1) + ++counter; + } +#undef BTN_MASK if (key_state_info) { key_state_info->time = NtGetTickCount(); key_state_info->counter = counter; } IIUC the problem is coming from one thread having a more up to date key state cache than the other because it received the X input and called `send_hardware_message`? And I understand this is caused by this part here, so what about instead:
1) Remove this optimization (as well as the key state buffer returned from `send_hardware_message` request), 2) Always invalidate the keystate for all threads by incrementing `global_key_state_counter` unconditionally in `case INPUT_KEYBOARD:` and `if (input->mi.dwFlags & ~(MOUSEEVENTF_ABSOLUTE | MOUSEEVENTF_MOVE))` in `case INPUT_MOUSE:`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_25787
On Tue Feb 28 19:43:54 2023 +0000, Rémi Bernon wrote:
IIUC the problem is coming from one thread having a more up to date key state cache than the other because it received the X input and called `send_hardware_message`? And I understand this is caused by this part here, so what about instead: 1) Remove this optimization (as well as the key state buffer returned from `send_hardware_message` request), 2) Always invalidate the keystate for all threads by incrementing `global_key_state_counter` unconditionally in `case INPUT_KEYBOARD:` and `if (input->mi.dwFlags & ~(MOUSEEVENTF_ABSOLUTE | MOUSEEVENTF_MOVE))` in `case INPUT_MOUSE:`? Or whatever the mouse button mask should be. I think a `static const` rather than a define would be better.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_25788
On Tue Feb 28 19:48:14 2023 +0000, Rémi Bernon wrote:
Or whatever the mouse button mask should be. I think a `static const` rather than a define would be better, or maybe just putting the mask as is. That should work as well, yes. It creates one more wineserver round-trip for a simple single-threaded app, but it avoids having to maintain two different update paths for the keystate cache and simplifies the logic. I'll do that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_25793
participants (3)
-
Henry Goffin -
Henry Goffin (＠AMZN-hgoffin)
-
Rémi Bernon