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

[Pekka Paalanen]

On Fri, 21 Jul 2017 18:43:41 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> On 5/15/17 4:40 PM, Pekka Paalanen wrote:
> > 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.  
> 
> Actually, the concepts involved don’t match the code perfectly.
> In libweston-desktop, the commit does *not* depend on content. It just 
> happens that Xwayland commit for content only (since the rest is done 
> via XWM). The compositor/shell has to deal with NULL-buffer surfaces 
> anyway, since wl_shell mandates that.
> 
> Mapping the surface needs this sequence:
> 1. libweston-desktop surface_added callback
> 2. libweston-desktop committed callback
> 
> Unmapping a surface, if I understand XWM enough, will destroy the 
> wl_surface and reset the desktop surface to NULL. That means creating a 
> new weston_desktop_surface on remap.
> 
> So the race can really only happen the first time the X window is 
> created and committed.
> No code leads to reset the weston_desktop_surface to NONE, it has to be 
> a new one.
> 
> So, AFAICT, this patch will just work™.
> 
> Most of libweston-desktop xwayland code was copied from shell.c, though 
> a bit modified since then, so it may be easy to do exactly the same.

Ok, yes. I only recently learnt that unmapping an X11 Window will cause
unrealize to be called in Xwyland, which destroys the wl_surface. So I
think you are right.

I will trust my old self and you, and give you an R-b after you have
written a commit message for this patch. ;-)


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/20170818/aa099a2d/attachment.sig>