[PATCH] user32: During destroy, don't send WM_ERASEBKGND to parent window if WS_CHILD is not set

[Andrew Eikum]

Prior to this change, Wine would hang during the DestroyWindow(child)
call because it is waiting for the parent window thread to process a
WM_ERASEBKGND message.

Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com>
---
 dlls/user32/tests/msg.c | 130 ++++++++++++++++++++++++++++++++++++++++
 dlls/user32/winpos.c    |   6 +-
 2 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c
index 9867e319d58..a3d30dd005b 100644
--- a/dlls/user32/tests/msg.c
+++ b/dlls/user32/tests/msg.c
@@ -11374,6 +11374,20 @@ static const struct message destroy_window_with_children[] = {
     { 0 }
 };
 
+static const struct message destroy_child_window_with_ws_child[] = {
+    { HCBT_DESTROYWND, hook },
+    { WM_PARENTNOTIFY, sent },
+    { WM_ERASEBKGND, sent },
+    { 0 }
+};
+
+static const struct message destroy_child_window_no_ws_child[] = {
+    { HCBT_DESTROYWND, hook },
+    { WM_PAINT, sent },
+    { WM_ERASEBKGND, sent|beginpaint },
+    { 0 }
+};
+
 static void test_DestroyWindow(void)
 {
     BOOL ret;
@@ -11478,6 +11492,121 @@ static void test_DestroyWindow(void)
     ok(!test, "wrong capture window %p\n", test);
 }
 
+struct destroy_child_window_test_data {
+    HANDLE evt;
+    HWND parent;
+    BOOL stop_processing;
+    BOOL quit;
+};
+
+static DWORD WINAPI destroy_child_window_parent_threadproc(void *user)
+{
+    struct destroy_child_window_test_data *data = user;
+
+    data->parent = CreateWindowA("TestWindowClass", "test parent",
+            WS_VISIBLE | WS_OVERLAPPEDWINDOW, 100, 200, 300, 400,
+            NULL, NULL, NULL, NULL);
+
+    SetEvent(data->evt);
+
+    while(!data->stop_processing)
+        flush_events();
+
+    SetEvent(data->evt);
+
+    while(data->stop_processing)
+        Sleep(10);
+
+    flush_events();
+
+    SetEvent(data->evt);
+
+    while(!data->quit)
+        Sleep(10);
+
+    DestroyWindow(data->parent);
+
+    flush_events();
+
+    return 0;
+}
+
+static void test_destroy_child_window(void)
+{
+    struct destroy_child_window_test_data data = {0};
+    HWND child;
+    HANDLE parent_thread;
+
+    /* create parent on this thread to demonstrate WM_ERASEBKGND message */
+    data.parent = CreateWindowA("TestWindowClass", "test parent",
+            WS_VISIBLE | WS_OVERLAPPEDWINDOW, 100, 200, 300, 400,
+            NULL, NULL, NULL, NULL);
+    ok(data.parent != NULL, "parent window create failed\n");
+
+    child = CreateWindowA("SimpleWindowClass", "test child",
+            WS_OVERLAPPEDWINDOW | WS_CLIPSIBLINGS | WS_CLIPCHILDREN,
+            100, 200, 50, 50, NULL, NULL, NULL, NULL);
+    ok(child != NULL, "child window create failed\n");
+
+    SetWindowLongPtrW(child, GWL_STYLE, GetWindowLongPtrW(child, GWL_STYLE) | WS_CHILD);
+    SetParent(child, data.parent);
+
+    ShowWindow(child, SW_SHOW);
+    ShowWindow(data.parent, SW_SHOW);
+
+    flush_events();
+    flush_sequence();
+
+    DestroyWindow(child);
+
+    ok_sequence(destroy_child_window_with_ws_child, "destroy child with WS_CHILD", FALSE);
+
+    DestroyWindow(data.parent);
+
+    /* create parent on separate thread to demonstrate avoiding deadlock on WM_ERASEBKGND message */
+    data.evt = CreateEventW(NULL, FALSE, FALSE, NULL);
+
+    parent_thread = CreateThread(NULL, 0, destroy_child_window_parent_threadproc, &data, 0, NULL);
+
+    WaitForSingleObject(data.evt, INFINITE);
+    ok(data.parent != NULL, "parent window create failed\n");
+
+    child = CreateWindowA("SimpleWindowClass", "test child",
+            WS_OVERLAPPEDWINDOW | WS_CLIPSIBLINGS | WS_CLIPCHILDREN,
+            100, 200, 50, 50, NULL, NULL, NULL, NULL);
+    ok(child != NULL, "child window create failed\n");
+
+    SetWindowLongPtrW(child, GWL_STYLE, GetWindowLongPtrW(child, GWL_STYLE) | WS_CHILD);
+    SetParent(child, data.parent);
+
+    ShowWindow(child, SW_SHOW);
+    ShowWindow(data.parent, SW_SHOW);
+
+    /* removing WS_CHILD avoids deadlock on WM_PARENTNOTIFY */
+    SetWindowLongPtrW(child, GWL_STYLE, GetWindowLongPtrW(child, GWL_STYLE) & ~WS_CHILD);
+
+    data.stop_processing = TRUE;
+    WaitForSingleObject(data.evt, INFINITE);
+
+    flush_events();
+    flush_sequence();
+
+    DestroyWindow(child);
+
+    data.stop_processing = FALSE;
+    WaitForSingleObject(data.evt, INFINITE);
+
+    ok_sequence(destroy_child_window_no_ws_child, "destroy child without WS_CHILD", FALSE);
+
+    data.quit = TRUE;
+    WaitForSingleObject(parent_thread, INFINITE);
+
+    flush_events();
+    flush_sequence();
+
+    CloseHandle(data.evt);
+    CloseHandle(parent_thread);
+}
 
 static const struct message WmDispatchPaint[] = {
     { WM_NCPAINT, sent },
@@ -18333,6 +18462,7 @@ START_TEST(msg)
         test_recursive_hook();
     }
     test_DestroyWindow();
