On Thu Sep 26 07:05:10 2024 +0000, Alexandros Frantzis wrote:
> The core problem seems to be that in
> `wayland_win_data_update_wayland_state` `surface->role` is used to
> determine what's the *current* role for the surface. However
> `surface->role` is really tracking what role was given in the past to
> this surface and thus can be given again in the future (because surfaces
> cannot change roles).
> In this particular osu! case, we have a previously toplevel surface (so
> surface->role = toplevel) that is now role-less, but
> `wayland_surface_update_state_toplevel` is called on it, which then
> unconditionally tries to access the NULL xdg_toplevel and crashes the
> thread while holding the 'win_data_mutex` lock.
One fix is to add `if (!surface->xdg_toplevel) return;` at the beginning of `wayland_surface_update_state_toplevel()`. It depends on what we want `surface->role` to mean. If it is "possible role" (which seems to match how other parts of the code are implemented) then this should be OK.
Alternatively, perhaps a more explicit arrangement `surface->role` vs `surface->possible_role` would make things clearer?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6560#note_83411
On Thu Sep 26 06:38:50 2024 +0000, William Horvath wrote:
> It starts occurring after commit 5/6 (af65d37c Introduce
> update_wayland_surface_state_toplevel). Reverting to the previous commit
> 4/6, 35c90946 Introduce wayland_surface_reconfigure_xdg, it behaves normally.
The core problem seems to be that in `wayland_win_data_update_wayland_state` `surface->role` is used to determine what's the *current* role for the surface. However `surface->role` is really tracking what role was given in the past to this surface and thus can be given again in the future (because surfaces cannot change roles).
In this particular osu! case, we have a previously toplevel surface (so surface->role = toplevel) that is now role-less, but `wayland_surface_update_state_toplevel` is called on it, which then unconditionally tries to access the NULL xdg_toplevel and crashes the thread while holding the 'win_data_mutex` lock.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6560#note_83410
On Thu Sep 26 06:15:21 2024 +0000, Aida Jonikienė wrote:
> Can you try bisecting the individual MR commits? :frog:
It starts occurring after commit 5/6 (af65d37c Introduce update_wayland_surface_state_toplevel). Reverting to the previous commit 4/6, 35c90946 Introduce wayland_surface_reconfigure_xdg, it behaves normally.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6560#note_83409
On Thu Sep 26 06:15:21 2024 +0000, William Horvath wrote:
> Applying this MR on top of `2b32c285` (master) prevents
> [osu!](https://osu.ppy.sh/)
> ([download](https://m1.ppy.sh/r/osu!install.exe)) from closing when the
> either the ingame "Fullscreen" or "Render at native resolution" options
> are enabled (i.e. no window frame). I don't want to speculate too much,
> but it seems that an invisible window just never closes, preventing
> Wine/the game from closing until a `wineserver -k` is sent. Only native
> dotnet48 is installed in the prefix.
> [fullscreen.log (hangs)](/uploads/8f2dc854833b96f4c84796b04edcbf57/fullscreen.log)
> [nofullscreen.log (closes properly)](/uploads/7ce90e3ad7adddb4f6fc7e2efbb05543/nofullscreen.log)
> Here are logs of
> `WINEDEBUG=+pid,+tid,+timestamp,+loaddll,+win,+waylanddrv` with both
> fullscreen enabled and disabled. There may be more useful debug channels
> that I'm missing.
>
> Also, irrelevant to this regression, but I believe that the
> `GetRawInputDeviceInfoA` spam is due to a different bug somewhere in wine(bus|hid|usb).
Can you try bisecting the individual MR commits? :frog:
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6560#note_83408
Applying this MR on top of `2b32c285` (master) prevents [osu!](https://osu.ppy.sh/) ([download](https://m1.ppy.sh/r/osu!install.exe)) from closing when the either the ingame "Fullscreen" or "Render at native resolution" options are enabled (i.e. no window frame). It just hangs indefinitely, due to a deadlock I presume. Only native dotnet48 is installed in the prefix.
[fullscreen.log (hangs)](/uploads/8f2dc854833b96f4c84796b04edcbf57/fullscreen.log)
[nofullscreen.log (closes properly)](/uploads/7ce90e3ad7adddb4f6fc7e2efbb05543/nofullscreen.log)
Here are logs of `WINEDEBUG=+pid,+tid,+timestamp,+loaddll,+win,+waylanddrv` with both fullscreen enabled and disabled, but I don't know if there's a more useful channel that I'm missing.
Also, irrelevant to this regression, but the `GetRawInputDeviceInfoA` spam on the `win` channel is due to a different bug somewhere in wine(bus|hid|usb).
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6560#note_83395