On Tue, 11 Feb 2014 16:34:13 +0100 Rui Matos <tiagoma...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel