[PATCH weston] libweston-desktop/xwayland: Make sure racy surfaces are properly mapped

[Pekka Paalanen]

On Mon, 15 May 2017 15:29:29 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> From: Quentin Glidic <sardemff7+git at sardemff7.net>
> 
> Signed-off-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> ---
>  libweston-desktop/xwayland.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/libweston-desktop/xwayland.c b/libweston-desktop/xwayland.c
> index 4f4b453f..4dcb5f03 100644
> --- a/libweston-desktop/xwayland.c
> +++ b/libweston-desktop/xwayland.c
> @@ -61,6 +61,7 @@ struct weston_desktop_xwayland_surface {
>  	const struct weston_xwayland_client_interface *client_interface;
>  	struct weston_geometry next_geometry;
>  	bool has_next_geometry;
> +	bool committed;
>  	bool added;
>  	enum weston_desktop_xwayland_surface_state state;
>  };
> @@ -99,6 +100,14 @@ weston_desktop_xwayland_surface_change_state(struct weston_desktop_xwayland_surf
>  			weston_desktop_api_surface_added(surface->desktop,
>  							 surface->surface);
>  			surface->added = true;
> +			if (surface->state == NONE && surface->committed)
> +				/* We had a race, and wl_surface.commit() was
> +				 * faster, just fake a commit to map the
> +				 * surface */
> +				weston_desktop_api_committed(surface->desktop,
> +							     surface->surface,
> +							     0, 0);
> +
>  		} else if (surface->added) {
>  			weston_desktop_api_surface_removed(surface->desktop,
>  							   surface->surface);
> @@ -134,6 +143,7 @@ weston_desktop_xwayland_surface_committed(struct weston_desktop_surface *dsurfac
>  	struct weston_geometry oldgeom;
>  
>  	assert(dsurface == surface->surface);
> +	surface->committed = true;
>  
>  #ifdef WM_DEBUG
>  	weston_log("%s: xwayland surface %p\n", __func__, surface);

Hi Quentin,

thanks for making the patch so fast! It clarified my thoughts.

I wonder, could it become a problem to never unset 'committed'? In case
something causes XWM or Xwayland to "unmap" the surface and later map
again.

We'd want to check for "has content", and checking for a buffer is not
always right as the renderer may have made a copy and released the
buffer, so... weston_surface width and height > 0?

Calling weston_desktop_api_commit() sounds like the right solution.

Is surface->state reset back to NONE when the surface is unmapped? What
about !weston_surface_is_mapped()?


I feel I should explain more background for the bug we are trying to
fix here, as I have no reliable way to reproduce. Hopefully that
inspires a commit message. ;-)

The question is about a fundamental race between the Wayland and X11
protocol streams for XWM. Wayland carries the surface content, which is
latched in with wl_surface.commit that leads to calling into the shell
plugin to map the window. X11 carries the window management
information, essentially the shell role for the window/surface.
Obviously we cannot actually map a window until it has both content and
role.

So the race is between setting the content and setting the role. IIRC
e.g. xdg_shell simply forbids this race, requiring role to be set
first. Unfortunately with Xwayland I don't think we have the luxury of
that. There is the _XWAYLAND_ALLOW_COMMITS feature which could mitigate
the problem, but it is not yet in any xorg-xserver release, and we need
to be able to do without it.

The actual bug is, that if content is set first, setting the role does
not complete mapping the surface. As the surface is not mapped, the frame
callbacks are not sent, and Xwayland will never commit again. The
window is lost in an unmapped limbo.

At least, I believe we have that bug. Actually I am helping to fix this
bug on Weston 1.11 which is before the introduction of
libweston-desktop. I have had reports of this issue on 1.11, and when I
looked at the code in master, I believe the problem is still there, but
have not been able to reproduce it. Doesn't help that I'm generally
running with _XWAYLAND_ALLOW_COMMITS support too.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170515/e0464845/attachment.sig>