[PATCH v2 1/6] user32/focus: Prevent a recursive loop with the activation messages
When activating a window and sending activation messages to the window procedure, we have to avoid a recursive loop by not sending more of these messages or hooks while we are still activating the window. Some applications actually depend on this behavior. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46274 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v2: Turns out that this patch also fixes an existing TODO wine in tests. Can someone please take a look at these patches and provide feedback if possible? The behavior is very consistent and the tests are pretty much reliable as far as I can see, so it is indeed Windows' behavior on which some apps do rely on (since it fixes an annoying issue in Total Commander's Multi-rename function). I understand it touches a sensitive part of Wine, but I think the tests are pretty conclusive. If not, I'd like some feedback on how to solve it better. Of course, I've been running with this patch for some months now and didn't find anything wrong with activation or focus. dlls/user32/focus.c | 44 +++++++++++++++++++++++++++-------------- dlls/user32/tests/msg.c | 2 +- dlls/user32/win.h | 1 + 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/dlls/user32/focus.c b/dlls/user32/focus.c index f1c8831..0d32d00 100644 --- a/dlls/user32/focus.c +++ b/dlls/user32/focus.c @@ -78,7 +78,7 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) { HWND previous = GetActiveWindow(); BOOL ret; - DWORD old_thread, new_thread; + DWORD winflags, old_thread, new_thread; CBTACTIVATESTRUCT cbt; if (previous == hwnd) @@ -87,16 +87,24 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) return TRUE; } - /* call CBT hook chain */ - cbt.fMouse = mouse; - cbt.hWndActive = previous; - if (HOOK_CallHooks( WH_CBT, HCBT_ACTIVATE, (WPARAM)hwnd, (LPARAM)&cbt, TRUE )) return FALSE; - - if (IsWindow(previous)) + /* Prevent a recursive activation loop with the activation messages */ + winflags = win_set_flags(hwnd, WIN_IS_IN_ACTIVATION, 0); + if (!(winflags & WIN_IS_IN_ACTIVATION)) { - SendMessageW( previous, WM_NCACTIVATE, FALSE, (LPARAM)hwnd ); - SendMessageW( previous, WM_ACTIVATE, - MAKEWPARAM( WA_INACTIVE, IsIconic(previous) ), (LPARAM)hwnd ); + ret = FALSE; + + /* call CBT hook chain */ + cbt.fMouse = mouse; + cbt.hWndActive = previous; + if (HOOK_CallHooks( WH_CBT, HCBT_ACTIVATE, (WPARAM)hwnd, (LPARAM)&cbt, TRUE )) + goto clear_flags; + + if (IsWindow(previous)) + { + SendMessageW( previous, WM_NCACTIVATE, FALSE, (LPARAM)hwnd ); + SendMessageW( previous, WM_ACTIVATE, + MAKEWPARAM( WA_INACTIVE, IsIconic(previous) ), (LPARAM)hwnd ); + } } SERVER_START_REQ( set_active_window ) @@ -106,9 +114,9 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) previous = wine_server_ptr_handle( reply->previous ); } SERVER_END_REQ; - if (!ret) return FALSE; + if (!ret) goto clear_flags; if (prev) *prev = previous; - if (previous == hwnd) return TRUE; + if (previous == hwnd) goto clear_flags; if (hwnd) { @@ -116,7 +124,11 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) if (SendMessageW( hwnd, WM_QUERYNEWPALETTE, 0, 0 )) SendMessageTimeoutW( HWND_BROADCAST, WM_PALETTEISCHANGING, (WPARAM)hwnd, 0, SMTO_ABORTIFHUNG, 2000, NULL ); - if (!IsWindow(hwnd)) return FALSE; + if (!IsWindow(hwnd)) + { + ret = FALSE; + goto clear_flags; + } } old_thread = previous ? GetWindowThreadProcessId( previous, NULL ) : 0; @@ -148,7 +160,7 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) } } - if (IsWindow(hwnd)) + if (!(winflags & WIN_IS_IN_ACTIVATION) && IsWindow(hwnd)) { SendMessageW( hwnd, WM_NCACTIVATE, (hwnd == GetForegroundWindow()), (LPARAM)previous ); SendMessageW( hwnd, WM_ACTIVATE, @@ -173,7 +185,9 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) } } - return TRUE; +clear_flags: + win_set_flags(hwnd, 0, WIN_IS_IN_ACTIVATION); + return ret; } diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 6d0f063..ef75d55 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -5142,7 +5142,7 @@ static void test_messages(void) ShowWindow(hwnd, SW_MINIMIZE); flush_events(); - ok_sequence(WmShowMinOverlappedSeq, "ShowWindow(SW_SHOWMINIMIZED):overlapped", TRUE); + ok_sequence(WmShowMinOverlappedSeq, "ShowWindow(SW_SHOWMINIMIZED):overlapped", FALSE); flush_sequence(); if (GetWindowLongW( hwnd, GWL_STYLE ) & WS_MINIMIZE) diff --git a/dlls/user32/win.h b/dlls/user32/win.h index 81b08fc..d53e1a5 100644 --- a/dlls/user32/win.h +++ b/dlls/user32/win.h @@ -79,6 +79,7 @@ typedef struct tagWND #define WIN_NEEDS_SHOW_OWNEDPOPUP 0x0020 /* WM_SHOWWINDOW:SC_SHOW must be sent in the next ShowOwnedPopup call */ #define WIN_CHILDREN_MOVED 0x0040 /* children may have moved, ignore stored positions */ #define WIN_HAS_IME_WIN 0x0080 /* the window has been registered with imm32 */ +#define WIN_IS_IN_ACTIVATION 0x0100 /* the window is in an activation process */ /* Window functions */ extern HWND get_hwnd_message_parent(void) DECLSPEC_HIDDEN; -- 2.21.0
Windows only sends the activation messages and hooks once, until the SetActiveWindow completes for that window. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Note that without the prior patch that fixes this, Wine would enter a long loop here. dlls/user32/tests/msg.c | 81 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index ef75d55..5a99d74 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -4772,6 +4772,39 @@ static void test_showwindow(void) flush_sequence(); } +static void test_recursive_activation(void) +{ + static const struct message seq[] = + { + { HCBT_ACTIVATE, hook }, + { WM_NCACTIVATE, sent|wparam, TRUE }, + { WM_ACTIVATE, sent|wparam, WA_ACTIVE }, + { HCBT_ACTIVATE, hook }, + { WM_NCACTIVATE, sent|wparam, FALSE }, + { WM_ACTIVATE, sent|wparam, WA_INACTIVE }, + { WM_SETFOCUS, sent|optional }, + { 0 } + }; + HWND hwnd, recursive; + + hwnd = CreateWindowExA(0, "SimpleWindowClass", NULL, WS_OVERLAPPED|WS_VISIBLE, + 100, 100, 200, 200, 0, 0, 0, NULL); + ok(hwnd != 0, "Failed to create simple window\n"); + + recursive = CreateWindowExA(0, "RecursiveActivationClass", NULL, WS_OVERLAPPED|WS_VISIBLE, + 10, 10, 50, 50, hwnd, 0, 0, NULL); + ok(recursive != 0, "Failed to create recursive activation window\n"); + SetActiveWindow(hwnd); + + flush_sequence(); + SetActiveWindow(recursive); + ok_sequence(seq, "Recursive Activation", FALSE); + + DestroyWindow(recursive); + DestroyWindow(hwnd); + flush_sequence(); +} + static void test_sys_menu(void) { HWND hwnd; @@ -9699,6 +9732,48 @@ static LRESULT WINAPI ShowWindowProcA(HWND hwnd, UINT message, WPARAM wParam, LP return ret; } +static LRESULT WINAPI recursive_activation_wndprocA(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam) +{ + static LONG defwndproc_counter = 0; + struct recvd_message msg; + LRESULT ret; + + switch (message) + { + /* log only specific messages we are interested in */ + case WM_NCACTIVATE: + case WM_ACTIVATE: + case WM_SETFOCUS: + case WM_KILLFOCUS: + break; + default: + return DefWindowProcA(hwnd, message, wParam, lParam); + } + + msg.hwnd = hwnd; + msg.message = message; + msg.flags = sent|wparam|lparam; + if (defwndproc_counter) msg.flags |= defwinproc; + msg.wParam = wParam; + msg.lParam = lParam; + msg.descr = "recursive_activation"; + add_message(&msg); + + /* recursively activate ourselves by first losing activation and changing it back */ + if (message == WM_ACTIVATE && LOWORD(wParam) != WA_INACTIVE) + { + SetActiveWindow((HWND)lParam); + SetActiveWindow(hwnd); + return 0; + } + + defwndproc_counter++; + ret = DefWindowProcA(hwnd, message, wParam, lParam); + defwndproc_counter--; + + return ret; +} + static LRESULT WINAPI PaintLoopProcA(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam) { switch (msg) @@ -9796,6 +9871,10 @@ static BOOL RegisterWindowClasses(void) cls.lpszClassName = "ShowWindowClass"; if(!RegisterClassA(&cls)) return FALSE; + cls.lpfnWndProc = recursive_activation_wndprocA; + cls.lpszClassName = "RecursiveActivationClass"; + if(!RegisterClassA(&cls)) return FALSE; + cls.lpfnWndProc = PopupMsgCheckProcA; cls.lpszClassName = "TestPopupClass"; if(!RegisterClassA(&cls)) return FALSE; @@ -9851,6 +9930,7 @@ static BOOL is_our_logged_class(HWND hwnd) { if (!lstrcmpiA(buf, "TestWindowClass") || !lstrcmpiA(buf, "ShowWindowClass") || + !lstrcmpiA(buf, "RecursiveActivationClass") || !lstrcmpiA(buf, "TestParentClass") || !lstrcmpiA(buf, "TestPopupClass") || !lstrcmpiA(buf, "SimpleWindowClass") || @@ -17617,6 +17697,7 @@ START_TEST(msg) test_messages(); test_setwindowpos(); test_showwindow(); + test_recursive_activation(); invisible_parent_tests(); test_mdi_messages(); test_button_messages(); -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=51399 Your paranoid android. === w7pro64 (32 bit report) === user32: msg.c:13469: Test failed: 54: ShowWindow(SW_MINIMIZE): 4: in msg 0x0047 expecting wParam 0x8170 got 0x1803 msg.c:13469: Test failed: 54: ShowWindow(SW_MINIMIZE): 5: the msg 0x0003 was expected, but got msg 0x0047 instead msg.c:13469: Test failed: 54: ShowWindow(SW_MINIMIZE): 6: the msg 0x0005 was expected, but got msg 0x0003 instead msg.c:13469: Test failed: 54: ShowWindow(SW_MINIMIZE): 7: the msg sequence is not complete: expected 0000 - actual 0005 === debian9 (32 bit report) === user32: msg.c:8746: Test failed: WaitForSingleObject failed 102 msg.c:8752: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead msg.c:8752: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead msg.c:8752: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000
On Windows, if WM_NCACTIVATE returns FALSE, the window should not be able to be deactivated. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/focus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/user32/focus.c b/dlls/user32/focus.c index 0d32d00..834ceb1 100644 --- a/dlls/user32/focus.c +++ b/dlls/user32/focus.c @@ -101,7 +101,7 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) if (IsWindow(previous)) { - SendMessageW( previous, WM_NCACTIVATE, FALSE, (LPARAM)hwnd ); + if (!SendMessageW( previous, WM_NCACTIVATE, FALSE, (LPARAM)hwnd )) goto clear_flags; SendMessageW( previous, WM_ACTIVATE, MAKEWPARAM( WA_INACTIVE, IsIconic(previous) ), (LPARAM)hwnd ); } -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=51400 Your paranoid android. === debian9 (32 bit WoW report) === user32: msg.c:8746: Test failed: WaitForSingleObject failed 102 msg.c:8752: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead msg.c:8752: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead msg.c:8752: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000
On Windows, if WM_NCACTIVATE returns FALSE, the window doesn't get deactivated. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/tests/win.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 639eef4..e839d86 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -899,6 +899,17 @@ static LRESULT WINAPI tool_window_procA(HWND hwnd, UINT msg, WPARAM wparam, LPAR return DefWindowProcA(hwnd, msg, wparam, lparam); } +static LRESULT WINAPI no_deactivation_procA(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) +{ + switch (msg) + { + case WM_NCACTIVATE: + DefWindowProcA(hwnd, msg, wparam, lparam); + return FALSE; + } + return DefWindowProcA(hwnd, msg, wparam, lparam); +} + static const WCHAR mainclassW[] = {'M','a','i','n','W','i','n','d','o','w','C','l','a','s','s','W',0}; static BOOL RegisterWindowClasses(void) @@ -3205,6 +3216,18 @@ static void test_SetActiveWindow(HWND hwnd) check_active_state(hwnd, hwnd, hwnd); } DestroyWindow(hwnd2); + + /* a window can disallow being deactivated by returning FALSE from WM_NCACTIVATE */ + hwnd2 = CreateWindowExA(0, "MainWindowClass", "No Deactivation window", WS_OVERLAPPED | WS_VISIBLE, + 10, 10, 50, 50, hwnd, 0, GetModuleHandleA(NULL), NULL); + ok(hwnd2 != NULL, "failed to create No Deactivation window: 0x%08x\n", GetLastError()); + SetWindowLongPtrA(hwnd2, GWLP_WNDPROC, (LONG_PTR)no_deactivation_procA); + + SetActiveWindow(hwnd2); + check_wnd_state(hwnd2, hwnd2, hwnd2, 0); + SetActiveWindow(hwnd); + check_wnd_state(hwnd2, hwnd2, hwnd2, 0); + DestroyWindow(hwnd2); } struct create_window_thread_params -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- See tests in next patch; changing the activation from within the WA_INACTIVE message is kept by Windows. dlls/user32/focus.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/user32/focus.c b/dlls/user32/focus.c index 834ceb1..1fac45d 100644 --- a/dlls/user32/focus.c +++ b/dlls/user32/focus.c @@ -104,6 +104,9 @@ static BOOL set_active_window( HWND hwnd, HWND *prev, BOOL mouse, BOOL focus ) if (!SendMessageW( previous, WM_NCACTIVATE, FALSE, (LPARAM)hwnd )) goto clear_flags; SendMessageW( previous, WM_ACTIVATE, MAKEWPARAM( WA_INACTIVE, IsIconic(previous) ), (LPARAM)hwnd ); + + /* Give up if the activation was changed */ + if (previous != GetActiveWindow()) goto clear_flags; } } -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=51402 Your paranoid android. === debian9 (32 bit report) === user32: msg.c:8746: Test failed: WaitForSingleObject failed 102 msg.c:8752: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead msg.c:8752: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead msg.c:8752: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/tests/win.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index e839d86..209161e 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -910,6 +910,17 @@ static LRESULT WINAPI no_deactivation_procA(HWND hwnd, UINT msg, WPARAM wparam, return DefWindowProcA(hwnd, msg, wparam, lparam); } +static LRESULT WINAPI change_activation_from_wa_inactive_procA(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) +{ + switch (msg) + { + case WM_ACTIVATE: + if (LOWORD(wparam) == WA_INACTIVE) + SetActiveWindow((HWND)GetWindowLongPtrA(hwnd, GWLP_USERDATA)); + } + return DefWindowProcA(hwnd, msg, wparam, lparam); +} + static const WCHAR mainclassW[] = {'M','a','i','n','W','i','n','d','o','w','C','l','a','s','s','W',0}; static BOOL RegisterWindowClasses(void) @@ -3137,7 +3148,7 @@ todo_wine static void test_SetActiveWindow(HWND hwnd) { - HWND hwnd2, ret; + HWND hwnd2, hwnd3, ret; flush_events( TRUE ); ShowWindow(hwnd, SW_HIDE); @@ -3228,6 +3239,24 @@ static void test_SetActiveWindow(HWND hwnd) SetActiveWindow(hwnd); check_wnd_state(hwnd2, hwnd2, hwnd2, 0); DestroyWindow(hwnd2); + + /* changing the activated window from within WM_ACTIVATE's WA_INACTIVE should work */ + hwnd2 = CreateWindowExA(0, "MainWindowClass", "No Deactivation window", WS_OVERLAPPED | WS_VISIBLE, + 10, 10, 50, 50, hwnd, 0, GetModuleHandleA(NULL), NULL); + ok(hwnd2 != NULL, "failed to create window: 0x%08x\n", GetLastError()); + hwnd3 = CreateWindowExA(0, "MainWindowClass", "No Deactivation window", WS_OVERLAPPED | WS_VISIBLE, + 70, 70, 50, 50, hwnd, 0, GetModuleHandleA(NULL), NULL); + ok(hwnd3 != NULL, "failed to create window: 0x%08x\n", GetLastError()); + + SetActiveWindow(hwnd2); + check_wnd_state(hwnd2, hwnd2, hwnd2, 0); + SetWindowLongPtrA(hwnd2, GWLP_USERDATA, (LONG_PTR)hwnd3); + SetWindowLongPtrA(hwnd2, GWLP_WNDPROC, (LONG_PTR)change_activation_from_wa_inactive_procA); + SetActiveWindow(hwnd); + check_wnd_state(hwnd3, hwnd3, hwnd3, 0); + + DestroyWindow(hwnd2); + DestroyWindow(hwnd3); } struct create_window_thread_params -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=51403 Your paranoid android. === debian9 (32 bit Chinese:China report) === user32: win.c:10185: Test failed: Expected foreground window 000E0120, got 00E300D4
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=51398 Your paranoid android. === wvistau64_zh_CN (32 bit report) === user32: msg.c:5152: Test failed: ShowWindow(SW_RESTORE):overlapped: 44: the msg sequence is not complete: expected 0000 - actual 0088 === debian9 (32 bit report) === user32: msg.c:8713: Test failed: WaitForSingleObject failed 102 msg.c:8719: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead msg.c:8719: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead msg.c:8719: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000
participants (2)
-
Gabriel Ivăncescu -
Marvin