GNOME Bugzilla – Bug 614099
[PATCH] GtkFileSystemModel may causes crash in g_file_get_child
Last modified: 2010-04-02 14:54:08 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:
+ Trace 221125
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
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...
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?
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.
Hmm, now I get a hang instead of a crash. More reproducable though! :)
+ Trace 221167
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.
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.
Nope. That patch gives the same hang and stacktrace as in comment 4...
You did unapply the first patch, right?
Yes, I started from scratch again. I can retest tonight if you think that result is outrageous.
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!
The following fix has been pushed: d80c63d Attach cancellable to async result and check in finish functions
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.