GNOME Bugzilla – Bug 795978
Crash in ev_media_player_keys_grab_keys
Last modified: 2018-05-14 09:28:38 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
Created attachment 371854 [details] [review] Fix crash in ev_media_player_keys_grab_keys
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.
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.
This is your crash: 166 EvMediaPlayerKeys *keys = EV_MEDIA_PLAYER_KEYS (user_data);• 167 ↦ GDBusProxy *proxy;•
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.
(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!
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.
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.
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.
Review of attachment 371937 [details] [review]: Looks good to me
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.