After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762716 - wayland: Don't unset surface->buffer when wl_buffer destroyed
wayland: Don't unset surface->buffer when wl_buffer destroyed
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2016-02-26 10:14 UTC by Jonas Ådahl
Modified: 2016-03-01 05:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: Don't unset surface->buffer when wl_buffer destroyed (7.37 KB, patch)
2016-02-26 10:14 UTC, Jonas Ådahl
committed Details | Review

Description Jonas Ådahl 2016-02-26 10:14:07 UTC
I was debugging bug 762468 and discovered that after having applied the fix to gdk, if I hold down F11 causing gnome-terminal to fullscreen->unfullscreen->fullscreen->..., it would eventually disappear. This was the reason.
Comment 1 Jonas Ådahl 2016-02-26 10:14:11 UTC
Created attachment 322446 [details] [review]
wayland: Don't unset surface->buffer when wl_buffer destroyed

Don't unset the surface->buffer if the associated wl_buffer object is
destroyed. The MetaWaylandBuffer doesn't really only represent a
wl_buffer object, but also the data (texture) created from the given
wl_buffer. Thus, for example destroying a released SHM wl_buffer should
not destroy the MetaWaylandBuffer instance, because the texture may
still be used.

This commit also fixes a race where calc_showing would hide a window
because, at the time of calculation whether it should be showing, the
surface's buffer had been destroyed as described above.
Comment 2 Rui Matos 2016-02-26 16:14:43 UTC
Review of attachment 322446 [details] [review]:

Looks good, just a couple minor comments below

::: src/wayland/meta-wayland-buffer.c
@@ +181,3 @@
+  GObjectClass *object_class = G_OBJECT_CLASS (klass);
+
+  object_class->dispose = meta_wayland_buffer_dispose;

nit: could be finalize instead of dispose

::: src/wayland/meta-wayland-surface.c
@@ +438,2 @@
   state->buffer = NULL;
+  wl_list_remove (&state->buffer_destroy_listener.link);

This isn't strictly necessary, right ? It's the right thing to do, and should have been here already, but unrelated to the rest of the patch. Perhaps do it a separate commit ?
Comment 3 Jonas Ådahl 2016-02-27 03:19:36 UTC
(In reply to Rui Matos from comment #2)
> Review of attachment 322446 [details] [review] [review]:
> 
> Looks good, just a couple minor comments below
> 
> ::: src/wayland/meta-wayland-buffer.c
> @@ +181,3 @@
> +  GObjectClass *object_class = G_OBJECT_CLASS (klass);
> +
> +  object_class->dispose = meta_wayland_buffer_dispose;
> 
> nit: could be finalize instead of dispose

Sure.

> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +438,2 @@
>    state->buffer = NULL;
> +  wl_list_remove (&state->buffer_destroy_listener.link);
> 
> This isn't strictly necessary, right ? It's the right thing to do, and
> should have been here already, but unrelated to the rest of the patch.
> Perhaps do it a separate commit ?

No. I'll remove that call. I intend to get rid of the wl_signal as well so I won't bother with it now.
Comment 4 Jonas Ådahl 2016-03-01 05:35:36 UTC
Attachment 322446 [details] pushed as d340c3a - wayland: Don't unset surface->buffer when wl_buffer destroyed