[PATCH 1/3] user32/tests: Remove todo_wine for ShowWindow(SW_SHOWMINIMIZED):overlapped.
It seems to consistently succeed now, although the failure origin is probably still there. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 7eaa2c67945..13e085dfacc 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -5405,7 +5405,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) -- 2.31.0
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> --- dlls/user32/tests/msg.c | 44 ++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 13e085dfacc..13a657e7a40 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -8791,6 +8791,8 @@ static void test_paint_messages(void) struct wnd_event { HWND hwnd; + HWND child_hwnd; + HANDLE child; HANDLE grand_child; HANDLE start_event; HANDLE stop_event; @@ -8822,42 +8824,44 @@ 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(); 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); } @@ -8999,7 +9003,7 @@ static void test_interthread_messages(void) log_all_parent_messages++; 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 ); for (;;) { ret = MsgWaitForMultipleObjects(1, &wnd_event.start_event, FALSE, 1000, QS_SENDMESSAGE); @@ -9009,9 +9013,9 @@ 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 ); + CloseHandle( wnd_event.child ); ret = WaitForSingleObject( wnd_event.grand_child, 5000 ); ok( !ret, "WaitForSingleObject failed %x\n", ret ); @@ -9020,7 +9024,7 @@ static void test_interthread_messages(void) CloseHandle( wnd_event.start_event ); CloseHandle( wnd_event.stop_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 13a657e7a40..be3a4802d6c 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -8838,7 +8838,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; } @@ -9024,7 +9024,7 @@ static void test_interthread_messages(void) CloseHandle( wnd_event.start_event ); CloseHandle( wnd_event.stop_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..0780681cd53 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 || child->thread->state == TERMINATED) 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 || child->thread->state == TERMINATED) 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=90703 Your paranoid android. === w8adm (32 bit report) === user32: msg.c:8838: Test failed: WaitForSingleObject returned ffffffff, error: 6
On 5/18/21 3:43 PM, 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=90703
Your paranoid android.
=== w8adm (32 bit report) ===
user32: msg.c:8838: Test failed: WaitForSingleObject returned ffffffff, error: 6
I think I missed a possible race, good opportunity to resend the two patches without the first one. -- Rémi Bernon <rbernon(a)codeweavers.com>
It's the third time someone tries to do this. We probably want to add a comment here :) See https://www.winehq.org/pipermail/wine-devel/2019-May/146432.html On 5/18/21 8:51 PM, Rémi Bernon wrote:
It seems to consistently succeed now, although the failure origin is probably still there.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 7eaa2c67945..13e085dfacc 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -5405,7 +5405,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)
On 5/18/21 3:07 PM, Zhiyi Zhang wrote:
It's the third time someone tries to do this. We probably want to add a comment here :) See https://www.winehq.org/pipermail/wine-devel/2019-May/146432.html
Yeah, and I believe it's the same old issue that happens elsewhere, and which I'm actually trying to fix, with X11 focus events being sometimes delayed and temporarily override Wine internal focus changes. Either way with the current code, I think it's going to fail from time to time or depending on the WM. It seems to pass all the time now on the testbot so I just wanted to get rid of the spurious todo_wine failure. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 5/18/21 3:22 PM, Rémi Bernon wrote:
On 5/18/21 3:07 PM, Zhiyi Zhang wrote:
It's the third time someone tries to do this. We probably want to add a comment here :) See https://www.winehq.org/pipermail/wine-devel/2019-May/146432.html
Yeah, and I believe it's the same old issue that happens elsewhere, and which I'm actually trying to fix, with X11 focus events being sometimes delayed and temporarily override Wine internal focus changes.
Either way with the current code, I think it's going to fail from time to time or depending on the WM. It seems to pass all the time now on the testbot so I just wanted to get rid of the spurious todo_wine failure.
In any case it's unrelated to the other patches so it can just be ignored. -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (3)
-
Marvin -
Rémi Bernon -
Zhiyi Zhang