GNOME Bugzilla – Bug 745479
Tracker search in file chooser causes a crash
Last modified: 2015-03-03 16:59:49 UTC
In a file open dialog, start a search, type a single character, and then immediately cancel the search again by pressing escape and it is likely to crash. Program received signal SIGSEGV, Segmentation fault. g_type_check_instance_cast (type_instance=type_instance@entry=0x16af9b0, iface_type=23344320) at gtype.c:4060 4060 node = lookup_type_node_I (type_instance->g_class->g_type); (gdb) bt
+ Trace 234763
Created attachment 298424 [details] [review] filechooser: Disconnect signal handlers from search engine before destroying The search engine might stay alive longer due to extra temporary refs, so the signal handlers should be removed for the filechooser to ignore these properly.
Created attachment 298425 [details] [review] searchenginetracker: Keep a reference on the search engine while querying The object might be destroyed then mid operation, causing crashes as the query callback still expects the object pointer to be valid. Also, remove the gdk_threads_enter/leave pairs, the callback will be executed on the caller (UI) thread, so this is not necessary.
Created attachment 298426 [details] [review] filechooser: Remove assert The code seems to be prepared for creating the search model on demand, and it's in fact destroyed and NULLified if the search operation is stopped, causing the assert to fail the next time we search something. This assert is not essential in that specific function, and the model will just be created again after/during this, so just remove the assert.
Review of attachment 298424 [details] [review]: sure
Review of attachment 298425 [details] [review]: ::: gtk/gtksearchenginetracker.c @@ +392,3 @@ g_debug ("SearchEngineTracker: query: %s", sparql->str); + get_query_results (tracker, sparql->str, query_callback, g_object_ref (tracker)); Are we 100% sure that query_callback gets always called ? Otherwise we leak a ref here... E.g. what if the dbus call is cancelled ?
Review of attachment 298426 [details] [review]: There's quite a few of these assertions around (for recent_model as well). Are we sure something else isn't amiss if this triggers ?
(In reply to Matthias Clasen from comment #5) > Review of attachment 298425 [details] [review] [review]: > > ::: gtk/gtksearchenginetracker.c > @@ +392,3 @@ > g_debug ("SearchEngineTracker: query: %s", sparql->str); > > + get_query_results (tracker, sparql->str, query_callback, g_object_ref > (tracker)); > > Are we 100% sure that query_callback gets always called ? Otherwise we leak > a ref here... > > E.g. what if the dbus call is cancelled ? Yes, it's called. On cancellation you get the callback called, and the async result returns an error with G_IO_ERROR_CANCELLED. The ::error signal would be emitted then within the callback, hence the first patch :) (In reply to Matthias Clasen from comment #6) > Review of attachment 298426 [details] [review] [review]: > > There's quite a few of these assertions around (for recent_model as well). > Are we sure something else isn't amiss if this triggers ? AFAICT, this happens due to GtkSearchEntry emitting the ::search-changed signal in a timeout after I hit Esc. So the model is populated, and still present the next time operation_mode_set_search() is called. I'm unsure on what's the best fix for GtkSearchEntry, we could just remove the timeout on unmap, although this widget doesn't seem to have any mapped vs unmapped difference of behavior otherwise. As for removing the assert, should be pretty safe to do, search_start_query() always ensures the model is freed/NULL before starting the query anyway.
Review of attachment 298425 [details] [review]: ok then
Created attachment 298454 [details] [review] filechooser: Only do search_start_query() if the search entry is visible This signal can be emitted by GtkSearchEntry after search has been cancelled, and the entry hidden. It doesn't make sense to populate the search model in that state anymore, so just avoid doing it.
Review of attachment 298454 [details] [review]: ::: gtk/gtkfilechooserwidget.c @@ +6302,3 @@ + if (!gtk_widget_get_mapped (priv->search_entry)) + return; + I guess another alternative would be to look for OPERATION_MODE_SEARCH, but this will do just fine.
Attachment 298424 [details] pushed as a994f4e - filechooser: Disconnect signal handlers from search engine before destroying Attachment 298425 [details] pushed as b2f3b67 - searchenginetracker: Keep a reference on the search engine while querying Entry visibility patch redone as suggested and pushed as 5751d4f