[PATCH 0/1] MR7306: winex11.drv: Don't call NtUserGetWindowLongW() from X11DRV_GetWindowStateUpdates() with win_data_mutex held.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winex11.drv/window.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index b033960fc26..401749c69e9 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1524,9 +1524,9 @@ static void unmap_window( HWND hwnd ) release_win_data( data ); } -static UINT window_update_client_state( struct x11drv_win_data *data ) +static UINT window_update_client_state( UINT old_style, struct x11drv_win_data *data ) { - UINT old_style = NtUserGetWindowLongW( data->hwnd, GWL_STYLE ), new_style; + UINT new_style; if (!data->managed) return 0; /* unmanaged windows are managed by the Win32 side */ if (data->desired_state.wm_state == WithdrawnState) return 0; /* ignore state changes on invisible windows */ @@ -1569,11 +1569,11 @@ static UINT window_update_client_state( struct x11drv_win_data *data ) return 0; } -static UINT window_update_client_config( struct x11drv_win_data *data ) +static UINT window_update_client_config( UINT old_style, struct x11drv_win_data *data ) { static const UINT fullscreen_mask = (1 << NET_WM_STATE_MAXIMIZED) | (1 << NET_WM_STATE_FULLSCREEN); - UINT old_style = NtUserGetWindowLongW( data->hwnd, GWL_STYLE ), flags; RECT rect, old_rect = data->rects.window, new_rect; + UINT flags; if (!data->managed) return 0; /* unmanaged windows are managed by the Win32 side */ if (data->desired_state.wm_state != NormalState) return 0; /* ignore config changes on invisible/minimized windows */ @@ -1621,12 +1621,13 @@ static UINT window_update_client_config( struct x11drv_win_data *data ) */ BOOL X11DRV_GetWindowStateUpdates( HWND hwnd, UINT *state_cmd, UINT *config_cmd, RECT *rect ) { + UINT old_style = NtUserGetWindowLongW( hwnd, GWL_STYLE ); struct x11drv_win_data *data; if (!(data = get_win_data( hwnd ))) return FALSE; - *state_cmd = window_update_client_state( data ); - *config_cmd = window_update_client_config( data ); + *state_cmd = window_update_client_state( old_style, data ); + *config_cmd = window_update_client_config( old_style, data ); *rect = window_rect_from_visible( &data->rects, data->current_state.rect ); release_win_data( data ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7306
NtUserGetWindowLongW() gets win32u user_lock (for getting window data). I am only reproducing the real deadlock with some tentative patches and not sure if that can happen without those. Yet it seems to me it is best to avoid nested locking between winex11's win data mutex and win32u's user lock, at least with winex11 locks taken first. In this place nesting the locks looks easily avoidable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94220
I don't think win32u should call into the drivers with the user lock held. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94238
One place I know of where that currently happens is from win32u.update_surface_region (which holds win_ptr throughout the function) -> window_surface_set_clip() -> surface->funcs->set_clip(). So we should probably be fixing that one instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94266
Possible, though I rather meant the user_driver functions shouldn't be called with user_lock. The window surface calls might be different, because I suspect it's more likely to be called while holding a win32u win ptr, but then the rule would rather be the other way around, with window surface functions should perhaps not call into win32u. I'm not sure it's as well respected there though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94282
Window surface functions maybe don't call into win32u, what I tried to do is to use x11 window data from surface functions (in particular, set_clip), that is enough to randomly trigger a deadlock (because of the path concerned in this MR which takes those locks in a different order). Not sure, maybe it is a moot point until a code which is doing that submitted. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94284
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7306
Closing as this isn't the right way, win32u simply shouldn't call into the user driver with user_lock held, it breaks many assumptions a bit everywhere. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_97081
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon