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

[Quentin Glidic]

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.

Cheers,

-- 

Quentin “Sardem FF7” Glidic