On Fri, 21 Jul 2017 18:43:41 +0200 Quentin Glidic <[email protected]> wrote:
> On 5/15/17 4:40 PM, Pekka Paalanen wrote: > > On Mon, 15 May 2017 15:29:29 +0200 > > Quentin Glidic <[email protected]> wrote: > > > >> From: Quentin Glidic <[email protected]> > >> > >> Signed-off-by: Quentin Glidic <[email protected]> > >> --- > >> 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
pgp1ZPXb10X5V.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
