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 614099 - [PATCH] GtkFileSystemModel may causes crash in g_file_get_child
[PATCH] GtkFileSystemModel may causes crash in g_file_get_child
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkFileChooser
2.20.x
Other Linux
: Normal major
: ---
Assigned To: gtk-bugs
Federico Mena Quintero
Depends on:
Blocks:
 
 
Reported: 2010-03-27 13:35 UTC by Michael Terry
Modified: 2010-04-02 14:54 UTC
See Also:
GNOME target: ---
GNOME version: 2.29/2.30


Attachments
Attach cancellable to async result and check in finish functions (5.20 KB, patch)
2010-03-31 14:01 UTC, Benjamin Otte (Company)
none Details | Review
Attach cancellable to async result and check in finish functions (5.20 KB, patch)
2010-04-02 14:54 UTC, Benjamin Otte (Company)
committed Details | Review

Description Michael Terry 2010-03-27 13:35:37 UTC
I was debugging a bug report [1] in my application, when I figured that something was wrong with GTK+'s use of my file chooser button.  Basically, here's the background:

My app's preference window had a FileChooserButton.  Sometimes when users would use samba shares, the preferences dialog would crash.  With a stack trace like:

  • #0 IA__g_file_get_child
    at gfile.c line 686
  • #1 gtk_file_system_model_got_files
    at gtkfilesystemmodel.c line 1100
  • #2 next_async_callback_wrapper
    at gfileenumerator.c line 299
  • #3 IA__g_simple_async_result_complete
    at gsimpleasyncresult.c line 588
  • #4 complete_in_idle_cb
    at gsimpleasyncresult.c line 598
  • #5 g_idle_dispatch
    at gmain.c line 4065
  • #6 g_main_dispatch
    at gmain.c line 1960
  • #7 IA__g_main_context_dispatch
    at gmain.c line 2513
  • #8 g_main_context_iterate
    at gmain.c line 2591
  • #9 IA__g_main_loop_run
    at gmain.c line 2799
  • #10 IA__gtk_main
    at gtkmain.c line 1219
  • #11 deja_dup_preferences_main
    at preferences.c line 191

Examining the GtkFileSystemModel object indicated that it had 0 references, and seemed to be getting finalized too early.  Looking further, it seemed to be because when gtk_file_system_model_set_directory got called on the model, it would be finalized before it finished enumerating the files in the directory.  The only place that sets the directory was the with_directory constructor, which is only called for GtkFileChooserDefault's browse_files_model.

Anyway, it looks like a simple g_object_ref before enumerating the files in the directory is called for.  The callback function even unrefs the model when it's done enumerating, as if it expected the model to be ref'ed beforehand.  It fixes the crash for me.  Here's the simple patch:

