[PATCH] xwayland: Destroy wl_buffers only after they are released

[Pekka Paalanen]

On Tue, 11 Feb 2014 16:34:13 +0100
Rui Matos <tiagomatos at gmail.com> wrote:

> Destroying a wl_buffer that is still attached to a wl_surface is
> undefined behavior according to the wayland protocol. We should delay
> the destruction until we get the release event.
> ---
> 
> So, I'm not sure why there was this comment saying that it was safe to
> do this, perhaps it was in an old protocol version?
> 
> In any case, this has been making xwayland crash under mutter ever
> since this mutter commit
> https://git.gnome.org/browse/mutter/commit/?h=wayland&id=3e98ffaf9958366b584b360ac12bbc03cd070c07 .
> 
>  hw/xfree86/xwayland/xwayland-window.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xfree86/xwayland/xwayland-window.c b/hw/xfree86/xwayland/xwayland-window.c
> index a2a8206..a005cc6 100644
> --- a/hw/xfree86/xwayland/xwayland-window.c
> +++ b/hw/xfree86/xwayland/xwayland-window.c
> @@ -43,6 +43,16 @@
>  static DevPrivateKeyRec xwl_window_private_key;
>  
>  static void
> +free_buffer(void *data, struct wl_buffer *buffer)
> +{
> +    wl_buffer_destroy(buffer);
> +}
> +
> +static const struct wl_buffer_listener buffer_listener = {
> +    free_buffer,

The name of the function should say it's the wl_buffer.release handler.

> +};
> +
> +static void
>  free_pixmap(void *data, struct wl_callback *callback, uint32_t time)
>  {
>      PixmapPtr pixmap = data;
> @@ -62,10 +72,8 @@ xwl_window_attach(struct xwl_window *xwl_window, PixmapPtr pixmap)
>      struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>      struct wl_callback *callback;
>  
> -    /* We can safely destroy the buffer because we only use one buffer
> -     * per surface in xwayland model */
>      if (xwl_window->buffer)
> -        wl_buffer_destroy(xwl_window->buffer);
> +        wl_buffer_add_listener(xwl_window->buffer, &buffer_listener, NULL);
>  
>      xwl_screen->driver->create_window_buffer(xwl_window, pixmap);
>  
> @@ -185,7 +193,7 @@ xwl_unrealize_window(WindowPtr window)
>  	return ret;
>  
>      if (xwl_window->buffer)
> -	wl_buffer_destroy(xwl_window->buffer);
> +        wl_buffer_add_listener(xwl_window->buffer, &buffer_listener, NULL);
>      wl_surface_destroy(xwl_window->surface);
>      xorg_list_del(&xwl_window->link);
>      if (RegionNotEmpty(DamageRegion(xwl_window->damage)))

I assume the code never added a wl_buffer listener before, because if
it did, this patch would be a no-op. "wl_buffer_add_listener" is a
misnomer, there can only ever be one listener, and trying to "add"
another will not actually do anything.

Also, you rely on wl_buffer.release not having arrived before you add
the listener. With weston's gl-renderer, the release comes very soon
after each wl_surface.commit for wl_shm buffers. Maybe it works, maybe
it doesn't, but it seems very fragile. Did you check you don't leak
wl_buffers now?


Thanks,
pq