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 750680 - Crash when destroying data offer
Crash when destroying data offer
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
unspecified
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on: 750007
Blocks:
 
 
Reported: 2015-06-10 04:39 UTC by Jonas Ådahl
Modified: 2015-06-30 03:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Full backtrace of gnome-shell. (7.54 KB, text/plain)
2015-06-10 04:39 UTC, Jonas Ådahl
  Details
wayland: Make MetaWaylandDataSource ownership protocol specific (27.82 KB, patch)
2015-06-18 02:44 UTC, Jonas Ådahl
none Details | Review
wayland: Make MetaWaylandDataSource ownership protocol specific (34.10 KB, patch)
2015-06-18 04:40 UTC, Jonas Ådahl
none Details | Review
wayland: Make MetaWaylandDataSource ownership protocol specific (34.44 KB, patch)
2015-06-26 11:00 UTC, Jonas Ådahl
none Details | Review
wayland: Make MetaWaylandDataSource ownership protocol specific (37.81 KB, patch)
2015-06-30 02:53 UTC, Jonas Ådahl
committed Details | Review

Description Jonas Ådahl 2015-06-10 04:39:31 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.
Comment 1 Jonas Ådahl 2015-06-18 02:44:47 UTC
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.
Comment 2 Jasper St. Pierre (not reading bugmail) 2015-06-18 03:15:58 UTC
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.
Comment 3 Jonas Ådahl 2015-06-18 03:25:06 UTC
(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.
Comment 4 Jonas Ådahl 2015-06-18 04:40:48 UTC
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.
Comment 5 Jonas Ådahl 2015-06-26 11:00:24 UTC
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.
Comment 6 Jonas Ådahl 2015-06-30 02:53:36 UTC
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.
Comment 7 Jasper St. Pierre (not reading bugmail) 2015-06-30 02:59:44 UTC
Review of attachment 306362 [details] [review]:

ok with Makefile.am changes
Comment 8 Jonas Ådahl 2015-06-30 03:25:12 UTC
Attachment 306362 [details] pushed as 5547c98 - wayland: Make MetaWaylandDataSource ownership protocol specific