[PATCH 0/1] MR2153: win32u: invalidate all cached keys after input

[Henry Goffin (＠AMZN-hgoffin)]

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.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2153

[PATCH 1/1] win32u: invalidate all cached keys after input

[Henry Goffin]

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 2c320377e34..daf645920b2 100644
--- a/dlls/win32u/message.c
+++ b/dlls/win32u/message.c
@@ -2582,7 +2582,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;
@@ -2671,6 +2671,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

[Henry Goffin (＠AMZN-hgoffin)]

I would love to kill the key cache entirely - but it's not clear how much of a performance regression it would actually be to do so, or what real applications the cache was meant to improve performance on when it was added 12+ years ago. Maintaining the cache certainly has created a lot of code complexity since then but this is the minimum change I need to fix the app I'm looking at.

Without this change, key state becomes inconsistent between windowproc threads and pooled worker threads, because key-down events only update the cache in a single receiving thread without invalidating any other thread caches.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_23622