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 702546 - Crash in in nautilus_trash_bar_dispose at nautilus-trash-bar.c:109
Crash in in nautilus_trash_bar_dispose at nautilus-trash-bar.c:109
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: Crashers
3.8.x
Other Linux
: High critical
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-06-18 11:34 UTC by Iain Lane
Modified: 2013-06-25 17:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
stacktrace of this crash (17.97 KB, text/plain)
2013-06-18 11:34 UTC, Iain Lane
  Details
trash-bar: Take a weak ref to the view for signal disconnection (2.43 KB, patch)
2013-06-19 09:54 UTC, Iain Lane
reviewed Details | Review
Remove extra location widgets when disposing NautilusWindowSlot (1002 bytes, patch)
2013-06-20 10:34 UTC, Iain Lane
committed Details | Review

Description Iain Lane 2013-06-18 11:34:54 UTC
Created attachment 247127 [details]
stacktrace of this crash

Reproduced with glib >= 2.37.1. Originally reported on Ubuntu but I just checked on current fc20.

1. Open nautilus
2. Click wastebasket/trash.
3. Close the window

Nautilus segfaults at this point. Trace attached (it's one reported by an Ubuntu user and retraced by launchpad).
Comment 1 Iain Lane 2013-06-18 11:36:02 UTC
Sorry, should have said that the trace is from 3.6.3 in current saucy but I reproduced using 3.8.2 on fedora.
Comment 2 Iain Lane 2013-06-19 09:54:35 UTC
Created attachment 247246 [details] [review]
trash-bar: Take a weak ref to the view for signal disconnection

We were attempting to disconnect the signals from the NautilusView after
it was already destroyed. Add a weak reference so we get our callback
called at the right moment to do the destruction.

Also switch to using g_signal_handlers_disconnect_by_func for
robustness.
Comment 3 Iain Lane 2013-06-19 09:54:54 UTC
Perhaps something like this?
Comment 4 Cosimo Cecchi 2013-06-19 18:05:31 UTC
Review of attachment 247246 [details] [review]:

::: src/nautilus-trash-bar.c
@@ -73,3 @@
-	bar->priv->selection_handler_id =
-		g_signal_connect (bar->priv->view, "selection-changed",
-				  G_CALLBACK (selection_changed_cb), bar);

I believe the root of the issue here is that from nautilus-window-slot.c:location_has_really_changed() we call nautilus_window_slot_switch_new_content_view(), which will destroy the NautilusView, and then after that nautilus_window_slot_update_for_new_location(), which will ultimately call the dispose() in NautilusTrashBar - at that point, NautilusView has already just been destroyed.

It looks to me we should be able to destroy the extra location widgets from nautilus_window_slot_switch_new_content_view() before destroying the view instead, and avoid this problem altogether.
Comment 5 Iain Lane 2013-06-20 10:33:53 UTC
(In reply to comment #4)
> I believe the root of the issue here is that from
> nautilus-window-slot.c:location_has_really_changed() we call
> nautilus_window_slot_switch_new_content_view(), which will destroy the
> NautilusView, and then after that
> nautilus_window_slot_update_for_new_location(), which will ultimately call the
> dispose() in NautilusTrashBar - at that point, NautilusView has already just
> been destroyed.
> 
> It looks to me we should be able to destroy the extra location widgets from
> nautilus_window_slot_switch_new_content_view() before destroying the view
> instead, and avoid this problem altogether.

Cheers. This led me to a better solution.

I don't think that we need to add that into nautilus_window_slot_switch_new_content_view() - this is called from location_has_really_changed() which calls nautilus_window_slot_update_for_new_location() and this function does remove the extra location widgets.

Where they aren't removed though is when disposing NautilusWindowSlot, which also unrefs the NautilusView. Doing the same there fixes it for me. WDYT?
Comment 6 Iain Lane 2013-06-20 10:34:52 UTC
Created attachment 247308 [details] [review]
Remove extra location widgets when disposing NautilusWindowSlot

This causes the widgets to be properly destroyed before the NautilusView
is.
Comment 7 Cosimo Cecchi 2013-06-25 16:07:37 UTC
Review of attachment 247308 [details] [review]:

Thanks, this looks good.
Comment 8 Iain Lane 2013-06-25 16:12:18 UTC
please commit (also to 3-8)
Comment 9 Cosimo Cecchi 2013-06-25 17:08:58 UTC
Pushed now.

Attachment 247308 [details] pushed as 5c00b42 - Remove extra location widgets when disposing NautilusWindowSlot