GNOME Bugzilla – Bug 722616
Segfault on window close due to mangled location entry pointer
Last modified: 2014-12-27 00:23:31 UTC
Created attachment 266757 [details] [review] Location entry pointer check See RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1013272 Nautilus crashes on close when more than one tabs are open. After some debugging I observed that nautilus is attempting to update the location entry widget when each tab is closed, but the widget has been mangled by the window being destroyed and is no longer valid. The attached patch adds a simple NULL check and widget test that keeps nautilus from crashing for me.
Review of attachment 266757 [details] [review]: Looks good, but needs some rework. - codestyle: you must use tabs instead of spaces (take a look at near code) - codestyle: closing ")" should be w/o/ spaces before - codestyle: opening "(" in GTK_IS_WIDGET should has space before - patch: would be good if you will attach patch, did w/ git format-patch BTW, this is default checkings. Nautilus maintainers should review this patch in the near future ;)
Created attachment 267080 [details] [review] Check for valid location entry widget #2 (In reply to comment #1) > - codestyle: you must use tabs instead of spaces (take a look at near code) > - codestyle: closing ")" should be w/o/ spaces before > - codestyle: opening "(" in GTK_IS_WIDGET should has space before I did this work in my personal editor as a quick trial. I wasn't planning on meeting Gnome code formatting. Updated. > - patch: would be good if you will attach patch, did w/ git format-patch I didn't believe a one (two technically) line patch would require a full review and would rather it be fixed without my name attached, but I will attach a new patch so that this does get fixed. > BTW, this is default checkings. Nautilus maintainers should review this patch > in the near future ;) Thanks.
Review of attachment 267080 [details] [review]: ::: src/nautilus-window.c @@ +1034,2 @@ location_entry = nautilus_toolbar_get_location_entry (NAUTILUS_TOOLBAR (window->details->toolbar)); + if (location_entry != NULL && GTK_IS_WIDGET (location_entry)) { I don't think the GTK_IS_WIDGET is needed here
(In reply to comment #3) > I don't think the GTK_IS_WIDGET is needed here I've seen a case where a non-null pointer was passed but it was no longer pointing to a valid widget (destroyed).
Er, right, like you said in the first post. I've pushed this since it's clearly harmless and since I've hit this crash multiple times, but I'll leave the bug open in case a Nautilus developer wants to investigate further. Thanks!
Why was this only pushed to the gnome-3-8 branch?
Debarashi, here's the gnome-3-10 commit: https://git.gnome.org/browse/nautilus/commit/?h=gnome-3-10&id=67ba854074ac48ec142bdf3b9848cb8d94070f82 and the master commit: https://git.gnome.org/browse/nautilus/commit/?id=8acef3c776f2ef4f5dac788ed81f7e2d1f504026
The bug is still open because I haven't checked if there's not a race. This stopped all the Fedora retrace reports [1], but there's a user in the downstream bug who says he still got it. [1] https://retrace.fedoraproject.org/faf/problems/1490268/
(In reply to comment #7) > Debarashi, here's the gnome-3-10 commit: > > https://git.gnome.org/browse/nautilus/commit/?h=gnome-3-10&id=67ba854074ac48ec142bdf3b9848cb8d94070f82 > > and the master commit: > > https://git.gnome.org/browse/nautilus/commit/?id=8acef3c776f2ef4f5dac788ed81f7e2d1f504026 Oh, right. Sorry about that. I had a stale Git checkout.
I just got Nautilus 3.12 to segfault when I tried to close a window with three tabs in it. I think it's the same bug. Here's the stack trace:
+ Trace 233618
Originally filed at https://bugs.launchpad.net/ubuntu-gnome/+bug/1321184 (which is a private bug at the moment).
*** Bug 719558 has been marked as a duplicate of this bug. ***
+ Trace 234007
Did it optimize away entire function calls? g_list_foreach() should have called destroy_slots_foreach(), not gtk_container_remove()....
This bug is same as bug 741952 which I've just attached a patch for (I would have attached here if I knew this bug beforehand), I think that fixes the root problem, while the patch committed here was only fixing bug symptom. In fact the new crashes where happening in next lines below the patched code, this time when accessing the nautilus path bar, because the real problem is that the function where the crashes are happening (nautilus_window_sync_location_widgets()) should not be called at all in this context. It was called as part of real_active() function which makes a slot active, this is normally good behaviour that when closing a tab to activate the next free one, but *not* when we are closing all tabs (as part of window closing). So while commit 8acef3c776f2ef4f5dac788ed81f7e2d1f504026 could be reverted, it's not strictly necessary as it's also harmless.
> So while commit 8acef3c776f2ef4f5dac788ed81f7e2d1f504026 could be reverted, > it's not strictly necessary as it's also harmless. Let's revert it, since it was incorrect. Sorry about that. *** This bug has been marked as a duplicate of bug 741952 ***