[PATCH v2 0/1] MR7039: server: Destroy global hook table in desktop_close_handle().
Commit f784374bcb5195c918754b1da6142a618a0da7dd introduced a cyclic reference between desktop and global hook through global hook table. So now if a hook was set and then not explicitly unset and the process was just terminated the hook, desktop, thread and some other related objects are leaked (which is displayed by wineserver on prefix shutdown in this case). -- v2: server: Cleanup all the global hooks owned by terminating thread. https://gitlab.winehq.org/wine/wine/-/merge_requests/7039
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/user32/tests/msg.c | 74 +++++++++++++++++++++++++++++++++++++++++ server/hook.c | 5 ++- 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 1b9c9eb3cba..cc28eceb0fb 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -20645,6 +20645,79 @@ static void test_radiobutton_focus(void) DestroyWindow(hwnd); } +static LONG test_hook_cleanup_hook_proc_child_id; +static DWORD test_hook_cleanup_hook_proc_thread_id, test_hook_cleanup_hook_proc_call_thread_id; + +static void CALLBACK test_hook_cleanup_hook_proc( HWINEVENTHOOK hook, DWORD event_id, HWND hwnd, LONG obj_id, + LONG child_id, DWORD thread_id, DWORD event_time ) +{ + test_hook_cleanup_hook_proc_child_id = child_id; + test_hook_cleanup_hook_proc_call_thread_id = GetCurrentThreadId(); + test_hook_cleanup_hook_proc_thread_id = thread_id; +} + +struct test_hook_cleanup_data +{ + HWND hwnd; + HANDLE hook_installed_event; + HANDLE done_event; + DWORD main_thread_id; +}; + +static DWORD WINAPI test_hook_cleanup_thread_proc( void *context ) +{ + struct test_hook_cleanup_data *d = context; + HWINEVENTHOOK hook; + + hook = SetWinEventHook( EVENT_MIN, EVENT_MAX, GetModuleHandleW( NULL ), test_hook_cleanup_hook_proc, + GetCurrentProcessId(), 0, WINEVENT_INCONTEXT ); + ok( !!hook, "got error %ld.\n", GetLastError() ); + + test_hook_cleanup_hook_proc_child_id = -1; + NotifyWinEvent( EVENT_MIN, d->hwnd, 1, 1 ); + ok( test_hook_cleanup_hook_proc_child_id == 1, "got %ld.\n", test_hook_cleanup_hook_proc_child_id ); + todo_wine ok( test_hook_cleanup_hook_proc_thread_id == d->main_thread_id, "got %#lx.\n", + test_hook_cleanup_hook_proc_thread_id ); + ok( test_hook_cleanup_hook_proc_call_thread_id == GetCurrentThreadId(), "got %#lx.\n", + test_hook_cleanup_hook_proc_call_thread_id ); + + SetEvent( d->hook_installed_event ); + WaitForSingleObject( d->done_event, INFINITE ); + return 0; +} + +static void test_hook_cleanup(void) +{ + struct test_hook_cleanup_data d; + HANDLE thread; + + d.main_thread_id = GetCurrentThreadId(); + d.hwnd = CreateWindowA ("static", "window", WS_OVERLAPPEDWINDOW, 0, 0, 100, 100, 0, 0, 0, 0 ); + d.hook_installed_event = CreateEventW( NULL, FALSE, FALSE, NULL ); + d.done_event = CreateEventW( NULL, FALSE, FALSE, NULL ); + + thread = CreateThread( NULL, 0, test_hook_cleanup_thread_proc, &d, 0, NULL ); + WaitForSingleObject( d.hook_installed_event, INFINITE ); + + test_hook_cleanup_hook_proc_child_id = -1; + NotifyWinEvent( EVENT_MIN, d.hwnd, 1, 2 ); + ok( test_hook_cleanup_hook_proc_child_id == 2, "got %ld.\n", test_hook_cleanup_hook_proc_child_id ); + ok( test_hook_cleanup_hook_proc_thread_id == GetCurrentThreadId(), "got %#lx.\n", + test_hook_cleanup_hook_proc_thread_id ); + ok( test_hook_cleanup_hook_proc_call_thread_id == GetCurrentThreadId(), "got %#lx.\n", + test_hook_cleanup_hook_proc_call_thread_id ); + + SetEvent( d.done_event ); + WaitForSingleObject( thread, INFINITE ); + + /* Hook is removed when thread which created it is terminated. */ + test_hook_cleanup_hook_proc_child_id = -1; + NotifyWinEvent( EVENT_MIN, d.hwnd, 1, 3 ); + ok( test_hook_cleanup_hook_proc_child_id == -1, "got %ld.\n", test_hook_cleanup_hook_proc_child_id ); + + DestroyWindow( d.hwnd ); +} + START_TEST(msg) { char **test_argv; @@ -20768,6 +20841,7 @@ START_TEST(msg) test_DoubleSetCapture(); test_create_name(); test_hook_changing_window_proc(); + test_hook_cleanup(); /* keep it the last test, under Windows it tends to break the tests * which rely on active/foreground windows being correct. */ diff --git a/server/hook.c b/server/hook.c index c2d2823cd61..ffe7206369e 100644 --- a/server/hook.c +++ b/server/hook.c @@ -389,14 +389,13 @@ void remove_thread_hooks( struct thread *thread ) if (!global_hooks) return; - /* only low-level keyboard/mouse global hooks can be owned by a thread */ - for (index = WH_KEYBOARD_LL - WH_MINHOOK; index <= WH_MOUSE_LL - WH_MINHOOK; index++) + for (index = 0; index < NB_HOOKS; index++) { struct hook *hook = get_first_hook( global_hooks, index ); while (hook) { struct hook *next = HOOK_ENTRY( list_next( &global_hooks->hooks[index], &hook->chain ) ); - if (hook->thread == thread) remove_hook( hook ); + if (hook->thread == thread || hook->owner == thread) remove_hook( hook ); hook = next; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7039
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=150435 Your paranoid android. === w7pro64 (64 bit report) === user32: msg.c:9026: Test failed: SetWindowPos:FrameChanged_clip: 2: the msg 0x0085 was expected in parent msg.c:9026: Test failed: SetWindowPos:FrameChanged_clip: 4: the msg 0x0014 was expected in parent msg.c:9026: Test failed: SetWindowPos:FrameChanged_clip: 5: the msg 0x0085 was expected, but got msg 0x0047 instead msg.c:9026: Test failed: SetWindowPos:FrameChanged_clip: 6: the msg 0x0014 was expected, but got msg 0x0047 instead msg.c:9036: Test failed: SetWindowPos:FrameChangedDeferErase: 4: the msg 0x000f was expected in parent msg.c:9036: Test failed: SetWindowPos:FrameChangedDeferErase: 5: the msg 0x0085 was expected in parent msg.c:9036: Test failed: SetWindowPos:FrameChangedDeferErase: 6: the msg sequence is not complete: expected msg 000f - actual msg 0000 msg.c:9064: Test failed: SetWindowPos:FrameChangedDeferErase: 4: the msg 0x000f was expected in parent msg.c:9064: Test failed: SetWindowPos:FrameChangedDeferErase: 5: the msg 0x0085 was expected in parent msg.c:9064: Test failed: SetWindowPos:FrameChangedDeferErase: 6: the msg sequence is not complete: expected msg 000f - actual msg 0000
@rbernon remove_thread_hooks() only removes low level keyboard or mouse hooks, the comment in remove_thread_hooks() says: ``` /* only low-level keyboard/mouse global hooks can be owned by a thread */ ``` My testing around shows that win event hooks, even with WINEVENT_INCONTEXT, are getting removed when the thread which created it exits. The included test shows that. So it looks right to do that (instead of fixing desktop destruction). That is the case even when the hook is not bound to the current process but I made it bound in included test as doing otherwise frequently crashes the explorer here (before the hook is removed on thread exit; such hooking needs hook function in a dll). The test also revealed that thread id delivered to win event hook function is wrong, it should be window thread (probably reflecting that those events are sent in some different way), but this is unrelated and probably not regressive. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7039#note_90816
Thinking of it, this behaviour seems to make a lot of sense: otherwise if anything had installed a global hook (like, e. g., uiautomation does in uia_event_thread_proc) and then the process which requested that dies, that global hook would be staying for every process until reboot; cleaning those up on hook creators process or thread exit (the test shows that it is the latter) avoids that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7039#note_90817
On the separate note, closing hook's handle in another thread returns an error on WIndows (unlike Wine), but this bit also looks unrelated. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7039#note_90818
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7039
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon