GNOME Bugzilla – Bug 750680
Crash when destroying data offer
Last modified: 2015-06-30 03:25:16 UTC
Created attachment 304915 [details] Full backtrace of gnome-shell. mutter crashes occacionally when a data offer is destroyed. I recently saw it when about to unlock the screen. It seems to have occured in FAF as well: https://retrace.fedoraproject.org/faf/problems/934847/ which has the same back trace I saw in my core file.
Created attachment 305512 [details] [review] wayland: Make MetaWaylandDataSource ownership protocol specific Firstly, this patch makes MetawaylandDataSource a GObject. This is in order to easier track its lifetime without adding destroy signals etc. It also makes the vfunc table GObject class functions instead while at it, as well as moves protocol specific part of the source into their own implementations. An important part of this patch is the change of ownership. Prior to this patch, MetaWaylandDataDevice would kind of own the source, but for Wayland sources it would remove it if the corresponding wl_resource was destroyed. For XWayland clients it would own it completely, and only remove it if the source was replaced. This patch changes so that the protocol implementation owns the source. For Wayland sources, the wl_resource owns the source, and the MetaWaylandDataDevice sets a weak reference (so in other words, no semantical changes really). For XWayland sources, the source is owned by the selection bridge, and not removed until replaced or if the client goes away. Given the changes in ownership, data offers may now properly track the lifetime of a source it represents. Prior to this patch, if a offer with an XWayland source would loose its source, it wouldn't get notified and have an invalid pointer it would potentally crash on. For Wayland sources, an offer would have a weak reference and clean itself up if the source went away. This patch changes so the behavior is consistent, meaning a weak reference is added to the source GObject so that the offer can behave correctly both for Wayland sources and XWayland sources.
Review of attachment 305512 [details] [review]: Looks like a nice cleanup. Thanks. Some minor nits, but overall a good approach. ::: src/wayland/meta-wayland-data-device.c @@ +44,3 @@ }; +#define META_TYPE_WAYLAND_DATA_SOURCE_WAYLAND (meta_wayland_data_source_wayland_get_type ()) Please put these in a header. @@ +152,3 @@ + if (offer->source) + g_object_remove_weak_pointer (G_OBJECT (offer->source), + (gpointer *)&offer->source); Do you really need this cast? ::: src/wayland/meta-wayland-data-device.h @@ +45,3 @@ }; +#define META_TYPE_WAYLAND_DATA_SOURCE (meta_wayland_data_source_get_type ()) I know, I know, but we have a thing for this now: G_DECLARE_FINAL_TYPE. I should probably go through at some point and clean up mutter so it uses these now. ::: src/wayland/meta-xwayland-selection.c @@ +38,3 @@ #include "meta-wayland-data-device.h" +#define META_TYPE_WAYLAND_DATA_SOURCE_XWAYLAND (meta_wayland_data_source_xwayland_get_type ()) In a header please.
(In reply to Jasper St. Pierre from comment #2) > Review of attachment 305512 [details] [review] [review]: > > Looks like a nice cleanup. Thanks. Some minor nits, but overall a good > approach. > > ::: src/wayland/meta-wayland-data-device.c > @@ +44,3 @@ > }; > > +#define META_TYPE_WAYLAND_DATA_SOURCE_WAYLAND > (meta_wayland_data_source_wayland_get_type ()) > > Please put these in a header. Even if the type will never be exposed outside of this file? Did not want to expose it outside because it was not intended to be. Do you still insist on exposing it in th header, or should I add some kind of data-device-private.h and xwayland-selection-private.h thing? > > @@ +152,3 @@ > + if (offer->source) > + g_object_remove_weak_pointer (G_OBJECT (offer->source), > + (gpointer *)&offer->source); > > Do you really need this cast? Yes. Same for g_object_weak_*. > > ::: src/wayland/meta-wayland-data-device.h > @@ +45,3 @@ > }; > > +#define META_TYPE_WAYLAND_DATA_SOURCE > (meta_wayland_data_source_get_type ()) > > I know, I know, but we have a thing for this now: G_DECLARE_FINAL_TYPE. ...a type which is not (at the present time) intended to be subclassed... Should G_DECLARE_DERIVABLE_TYPE be used instead for this one, and FINAL_TYPE for the others? > > I should probably go through at some point and clean up mutter so it uses > these now. > > ::: src/wayland/meta-xwayland-selection.c > @@ +38,3 @@ > #include "meta-wayland-data-device.h" > > +#define META_TYPE_WAYLAND_DATA_SOURCE_XWAYLAND > (meta_wayland_data_source_xwayland_get_type ()) > > In a header please. Same question as above.
Created attachment 305515 [details] [review] wayland: Make MetaWaylandDataSource ownership protocol specific Firstly, this patch makes MetawaylandDataSource a GObject. This is in order to easier track its lifetime without adding destroy signals etc. It also makes the vfunc table GObject class functions instead while at it, as well as moves protocol specific part of the source into their own implementations. An important part of this patch is the change of ownership. Prior to this patch, MetaWaylandDataDevice would kind of own the source, but for Wayland sources it would remove it if the corresponding wl_resource was destroyed. For XWayland clients it would own it completely, and only remove it if the source was replaced. This patch changes so that the protocol implementation owns the source. For Wayland sources, the wl_resource owns the source, and the MetaWaylandDataDevice sets a weak reference (so in other words, no semantical changes really). For XWayland sources, the source is owned by the selection bridge, and not removed until replaced or if the client goes away. Given the changes in ownership, data offers may now properly track the lifetime of a source it represents. Prior to this patch, if a offer with an XWayland source would loose its source, it wouldn't get notified and have an invalid pointer it would potentally crash on. For Wayland sources, an offer would have a weak reference and clean itself up if the source went away. This patch changes so the behavior is consistent, meaning a weak reference is added to the source GObject so that the offer can behave correctly both for Wayland sources and XWayland sources.
Created attachment 306157 [details] [review] wayland: Make MetaWaylandDataSource ownership protocol specific Firstly, this patch makes MetawaylandDataSource a GObject. This is in order to easier track its lifetime without adding destroy signals etc. It also makes the vfunc table GObject class functions instead while at it, as well as moves protocol specific part of the source into their own implementations. An important part of this patch is the change of ownership. Prior to this patch, MetaWaylandDataDevice would kind of own the source, but for Wayland sources it would remove it if the corresponding wl_resource was destroyed. For XWayland clients it would own it completely, and only remove it if the source was replaced. This patch changes so that the protocol implementation owns the source. For Wayland sources, the wl_resource owns the source, and the MetaWaylandDataDevice sets a weak reference (so in other words, no semantical changes really). For XWayland sources, the source is owned by the selection bridge, and not removed until replaced or if the client goes away. Given the changes in ownership, data offers may now properly track the lifetime of a source it represents. Prior to this patch, if a offer with an XWayland source would loose its source, it wouldn't get notified and have an invalid pointer it would potentally crash on. For Wayland sources, an offer would have a weak reference and clean itself up if the source went away. This patch changes so the behavior is consistent, meaning a weak reference is added to the source GObject so that the offer can behave correctly both for Wayland sources and XWayland sources. ----- This new version also makes the DND data device source pointer a weak pointer.
Created attachment 306362 [details] [review] wayland: Make MetaWaylandDataSource ownership protocol specific Firstly, this patch makes MetawaylandDataSource a GObject. This is in order to easier track its lifetime without adding destroy signals etc. It also makes the vfunc table GObject class functions instead while at it, as well as moves protocol specific part of the source into their own implementations. An important part of this patch is the change of ownership. Prior to this patch, MetaWaylandDataDevice would kind of own the source, but for Wayland sources it would remove it if the corresponding wl_resource was destroyed. For XWayland clients it would own it completely, and only remove it if the source was replaced. This patch changes so that the protocol implementation owns the source. For Wayland sources, the wl_resource owns the source, and the MetaWaylandDataDevice sets a weak reference (so in other words, no semantical changes really). For XWayland sources, the source is owned by the selection bridge, and not removed until replaced or if the client goes away. Given the changes in ownership, data offers may now properly track the lifetime of a source it represents. Prior to this patch, if a offer with an XWayland source would loose its source, it wouldn't get notified and have an invalid pointer it would potentally crash on. For Wayland sources, an offer would have a weak reference and clean itself up if the source went away. This patch changes so the behavior is consistent, meaning a weak reference is added to the source GObject so that the offer can behave correctly both for Wayland sources and XWayland sources. ---- Had forgotten to git add two new -private.h files.
Review of attachment 306362 [details] [review]: ok with Makefile.am changes
Attachment 306362 [details] pushed as 5547c98 - wayland: Make MetaWaylandDataSource ownership protocol specific