Signed-off-by: Paul Gofman pgofman@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 ); }
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
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.
-
Marvin -
Paul Gofman