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 795978 - Crash in ev_media_player_keys_grab_keys
Crash in ev_media_player_keys_grab_keys
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-05-09 15:14 UTC by Marek Kašík
Modified: 2018-05-14 09:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix crash in ev_media_player_keys_grab_keys (2.04 KB, patch)
2018-05-09 15:16 UTC, Marek Kašík
none Details | Review
shell: Fix crash when exiting early (1.34 KB, patch)
2018-05-11 08:22 UTC, Bastien Nocera
none Details | Review
Fix crash in ev_media_player_keys_grab_keys (1.95 KB, patch)
2018-05-11 08:38 UTC, Marek Kašík
none Details | Review
Fix crash in ev_media_player_keys_grab_keys (2.75 KB, patch)
2018-05-11 09:48 UTC, Marek Kašík
committed Details | Review

Description Marek Kašík 2018-05-09 15:14:32 UTC
There is quite a lot of reports on crash in ev_media_player_keys_grab_keys() or its successors for Fedora (see https://retrace.fedoraproject.org/faf/reports/1649005/).

Unfortunately, I can not reproduce this one but I think that I've found the reason for this. It seems that asynchronous getting of a DBus proxy can finish after EvMediaPlayerKeys is finalized but this is not secured by a cancellable and hence it crashes in a callback of the getting of the proxy.

I've pushed a patch which seems to fix this to Fedora 27, the statistics has improved :) and the version with the patch has not shown in the statistics of crashes for last 3 weeks (see the decreasing presence of the crash on daily history on the faf page). So I take it as the patch fixed the issue.

Additional info:
https://bugzilla.redhat.com/show_bug.cgi?id=1359507
Comment 1 Marek Kašík 2018-05-09 15:16:03 UTC
Created attachment 371854 [details] [review]
Fix crash in ev_media_player_keys_grab_keys
Comment 2 Marek Kašík 2018-05-10 14:53:40 UTC
There are still some crashes in this area but the amount is much lower (~3 vs ~33 for the last 5 days, see https://retrace.fedoraproject.org/faf/reports/2153233/ and https://retrace.fedoraproject.org/faf/reports/2158043/). So I think that the EvMediaPlayerKeys object is finalized when the mediakeys_service_appeared_cb() has been already called. Maybe a mutex would help here.
Comment 3 Bastien Nocera 2018-05-11 08:16:02 UTC
Review of attachment 371854 [details] [review]:

::: shell/ev-media-player-keys.c
@@ +44,3 @@
+        GDBusProxy   *proxy;
+	gboolean      has_name_owner;
+	GCancellable *service_appearance_cancellable;

This isn't a cancellable for "service_appearance". You might as well call this cancellable.

Looks fine otherwise.
Comment 4 Bastien Nocera 2018-05-11 08:22:14 UTC
This is your crash:
166         EvMediaPlayerKeys *keys = EV_MEDIA_PLAYER_KEYS (user_data);•
167 ↦       GDBusProxy *proxy;•
Comment 5 Bastien Nocera 2018-05-11 08:22:32 UTC
Created attachment 371931 [details] [review]
shell: Fix crash when exiting early

When g_dbus_proxy_new_for_bus() is cancelled because EvMediaPlayerKeys
is finalized, don't use the user_data (which will be invalid) before
checking the return of the call.

The user_data will only be valid if the error returned by the _finish()
call is not G_IO_ERROR_CANCELLED.
Comment 6 Marek Kašík 2018-05-11 08:30:17 UTC
(In reply to Bastien Nocera from comment #4)
> This is your crash:
> 166         EvMediaPlayerKeys *keys = EV_MEDIA_PLAYER_KEYS (user_data);•
> 167 ↦       GDBusProxy *proxy;•

You are right, I overlooked this. Thanks!
Comment 7 Marek Kašík 2018-05-11 08:38:54 UTC
Created attachment 371932 [details] [review]
Fix crash in ev_media_player_keys_grab_keys

I've changed the name of the GCancellable to just cancellable.
Comment 8 Bastien Nocera 2018-05-11 08:51:32 UTC
Review of attachment 371932 [details] [review]:

> It seems that asynchronous getting of a DBus proxy can
> finish after EvMediaPlayerKeys is finalized but this

You can remove the "seems to".

> is not secured by a cancellable and hence it crashes
> in callback of the getting of the proxy.

> This commit adds the cancellable which seems to fix this issue.

Same here.

You should probably squash the 2 patches together as well.
Comment 9 Marek Kašík 2018-05-11 09:48:41 UTC
Created attachment 371937 [details] [review]
Fix crash in ev_media_player_keys_grab_keys

Thank you for the review.

(In reply to Bastien Nocera from comment #8)
> Review of attachment 371932 [details] [review] [review]:
> 
> > It seems that asynchronous getting of a DBus proxy can
> > finish after EvMediaPlayerKeys is finalized but this
> 
> You can remove the "seems to".

Done.

> > is not secured by a cancellable and hence it crashes
> > in callback of the getting of the proxy.
> 
> > This commit adds the cancellable which seems to fix this issue.
> 
> Same here.

I've modified this.

> You should probably squash the 2 patches together as well.

Thanks, I've merged them together.
Comment 10 Bastien Nocera 2018-05-11 11:07:57 UTC
Review of attachment 371937 [details] [review]:

Looks good to me
Comment 11 Marek Kašík 2018-05-14 09:28:21 UTC
Comment on attachment 371937 [details] [review]
Fix crash in ev_media_player_keys_grab_keys

Thank you all for your reviews. I've pushed the patch to master and 3.28 branches.