+    test_destroy_child_window();
     test_DispatchMessage();
     test_SendMessageTimeout();
     test_edit_messages();
diff --git a/dlls/user32/winpos.c b/dlls/user32/winpos.c
index 6e96a4b5964..d047d714e79 100644
--- a/dlls/user32/winpos.c
+++ b/dlls/user32/winpos.c
@@ -2367,8 +2367,10 @@ BOOL USER_SetWindowPos( WINDOWPOS * winpos, int parent_x, int parent_y )
           (winpos->flags & SWP_AGG_STATUSFLAGS) != SWP_AGG_NOGEOMETRYCHANGE))
         {
             HWND parent = GetAncestor( winpos->hwnd, GA_PARENT );
-            if (!parent || parent == GetDesktopWindow()) parent = winpos->hwnd;
-            erase_now( parent, 0 );
+            if (!parent || parent == GetDesktopWindow())
+                erase_now( winpos->hwnd, 0 );
+            else if((GetWindowLongW(winpos->hwnd, GWL_STYLE) & WS_CHILD) == WS_CHILD)
+                erase_now( parent, 0 );
         }
 
         /* Give newly shown windows a chance to redraw */
-- 
2.32.0

[Zebediah Figura (she/her)]

Hello Andrew,

I don't think this is the right fix. Based on my tests, and bug 51140 
[1], I don't think we should be sending WM_ERASEBKGND even if the window 
does have WM_PARENT. E.g. the attached patch on top of your tests works 
on my Windows machine, but still fails on Wine.

When I tested it earlier I was unable to find a set of conditions where 
Windows does send WM_ERASEBKGND to the parent of a hidden window, but I 
didn't want to just remove that code without re-debugging bug 6006 [2].

ἔρρωσο,
Zebediah

[1] https://bugs.winehq.org/show_bug.cgi?id=51140

[2] https://bugs.winehq.org/show_bug.cgi?id=6006

[Zebediah Figura (she/her)]

On 7/15/21 5:56 PM, Zebediah Figura (she/her) wrote:
> Hello Andrew,
>
> I don't think this is the right fix. Based on my tests, and bug 51140
> [1], I don't think we should be sending WM_ERASEBKGND even if the window
> does have WM_PARENT. E.g. the attached patch on top of your tests works
> on my Windows machine, but still fails on Wine.
>
> When I tested it earlier I was unable to find a set of conditions where
> Windows does send WM_ERASEBKGND to the parent of a hidden window, but I
> didn't want to just remove that code without re-debugging bug 6006 [2].

It's also probably worth pointing out that the subject message and tests 
for this patch are misleading—the failure path doesn't really have 
anything to do with window destruction; it just happens that 
DestroyWindow() implicitly hides the window. Hence the SW_HIDE call in 
the attached patch.
> ἔρρωσο,
> Zebediah
>
> [1] https://bugs.winehq.org/show_bug.cgi?id=51140
>
> [2] https://bugs.winehq.org/show_bug.cgi?id=6006

[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=94125

Your paranoid android.


=== build (build log) ===

error: patch failed: dlls/user32/tests/msg.c:11571
Task: Patch failed to apply

=== debiant2 (build log) ===

error: patch failed: dlls/user32/tests/msg.c:11571
Task: Patch failed to apply

=== debiant2 (build log) ===

error: patch failed: dlls/user32/tests/msg.c:11571
Task: Patch failed to apply