[PATCH] user32: Reimplement destroy_thread_windows() on top of WIN_DestroyWindow().

[Paul Gofman]

Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com>
---
    Supersedes 217103.

    USER_Driver->pThreadDetach() destroys thread data part of which
    (e. g., display) is still present in window data in winex11.drv
    and accessible through hwnd. That causes all sort of hangs
    and crashes when, for instance, the windows is still used for Vulkan
    rendering (even if only to tear down the device and swapchain).
    Besides, the windows destroyed in destroy_thread_windows() are
    still considered valid in the server until thread terminates and,
    e. g., calling IsWindow() on already destroyed window will still
    show that as valid between destroy_thread_windows() and thread
    termination. Then, it looks like a few objects referenced in WND
    structure are getting leaked.

    It looks like WIN_DestroyWindow() does everything which is needed.
    The message(s) it may send to the window being destroyed do not matter  
    as the messages to the thread being destroyed are filtered out anyway.

 dlls/user32/user_main.c |  4 +++-
 dlls/user32/win.c       | 51 +++++++++--------------------------------
 2 files changed, 14 insertions(+), 41 deletions(-)

diff --git a/dlls/user32/user_main.c b/dlls/user32/user_main.c
index 30e5d154d3d..f4b0f110dc2 100644
--- a/dlls/user32/user_main.c
+++ b/dlls/user32/user_main.c
@@ -358,9 +358,11 @@ static void thread_detach(void)
     exiting_thread_id = GetCurrentThreadId();
 
     WDML_NotifyThreadDetach();
-    USER_Driver->pThreadDetach();
 
     destroy_thread_windows();
+
+    USER_Driver->pThreadDetach();
+
     CloseHandle( thread_info->server_queue );
     HeapFree( GetProcessHeap(), 0, thread_info->wmchar_data );
     HeapFree( GetProcessHeap(), 0, thread_info->key_state );
diff --git a/dlls/user32/win.c b/dlls/user32/win.c
index 5e89f4c2c97..43207484790 100644
--- a/dlls/user32/win.c
+++ b/dlls/user32/win.c
@@ -1181,24 +1181,25 @@ LRESULT WIN_DestroyWindow( HWND hwnd )
 /***********************************************************************
  *           next_thread_window
  */
-static WND *next_thread_window( HWND *hwnd )
+static HWND first_thread_window(void)
 {
     struct user_object *ptr;
+    HWND ret = NULL;
+    WORD index;
     WND *win;
-    WORD index = *hwnd ? USER_HANDLE_TO_INDEX( *hwnd ) + 1 : 0;
 
     USER_Lock();
-    while (index < NB_USER_HANDLES)
+    for (index = 0; index < NB_USER_HANDLES; ++index)
     {
-        if (!(ptr = user_handles[index++])) continue;
+        if (!(ptr = user_handles[index])) continue;
         if (ptr->type != USER_WINDOW) continue;
         win = (WND *)ptr;
         if (win->tid != GetCurrentThreadId()) continue;
-        *hwnd = ptr->handle;
-        return win;
+        ret = ptr->handle;
+        break;
     }
     USER_Unlock();
-    return NULL;
+    return ret;
 }
 
 
@@ -1209,40 +1210,10 @@ static WND *next_thread_window( HWND *hwnd )
  */
 void destroy_thread_windows(void)
 {
-    WND *wndPtr;
-    HWND hwnd = 0, *list;
-    HMENU menu, sys_menu;
-    struct window_surface *surface;
-    int i;
-
-    while ((wndPtr = next_thread_window( &hwnd )))
-    {
-        /* destroy the client-side storage */
+    HWND hwnd;
 
-        list = WIN_ListChildren( hwnd );
-        menu = ((wndPtr->dwStyle & (WS_CHILD | WS_POPUP)) != WS_CHILD) ? (HMENU)wndPtr->wIDmenu : 0;
-        sys_menu = wndPtr->hSysMenu;
-        free_dce( wndPtr->dce, hwnd );
-        surface = wndPtr->surface;
-        InterlockedCompareExchangePointer( &user_handles[USER_HANDLE_TO_INDEX(hwnd)], NULL, wndPtr );
-        WIN_ReleasePtr( wndPtr );
-        HeapFree( GetProcessHeap(), 0, wndPtr );
-        if (menu) DestroyMenu( menu );
-        if (sys_menu) DestroyMenu( sys_menu );
-        if (surface)
-        {
-            register_window_surface( surface, NULL );
-            window_surface_release( surface );
-        }
-
-        /* free child windows */
-
-        if (!list) continue;
-        for (i = 0; list[i]; i++)
-            if (!WIN_IsCurrentThread( list[i] ))
-                SendNotifyMessageW( list[i], WM_WINE_DESTROYWINDOW, 0, 0 );
-        HeapFree( GetProcessHeap(), 0, list );
-    }
+    while ((hwnd = first_thread_window()))
+        WIN_DestroyWindow( hwnd );
 }
 
 
-- 
2.31.1

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

Your paranoid android.


=== debiant2 (32 bit WoW report) ===

user32:
msg.c:9208: Test failed: WaitForSingleObject failed 102
msg.c:9214: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead
msg.c:9214: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead
msg.c:9214: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000

[Paul Gofman]

On 10/13/21 20:08, 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=99988
>
> Your paranoid android.
>
> === debiant2 (32 bit WoW report) ===
>
> user32:
> msg.c:9208: Test failed: WaitForSingleObject failed 102
> msg.c:9214: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead
> msg.c:9214: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead
> msg.c:9214: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000

It looks like these test failures (which are in fact random) are not a 
coincidence and are triggered by the patch, although the core issue is 
probably the preexisting race in WIN_DestroyWindow(): 
WM_WINE_DESTROYWINDOW sent to other thread child currently should be 
processed before WIN_DestroyWindow() reaches server with destroy_window 
for the parent. Otherwise, during destroy_window() in the server the 
server part of child windows is destroyed and the messages to them get 
cleaned up from the queue. So the grandchild thread in test waiting for 
the message for its grandchild window never gets it. I am looking into 
how that can be solved.