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 745479 - Tracker search in file chooser causes a crash
Tracker search in file chooser causes a crash
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkFileChooser
3.15.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
Federico Mena Quintero
Depends on:
Blocks:
 
 
Reported: 2015-03-02 19:03 UTC by Sebastian Keller
Modified: 2015-03-03 16:59 UTC
See Also:
GNOME target: 3.16
GNOME version: ---


Attachments
filechooser: Disconnect signal handlers from search engine before destroying (1.02 KB, patch)
2015-03-03 13:11 UTC, Carlos Garnacho
accepted-commit_now Details | Review
searchenginetracker: Keep a reference on the search engine while querying (1.99 KB, patch)
2015-03-03 13:11 UTC, Carlos Garnacho
accepted-commit_now Details | Review
filechooser: Remove assert (1.16 KB, patch)
2015-03-03 13:11 UTC, Carlos Garnacho
reviewed Details | Review
filechooser: Only do search_start_query() if the search entry is visible (1.04 KB, patch)
2015-03-03 16:48 UTC, Carlos Garnacho
accepted-commit_now Details | Review

Description Sebastian Keller 2015-03-02 19:03:17 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
  • #0 g_type_check_instance_cast
    at gtype.c line 4060
  • #1 query_callback
    at gtksearchenginetracker.c line 292
  • #2 g_simple_async_result_complete
    at gsimpleasyncresult.c line 763
  • #3 g_dbus_connection_call_done
    at gdbusconnection.c line 5502
  • #4 g_simple_async_result_complete
    at gsimpleasyncresult.c line 763
  • #5 complete_in_idle_cb
    at gsimpleasyncresult.c line 775
  • #6 g_main_dispatch
    at gmain.c line 3122
  • #7 g_main_context_dispatch
    at gmain.c line 3737
  • #8 g_main_context_iterate
    at gmain.c line 3808
  • #9 g_main_context_iteration
    at gmain.c line 3869
  • #10 g_application_run
    at gapplication.c line 2308
  • #11 main

Comment 1 Carlos Garnacho 2015-03-03 13:11:18 UTC
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.
Comment 2 Carlos Garnacho 2015-03-03 13:11:24 UTC
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.
Comment 3 Carlos Garnacho 2015-03-03 13:11:29 UTC
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.
Comment 4 Matthias Clasen 2015-03-03 14:35:22 UTC
Review of attachment 298424 [details] [review]:

sure
Comment 5 Matthias Clasen 2015-03-03 14:38:18 UTC
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 ?
Comment 6 Matthias Clasen 2015-03-03 14:40:34 UTC
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 ?
Comment 7 Matthias Clasen 2015-03-03 14:42:56 UTC
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 ?
Comment 8 Carlos Garnacho 2015-03-03 16:29:15 UTC
(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.
Comment 9 Matthias Clasen 2015-03-03 16:46:08 UTC
Review of attachment 298425 [details] [review]:

ok then
Comment 10 Carlos Garnacho 2015-03-03 16:48:53 UTC
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.
Comment 11 Matthias Clasen 2015-03-03 16:51:16 UTC
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.
Comment 12 Carlos Garnacho 2015-03-03 16:59:45 UTC
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