GNOME Bugzilla – Bug 600992
File chooser reference counting issues
Last modified: 2010-03-09 05:06:35 UTC
My application (HandBrake) monitors for disc removal and insertion with GVolumeMonitor. About every 30th time the disc is ejected, I get the collection of critical messages shown below. About every 100th time I get a segfault. Also below is a backtrace that shows what modules were involved in the problem. I've seen this on Ubuntu 9.10 and Pre-fedora 12. (ghb:20578): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (ghb:20578): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (ghb:20578): GLib-GObject-WARNING **: instance of invalid non-instantiatable type `(null)' (ghb:20578): GLib-GObject-CRITICAL **: g_signal_emit_by_name: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed (ghb:20578): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (ghb:20578): GLib-GObject-CRITICAL **: g_value_type_compatible: assertion `G_TYPE_IS_VALUE (src_type)' failed (ghb:20578): GLib-GObject-WARNING **: gsignal.c:2961: invalid object type ` (ghb:20578): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (ghb:20578): GLib-GObject-CRITICAL **: g_object_run_dispose: assertion `G_IS_OBJECT (object)' failed (ghb:20578): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed
+ Trace 218879
Please install debug symbols for libglib/gio/gtk+/gvfs/etc and get a backtrace from the first critical warning.
Your wish is my command.
+ Trace 218889
(In reply to comment #0) > My application (HandBrake) monitors for disc removal and insertion with > GVolumeMonitor. About every 30th time the disc is ejected, I get the > collection of critical messages shown below. About every 100th time I get a > segfault. Also below is a backtrace that shows what modules were involved in > the problem. I've seen this on Ubuntu 9.10 and Pre-fedora 12. The stack trace in comment 2 indicates this happens in the GTK+ file chooser. So either this happens when you are showing a file chooser or maybe your app is leaking one? Are you running Nautilus or the panel when this happens? Do either of these crash? If no, then it's probably not a bug in the proxy volume monitor code (they receive the same signals from the volume monitor process). Otherwise, looks to me like the root bug is in the file chooser - probably the file chooser is forgetting to disconnect a signal on GVolumeMonitor. If you start, say, gedit and bring up the file chooser, does that process also crash at the same time?
My app has a couple file chooser buttons and a file chooser dialog that is hidden when not in use. I was not running nautilus at the time of the test. The panel survives. I'll try the gedit test tomorrow and get back to you on that.
Created attachment 147227 [details] Test program to reproduce crash
I tried to reproduce with gedit, and could not. So I created a minimal program that reproduces the problem. Although in this case the failure seems to happen in a different place, see backtrace. The test program is attached. To reproduce the problem I have to: 1. Instantiate a GtkFileChooserDialog 2. monitor drive-changed events from GVolumeMonitor 3. upon drive-changed, start a thread that gets a list of drives from GVolumeMonitor, then unref them in the thread. The problem does not seem to occur if either 1 or 3 are omitted from the test. Backtrace:
+ Trace 218936
While fiddling around to see if I could work around this problem, I found that it is not necessary to do step 3 above on a separate thread to provoke the problem. Just performing those operations in the drive-changed callback will trigger a segfault. I'm attaching a simplified code example.
Created attachment 147401 [details] Simplified example code that causes segfault
(In reply to comment #8) > Created an attachment (id=147401) [details] > Simplified example code that causes segfault Thanks, I can easily reproduce this by plugging in / removing a SD card a couple of times. FWIW, it only happens when there's a file chooser so I think there's something wrong in that code.
Actually I can reproduce this with gedit (with a open file chooser) as well.
Reassigning to the file chooser. I just looked at the code (in the gtk-2-18 branch) together with Matthias and there's some funky ref/unref logic (or lack of) going on. And the panel and nautilus, both heavy users of the GVolumeMonitor/GDrive/GVolume/GMount code never crashes. So I'm pretty sure this is not a bug in the GVfs proxy volume monitor code I'm also going to try this with gtk+'s master branch - Company's file chooser rewrite might fix this. (In reply to comment #4) > My app has a couple file chooser buttons and a file chooser dialog that is > hidden when not in use. I was not running nautilus at the time of the test. > The panel survives. I'll try the gedit test tomorrow and get back to you on > that. John, one work-around that might work is to avoid having a file chooser dialog that is hidden. Just construct a new one when you need it and destroy it when you are done with it.
(In reply to comment #11) > I'm also going to try this with gtk+'s master branch - Company's file chooser > rewrite might fix this. Same issue. Not surprising since 'git diff master origin/gtk-2-18 gtkfilesystem.c' is empty...
Created attachment 147433 [details] [review] Proposed patch Matthias asked me to try this patch - it worked for me. John, can you try it out and see if it fixes the problems on your end too? Thanks.
To point out the obvious, this patch fixes a few leaks in gtkfilesystem.c:get_volumes_list and among the calls of _gtk_file_system_get_volume_for_file. And it probably fixes the crash by removing the _gtk_file_system_volume_free calls for the GtkFileSystemVolume pointers that we stuff in the shortcuts model. This is necessary since most of those are 'non-allocated' pointers that we got from _gtk_file_system_list_volumes. On the flip side, this probably introduces a small leak again because we also sometimes insert allocated volumes gotten from _gtk_file_system_get_volume_for_file (see e.g. shortcuts_add_current_folder)
David, regarding instantiating a new file chooser dialog when needed, that won't help because I also have a couple file chooser buttons. File chooser buttons create their own private file chooser dialog when the button is created. I'll give the patch a try. By the sound of it, it will do the trick.
Created attachment 147436 [details] [review] another patch I've since discovered more volume-related issues; here is an alternative patch. This one make _gtk_file_system_list_volumes() return new references; most callers kinda expected that anyway...
I hammered away on the first patch for a while and it seemed to work fine. Then I saw your second patch and hammered on it for a while as well. Both seem to fix my problem.
Thanks for testing !
Review of attachment 147436 [details] [review]: Except for the comment in gtkfilechooserdefault.c, everything else looks correct. Thanks for that; debugging refcounting is always a pain in the ass. ::: pristine/gtk/gtkfilechooserdefault.c @@ +2095,3 @@ + { + _gtk_file_system_volume_free (volume); + continue; This seems wrong. We are in a "for (all volumes)" loop. However, none of the functions called from that loop seem to take references to the volume. So, all the loop's iterations should unref the volume. It would be cleaner to just unref each volume in the loop (or in a separate g_slist_foreach()), and just let each of the called functions ref the volume if they need to keep it around.
There is no api to ref GtkFileSystemVolume objects, only the _free function :-( That is why I changed the list_volumes function to return new references. Which is why we need to drop the reference in the continue case. If we don't hit the continue case, we keep the reference to stuff it in the shortcuts model. It gets dropped in shortcuts_free_row_data
GtkFileSystemVolume is internal to GTK+, so we can easily have ref/unref functions for it without breaking the ABI. This may make things easier to maintain in the future, rather than being extra-careful about when to free things in loops.
*** Bug 602964 has been marked as a duplicate of this bug. ***
Created attachment 155614 [details] [review] another iteration
This patch replaces _gtk_file_system_volume_free with _ref/_unref