[PATCH v2 0/4] MR4711: Fix Toad for Oracle F3 find next function not working properly
Fix Toad for Oracle F3 find next function not working properly because its WH_KEYBOARD hook gets called too many times. There is another case that could cause WH_CBT HCBT_CLICKSKIPPED hooks to get called recursively as the tests show. I will send patches for that after the code freeze. -- v2: user32/tests: Add recursive WM_SETCURSOR message tests. win32u: Avoid calling WH_CBT HCBT_CLICKSKIPPED hooks recursively. https://gitlab.winehq.org/wine/wine/-/merge_requests/4711
From: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/user32/tests/msg.c | 167 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 166 insertions(+), 1 deletion(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 0059afcbac7..9f7da2fe5a5 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -12430,6 +12430,29 @@ todo_wine { static HWND hook_hwnd; static HHOOK recursive_hook; static int hook_depth, max_hook_depth; +static BOOL skip_WH_KEYBOARD_hook, skip_WH_MOUSE_hook; + +static void simulate_click(BOOL left, int x, int y) +{ + POINT old_pt; + INPUT input[2]; + UINT events_no; + + GetCursorPos(&old_pt); + SetCursorPos(x, y); + memset(input, 0, sizeof(input)); + input[0].type = INPUT_MOUSE; + input[0].mi.dx = x; + input[0].mi.dy = y; + input[0].mi.dwFlags = left ? MOUSEEVENTF_LEFTDOWN : MOUSEEVENTF_RIGHTDOWN; + input[1].type = INPUT_MOUSE; + input[1].mi.dx = x; + input[1].mi.dy = y; + input[1].mi.dwFlags = left ? MOUSEEVENTF_LEFTUP : MOUSEEVENTF_RIGHTUP; + events_no = SendInput(2, input, sizeof(input[0])); + ok(events_no == 2, "SendInput returned %d\n", events_no); + SetCursorPos(old_pt.x, old_pt.y); +} static LRESULT WINAPI rec_get_message_hook(int code, WPARAM w, LPARAM l) { @@ -12450,12 +12473,85 @@ static LRESULT WINAPI rec_get_message_hook(int code, WPARAM w, LPARAM l) return res; } +static LRESULT CALLBACK keyboard_recursive_hook_proc(int code, WPARAM wp, LPARAM lp) +{ + MSG msg; + + if (code < 0) + return CallNextHookEx(0, code, wp, lp); + + if (skip_WH_KEYBOARD_hook) + return 1; + + hook_depth++; + max_hook_depth = max(max_hook_depth, hook_depth); + PeekMessageW(&msg, NULL, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE); + hook_depth--; + return CallNextHookEx(0, code, wp, lp); +} + +static LRESULT CALLBACK mouse_recursive_hook_proc(int code, WPARAM wp, LPARAM lp) +{ + MSG msg; + + if (code < 0) + return CallNextHookEx(0, code, wp, lp); + + if (skip_WH_MOUSE_hook) + return 1; + + hook_depth++; + max_hook_depth = max(max_hook_depth, hook_depth); + PeekMessageW(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, PM_REMOVE); + hook_depth--; + return CallNextHookEx(0, code, wp, lp); +} + +static LRESULT CALLBACK keyboard_recursive_cbt_hook_proc(int code, WPARAM wp, LPARAM lp) +{ + MSG msg; + + if (code < 0) + return CallNextHookEx(0, code, wp, lp); + + if (code == HCBT_KEYSKIPPED) + { + hook_depth++; + max_hook_depth = max(max_hook_depth, hook_depth); + PeekMessageW(&msg, NULL, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE); + hook_depth--; + } + + return CallNextHookEx(0, code, wp, lp); +} + +static LRESULT CALLBACK mouse_recursive_cbt_hook_proc(int code, WPARAM wp, LPARAM lp) +{ + MSG msg; + + if (code < 0) + return CallNextHookEx(0, code, wp, lp); + + if (code == HCBT_CLICKSKIPPED) + { + hook_depth++; + max_hook_depth = max(max_hook_depth, hook_depth); + PeekMessageW(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, PM_REMOVE); + hook_depth--; + } + + return CallNextHookEx(0, code, wp, lp); +} + static void test_recursive_hook(void) { + HHOOK hook, cbt_hook; + INPUT input = {0}; MSG msg; BOOL b; - hook_hwnd = CreateWindowA("Static", NULL, WS_POPUP, 0, 0, 200, 60, NULL, NULL, NULL, NULL); + hook_hwnd = CreateWindowExA(WS_EX_TOPMOST, "Static", NULL, WS_POPUP | WS_VISIBLE, 0, 0, 200, 60, + NULL, NULL, NULL, NULL); ok(hook_hwnd != NULL, "CreateWindow failed\n"); recursive_hook = SetWindowsHookExW(WH_GETMESSAGE, rec_get_message_hook, NULL, GetCurrentThreadId()); @@ -12472,6 +12568,75 @@ static void test_recursive_hook(void) b = UnhookWindowsHookEx(recursive_hook); ok(b, "UnhokWindowsHookEx failed\n"); + /* Test possible recursive hook conditions */ + b = SetForegroundWindow(hook_hwnd); + ok(b, "SetForegroundWindow failed, error %ld.\n", GetLastError()); + + /* Test a possible recursive WH_KEYBOARD hook condition */ + max_hook_depth = 0; + hook = SetWindowsHookExA(WH_KEYBOARD, keyboard_recursive_hook_proc, NULL, GetCurrentThreadId()); + ok(!!hook, "SetWindowsHookExA failed, error %ld.\n", GetLastError()); + + flush_events(); + input.type = INPUT_KEYBOARD; + input.ki.wVk = VK_F3; + SendInput(1, &input, sizeof(INPUT)); + flush_events(); + + /* Expect the WH_KEYBOARD hook not gets called recursively */ + todo_wine + ok(max_hook_depth == 1, "Got expected %d.\n", max_hook_depth); + + /* Test a possible recursive WH_CBT HCBT_KEYSKIPPED hook condition */ + max_hook_depth = 0; + skip_WH_KEYBOARD_hook = 1; + cbt_hook = SetWindowsHookExA(WH_CBT, keyboard_recursive_cbt_hook_proc, NULL, GetCurrentThreadId()); + ok(!!cbt_hook, "SetWindowsHookExA failed, error %ld.\n", GetLastError()); + + flush_events(); + input.type = INPUT_KEYBOARD; + input.ki.wVk = VK_F3; + SendInput(1, &input, sizeof(INPUT)); + while (PeekMessageA(&msg, hook_hwnd, WM_KEYFIRST, WM_KEYLAST, 0)) DispatchMessageA(&msg); + + /* Expect the WH_CBT HCBT_KEYSKIPPED hook not gets called recursively */ + todo_wine + ok(max_hook_depth == 1, "Got expected %d.\n", max_hook_depth); + + UnhookWindowsHookEx(cbt_hook); + UnhookWindowsHookEx(hook); + + /* Test a recursive WH_MOUSE hook condition */ + SetCapture(hook_hwnd); + + max_hook_depth = 0; + hook = SetWindowsHookExA(WH_MOUSE, mouse_recursive_hook_proc, NULL, GetCurrentThreadId()); + ok(!!hook, "SetWindowsHookExA failed, error %ld.\n", GetLastError()); + + flush_events(); + simulate_click(FALSE, 50, 50); + flush_events(); + + /* Expect the WH_MOUSE hook gets called recursively */ + ok(max_hook_depth > 10, "Got expected %d.\n", max_hook_depth); + + /* Test a possible recursive WH_CBT HCBT_CLICKSKIPPED hook condition */ + max_hook_depth = 0; + skip_WH_MOUSE_hook = 1; + cbt_hook = SetWindowsHookExA(WH_CBT, mouse_recursive_cbt_hook_proc, NULL, GetCurrentThreadId()); + ok(!!cbt_hook, "SetWindowsHookExA failed, error %ld.\n", GetLastError()); + + flush_events(); + simulate_click(FALSE, 50, 50); + flush_events(); + + /* Expect the WH_CBT HCBT_CLICKSKIPPED hook not gets called recursively */ + todo_wine + ok(max_hook_depth <= 10, "Got expected %d.\n", max_hook_depth); + + UnhookWindowsHookEx(cbt_hook); + UnhookWindowsHookEx(hook); + ReleaseCapture(); DestroyWindow(hook_hwnd); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4711
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Previously, accept_hardware_message() is called after WH_KEYBOARD and WH_CBT HCBT_KEYSKIPPED hooks. So when PeekMessage() gets called in WH_KEYBOARD and WH_CBT HCBT_KEYSKIPPED hooks, the hooks will be called recursively because the message is still in the server message queue. Fix Toad for Oracle F3 find next function not working properly because its WH_KEYBOARD hook gets called too many times. --- dlls/user32/tests/msg.c | 2 -- dlls/win32u/message.c | 6 ++++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 9f7da2fe5a5..98b1dcc6f99 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -12584,7 +12584,6 @@ static void test_recursive_hook(void) flush_events(); /* Expect the WH_KEYBOARD hook not gets called recursively */ - todo_wine ok(max_hook_depth == 1, "Got expected %d.\n", max_hook_depth); /* Test a possible recursive WH_CBT HCBT_KEYSKIPPED hook condition */ @@ -12600,7 +12599,6 @@ static void test_recursive_hook(void) while (PeekMessageA(&msg, hook_hwnd, WM_KEYFIRST, WM_KEYLAST, 0)) DispatchMessageA(&msg); /* Expect the WH_CBT HCBT_KEYSKIPPED hook not gets called recursively */ - todo_wine ok(max_hook_depth == 1, "Got expected %d.\n", max_hook_depth); UnhookWindowsHookEx(cbt_hook); diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index 91f836a60bd..376af30554e 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -2384,15 +2384,17 @@ static BOOL process_keyboard_message( MSG *msg, UINT hw_id, HWND hwnd_filter, } } + /* if remove is TRUE, remove message first before calling hooks to avoid recursive hook calls */ + if (remove) accept_hardware_message( hw_id ); if (call_hooks( WH_KEYBOARD, remove ? HC_ACTION : HC_NOREMOVE, LOWORD(msg->wParam), msg->lParam, 0 )) { + /* if the message has not been removed, remove it */ + if (!remove) accept_hardware_message( hw_id ); /* skip this message */ call_hooks( WH_CBT, HCBT_KEYSKIPPED, LOWORD(msg->wParam), msg->lParam, 0 ); - accept_hardware_message( hw_id ); return FALSE; } - if (remove) accept_hardware_message( hw_id ); msg->pt = point_phys_to_win_dpi( msg->hwnd, msg->pt ); if (remove && msg->message == WM_KEYDOWN) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4711
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Previously, accept_hardware_message() is called after WH_CBT HCBT_CLICKSKIPPED hooks. So when these hooks call PeekMessage(), they will be called recursively. Note that WH_MOUSE hooks do get called recursively according to tests. --- dlls/user32/tests/msg.c | 1 - dlls/win32u/message.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 98b1dcc6f99..ec8311e0568 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -12629,7 +12629,6 @@ static void test_recursive_hook(void) flush_events(); /* Expect the WH_CBT HCBT_CLICKSKIPPED hook not gets called recursively */ - todo_wine ok(max_hook_depth <= 10, "Got expected %d.\n", max_hook_depth); UnhookWindowsHookEx(cbt_hook); diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index 376af30554e..c567d697a41 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -2570,8 +2570,8 @@ static BOOL process_mouse_message( MSG *msg, UINT hw_id, ULONG_PTR extra_info, H hook.wHitTestCode = hittest; hook.dwExtraInfo = extra_info; hook.mouseData = msg->wParam; - call_hooks( WH_CBT, HCBT_CLICKSKIPPED, message, (LPARAM)&hook, sizeof(hook) ); accept_hardware_message( hw_id ); + call_hooks( WH_CBT, HCBT_CLICKSKIPPED, message, (LPARAM)&hook, sizeof(hook) ); return FALSE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4711
From: Zhiyi Zhang <zzhang(a)codeweavers.com> This test show that WM_SETCURSOR gets handled recursively, so in process_mouse_message(), accept_hardware_message() can be called after sending WM_SETCURSOR and no changes are needed there. --- dlls/user32/tests/msg.c | 47 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index ec8311e0568..d2c7fb678ce 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -12637,6 +12637,52 @@ static void test_recursive_hook(void) DestroyWindow(hook_hwnd); } +static int max_msg_depth; + +static LRESULT WINAPI recursive_messages_proc(HWND hwnd, UINT message, WPARAM wp, LPARAM lp) +{ + static int msg_depth; + MSG msg; + + if (message == WM_SETCURSOR && max_msg_depth < 25) + { + msg_depth++; + max_msg_depth = max(max_msg_depth, msg_depth); + PeekMessageW(&msg, NULL, 0, 0, PM_REMOVE); + msg_depth--; + } + return DefWindowProcA(hwnd, message, wp, lp); +} + +static void test_recursive_messages(void) +{ + WNDCLASSA cls = {0}; + HWND hwnd; + + cls.lpfnWndProc = recursive_messages_proc; + cls.hInstance = GetModuleHandleA(0); + cls.hCursor = LoadCursorA(0, (LPCSTR)IDC_ARROW); + cls.hbrBackground = GetStockObject(WHITE_BRUSH); + cls.lpszClassName = "TestRecursiveMsgClass"; + register_class(&cls); + + hwnd = CreateWindowExA(WS_EX_TOPMOST, "TestRecursiveMsgClass", NULL, WS_POPUP | WS_DISABLED | WS_VISIBLE, 0, 0, + 100, 100, NULL, NULL, NULL, NULL); + ok(hwnd != NULL, "CreateWindowExA failed, error %ld.\n", GetLastError()); + SetForegroundWindow(hwnd); + flush_events(); + + max_msg_depth = 0; + simulate_click(FALSE, 50, 50); + flush_events(); + + /* Expect recursive_messages_proc() gets called recursively for WM_SETCURSOR */ + ok(max_msg_depth == 25, "Got expected %d.\n", max_msg_depth); + + DestroyWindow(hwnd); + UnregisterClassA(cls.lpszClassName, cls.hInstance); +} + static const struct message ScrollWindowPaint1[] = { { WM_PAINT, sent }, { WM_ERASEBKGND, sent|beginpaint }, @@ -20465,6 +20511,7 @@ START_TEST(msg) test_set_hook(); test_recursive_hook(); } + test_recursive_messages(); test_DestroyWindow(); test_DispatchMessage(); test_SendMessageTimeout(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4711
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=141995 Your paranoid android. === w7u_2qxl (32 bit report) === user32: msg.c:12680: Test failed: Got expected 24. === w7u_adm (32 bit report) === user32: msg.c:12680: Test failed: Got expected 24. === w7u_el (32 bit report) === user32: msg.c:12680: Test failed: Got expected 24. === w7pro64 (64 bit report) === user32: msg.c:12680: Test failed: Got expected 16. === debian11 (32 bit de report) === user32: msg.c:12587: Test failed: Got expected 0. msg.c:12602: Test failed: Got expected 0.
v2: Add more related patches now that the code freeze is over. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4711#note_57654
participants (3)
-
Marvin -
Zhiyi Zhang -
Zhiyi Zhang (＠zhiyi)