[PATCH] user32: Call driver's DestroyWindow in destroy_thread_windows().
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). Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/user32/user_main.c | 4 +++- dlls/user32/win.c | 3 ++- 2 files changed, 5 insertions(+), 2 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..bafd0a3a7f0 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -1218,7 +1218,6 @@ void destroy_thread_windows(void) while ((wndPtr = next_thread_window( &hwnd ))) { /* destroy the client-side storage */ - list = WIN_ListChildren( hwnd ); menu = ((wndPtr->dwStyle & (WS_CHILD | WS_POPUP)) != WS_CHILD) ? (HMENU)wndPtr->wIDmenu : 0; sys_menu = wndPtr->hSysMenu; @@ -1235,6 +1234,8 @@ void destroy_thread_windows(void) window_surface_release( surface ); } + USER_Driver->pDestroyWindow( hwnd ); + /* free child windows */ if (!list) continue; -- 2.31.1
On 10/13/21 12:39 PM, Paul Gofman wrote:
index 5e89f4c2c97..bafd0a3a7f0 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -1218,7 +1218,6 @@ void destroy_thread_windows(void) while ((wndPtr = next_thread_window( &hwnd ))) { /* destroy the client-side storage */ - list = WIN_ListChildren( hwnd ); menu = ((wndPtr->dwStyle & (WS_CHILD | WS_POPUP)) != WS_CHILD) ? (HMENU)wndPtr->wIDmenu : 0; sys_menu = wndPtr->hSysMenu; @@ -1235,6 +1234,8 @@ void destroy_thread_windows(void) window_surface_release( surface ); }
+ USER_Driver->pDestroyWindow( hwnd ); + /* free child windows */
if (!list) continue;
One thing I wondered was whether we should call the driver only after releasing the child windows, and what would happen if these child windows are owned by a different thread. If the windows are owned by the current thread, we'll maybe destroy the child window driver data (ie: client windows I think) after the parent. I'm not sure if it's a problem, but WIN_DestroyWindow does it the other way around. In the other case I believe there's already a pre-existing race condition (and I have a test exhibiting it), so maybe we don't need to worry too much about it: We send the WM_WINE_DESTROYWINDOW to the child windows, and expect their owning thread to peek their message and destroy them. These thread may not be peeking their messages, and we may terminate the current thread before, which will make wineserver detach the child windows from their owning thread, losing the message forever. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 10/13/21 14:28, Rémi Bernon wrote:
On 10/13/21 12:39 PM, Paul Gofman wrote:
index 5e89f4c2c97..bafd0a3a7f0 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -1218,7 +1218,6 @@ void destroy_thread_windows(void) while ((wndPtr = next_thread_window( &hwnd ))) { /* destroy the client-side storage */ - list = WIN_ListChildren( hwnd ); menu = ((wndPtr->dwStyle & (WS_CHILD | WS_POPUP)) != WS_CHILD) ? (HMENU)wndPtr->wIDmenu : 0; sys_menu = wndPtr->hSysMenu; @@ -1235,6 +1234,8 @@ void destroy_thread_windows(void) window_surface_release( surface ); } + USER_Driver->pDestroyWindow( hwnd ); + /* free child windows */ if (!list) continue;
One thing I wondered was whether we should call the driver only after releasing the child windows, and what would happen if these child windows are owned by a different thread.
If the windows are owned by the current thread, we'll maybe destroy the child window driver data (ie: client windows I think) after the parent. I'm not sure if it's a problem, but WIN_DestroyWindow does it the other way around.
Well, even if that is not a problem I agree that at very least this leaves things too messy. I think I will try to redo destroy_thread_windows() so that it just uses WIN_DestroyWindow(). Not sure if WM_NCDESTROY message still needs to be send in this case, will add a test. There is also other stuff skipped in destroy_thread_windows() like wndPtr->hIconSmall2 or wndPtr->text which seem to be leaked. Also, notifying the server about window destroy (server currently destroys window only when the thread is fully terminated so even after destroy_thread_windows freed wnddata the queries about hwnd still succeed through server and report the window as alive).
In the other case I believe there's already a pre-existing race condition (and I have a test exhibiting it), so maybe we don't need to worry too much about it:
We send the WM_WINE_DESTROYWINDOW to the child windows, and expect their owning thread to peek their message and destroy them.
These thread may not be peeking their messages, and we may terminate the current thread before, which will make wineserver detach the child windows from their owning thread, losing the message forever.
I suppose this is a different issue from what I am trying to address, it is preexisting and the one concerned here happens without any race involved. So I guess that one may be addressed separately.
participants (2)
-
Paul Gofman -
Rémi Bernon