[PATCH resend 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> --- 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 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 focus. dlls/user32/focus.c | 44 +++++++++++++++++++++++++++++--------------- dlls/user32/win.h | 1 + 2 files changed, 30 insertions(+), 15 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/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 6d0f063..a38e751 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
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=51370 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 === debian9 (64 bit WoW report) === user32: clipboard.c:1630: Test failed: gle 5 clipboard.c:1632: Test failed: expected moveable mem 0000000000000000 clipboard.c:1637: Test failed: expected moveable mem 0000000000000000 clipboard.c:1642: Test failed: expected moveable mem 0000000000000000 clipboard.c:1648: Test failed: expected moveable mem 0000000000000000 clipboard.c:1654: Test failed: expected bitmap 0000000000000000 clipboard.c:1655: Test failed: different bitmap 0000000000000000 / 00000000005E003F clipboard.c:1658: Test failed: expected bitmap 0000000000000000 clipboard.c:1659: Test failed: different bitmap 0000000000000000 / 00000000000B0045 clipboard.c:1662: Test failed: expected palette 0000000000000000 clipboard.c:1663: Test failed: different palette 0000000000000000 / 00000000000C0041 clipboard.c:1666: Test failed: expected fixed mem 0000000000000000 clipboard.c:1668: Test failed: expected fixed mem 0000000000000000 clipboard.c:1670: Test failed: gle 1418
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
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=51371 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> --- 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=51372 Your paranoid android. === debian9 (64 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
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=51373 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
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=51368 Your paranoid android. === debian9 (32 bit WoW 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