[PATCH resend 1/2] user32/tests: Wait for parent thread before checking child window.

[Rémi Bernon]

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 | 61 +++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c
index 423157b623d..55b4fe2228b 100644
--- a/dlls/user32/tests/msg.c
+++ b/dlls/user32/tests/msg.c
@@ -8539,6 +8539,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;
@@ -8571,44 +8573,50 @@ 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);
-    }
+    ret = WaitForSingleObject( wnd_event->child, 5000 );
+    ok( !ret, "WaitForSingleObject returned %x, error: %u\n", ret, GetLastError() );
+
+    while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg);
+    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);
+        ret = MsgWaitForMultipleObjects(1, &wnd_event->start_event, FALSE, 5000, QS_SENDMESSAGE);
         if (ret != 1) break;
         while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg);
     }
+    ok( !ret, "MsgWaitForMultipleObjects failed %x, error: %u\n", ret, GetLastError() );
+
     ret = WaitForSingleObject( wnd_event->stop_event, 5000 );
     ok( !ret, "WaitForSingleObject failed %x\n", ret );
     return 0;
@@ -8742,33 +8750,38 @@ static void test_interthread_messages(void)
     wnd_event.hwnd = CreateWindowExA(0, "TestParentClass", "Test parent", WS_OVERLAPPEDWINDOW | WS_VISIBLE,
                               100, 100, 200, 200, 0, 0, 0, NULL);
     ok (wnd_event.hwnd != 0, "Failed to create parent window\n");
+
+    wnd_event.start_event = CreateEventA( NULL, TRUE, FALSE, NULL );
+    wnd_event.stop_event = CreateEventA( NULL, TRUE, FALSE, NULL );
+
+    wnd_event.child = CreateThread( NULL, 0, create_child_thread, &wnd_event, 0, &tid );
+    ok( wnd_event.child != 0, "CreateThread failed, error %u\n", GetLastError() );
+
     flush_events();
     flush_sequence();
     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 );
     for (;;)
     {
-        ret = MsgWaitForMultipleObjects(1, &wnd_event.start_event, FALSE, 1000, QS_SENDMESSAGE);
+        ret = MsgWaitForMultipleObjects(1, &wnd_event.start_event, FALSE, 5000, QS_SENDMESSAGE);
         if (ret != 1) break;
         while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg);
     }
     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 );
+    ok( !ret, "WaitForSingleObject failed %x, error: %u\n", ret, GetLastError() );
 
+    CloseHandle( wnd_event.child );
+    CloseHandle( wnd_event.grand_child );
     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.25.0

[PATCH resend 2/2] server: Destroy child window from another thread only if it's not running.

[Rémi Bernon]

On thread destroy, a WM_WINE_DESTROYWINDOW is sent to the child windows
living in other threads. However 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.

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/user32/tests/msg.c |  3 +--
 server/window.c         | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c
index 55b4fe2228b..3aa8c83f64d 100644
--- a/dlls/user32/tests/msg.c
+++ b/dlls/user32/tests/msg.c
@@ -8586,7 +8586,6 @@ static DWORD CALLBACK create_grand_child_thread( void *param )
     ok( !ret, "WaitForSingleObject returned %x, error: %u\n", ret, GetLastError() );
 
     while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg);
-    todo_wine
     ok( !IsWindow( hchild ), "Child window not destroyed\n" );
 
     return 0;
@@ -8781,7 +8780,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 c9b131cba5d..cd413ba1023 100644
--- a/server/window.c
+++ b/server/window.c
@@ -1888,9 +1888,21 @@ 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 != RUNNING)
+            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 != RUNNING)
+            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.25.0

[PATCH resend 2/2] server: Destroy child window from another thread only if it's not running.

[Marvin]

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=66334

Your paranoid android.


=== w8 (32 bit report) ===

user32:
msg.c:14689: Test failed: unexpected message 31f
msg.c:14690: Test failed: bad wparam 1
msg.c:14696: Test failed: unicode WM_CHAR: 0: the msg sequence is not complete: expected 0102 - actual 0000

[Marvin]

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=66333

Your paranoid android.


=== w8adm (32 bit report) ===

user32:
msg.c:17569: Test failed: wrong status 00080000

=== w1064v1809_ja (32 bit report) ===

user32:
msg.c:16303: Test failed: 16: WaitForSingleObject failed
msg.c:8769: Test failed: MsgWaitForMultipleObjects failed 102
msg: Timeout

=== w1064v1809_ja (testbot log) ===

The task timed out

=== w864 (64 bit report) ===

user32:
msg.c:17569: Test failed: thread: call SendMessage