[PATCH v3 1/2] user32/tests: Wait for parent window thread before checking window.
This test is failing from time to time. Making sure the parent thread has terminated before continuing triggers the underlying race condition, and makes the test to always fail. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- Supersedes: 205970-205971 v3: Fix another stupid mistake where the thread handle is closed before the grand child had a chance to wait on it. dlls/user32/tests/msg.c | 52 +++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 13e085dfacc..fab8f291a52 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -8791,7 +8791,10 @@ static void test_paint_messages(void) struct wnd_event { HWND hwnd; + HWND child_hwnd; + HANDLE child; HANDLE grand_child; + HANDLE ready_event; HANDLE start_event; HANDLE stop_event; }; @@ -8822,42 +8825,48 @@ static DWORD CALLBACK create_grand_child_thread( void *param ) { struct wnd_event *wnd_event = param; HWND hchild; - MSG msg; + DWORD ret; hchild = CreateWindowExA(0, "TestWindowClass", "Test child", - WS_CHILD | WS_VISIBLE, 0, 0, 10, 10, wnd_event->hwnd, 0, 0, NULL); + WS_CHILD | WS_VISIBLE, 0, 0, 10, 10, wnd_event->child_hwnd, 0, 0, NULL); ok (hchild != 0, "Failed to create child window\n"); flush_events(); flush_sequence(); + + /* wait for wnd_event->child to be set */ + ret = WaitForSingleObject( wnd_event->ready_event, 1000 ); + ok( !ret, "WaitForSingleObject failed %x\n", ret ); SetEvent( wnd_event->start_event ); - for (;;) - { - MsgWaitForMultipleObjects(0, NULL, FALSE, 1000, QS_ALLINPUT); - if (!IsWindow( hchild )) break; /* will be destroyed when parent thread exits */ - while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); - } + /* wait for parent window thread to exit */ + ret = WaitForSingleObject( wnd_event->child, 1000 ); + ok( !ret, "WaitForSingleObject returned %x, error: %u\n", ret, GetLastError() ); + ok( IsWindow( hchild ), "Child window already destroyed\n" ); + flush_events(); + todo_wine ok( !IsWindow( hchild ), "Child window not destroyed\n" ); + return 0; } static DWORD CALLBACK create_child_thread( void *param ) { struct wnd_event *wnd_event = param; - struct wnd_event child_event; DWORD ret, tid; MSG msg; - child_event.hwnd = CreateWindowExA(0, "TestWindowClass", "Test child", - WS_CHILD | WS_VISIBLE, 0, 0, 10, 10, wnd_event->hwnd, 0, 0, NULL); - ok (child_event.hwnd != 0, "Failed to create child window\n"); - SetFocus( child_event.hwnd ); + wnd_event->child_hwnd = CreateWindowExA( 0, "TestWindowClass", "Test child", WS_CHILD | WS_VISIBLE, + 0, 0, 10, 10, wnd_event->hwnd, 0, 0, NULL ); + ok( wnd_event->child_hwnd != 0, "Failed to create child windows\n" ); + SetFocus( wnd_event->child_hwnd ); + + wnd_event->grand_child = CreateThread( NULL, 0, create_grand_child_thread, wnd_event, 0, &tid ); + ok( wnd_event->grand_child != 0, "CreateThread failed, error %u\n", GetLastError() ); + flush_events(); flush_sequence(); - child_event.start_event = wnd_event->start_event; - wnd_event->grand_child = CreateThread(NULL, 0, create_grand_child_thread, &child_event, 0, &tid); for (;;) { - DWORD ret = MsgWaitForMultipleObjects(1, &child_event.start_event, FALSE, 1000, QS_SENDMESSAGE); + DWORD ret = MsgWaitForMultipleObjects(1, &wnd_event->start_event, FALSE, 1000, QS_SENDMESSAGE); if (ret != 1) break; while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); } @@ -8997,9 +9006,11 @@ static void test_interthread_messages(void) flush_events(); flush_sequence(); log_all_parent_messages++; + wnd_event.ready_event = CreateEventA( NULL, TRUE, FALSE, NULL ); wnd_event.start_event = CreateEventA( NULL, TRUE, FALSE, NULL ); wnd_event.stop_event = CreateEventA( NULL, TRUE, FALSE, NULL ); - hThread = CreateThread( NULL, 0, create_child_thread, &wnd_event, 0, &tid ); + wnd_event.child = CreateThread( NULL, 0, create_child_thread, &wnd_event, 0, &tid ); + SetEvent( wnd_event.ready_event ); for (;;) { ret = MsgWaitForMultipleObjects(1, &wnd_event.start_event, FALSE, 1000, QS_SENDMESSAGE); @@ -9009,18 +9020,19 @@ static void test_interthread_messages(void) ok( !ret, "MsgWaitForMultipleObjects failed %x\n", ret ); /* now wait for the thread without processing messages; this shouldn't deadlock */ SetEvent( wnd_event.stop_event ); - ret = WaitForSingleObject( hThread, 5000 ); + ret = WaitForSingleObject( wnd_event.child, 5000 ); ok( !ret, "WaitForSingleObject failed %x\n", ret ); - CloseHandle( hThread ); ret = WaitForSingleObject( wnd_event.grand_child, 5000 ); ok( !ret, "WaitForSingleObject failed %x\n", ret ); CloseHandle( wnd_event.grand_child ); + CloseHandle( wnd_event.child ); CloseHandle( wnd_event.start_event ); CloseHandle( wnd_event.stop_event ); + CloseHandle( wnd_event.ready_event ); flush_events(); - ok_sequence(WmExitThreadSeq, "destroy child on thread exit", FALSE); + ok_sequence( WmExitThreadSeq, "destroy child on thread exit", TRUE ); log_all_parent_messages--; DestroyWindow( wnd_event.hwnd ); -- 2.31.0
On thread destroy, a WM_WINE_DESTROYWINDOW is sent to the child windows living in other threads. There's then a race condition between these threads peeking for messages and the current thread detaching its child windows from their threads and clearing their message queues, and the message may never be received from these threads and the windows kept alive. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/msg.c | 4 ++-- server/window.c | 12 ++++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index fab8f291a52..23ba4f1d3d5 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -8843,7 +8843,7 @@ static DWORD CALLBACK create_grand_child_thread( void *param ) ok( !ret, "WaitForSingleObject returned %x, error: %u\n", ret, GetLastError() ); ok( IsWindow( hchild ), "Child window already destroyed\n" ); flush_events(); - todo_wine ok( !IsWindow( hchild ), "Child window not destroyed\n" ); + ok( !IsWindow( hchild ), "Child window not destroyed\n" ); return 0; } @@ -9032,7 +9032,7 @@ static void test_interthread_messages(void) CloseHandle( wnd_event.stop_event ); CloseHandle( wnd_event.ready_event ); flush_events(); - ok_sequence( WmExitThreadSeq, "destroy child on thread exit", TRUE ); + ok_sequence( WmExitThreadSeq, "destroy child on thread exit", FALSE ); log_all_parent_messages--; DestroyWindow( wnd_event.hwnd ); diff --git a/server/window.c b/server/window.c index ac12912db3a..026d54e6d32 100644 --- a/server/window.c +++ b/server/window.c @@ -1890,9 +1890,17 @@ void destroy_window( struct window *win ) /* destroy all children */ while (!list_empty(&win->children)) - destroy_window( LIST_ENTRY( list_head(&win->children), struct window, entry )); + { + struct window *child = LIST_ENTRY( list_head( &win->children ), struct window, entry ); + if (!child->thread || child->thread == win->thread) destroy_window( child ); + else list_remove( &child->entry ); + } while (!list_empty(&win->unlinked)) - destroy_window( LIST_ENTRY( list_head(&win->unlinked), struct window, entry )); + { + struct window *child = LIST_ENTRY( list_head( &win->unlinked ), struct window, entry ); + if (!child->thread || child->thread == win->thread) destroy_window( child ); + else list_remove( &child->entry ); + } /* reset global window pointers, if the corresponding window is destroyed */ if (win == shell_window) shell_window = NULL; -- 2.31.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=90712 Your paranoid android. === w1064_tsign (32 bit report) === user32: msg: Timeout
On 5/19/21 1:23 AM, Marvin wrote:
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=90712
Your paranoid android.
=== w1064_tsign (32 bit report) ===
user32: msg: Timeout
I think this is actually unrelated to the changes, as it's a time out that happens randomly. I've sent https://source.winehq.org/patches/data/206180 trying to address it (at least the spurious timeout). -- Rémi Bernon <rbernon(a)codeweavers.com>
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=90711 Your paranoid android. === w1064_tsign (32 bit report) === user32: msg: Timeout === w1064 (64 bit report) === user32: msg.c:5564: Test failed: RedrawWindow:show_popup_extreme_location: 24: the msg 0x0085 was expected, but got msg 0x0047 instead msg.c:5564: Test failed: RedrawWindow:show_popup_extreme_location: 25: the msg sequence is not complete: expected 0014 - actual 0000 === w10pro64_zh_CN (64 bit report) === user32: msg: Timeout
participants (2)
-
Marvin -
Rémi Bernon