[PATCH 0/1] MR6580: win32u: Correctly fill new foreground window TID in WM_ACTIVATEAPP.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/user32/tests/win.c | 87 ++++++++++++++++++++++++++++++++++++++++- dlls/win32u/input.c | 7 +++- server/protocol.def | 1 + server/queue.c | 13 +++++- server/user.h | 1 + server/winstation.c | 1 + 6 files changed, 106 insertions(+), 4 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 775164e3e9f..8a51ca615aa 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -69,6 +69,7 @@ static HWND hwndMessage; static HWND hwndMain, hwndMain2; static HHOOK hhook; static BOOL app_activated, app_deactivated; +static LPARAM activate_app_lparam; static const char* szAWRClass = "Winsize"; static HMENU hmenu; @@ -1076,6 +1077,7 @@ static LRESULT WINAPI main_window_procA(HWND hwnd, UINT msg, WPARAM wparam, LPAR case WM_ACTIVATEAPP: if (wparam) app_activated = TRUE; else app_deactivated = TRUE; + activate_app_lparam = lparam; break; case WM_MOUSEACTIVATE: return MA_ACTIVATE; @@ -10896,6 +10898,7 @@ static void test_GetMessagePos(void) #define SET_FOREGROUND_STEAL_2 0x04 #define SET_FOREGROUND_SET_2 0x08 #define SET_FOREGROUND_INJECT 0x10 +#define SET_FOREGROUND_DESKTOP 0x20 struct set_foreground_thread_params { @@ -10944,6 +10947,8 @@ static DWORD WINAPI set_foreground_thread(void *params) SetForegroundWindow(p->window2); check_wnd_state(0, p->window2, 0, 0); } + if (msg.wParam & SET_FOREGROUND_DESKTOP) + SetForegroundWindow(GetDesktopWindow()); SetEvent(p->command_executed); continue; @@ -10991,6 +10996,7 @@ static void test_activateapp(HWND window1) * check_wnd_state(window1, thread_params.thread_window, window1, 0); */ ok(!app_activated, "Received WM_ACTIVATEAPP(1), did not expect it.\n"); ok(!app_deactivated, "Received WM_ACTIVATEAPP(0), did not expect it.\n"); + activate_app_lparam = 0; while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); check_wnd_state(0, thread_params.thread_window, 0, 0); test_window = GetForegroundWindow(); @@ -11000,6 +11006,8 @@ static void test_activateapp(HWND window1) /* This message is reliable on Windows and inside a virtual desktop. * It is unreliable on KDE (50/50) and never arrives on FVWM. * ok(app_deactivated, "Expected WM_ACTIVATEAPP(0), did not receive it.\n"); */ + if (app_deactivated) + ok(activate_app_lparam == tid, "got thread id %Iu, expected %lu.\n", activate_app_lparam, tid); /* Set foreground: WM_ACTIVATEAPP (1) is delivered. */ app_activated = app_deactivated = FALSE; @@ -11102,11 +11110,88 @@ static void test_activateapp(HWND window1) ok(!app_activated, "Received WM_ACTIVATEAPP(1), did not expect it.\n"); ok(!app_deactivated, "Received WM_ACTIVATEAPP(0), did not expect it.\n"); + DestroyWindow(window2); + + if (winetest_interactive) + { + /* As soon as window looses focus to other process window there are no good and sure ways to make the process + * foreground again through WINAPI (thus blocking any further tests, so once setting, e. g., desktop window as + * foregroung the app window should be set to foreground manually. */ + ShowWindow(thread_params.thread_window, SW_HIDE); + SetActiveWindow(window1); + SetForegroundWindow(window1); + test_window = GetForegroundWindow(); + ok(test_window == window1, "Expected foreground window %p, got %p\n", + window1, test_window); + app_activated = app_deactivated = FALSE; + activate_app_lparam = 0; + trace("Now switch to any other window manually either way, like alt+tab, clicking other window, Win key + D.\n"); + while (GetForegroundWindow() == window1) + { + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + Sleep(10); + } + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + ok(!app_activated, "Received WM_ACTIVATEAPP(1), did not expect it.\n"); + ok(app_deactivated, "Expected WM_ACTIVATEAPP(0), did not receive it.\n"); + ok(activate_app_lparam && activate_app_lparam != GetCurrentThreadId(), + "got %Iu, tid %lu.\n", activate_app_lparam, tid); + + /* Switch to desktop window from the same thread. */ + trace("Now switch to test window manually.\n"); + while (GetForegroundWindow() != window1) + { + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + Sleep(10); + } + Sleep(30); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + check_wnd_state(window1, window1, window1, 0); + app_activated = app_deactivated = FALSE; + activate_app_lparam = 0; + SetForegroundWindow(GetDesktopWindow()); + test_window = GetForegroundWindow(); + ok(test_window != window1, "Got the same foregorund window.\n"); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + ok(!app_activated, "Received WM_ACTIVATEAPP(1), did not expect it.\n"); + ok(app_deactivated, "Expected WM_ACTIVATEAPP(0), did not receive it.\n"); + ok(activate_app_lparam && activate_app_lparam != GetCurrentThreadId() && activate_app_lparam != tid, + "got %Iu, tid %lu.\n", activate_app_lparam, tid); + + /* Switch to desktop window from the other thread. */ + trace("Now switch to test window manually, again.\n"); + while (GetForegroundWindow() != window1) + { + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + Sleep(10); + } + Sleep(30); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + check_wnd_state(window1, window1, window1, 0); + app_activated = app_deactivated = FALSE; + activate_app_lparam = 0; + PostThreadMessageA(tid, thread_params.msg_command, SET_FOREGROUND_DESKTOP, 0); + WaitForSingleObject(thread_params.command_executed, INFINITE); + test_window = GetForegroundWindow(); + ok(test_window != window2, "Got the same foregorund window.\n"); + Sleep(30); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + ok(!app_activated, "Received WM_ACTIVATEAPP(1), did not expect it.\n"); + ok(app_deactivated, "Expected WM_ACTIVATEAPP(0), did not receive it.\n"); + ok(activate_app_lparam && activate_app_lparam != tid, "got %Iu, tid %lu.\n", activate_app_lparam, tid); + trace("Switch to test window manually, for the last time for now.\n"); + while (GetForegroundWindow() != window1) + { + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + Sleep(10); + } + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); + } + PostThreadMessageA(tid, thread_params.msg_quit, 0, 0); WaitForSingleObject(thread, INFINITE); CloseHandle(thread_params.command_executed); - DestroyWindow(window2); } static LRESULT WINAPI winproc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index 14862e9a8a4..7bd3985edc6 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -1905,7 +1905,7 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) { HWND previous = get_active_window(); BOOL ret; - DWORD old_thread, new_thread; + DWORD old_thread, new_thread, foreground_tid; CBTACTIVATESTRUCT cbt; if (previous == hwnd) @@ -1930,7 +1930,10 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) { req->handle = wine_server_user_handle( hwnd ); if ((ret = !wine_server_call_err( req ))) + { previous = wine_server_ptr_handle( reply->previous ); + foreground_tid = reply->foreground_tid; + } } SERVER_END_REQ; if (!ret) return FALSE; @@ -1962,7 +1965,7 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) for (phwnd = list; *phwnd; phwnd++) { if (get_window_thread( *phwnd, NULL ) == old_thread) - send_message( *phwnd, WM_ACTIVATEAPP, 0, new_thread ); + send_message( *phwnd, WM_ACTIVATEAPP, 0, foreground_tid ); } } if (new_thread) diff --git a/server/protocol.def b/server/protocol.def index f2db46bd87d..12118ecb5bc 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2982,6 +2982,7 @@ enum coords_relative user_handle_t handle; /* handle to the active window */ @REPLY user_handle_t previous; /* handle to the previous active window */ + thread_id_t foreground_tid; /* thread id of the new active window */ @END /* Set the current thread capture window */ diff --git a/server/queue.c b/server/queue.c index c99c8d8661b..3f746c9936b 100644 --- a/server/queue.c +++ b/server/queue.c @@ -1356,7 +1356,11 @@ static void thread_input_destroy( struct object *obj ) empty_msg_list( &input->msg_list ); if ((desktop = input->desktop)) { - if (desktop->foreground_input == input) desktop->foreground_input = NULL; + if (desktop->foreground_input == input) + { + desktop->foreground_input = NULL; + desktop->foreground_tid = 0; + } release_object( desktop ); } if (input->shared) free_shared_object( input->shared ); @@ -3802,6 +3806,7 @@ DECL_HANDLER(set_foreground_window) thread->queue->input->desktop == desktop) { set_foreground_input( desktop, thread->queue->input ); + desktop->foreground_tid = thread->id; reply->send_msg_new = (desktop->foreground_input != queue->input); } else set_win32_error( ERROR_INVALID_WINDOW_HANDLE ); @@ -3834,6 +3839,7 @@ DECL_HANDLER(set_focus_window) DECL_HANDLER(set_active_window) { struct msg_queue *queue = get_current_queue(); + struct desktop *desktop; reply->previous = 0; if (queue && check_queue_input_window( queue, req->handle )) @@ -3847,6 +3853,11 @@ DECL_HANDLER(set_active_window) shared->active = get_user_full_handle( req->handle ); } SHARED_WRITE_END; + if ((desktop = get_thread_desktop( current, 0 ))) + { + reply->foreground_tid = desktop->foreground_tid; + release_object( desktop ); + } else reply->foreground_tid = 0; } else set_error( STATUS_INVALID_HANDLE ); } diff --git a/server/user.h b/server/user.h index 6f10f296200..f4def90a0dc 100644 --- a/server/user.h +++ b/server/user.h @@ -78,6 +78,7 @@ struct desktop struct list pointers; /* list of active pointers */ struct timeout_user *close_timeout; /* timeout before closing the desktop */ struct thread_input *foreground_input; /* thread input of foreground thread */ + thread_id_t foreground_tid; /* thread id of the foreground window */ unsigned int users; /* processes and threads using this desktop */ unsigned char keystate[256]; /* asynchronous key state */ unsigned char alt_pressed; /* last key press was Alt (used to determine msg on release) */ diff --git a/server/winstation.c b/server/winstation.c index f5981721e55..02797a2cd19 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -288,6 +288,7 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned desktop->global_hooks = NULL; desktop->close_timeout = NULL; desktop->foreground_input = NULL; + desktop->foreground_tid = 0; desktop->users = 0; list_init( &desktop->threads ); desktop->clip_flags = 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6580
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=148751 Your paranoid android. === w1064v1507 (32 bit report) === user32: win.c:13147: Test failed: window is minimized. === w10pro64 (32 bit report) === user32: win.c:3820: Test failed: GetForegroundWindow returned 000202C8 win.c:3751: Test failed: SetForegroundWindow failed, error 0 win.c:3754: Test failed: GetForegroundWindow returned 000202C8 win.c:3791: Test failed: GetForegroundWindow returned 000202C8 win.c:3867: Test failed: GetActiveWindow() = 0005004E win.c:3870: Test failed: GetFocus() = 00000000 win.c:3873: Test failed: GetFocus() = 00000000 win.c:3876: Test failed: GetFocus() = 00000000 win.c:3879: Test failed: GetActiveWindow() = 0005004E win.c:3883: Test failed: GetFocus() = 00000000 win.c:3886: Test failed: GetFocus() = 00000000 === w10pro64_ja (64 bit report) === user32: win.c:13143: Test failed: window is not minimized. === debian11 (32 bit ar:MA report) === user32: win.c:10347: Test failed: Timed out waiting for the child process
Quake Champions depends on that to handle loosing focus correctly. It is supposed to minimize itself on WM_ACTIVATEAPP(hwnd=game_hwnd, wparam=0) but refuses to do so if lparam is 0. With my testing on Windows I could not ever get zero new foreground window tid in WM_ACTIVATEAPP(wparam=0). Depending on the way of loosing focus (alt-tabbing with popping up app swithcer, Win Key + D (show desktop), clicking other windows, programmatically setting foreground window) that may be tid of explorer, terminal services, csrss.exe, destination window thread or application but never 0. In Wine when focus switches to some native non-Wine window the focus is set to desktop, with this patch desktop window exploerer's tid will be reported which I hope is fine. I added the new tid to the server as a new stored field because there is nowhere else to get it from in the generic case at the moment of clearing active window on the old thread and sending WM_ACTIVATEAPP(wparam=0) to the old foreground window: - (set_active_window) request for the new active window is not called yet, old thread is handled first; - set_foreground_window() win32u function knows the new window and its thread, but passing it around (including through WM_WINE_SETACTIVEWINDOW (which uses NtUserSetActiveWindow) looks more convoluted than getting that in server's reply; - server currently knows the foreground input queue after (set_foreground_window) and before (set_active_window) for the new active window, but not the exact thread for the new active window (as the input may be attached to multiple threads). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83606
Rémi Bernon (@rbernon) commented about dlls/win32u/input.c:
for (phwnd = list; *phwnd; phwnd++) { if (get_window_thread( *phwnd, NULL ) == old_thread) - send_message( *phwnd, WM_ACTIVATEAPP, 0, new_thread );
How do we end up in the case where new_thread is 0 exactly? Is this because of a `SetForegroundWindow( 0 )` call or `SetActiveWindow( 0 )`? Fwiw I believe that `SetForegroundWindow( 0 )` is supposed to relinquish foreground, but it's not supposed to make it dangling, and Windows implements it by giving it to a different window/process. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83612
How do we end up in the case where new_thread is 0 exactly? Is this because of a `SetForegroundWindow( 0 )` call or `SetActiveWindow( 0 )`?
No, we end up 0 new thread id with SetForegroundWindow() regardless of the argument. See win32u/input.c:set_foreground_window(). set_active_window() is always called twice, once for the thread / windows loosing foreground and once for the new thread / windows gaining foreground. On line 2129 set_active_window() is explicitly called with NULL hwnd (which warrants 0 new_thread_id). On line 2126 (a different thread configuration case) WM_WINE_SETACTIVEWINDOW is called with NULL hwnd (wparam), that will result in set_active_window() with NULL thread called by WM_WINE_SETACTIVEWINDOW handler (through NtUserSetActiveWindow). This logic so far looks all correct WRT proper WM_ACTIVATEPP delivery logic (when it is delivered and when not; the threads which do that). The issue I am concerning is only lparam (thread id) for WM_ACTIVATEPP in the first set_active_window call (for the thread / windows which are loosing foreground). Passing the new_thread_id as lparam looks plainly wrong to me, it is probably guaranteed to be 0 for the loosing focus thread (and for the cases when it maybe won't, like SetActiveWindow, WM_ACTIVATEPP won't be sent at all because SetActiveWindow only works withing the same thread windows while WM_ACTIVATEPP is only sent when the active thread changes). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83623
On Sat Sep 28 20:47:21 2024 +0000, Paul Gofman wrote:
How do we end up in the case where new_thread is 0 exactly? Is this because of a `SetForegroundWindow( 0 )` call or `SetActiveWindow( 0 )`? No, we end up 0 new thread id with SetForegroundWindow() regardless of the argument. See win32u/input.c:set_foreground_window(). set_active_window() is always called twice, once for the thread / windows loosing foreground and once for the new thread / windows gaining foreground. On line 2129 set_active_window() is explicitly called with NULL hwnd (which warrants 0 new_thread_id). On line 2126 (a different thread configuration case) WM_WINE_SETACTIVEWINDOW is called with NULL hwnd (wparam), that will result in set_active_window() with NULL thread called by WM_WINE_SETACTIVEWINDOW handler (through NtUserSetActiveWindow). This logic so far looks all correct WRT proper WM_ACTIVATEPP delivery logic (when it is delivered and when not; the threads which do that). The issue I am concerning is only lparam (thread id) for WM_ACTIVATEPP in the first set_active_window call (for the thread / windows which are loosing foreground). Passing the new_thread_id as lparam looks plainly wrong to me, it is probably guaranteed to be 0 for the loosing focus thread (and for the cases when it maybe won't, like SetActiveWindow, WM_ACTIVATEPP won't be sent at all because SetActiveWindow only works withing the same thread windows while WM_ACTIVATEPP is only sent when the active thread changes). Maybe I could use that TID from server only when hwnd for set_active_window() is NULL, to leave the logic intact when set_active_window() is used SetFocus or SetActiveWindow.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83624
On Sat Sep 28 20:51:02 2024 +0000, Paul Gofman wrote:
Maybe I could use that TID from server only when hwnd for set_active_window() is NULL, to leave the logic intact when set_active_window() is used SetFocus or SetActiveWindow. I see. What about getting the tid from the window in set_foreground_window, and passing it to set_active_window instead? I think it could otherwise happen that the tid of the foreground window you later get does not match anymore the foreground window at the time of the original call. It might, or might not be an issue and I don't know what behavior Windows has here, but it seems like the correct thing to do if we consider SetForegroundWindow operation as a whole.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83641
Rémi Bernon (@rbernon) commented about dlls/user32/tests/win.c:
ok(!app_activated, "Received WM_ACTIVATEAPP(1), did not expect it.\n"); ok(!app_deactivated, "Received WM_ACTIVATEAPP(0), did not expect it.\n");
+ DestroyWindow(window2); + + if (winetest_interactive)
IMO interactive tests aren't useful as they will very rarely get executed, if executed at all. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83642
On Mon Sep 30 09:07:18 2024 +0000, Rémi Bernon wrote:
IMO interactive tests aren't useful as they will very rarely get executed, if executed at all. I think it's the responsibility of a programmer and reviewer to run interactive tests when working on the component. I run interactive tests whenever I review patches in an area.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83665
On Mon Sep 30 16:37:19 2024 +0000, Elizabeth Figura wrote:
I think it's the responsibility of a programmer and reviewer to run interactive tests when working on the component. I run interactive tests whenever I review patches in an area. user32 tests crash the entire host desktop when they are run interactively.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6580#note_83666
participants (5)
-
Elizabeth Figura (＠zfigura) -
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Rémi Bernon