GNOME Bugzilla – Bug 785723
Search providers handling can make Nautilus crash
Last modified: 2021-06-18 15:53:41 UTC
See patch.
Created attachment 356770 [details] [review] search-engine: fix search providers handling When starting the search providers, some provider might finish before all providers are started, so a wrong value of providers_running will be used, making Nautilus crash. To fix this set the value of providers_running right before starting the providers, so a correct value of it will be used.
Review of attachment 356770 [details] [review]: ::: src/nautilus-search-engine.c @@ +92,3 @@ + priv->providers_running++; + } + maybe you can just move this before every provider start instead of doing the sum of all of them at once?
Created attachment 357194 [details] [review] search-engine: fix search providers handling When starting the search providers, some provider might finish before all providers are started, so a wrong value of providers_running will be used, making Nautilus crash. To fix this, keep a queue of the started providers and whenever the value of the finised/running providers is needed, check the status of each provider.
Review of attachment 357194 [details] [review]: Cool! Just some nitpicks ::: src/nautilus-search-engine.c @@ +40,2 @@ guint providers_error; + GQueue *providers_started; This should probably be just "providers". @@ +88,3 @@ + g_queue_clear (priv->providers_started); + + g_queue_push_head (priv->providers_started, priv->tracker); this would need to change, and not have a pointer to a provider we don't use. You can just create them on demand, right? @@ +125,3 @@ + for (l = g_queue_peek_head_link (priv->providers_started); l != NULL; l = l->next) + { + all_providers_finished &= (!nautilus_search_provider_is_running (NAUTILUS_SEARCH_PROVIDER (l->data))); It's usually better to use positive questions when possible. Can we have one API for search_provider_finished?
Created attachment 357212 [details] [review] search-engine: fix search providers handling When starting the search providers, some provider might finish before all providers are started, so a wrong value of providers_running will be used, making Nautilus crash. To fix this, keep a queue of the started providers and whenever the value of the finised/running providers is needed, check the status of each provider.
Review of attachment 357212 [details] [review]: Looking good ::: src/nautilus-search-engine.c @@ +73,2 @@ static void +nautilus_search_engine_clear_providers_queue (NautilusSearchEngine *engine) no nautilus prefix for non public API @@ +96,3 @@ + priv = nautilus_search_engine_get_instance_private (engine); + + } either use always nautilus_directory_unref/ref or g_object_* functions. I think we agreed to just use g_object_* so you can change the next one. @@ +111,3 @@ + g_object_ref (query); + g_clear_object (&priv->query); could the query be the same (and therefore this order of refs/unrefs)? If so, is better if you add a comment, if not, ignore this comment. @@ +450,2 @@ static void +nautilus_search_engine_set_property (GObject *object, for each property we usually put public API for set/get.
Created attachment 357741 [details] [review] search-engine: fix search providers handling When starting the search providers, some provider might finish before all providers are started, so a wrong value of providers_running will be used, making Nautilus crash. To fix this, keep a queue of the started providers and whenever the value of the finised/running providers is needed, check the status of each provider.
> @@ +450,2 @@ > static void > +nautilus_search_engine_set_property (GObject *object, > > for each property we usually put public API for set/get. The running property is handled internally and it shouldn't be settable from outside right?
Attachment 357741 [details] pushed as 37693c4 - search-engine: fix search providers handling
The patch was pushed by accident
Created attachment 357798 [details] [review] search-engine: fix search providers handling When starting the search providers, some provider might finish before all providers are started, so a wrong value of providers_running will be used, making Nautilus crash. To fix this, keep a queue of the started providers and whenever the value of the finised/running providers is needed, check the status of each provider.
Review of attachment 357798 [details] [review]: Looks mostly good! Just a few nitpicks, and I would like Ernestas to give a quick look at it. ::: src/nautilus-search-engine-tracker.c @@ +496,3 @@ +} + + extra line ::: src/nautilus-search-engine.c @@ +131,3 @@ priv = nautilus_search_engine_get_instance_private (engine); + /* first ref the new query and then clear the previous Capitalize first char. ::: src/nautilus-search-provider.c @@ +149,3 @@ +{ + g_return_val_if_fail (NAUTILUS_IS_SEARCH_PROVIDER (provider), FALSE); + g_return_val_if_fail (NAUTILUS_SEARCH_PROVIDER_GET_IFACE (provider)->is_finished, FALSE); can this ever happen? If so, does a default FALSE make sense? Couldn't it be dangerous to never finish?
Review of attachment 357798 [details] [review]: I don’t see anything wrong, really, just some nitpicks, too. ::: src/nautilus-search-engine-model.c @@ +291,3 @@ +} + + Extra line. ::: src/nautilus-search-engine.c @@ +100,3 @@ + priv = nautilus_search_engine_get_instance_private (engine); + + for (l = g_queue_peek_head_link (priv->providers); l != NULL; l = l->next) Why not just pop here if you’re clearing it afterwards? @@ +117,3 @@ + priv = nautilus_search_engine_get_instance_private (engine); + + g_clear_object (&priv->directory); No reason to clear if we’re resetting it right after, but it’s fine. @@ +136,3 @@ + g_object_ref (query); + g_clear_object (&priv->query); + priv->query = query; This is a bit messy. I think the comment offsets the cleverness of the approach. @@ +161,3 @@ + g_queue_push_head (priv->providers, simple); + + if (priv->directory) != NULL ::: src/nautilus-search-engine.h @@ +45,3 @@ +void nautilus_search_engine_set_recursive (NautilusSearchEngine *engine, + gboolean recursive); +gboolean nautilus_search_engine_get_recursive (NautilusSearchEngine *engine); Should all of these be grouped? Also, the function names aren’t aligned. ::: src/nautilus-search-provider.c @@ +149,3 @@ +{ + g_return_val_if_fail (NAUTILUS_IS_SEARCH_PROVIDER (provider), FALSE); + g_return_val_if_fail (NAUTILUS_SEARCH_PROVIDER_GET_IFACE (provider)->is_finished, FALSE); Gah, this is hairy. Yeah, defaulting to TRUE would probably be better. It’s better than to bring everything down if someone forgets to assign a pointer in the vtable.
Created attachment 357833 [details] [review] search-engine: fix search providers handling When starting the search providers, some provider might finish before all providers are started, so a wrong value of providers_running will be used, making Nautilus crash. To fix this, keep a queue of the started providers and whenever the value of the finised/running providers is needed, check the status of each provider.
Review of attachment 357833 [details] [review]: Looks good now!
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of Files (nautilus), then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/nautilus/-/issues/ Thank you for your understanding and your help.