GNOME Bugzilla – Bug 759717
Crash on search from list view
Last modified: 2017-11-30 03:14:49 UTC
Searching while in list-view currently almost always crashes (not instantly if only 2-3 characters are entered immediately): Program received signal SIGSEGV, Segmentation fault. 0x00007ffff5240811 in g_action_map_lookup_action () from /usr/lib/libgio-2.0.so.0 (gdb) bt
+ Trace 235832
glib2, gtk3 and nautilus are at their most recent git versions.
-> 3.20 Thanks, it's helpful to have people trying master.
bug #759776 seems similar and state it can be trigger by changing the view to list mode before the content is loaded
can easily be triggered indeed with - nautilus -q - nautilus /usr/bin - click on the view mode -> list while it's loading
let's make this one the main bug report.
*** Bug 760055 has been marked as a duplicate of this bug. ***
*** Bug 759940 has been marked as a duplicate of this bug. ***
*** Bug 759776 has been marked as a duplicate of this bug. ***
Created attachment 319120 [details] [review] files-view: use finalize instead of dispose Dispose is intended to let the object still valid for external calls. Freeing the action group on dispose makes the view somehow invalid, so some calls were made at that time and were making the view to crash. This change to dispose was made in commit 347369d18eb6 to fix a crash as well. However, an upcoming patch fixed it as well as noted in the commit message. So although dispose seemed good, it's not if it makes the object invalid. On the other hand, instead of dispose we should use destroy, since we are already using it, and implementing dispose was kind of an error. In upcoming patches we will move some disconnect_signal calls from finalized to destroy.
Created attachment 319121 [details] [review] files-view: move disconnect_signal calls to destroy So we have all the disconnection in one place and we avoid random callbacks while destroying.
Created attachment 319122 [details] [review] files-view: disconnect leaked signals Even if we disconnect them on the subclasses, as long as we connect them here we should disconnect them as well.
Created attachment 319123 [details] [review] files-view: use explicit in_destruction boolean So we were relying on the model to be NULL to not update all the view widgets and details in the done_loading callback. However, we need the model alive so in the signal of is-loading emitted on the done_loading function, external objects can peek the model, etc. even in destruction time. So instead of setting the model as NULL on destroy and relying on it, use a explicit boolean to not update view widgets on destroy.
Created attachment 319124 [details] [review] window-slot: use cancel_change instead of end_change Nautilus slot ends any location change before starting a new one or when is done loading. The done loading signal is emitted when the view finalize to load a directory. However, when loading a new location, if the previous one was not finalized to load, nautilus was crashing, due to the is-loading signal in the view first emitting it for finalizing the previous location change. Then the callback of window slot was freeing the new_content_view, because this callback can be done when canceling a location change. However, at that time, the new_content_view is actually the one which is going to be used as a content view when we are just changing locations, for example, while searching. It's kind of strange that the callback of the view ending a load frees the new_content_view, since this is already managed by setup_view. And since we already have a cancel_location_change, we can use that on the majority of situations when we actually want to cancel a change, and let the end_location callback for the specific case of ending a load of the view. So now only the cancel_location_change frees the new_content_view, and we avoid a crash freeing the current view while loading it when changing locations. This is the last patch of the series of patches to fix the crashing nautilus while searching.
Created attachment 319125 [details] [review] window-slot: move cancel_location to stop_loading Since now they are the same, just use the public api.
Created attachment 319126 [details] [review] files-view: don't remove model when stoping view So when searching, window-slot is able to get the location.
Created attachment 319127 [details] [review] files-view: make search robust This is the last patch of a series of patches to fix all the related issues with search synchronization, robustness on search, etc. This patch orders fix when the view is requested to load a search directory. This is tricky because the views expect real locations, and when a caller sets the view to start searching the view expect to already have a real location loaded, so it can set the invented search directory with the model as a backing uri. In the case that window slot request a search directory, we are screwed, because either we load first the real backing uri, which will screw the window slot and all of the connected users of the view when the is-location signal is done, or we first set the query and then load the search directory, which will screw it anyway because the real location set up at that point in the view is not the one associated with the search directory. So we effectively need to do both at the same time. To do that, implement a set_search_query_internal which will allow us to pass a backing uri, so when the slot sets a location that is a search directory, instead of loading the directory itself, we only set the query indicating the real location behind, so it can set up everything in the search directory before loading it. This patch was done with a few things in mind, and it actually fixes few other issues. For example, now then we reuse the directory from the slot, we actually create a new search directory, so we actually don't reuse it. The point for this is because if we reuse the directory, any stop to the search directory will stop the future search that we just set. This happens when the slot stops the old view to load (which had the same search directory before this patch) after setting the new view to load. A better fix would be to get rid of the nautilus-search directory invention or to make it only internal, and make the window-slot be aware when a location was also searching (with a struct instead of using nautilus-bookmarks).
Created attachment 319128 [details] [review] files-view: fix management when using internal model We were leaking it in these cases. But we need to make sure we don't free the directory if we are using the same as the internal one. So manage that in the load_directory as a generic function.
Created attachment 319129 [details] [review] files-view: remove modeline No longer using tabs.
Someone can test master? All theses are intended for 3.18, but before actually doing another 3.18 release I would like some testing on this front. Any help on testing is appreciated. Attachment 319120 [details] pushed as 180c26f - files-view: use finalize instead of dispose Attachment 319121 [details] pushed as a142e2a - files-view: move disconnect_signal calls to destroy Attachment 319122 [details] pushed as 4d94a88 - files-view: disconnect leaked signals Attachment 319123 [details] pushed as 7d45760 - files-view: use explicit in_destruction boolean Attachment 319124 [details] pushed as 07fa5cd - window-slot: use cancel_change instead of end_change Attachment 319125 [details] pushed as 45db870 - window-slot: move cancel_location to stop_loading Attachment 319126 [details] pushed as 2d5ea4d - files-view: don't remove model when stoping view Attachment 319127 [details] pushed as 63d0425 - files-view: make search robust Attachment 319128 [details] pushed as e55ab26 - files-view: fix management when using internal model Attachment 319129 [details] pushed as 3ad3b3e - files-view: remove modeline
Looks like it's gone. Thanks! (I did get one crash while appending _ to a 2 character query, but about 300 searches later and I still can't reproduce it.)
(In reply to Carlos Soriano from comment #18) > Someone can test master? > All theses are intended for 3.18, but before actually doing another 3.18 > release I would like some testing on this front. > Any help on testing is appreciated. > I will try applying to 3.18 local tree and test :)
(In reply to Carlos Soriano from comment #18) > Someone can test master? > All theses are intended for 3.18, but before actually doing another 3.18 > release I would like some testing on this front. > Any help on testing is appreciated. > Attachment 319127 [details] pushed as 63d0425 - files-view: make search robust this one won't apply to 3-18 branch. patching file src/nautilus-files-view.c Hunk #1 succeeded at 2327 (offset 5 lines). Hunk #2 FAILED at 7105. Hunk #3 FAILED at 7880. 2 out of 3 hunks FAILED -- saving rejects to file src/nautilus-files-view.c.rej
Actually the 8th and 9th patch in the list don't apply. If you have time to make a patch for gnome-3-18 patch, I can test it as I am affected by the crash (one of the duplicates). Thank you!
Created attachment 319251 [details] [review] search-directory: disconnect leaked signals We were not disconnecting them, so they could be called after the search-directory is freed, causing a crash. To be honest, not sure how it survived until now, probably because previously (I didn't check) the search engine was freed when stopped.
Comment on attachment 319251 [details] [review] search-directory: disconnect leaked signals Another one needed, in a few minutes I will push to 3.18 so you can directly test there, but I will hold the release until I have some tests as you already tried (thanks for it!) Attachment 319251 [details] pushed as 6b64287 - search-directory: disconnect leaked signals
To 3.18
I just did a build from gnome-3-18 branch. Switching views while /usr/lib is loading no longer causes a crash. Switching views while searching works as expected too. So pretty much everything works fine again. I did see a warning in terminal when closing nautilus: (nautilus:31217): Gtk-CRITICAL **: gtk_notebook_get_tab_label: assertion 'list != NULL' failed ** (nautilus:31217): CRITICAL **: nautilus_notebook_sync_loading: assertion 'GTK_IS_WIDGET (tab_label)' failed But not sure if related.
(In reply to Hussam Al-Tayeb from comment #26) > I just did a build from gnome-3-18 branch. > Switching views while /usr/lib is loading no longer causes a crash. > Switching views while searching works as expected too. > So pretty much everything works fine again. > > I did see a warning in terminal when closing nautilus: > > (nautilus:31217): Gtk-CRITICAL **: gtk_notebook_get_tab_label: assertion > 'list != NULL' failed > > ** (nautilus:31217): CRITICAL **: nautilus_notebook_sync_loading: assertion > 'GTK_IS_WIDGET (tab_label)' failed > > But not sure if related. Looks unrelated, can you checkout to 3.18.5 and see if that happens?
(In reply to Carlos Soriano from comment #27) > (In reply to Hussam Al-Tayeb from comment #26) > > I just did a build from gnome-3-18 branch. > > Switching views while /usr/lib is loading no longer causes a crash. > > Switching views while searching works as expected too. > > So pretty much everything works fine again. > > > > I did see a warning in terminal when closing nautilus: > > > > (nautilus:31217): Gtk-CRITICAL **: gtk_notebook_get_tab_label: assertion > > 'list != NULL' failed > > > > ** (nautilus:31217): CRITICAL **: nautilus_notebook_sync_loading: assertion > > 'GTK_IS_WIDGET (tab_label)' failed > > > > But not sure if related. > > Looks unrelated, can you checkout to 3.18.5 and see if that happens? I am using gtk+ 3.18.6 and latest nautilus from gnome-3-18 branch already.
(In reply to Hussam Al-Tayeb from comment #28) > (In reply to Carlos Soriano from comment #27) > > (In reply to Hussam Al-Tayeb from comment #26) > > > I just did a build from gnome-3-18 branch. > > > Switching views while /usr/lib is loading no longer causes a crash. > > > Switching views while searching works as expected too. > > > So pretty much everything works fine again. > > > > > > I did see a warning in terminal when closing nautilus: > > > > > > (nautilus:31217): Gtk-CRITICAL **: gtk_notebook_get_tab_label: assertion > > > 'list != NULL' failed > > > > > > ** (nautilus:31217): CRITICAL **: nautilus_notebook_sync_loading: assertion > > > 'GTK_IS_WIDGET (tab_label)' failed > > > > > > But not sure if related. > > > > Looks unrelated, can you checkout to 3.18.5 and see if that happens? > > I am using gtk+ 3.18.6 and latest nautilus from gnome-3-18 branch already. latest nautilus you mean the 3.18 branch from the repo right? So you are building it with jhbuild right? I'm asking to git checkout 3.18.5 to see if the warning is related to the patches I pushed a few minutes ago or not.
No, not using jhbuild. I manually did: git clone --depth=1 git://git.gnome.org/nautilus -b gnome-3-18
(In reply to Hussam Al-Tayeb from comment #30) > No, not using jhbuild. I manually did: > git clone --depth=1 git://git.gnome.org/nautilus -b gnome-3-18 ah fine. Do git checkout 3.18.5 as said, build, and test it again to see if the critical is still present.
But I can reproduce it. It is happening only if I close nautilus in the middle of loading /usr/lib.
(In reply to Hussam Al-Tayeb from comment #32) > But I can reproduce it. It is happening only if I close nautilus in the > middle of loading /usr/lib. you can reproduce with 3.18 branch or with 3.18.5 checkout? or both?
(In reply to Carlos Soriano from comment #33) > (In reply to Hussam Al-Tayeb from comment #32) > > But I can reproduce it. It is happening only if I close nautilus in the > > middle of loading /usr/lib. > > you can reproduce with 3.18 branch or with 3.18.5 checkout? or both? I can reproduce it on 3.18 branch. I will try 3.18.5 checkout now.
error: pathspec '3.18.5' did not match any file(s) known to git. Are you sure it is 3.18.5? The previous release was 3.18.4
(In reply to Hussam Al-Tayeb from comment #35) > error: pathspec '3.18.5' did not match any file(s) known to git. > > Are you sure it is 3.18.5? > The previous release was 3.18.4 whops, 3.18.4 indeed.
*** Bug 757455 has been marked as a duplicate of this bug. ***