[PATCH] user32: Add a timeout when sending WM_WINE_DESTROYWINDOW
Without this change, the test will deadlock in DestroyWindow waiting for the other thread to process the WM_WINE_DESTROYWINDOW message. Note that this adds about 1 second of runtime to the test, waiting for the SendMessage to time out. Windows does not time out like this. Another option would be to use PostMessage, but that seems like a larger change. Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> --- dlls/user32/tests/win.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++ dlls/user32/win.c | 4 +- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 96a2467da0..b8fa44878c 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -10433,6 +10433,103 @@ if (!is_wine) /* FIXME: remove once Wine is fixed */ DestroyWindow(owner); } +static struct destroy_data +{ + HWND main_wnd; + HWND thread_wnd; + HANDLE evt; + DWORD main_tid; +} destroy_data; + +static LRESULT WINAPI destroy_thread_wndproc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) +{ + switch (msg) + { + case WM_DESTROY: + PostQuitMessage(0); + break; + } + return DefWindowProcA(hwnd, msg, wparam, lparam); +} + +static DWORD CALLBACK destroy_thread(void *user) +{ + MSG msg; + + destroy_data.thread_wnd = CreateWindowExA(0, "destroy_test_thread", + "destroy test thread", WS_CHILD, 100, 100, 100, 100, + destroy_data.main_wnd, 0, GetModuleHandleA(NULL), NULL); + ok(destroy_data.thread_wnd != NULL, "CreateWindowEx failed with error %d\n", GetLastError()); + if (!destroy_data.thread_wnd) + return 0; + + PostThreadMessageW(destroy_data.main_tid, WM_USER, 0, 0); + + while (GetMessageA(&msg, 0, 0, 0)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + + PostThreadMessageW(destroy_data.main_tid, WM_USER + 1, 0, 0); + WaitForSingleObject(destroy_data.evt, INFINITE); + + return 0; +} + +static void test_destroy_quit(void) +{ + MSG msg; + WNDCLASSA wnd_classA; + ATOM ret; + HANDLE thread; + + destroy_data.main_tid = GetCurrentThreadId(); + destroy_data.evt = CreateEventW(NULL, FALSE, FALSE, NULL); + + memset(&wnd_classA, 0, sizeof(wnd_classA)); + wnd_classA.lpszClassName = "destroy_test_main"; + wnd_classA.lpfnWndProc = DefWindowProcA; + ret = RegisterClassA(&wnd_classA); + ok(ret, "RegisterClass failed with error %d\n", GetLastError()); + + memset(&wnd_classA, 0, sizeof(wnd_classA)); + wnd_classA.lpszClassName = "destroy_test_thread"; + wnd_classA.lpfnWndProc = destroy_thread_wndproc; + ret = RegisterClassA(&wnd_classA); + ok(ret, "RegisterClass failed with error %d\n", GetLastError()); + + destroy_data.main_wnd = CreateWindowExA(0, "destroy_test_main", + "destroy test main", WS_OVERLAPPED | WS_CAPTION, 100, 100, 100, 100, + 0, 0, GetModuleHandleA(NULL), NULL); + ok(destroy_data.main_wnd != NULL, "CreateWindowEx failed with error %d\n", GetLastError()); + if (!destroy_data.main_wnd) + { + CloseHandle(destroy_data.evt); + return; + } + + thread = CreateThread(NULL, 0, &destroy_thread, 0, 0, NULL); + + while (GetMessageA(&msg, 0, 0, 0)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + if (msg.message == WM_USER) + { + DestroyWindow(destroy_data.main_wnd); + } + else if (msg.message == WM_USER + 1) + { + SetEvent(destroy_data.evt); + break; + } + } + + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); +} + START_TEST(win) { char **argv; @@ -10583,6 +10680,7 @@ START_TEST(win) test_desktop(); test_hide_window(); test_minimize_window(hwndMain); + test_destroy_quit(); /* add the tests above this line */ if (hhook) UnhookWindowsHookEx(hhook); diff --git a/dlls/user32/win.c b/dlls/user32/win.c index b13032bd6e..939903e053 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -971,7 +971,7 @@ LRESULT WIN_DestroyWindow( HWND hwnd ) for (i = 0; list[i]; i++) { if (WIN_IsCurrentThread( list[i] )) WIN_DestroyWindow( list[i] ); - else SendMessageW( list[i], WM_WINE_DESTROYWINDOW, 0, 0 ); + else SendMessageTimeoutW( list[i], WM_WINE_DESTROYWINDOW, 0, 0, SMTO_ABORTIFHUNG, 1000, NULL ); } HeapFree( GetProcessHeap(), 0, list ); } @@ -1047,7 +1047,7 @@ static void destroy_thread_window( HWND hwnd ) for (i = 0; list[i]; i++) { if (WIN_IsCurrentThread( list[i] )) destroy_thread_window( list[i] ); - else SendMessageW( list[i], WM_WINE_DESTROYWINDOW, 0, 0 ); + else SendMessageTimeoutW( list[i], WM_WINE_DESTROYWINDOW, 0, 0, SMTO_ABORTIFHUNG, 1000, NULL ); } HeapFree( GetProcessHeap(), 0, list ); } -- 2.16.2
Hi, While running your changed tests on Windows, 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=36146 Your paranoid android. === w864 (32 bit win) === win.c:2257: Test failed: expected !100 win.c:2257: Test failed: expected !100
Andrew Eikum <aeikum(a)codeweavers.com> writes:
Without this change, the test will deadlock in DestroyWindow waiting for the other thread to process the WM_WINE_DESTROYWINDOW message.
Note that this adds about 1 second of runtime to the test, waiting for the SendMessage to time out. Windows does not time out like this. Another option would be to use PostMessage, but that seems like a larger change.
Your test doesn't deadlock here, and I'm not sure I see why it should, since the thread is processing messages. Do you have a backtrace of the deadlock? -- Alexandre Julliard julliard(a)winehq.org
On Fri, Feb 23, 2018 at 10:11:06AM +0100, Alexandre Julliard wrote:
Andrew Eikum <aeikum(a)codeweavers.com> writes:
Without this change, the test will deadlock in DestroyWindow waiting for the other thread to process the WM_WINE_DESTROYWINDOW message.
Note that this adds about 1 second of runtime to the test, waiting for the SendMessage to time out. Windows does not time out like this. Another option would be to use PostMessage, but that seems like a larger change.
Your test doesn't deadlock here, and I'm not sure I see why it should, since the thread is processing messages. Do you have a backtrace of the deadlock?
Backtraces of the two threads below. My understanding is the DestroyWindow is waiting for the second thread to handle the WM_WINE_DESTROYWINDOW, while the second thread is waiting for the first thread to call SetEvent. Windows doesn't send WM_WINE_DESTROYWINDOW, so DestroyWindow exits and the test calls SetEvent. There might be a race condition in the test, if the first thread sends WM_WINE_DESTROYWINDOW before the PostQuitMessage's WM_QUIT causes the second thread's event loop to exit. However, it deadlocks every time for me. Andrew Backtracing for thread 002d in process 0008 (Z:\home\aeikum\src\wine\dlls\user32\tests\user32_test.exe): Backtrace: =>0 0xf7f9bdb7 __kernel_vsyscall+0x7() in [vdso].so (0x0046f9d8) 1 0xf7d6383b __libc_read+0x5a() in libpthread.so.0 (0x0046f9d8) 2 0x7bc7fa27 wait_select_reply+0x66(cookie=0x46fa28) [/home/aeikum/src/wine/dlls/ntdll/server.c:359] in ntdll (0x0046f9d8) 3 0x7bc81233 server_select+0x252(select_op=0x46fb38, size=0x8, flags=0x2, timeout=(nil)) [/home/aeikum/src/wine/dlls/ntdll/server.c:618] in ntdll (0x0046fb08) 4 0x7bc888f4 wait_objects+0x83(count=<is not available>, handles=<is not available>, wait_any=<is not available>, alertable=0, timeout=(nil)) [/home/aeikum/src/wine/dlls/ntdll/sync.c:1031] in ntdll (0x0046f 5 0x7bc8ae77 NtWaitForMultipleObjects+0x36(count=<couldn't compute location>, handles=<couldn't compute location>, wait_any=<couldn't compute location>, alertable=<couldn't compute location>, timeout=<could 6 0x7b4767dc WaitForMultipleObjectsEx.part+0xcb() in kernel32 (0x0046fde8) 7 0x7b476ac0 WaitForMultipleObjectsEx+0x3f() in kernel32 (0x0046fe38) 8 0x7b476b29 WaitForSingleObject+0x38(handle=<couldn't compute location>, timeout=<couldn't compute location>) [/home/aeikum/src/wine/dlls/kernel32/sync.c:128] in kernel32 (0x0046fe78) 9 0x7eab926e destroy_thread+0x11d() in user32_test (0x0046fed8) 10 0x7bc8264c call_thread_func_wrapper+0xb() in ntdll (0x0046feec) 11 0x7bc85c23 call_thread_func+0x102(entry=0x7eab9150, arg=0x0(nil)) [/home/aeikum/src/wine/dlls/ntdll/signal_i386.c:2623] in ntdll (0x0046ffdc) 12 0x7bc8263e call_thread_entry+0x9() in ntdll (0x0046ffec) Backtracing for thread 0009 in process 0008 (Z:\home\aeikum\src\wine\dlls\user32\tests\user32_test.exe): Backtrace: =>0 0xf7f9bdb7 __kernel_vsyscall+0x7() in [vdso].so (0x0033f4a8) 1 0xf7d6383b __libc_read+0x5a() in libpthread.so.0 (0x0033f4a8) 2 0x7bc7fa27 wait_select_reply+0x66(cookie=0x33f4f8) [/home/aeikum/src/wine/dlls/ntdll/server.c:359] in ntdll (0x0033f4a8) 3 0x7bc81233 server_select+0x252(select_op=0x33f608, size=0x8, flags=0x2, timeout=(nil)) [/home/aeikum/src/wine/dlls/ntdll/server.c:618] in ntdll (0x0033f5d8) 4 0x7bc888f4 wait_objects+0x83(count=<is not available>, handles=<is not available>, wait_any=<is not available>, alertable=0, timeout=(nil)) [/home/aeikum/src/wine/dlls/ntdll/sync.c:1031] in ntdll (0x0033f 5 0x7bc8ae77 NtWaitForMultipleObjects+0x36(count=<couldn't compute location>, handles=<couldn't compute location>, wait_any=<couldn't compute location>, alertable=<couldn't compute location>, timeout=<could 6 0x7b4767dc WaitForMultipleObjectsEx.part+0xcb() in kernel32 (0x0033f8b8) 7 0x7b476ac0 WaitForMultipleObjectsEx+0x3f() in kernel32 (0x0033f908) 8 0x7e12b0fb X11DRV_MsgWaitForMultipleObjectsEx+0x9a(count=<couldn't compute location>, handles=<couldn't compute location>, timeout=<couldn't compute location>, mask=<couldn't compute location>, flags=<cou 9 0x7e8c76eb wait_message+0x3a() in user32 (0x0033f9c8) 10 0x7e89000f wait_message_reply+0xbe(flags=<is not available>) [/home/aeikum/src/wine/dlls/user32/message.c:3035] in user32 (0x0033fab8) 11 0x7e8903ac send_inter_thread_message+0x6b(info=0x33fb60, res_ptr=0x33fb28) [/home/aeikum/src/wine/dlls/user32/message.c:3213] in user32 (0x0033faf8) 12 0x7e8906b2 send_message+0x161(info=0x33fb60, res_ptr=0x33fb5c, unicode=<is not available>) [/home/aeikum/src/wine/dlls/user32/message.c:3281] in user32 (0x0033fb48) 13 0x7e8909cd SendMessageW+0x6c(hwnd=<couldn't compute location>, msg=<couldn't compute location>, wparam=<couldn't compute location>, lparam=<couldn't compute location>) [/home/aeikum/src/wine/dlls/user32/ 14 0x7e8bad2e WIN_DestroyWindow+0xbd(hwnd=0x30056) [/home/aeikum/src/wine/dlls/user32/win.c:974] in user32 (0x0033fc68) 15 0x7e8bab84 DestroyWindow+0x263(hwnd=<couldn't compute location>) [/home/aeikum/src/wine/dlls/user32/win.c:1911] in user32 (0x0033fca8) 16 0x7eabc669 func_win+0x2888() [/home/aeikum/src/wine/dlls/user32/tests/win.c:10520] in user32_test (0x0033fdb8) 17 0x7ea0cfee main+0x2fd(argc=<is not available>, argv=<is not available>) [/home/aeikum/src/wine/dlls/user32/tests/../../../include/wine/test.h:603] in user32_test (0x0033fe78) 18 0x7eac3211 __wine_spec_exe_entry+0x70(peb=<couldn't compute location>) [/home/aeikum/src/wine/dlls/winecrt0/exe_entry.c:36] in user32_test (0x0033feb8) 19 0x7b463a9c call_process_entry+0xb() in kernel32 (0x0033fed8) 20 0x7b465578 start_process+0x1c7(entry=<couldn't compute location>, peb=<couldn't compute location>) [/home/aeikum/src/wine/dlls/kernel32/process.c:1098] in kernel32 (0x0033ffd8) 21 0x7b463aaa start_process_wrapper+0x9() in kernel32 (0x0033ffec) 0xf7f70db7 __kernel_vsyscall+0x7 in [vdso].so: int»·····$0x80
Andrew Eikum <aeikum(a)codeweavers.com> writes:
Backtraces of the two threads below. My understanding is the DestroyWindow is waiting for the second thread to handle the WM_WINE_DESTROYWINDOW, while the second thread is waiting for the first thread to call SetEvent.
Windows doesn't send WM_WINE_DESTROYWINDOW, so DestroyWindow exits and the test calls SetEvent.
There might be a race condition in the test, if the first thread sends WM_WINE_DESTROYWINDOW before the PostQuitMessage's WM_QUIT causes the second thread's event loop to exit. However, it deadlocks every time for me.
I don't think we want to simply ignore the failure and keep child windows alive when their parent is gone. Probably the sequence could be changed so that WM_WINE_DESTROYWINDOW is sent first, and WM_DESTROY is sent in the context of the target thread. -- Alexandre Julliard julliard(a)winehq.org
On Fri, Feb 23, 2018 at 03:54:14PM +0100, Alexandre Julliard wrote:
I don't think we want to simply ignore the failure and keep child windows alive when their parent is gone. Probably the sequence could be changed so that WM_WINE_DESTROYWINDOW is sent first, and WM_DESTROY is sent in the context of the target thread.
Makes sense, I can experiment with that. Thanks. Andrew
participants (3)
-
Alexandre Julliard -
Andrew Eikum -
Marvin