GNOME Bugzilla – Bug 145200
Object leak in FileChooser and corresponding Bonobo reference leak.
Last modified: 2009-02-26 00:55:13 UTC
Usage of gnome-vfs based file chooser brokes reference counting of Bonobo. The reason for that is that on gtkfilesystemgnomevfs.c gnome-vfs volume manager is created with refcount 2 by this line: system_vfs->volume_monitor = gnome_vfs_volume_monitor_ref (gnome_vfs_get_volume_monitor ()); On finalize, only it is unreffered only with gnome_vfs_volume_monitor_unref (system_vfs->volume_monitor); So, GnomeVfsVolumeMonitor is not properly finalized. These as consequence causes BonoboReference leak, since VolumeMonitor creates BonoboObject GnomeVFSClient. As a result, all bonobo reference counting is broken, an a control that use FileChooser will never be properly finalized. I think, the reason is incorrect abi of libgnomevfs, since in one case after creation GnomeVFSVolumeMonitor * _gnome_vfs_get_volume_monitor_internal (gboolean create) { G_LOCK (the_volume_monitor); if (the_volume_monitor == NULL && create && !volume_monitor_was_shutdown) { if (gnome_vfs_get_is_daemon ()) { the_volume_monitor = g_object_new (GNOME_VFS_TYPE_VOLUME_MONITOR_DAEMON, NULL); } else { the_volume_monitor = g_object_new (GNOME_VFS_TYPE_VOLUME_MONITOR_CLIENT, NULL); } } G_UNLOCK (the_volume_monitor); return the_volume_monitor; } bumps ref count to 1, in other cases, it leaves unmodified. For consitensy, I think that even if object is not created, it refcount should be incremented. That will allow to implement proper object management in gtkfilesystemgnomevfs.c
There is a number of problem with previous solution. GnomeVfsMonitor designed to be a global structure, that should not use gobject memory management, instead it should be finalized manually with volume_monitor shutdown. So, probably it would be better to just remove GnomeVfsClient from bonobo object management by bonobo_running_context_ignore.
OK, there are two parts to the problem: 1. GtkFileSystemGnomeVFS indeed creates an unneeded reference to the volume monitor; 2. _gnome_vfs_get_volume_monitor_internal() does not reference the volume monitor if it already exists. I filed bug #151463 for the second one. I'm fixing the first one in libgnomeui.
Marking as major because this makes applications crash when you mount/unmount volumes.
You're not supposed to ref the return value from gnome_vfs_get_volume_monitor(). The volume monitor is a singleton that lives while the app does. So you can do things like: l = gnome_vfs_volume_monitor_get_mounted_volumes (gnome_vfs_get_volume_monitor()); I'm not sure why we export refcounting functions for it though. They seem to cause problems like in bug 151244.
The real bug here is that the file selector calls gnome_vfs_init(), but not gnome_vfs_shutdown(). Unfortunately, gnome_vfs_init() itself isn't refcounted, so if you call gnome_vfs_shutdown() several times it might cause problems.
OK, it makes sense for the volume monitor to be unkillable. I just assumed I had to use the refcounting functions because they were there. Calling gnome_vfs_init() multiple times is idempotent. This is good. But we can't really call gnome_vfs_shutdown() in the file chooser code, not even in the module's exit callback --- what if the app itself is using gnome-vfs. How bad is it if an app doesn't ever call gnome_vfs_shutdown()? I remember there being some problems in the past, but I don't remember what the problems were. I assume many apps still don't do that.
Maybe we could refcount the init? so gnome_vfs_init() would: if (count == 0) init() count++ and shutdown: count--; if (count == 0) shutdown() Of course, currently gnome-vfs doesn't handle re-initing after shutdown, so this might not be as useful as you think. I'm not sure not calling vfs_shutdown is that horrible. On clear problem is that the volume monitor leaks as reported in this bug.
What would it take to allow re-initing after shutdown? By "how horrible", I mean that I don't know if not calling shutdown leads to unkilled daemons and such. I haven't seen any problems so far, but I haven't really looked for them :)
Nah, the gnome-vfs daemon is very carefully constructed to cleanly handle unclean shutdowns on both sides. So, the only way to leak the daemon is to create a volume monitor and then hang the process.
OK, the libgnomeui part of this bug is fixed on CVS in both the gnome-2-6 and HEAD branches: 2004-08-30 Federico Mena Quintero <federico@ximian.com> Fix #145200: * file-chooser/gtkfilesystemgnomevfs.c (struct _GtkFileSystemGnomeVFS): Added fields to store the signal connection IDs for the volume monitor. (gtk_file_system_gnome_vfs_init): Store the signal connection IDs. Do not create an extra reference to the volume monitor. (gtk_file_system_gnome_vfs_finalize): Disconnect from the volume monitor's signals. For now, don't unref the monitor. See http://bugzilla.gnome.org/show_bug.cgi?id=151463 for reference.
Can you remove GnomeVfsClient from Bonobo's running context as described it comment #1? It causes problem with bonobo session management.....
What do you mean about session management? Can you explain what you mean in more detail.
There is a conception of RunningContext in bonobo. For example, you have some bonobo objects, implemented in binary. When you create object, the binary is forked from bonobo_activation_server. When after some time client unrefs object, binary exits, because there is no more objects in RunningContext. After application exit all components that are used by this application should exit and leave clean environment. Of course, you know it :) Imagine when such component creates file dialog, then GnomeVFSClient bonobo object is created in RunningContext. So that component will never exit, since there is no ability to execute unref on GnomeVFSClient (And I agree, there is no need in doing it). If you give no ability to unref GnomeVFSClient, please, remove GnomeVFSClient from BonoboRunningContext by calling bonobo_running_context_ignore.
Nickolay is correct; I've run into this issue myself in the past. By creating a VFS-enabled file chooser the running context never again emits a last-unref signal, and that causes bonobo generic factories to never _ever_ exit unless killed. The running context is documented here: http://developer.gnome.org/doc/API/2.0/libbonobo/libbonobo-bonobo-running-context.html "The running context is particularly useful for ensuring that server processes exit cleanly when all their objects and derived objects are dead." The function prototype is: void bonobo_running_context_ignore_object (CORBA_Object object);
Given the singleton nature of GnomeVFSClient, it does make sense to remove it from the running context, I think. I'd still prefer to have refcounted vfs_init() and vfs_shutdown() :)
Marking the bug as obsolete since the file chooser uses gio now instead of gnome-vfs.