diff --git a/gtk/gtkfilesystemmodel.c b/gtk/gtkfilesystemmodel.c
index 5449d30..7a37724 100644
--- a/gtk/gtkfilesystemmodel.c
+++ b/gtk/gtkfilesystemmodel.c
@@ -1271,6 +1271,7 @@ gtk_file_system_model_set_directory (GtkFileSystemModel *m
   model->dir = g_object_ref (dir);
   model->attributes = g_strdup (attributes);
 
+  g_object_ref (model);
   g_file_enumerate_children_async (model->dir,
                                    attributes,
                                    G_FILE_QUERY_INFO_NONE,

I'm not attaching a test suite, because I think this looks pretty straightforward.  If you do want to reproduce, you can "jhbuild deja-dup" and then "jhbuild run deja-dup-preferences" and toggle the backup location between a samba share and a local directory.  It will crash eventually.

[1] https://launchpad.net/bugs/544306
Comment 1 Matthias Clasen 2010-03-29 12:49:02 UTC
doesn't look right to me. if you look at got_enumerator, it doesn't always unref, so adding the ref will cause leaks. I rather think the unref is a leftover that should be removed.

CC'ing the person who last made extensive changes to file system model code...
Comment 2 Michael Terry 2010-03-29 13:18:17 UTC
It does always unref, *when the enumeration completes*.  If the enumeration is not done yet, it keeps the ref, as I would expect.

Note that the crash I was seeing was not due to the unref in got_enumerator.  It crashed before then, and even when I took it out.  The problem is that the model is scrapped by upper-level code in GtkFileChooserDefault in the middle of the enumeration.  So when got_enumerator is called, it has been freed already.  Hence the suggested ref at the beginning of the enumeration to keep it around.

I thought such ref/unref logic was commonplace for async operations?
Comment 3 Benjamin Otte (Company) 2010-03-29 14:36:25 UTC
Yep, that unref should have been gone in a previous patch. I made it go away in master with http://git.gnome.org/browse/gtk+/commit/?id=9514e741cd153fc36d9e9d7982db5d7f6aad097e It is unrelated to this crash though, because code that takes that error path cannot get a stacktrace like the crasher one.

The real problem I think was in gvfs, which didn't return CANCELLED properly in the file enumerator. This should be fixed with http://git.gnome.org/browse/gvfs/commit/?id=5af6a1dbdd2c6a61b515a5bd64350db3af16dae7 - I didn't test it though (F12 has no deja-dup), so I'd be happy if you could make sure it gets rid of that error.
Comment 4 Michael Terry 2010-03-30 23:48:26 UTC
Hmm, now I get a hang instead of a crash.  More reproducable though!  :)

  • #0 ??
    from /lib/ld-linux.so.2
  • #1 __lll_lock_wait
    from /lib/tls/i686/cmov/libpthread.so.0
  • #2 _L_lock_748
    from /lib/tls/i686/cmov/libpthread.so.0
  • #3 pthread_mutex_lock
    from /lib/tls/i686/cmov/libpthread.so.0
  • #4 ??
    from /usr/lib/gio/modules/libgvfsdbus.so
  • #5 IA__g_file_enumerator_next_files_async
    at /build/buildd/glib2.0-2.24.0/gio/gfileenumerator.c line 381
  • #6 gtk_file_system_model_got_files
    at /build/buildd/gtk+2.0-2.20.0/gtk/gtkfilesystemmodel.c line 1107
  • #7 next_async_callback_wrapper
    at /build/buildd/glib2.0-2.24.0/gio/gfileenumerator.c line 299
  • #8 IA__g_simple_async_result_complete
    at /build/buildd/glib2.0-2.24.0/gio/gsimpleasyncresult.c line 588
  • #9 ??
    from /usr/lib/gio/modules/libgvfsdbus.so
  • #10 ??
    from /usr/lib/gio/modules/libgvfsdbus.so
  • #11 ??
    from /usr/lib/gio/modules/libgvfsdbus.so
  • #12 ??
    from /usr/lib/gio/modules/libgvfsdbus.so
  • #13 dbus_connection_dispatch
    from /lib/libdbus-1.so.3
  • #14 ??
    from /usr/lib/libgvfscommon.so.0
  • #15 g_main_dispatch
    at /build/buildd/glib2.0-2.24.0/glib/gmain.c line 1960
  • #16 IA__g_main_context_dispatch
    at /build/buildd/glib2.0-2.24.0/glib/gmain.c line 2513
  • #17 g_main_context_iterate
    at /build/buildd/glib2.0-2.24.0/glib/gmain.c line 2591
  • #18 IA__g_main_loop_run
    at /build/buildd/glib2.0-2.24.0/glib/gmain.c line 2799
  • #19 IA__gtk_main
    at /build/buildd/gtk+2.0-2.20.0/gtk/gtkmain.c line 1219
  • #20 deja_dup_preferences_main
    at preferences.c line 191
  • #21 main
    at preferences.c line 202

Comment 5 Benjamin Otte (Company) 2010-03-31 14:01:08 UTC
Created attachment 157587 [details] [review]
Attach cancellable to async result and check in finish functions

This makes sure we always return ERROR_CANCELLED, just like we want to
guarantee and avoids crashes in the file chooser.

There should be API in GSimpleAsyncResult to attach a GCancellable that
does this job for us, but so far there isn't, so we use
g_object_get/set_data() and check the cancellables manually in the
finish function.
Comment 6 Benjamin Otte (Company) 2010-03-31 14:11:42 UTC
Please try this patch instead. The first one was obviously wrong (and is reverted in git master already). I'm not gonna commit this before I know it's okay (embarrassing myself once is enough).

I tried reproducing the problem with FTP and failed - I got no Samba server here, so I guess I need to rely on your testing.
Comment 7 Michael Terry 2010-04-01 00:23:17 UTC
Nope.  That patch gives the same hang and stacktrace as in comment 4...
Comment 8 Benjamin Otte (Company) 2010-04-01 06:47:22 UTC
You did unapply the first patch, right?
Comment 9 Michael Terry 2010-04-01 12:37:23 UTC
Yes, I started from scratch again.  I can retest tonight if you think that result is outrageous.
Comment 10 Michael Terry 2010-04-01 22:06:32 UTC
After fresh testing (on a new install in a VM), your latest patch works fine for me.  I either screwed up my test or had other weirdness going on.

Thanks, Ben!
Comment 11 Benjamin Otte (Company) 2010-04-02 14:54:03 UTC
The following fix has been pushed:
d80c63d Attach cancellable to async result and check in finish functions
Comment 12 Benjamin Otte (Company) 2010-04-02 14:54:08 UTC
Created attachment 157759 [details] [review]
Attach cancellable to async result and check in finish functions

This makes sure we always return ERROR_CANCELLED, just like we want to
guarantee and avoids crashes in the file chooser.

There should be API in GSimpleAsyncResult to attach a GCancellable that
does this job for us, but so far there isn't, so we use
g_object_get/set_data() and check the cancellables manually in the
finish function.