Re: [PATCH v10 0/6] MR6569: winex11: Refactor window state/config tracker to fix race conditions.

[Robbert van der Helm (＠robbert-vdh)]

On Sun Dec  1 20:18:56 2024 +0000, Rémi Bernon wrote:
> Hi, I don't think this checks does what you describe.
> This specific `if` is there for *unmanaged* windows, which are X
> override-redirect windows which are fully under Wine control, and which
> won't receive WM_STATE property changes. We clear the `wm_state_serial`
> for them, in order to allow sending more window state changes without
> waiting, as well as window configure requests. That if doesn't set
> wm_state_serial, it clears it, and it shouldn't prevent the
> `ConfigureNotify` to be processed, rather the other way around.
> For *managed* windows, when Wine requests a window state change, the
> Window Manager is supposed to change the `WM_STATE` property accordingly
> once it is done, and we should then receive the corresponding
> `PropertyNotify` event. We still process `ConfigureNotify` events, but
> we don't apply them to the Win32 state until all the requested changes
> have been processed.
> If the `WM_STATE` property isn't modified or isn't present, then yes the
> Win32 state will appear to be stuck forever. This is however a bug in
> the window manager, according to the ICCCM, and we can really only
> support ICCCM-compatible window manager (or just uncheck "allow the
> window manager to manage the windows" in `winecfg`).
> In any case, `ConfigureNotify` events are always processed, and applied
> to the Win32 state when there's no reason to wait. XEMBED support may be
> what makes a difference here, as we used only for systray embedded
> windows. I don't think any other use case was very intended before or
> well supported, and there's assumptions being made that embedded windows
> are always systray windows
> It's probably possible to support it better, and maybe even easier to do
> so now that we track X parent and embedder windows without leaking them
> into the Win32 space anymore. If there's a regression, please open a bug
> on bugs.winehq.org, describe your use case with reproducible steps, and
> we will try to fix it.

Thanks for the detailed write up! I only ran into this commit after bisecting the changes between Wine 9.21 and 9.22, so I'm almost certainly missing some context!

The problem I'm facing seems to be that as soon as `SWP_STATECHANGED` is set and `window_set_wm_state()` gets called, `data->wm_state_serial` gets stuck on a nonzero value since it's set to the X11 connection's next sequence number and is then never cleared again. That causes `window_update_client_config()` to always bail before it does anything, which in turn causes the `NtUserPostMessage( hwnd, WM_WINE_WINDOW_STATE_CHANGED, 0, 0 )` called from `X11DRV_ConfigureNotify()` to not do anything anymore.

This doesn't seem to cause any problems under normal use cases, since normally once a window is mapped, its coordinates relative to the root window will never suddenly change (not sure if there's a spicy interaction with multiple monitors though). But in yabridge I have to reparent a Wine window to another window, and to make that work I've always had to send these `ConfigureNotify` events to inform Wine about the window's actual position on screen. Otherwise it thinks it's located in the top left corner of the screen, and all mouse events will be at an offset.

I can open a regression bug report on bugs.winehq.org, but since what yabridge is doing is almost certainly not the ended way to interact with Wine, I thought I'd check here first.