GNOME Bugzilla – Bug 751537
unregister_mount_got_proxy_cb(): gvfsd-afc killed by SIGSEGV
Last modified: 2015-07-03 12:06:06 UTC
It seems the crash with following backtrace is pretty common: https://retrace.fedoraproject.org/faf2/problems/813282/ It is caused by commit 32adf492a9a2ab5cef953cf2dfd9f00c25c785e7 from Bug 708288. Truncated backtrace: Thread no. 1 (4 frames) #0 unregister_mount_got_proxy_cb at gvfsbackend.c:766 #1 g_simple_async_result_complete at gsimpleasyncresult.c:763 #2 complete_in_idle_cb at gsimpleasyncresult.c:775 #7 daemon_main at daemon-main.c:405 See for more info: https://bugzilla.redhat.com/show_bug.cgi?id=1232951
Created attachment 306161 [details] [review] afc: Fix unmount race Crash might happen when the device is hard unplugged immediately after unmount job is executed. Conseqently force_umount_idle() is called with already unrefed backend. But I can't prove it without iPhone. Attached patch might fix it, what do you think?
This might fit the backtrace, but there would be some more warnings in the logs if this is what happens (in particular the G_VFS_BACKEND_AFC (user_data); in force_umount_idle(). However I think I'd go with a different patch: call idevice_event_unsubscribe() from _finalize(), track the idle id in GvfsBackendAfc, and g_source_remove() it if it's pending. This would avoid artificially keeping alive the instance just so that we can run the idle (I expect most of the calls we make would be failing anyway ?)
Hmm, it might fix the problem also, but couldn't you try to reproduce the crash please and look what is happening?
I've managed to reproduce the crash by doing what you suggested, I haven't been able to reproduce the crash with your suggested fix applied, and I haven't been able to reproduce it either with the alternate patch I'm going to attach.
Created attachment 306542 [details] [review] alternate fix
Review of attachment 306542 [details] [review]: ::: daemon/gvfsbackendafc.c @@ +2730,3 @@ + self->force_umount_id = 0; + } + idevice_event_unsubscribe (); Maybe we can get rid of the idevice_event_unsubcribe() from force_umount() with this one in place.
Created attachment 306618 [details] [review] afc: Cleanup force-unmount idle on finalize When GvfsBackendAfc is finalized, if we have a pending idle for a force unmount, we need to remove it as by the time it runs, the GvfsBackendAfc it's acting on will no longer be valid. This fixes https://bugzilla.gnome.org/show_bug.cgi?id=751537 which can be reproduced by plugging an iDevice, unmounting it in Nautilus, and quickly unplugging it right after clicking on the eject icon.
I realized that even the code which is currently in git could benefit from something like the diff below, which is a (small) argument in favour of my approach compared to Ondrej's. If we start worrying about the device getting quickly unplugged and replugged, it's best to keep the idevice_event_unsubscribe () in the idle callback (was present in v1 and removed in v2). diff --git a/daemon/gvfsbackendafc.c b/daemon/gvfsbackendafc.c index efd19e8..7912cb2 100644 --- a/daemon/gvfsbackendafc.c +++ b/daemon/gvfsbackendafc.c @@ -376,6 +376,11 @@ _idevice_event_cb (const idevice_event_t *event, void *user_data) g_print ("Shutting down AFC backend for device uuid %s\n", afc_backend->uuid); + /* This might happen if the user manages to unplug/replug/unplug the same device + * before the idle runs + */ + g_return_if_fail(afc_backend->force_umount_id != 0); + /* idevice_event_unsubscribe() will terminate the thread _idevice_event_cb * is running in, so we need to call back into our main loop */ afc_backend->force_umount_id = g_idle_add(force_umount_idle, afc_backend);
Review of attachment 306618 [details] [review]: Ok, let's use yours patch. Please amend the patch with the diff from the Comment 8. ::: daemon/gvfsbackendafc.c @@ +379,3 @@ /* idevice_event_unsubscribe() will terminate the thread _idevice_event_cb * is running in, so we need to call back into our main loop */ + afc_backend->force_umount_id = g_idle_add(force_umount_idle, afc_backend); add space between function name and parenthesis @@ +2727,3 @@ g_vfs_backend_afc_close_connection (self); + idevice_event_unsubscribe (); move idevice_event_unsubscribe () into the g_vfs_afc_backend_unmount () (and do not remove the one from force_umount_idle ()) @@ +2733,3 @@ + * an idle after we removed it */ + if (self->force_umount_id != 0) { + g_source_remove(self->force_umount_id); add space between function name and parenthesis
Created attachment 306693 [details] [review] afc: Cleanup force-unmount idle on finalize When GvfsBackendAfc is finalized, if we have a pending idle for a force unmount, we need to remove it as by the time it runs, the GvfsBackendAfc it's acting on will no longer be valid. This fixes https://bugzilla.gnome.org/show_bug.cgi?id=751537 which can be reproduced by plugging an iDevice, unmounting it in Nautilus, and quickly unplugging it right after clicking on the eject icon.
Review of attachment 306693 [details] [review]: Looks good!
Please push it also in gnome-3-16 and gnome-3-14.
Thanks, pushed now https://git.gnome.org/browse/gvfs/commit/?id=63e3d144784eefe112de6b4e821664ebef94b0ab https://git.gnome.org/browse/gvfs/commit/?h=gnome-3-16&id=5740ee6860342c37b4967d529271d0851400567f https://git.gnome.org/browse/gvfs/commit/?h=gnome-3-14&id=4892dbe2b8c9d3707387d3d578a3ed6dddbc3b8d