On 5/15/17 4:40 PM, Pekka Paalanen wrote:
On Mon, 15 May 2017 15:29:29 +0200
Quentin Glidic <sardemff7+wayl...@sardemff7.net> wrote:
From: Quentin Glidic <sardemff7+...@sardemff7.net>
Signed-off-by: Quentin Glidic <sardemff7+...@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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
