Re: [Nautilus-list] Memleak patch

[Darin Adler <darin bentspoon com>]

On Wednesday, August 22, 2001, at 03:13  PM, Anders Carlsson wrote:

> here's a patch that fixes some of the memory leaks I came across while
> using memprof to run nautilus.

This patch is great!

I love using leak checkers. I am so glad that memprof worked well for you 
with Nautilus.

And I'm so happy that someone finally fixed the 1-pixel splitter bug. 
There's a bug in bugzilla.eazel.com that you can close, but I don't know 
the number off hand.

> Most stuff is straightforward, the only
> semi-complex thing I do is put the volume_mount_uri inside a struct in
> nautilus-trash-directory.c so that it can be freed.

You actually don't need to do this. The GnomeVFSURI can just be unref'd 
just the call to gnome_vfs_async_find_directory -- it doesn't need to stay 
around until the call completes. The async. back end makes a copy of the 
URIs in the URI list. See line 598 of gnome-vfs/libgnomevfs-
pthread/gnome-vfs-async-ops.c.

> Index: components/sample/Makefile.am
> ===================================================================
> RCS file: /cvs/gnome/nautilus/components/sample/Makefile.am,v
> retrieving revision 1.28
> diff -u -r1.28 Makefile.am
> --- components/sample/Makefile.am	2001/07/21 17:06:44	1.28
> +++ components/sample/Makefile.am	2001/08/22 22:08:05
> @@ -12,6 +12,7 @@
>  	-DICON_DIR=\"$(datadir)/pixmaps/nautilus\" \
>  	-I$(top_srcdir)				\
>  	-I$(top_builddir)			\
> +	$(EEL_CFLAGS)				\
>  	$(GNOMEUI_CFLAGS)                       \
>  	$(BONOBO_CFLAGS)			\
>  	$(OAF_CFLAGS)
> @@ -26,6 +27,7 @@
>
>  nautilus_sample_content_view_LDADD =		\
>  	$(top_builddir)/libnautilus/libnautilus.la	\
> +	$(EEL_LIBS)				\
>  	$(BONOBO_LIBS)				\
>  	$(BONOBOX_LIBS)				\
>  	$(GCONF_LIBS)				\

I am slightly worried about this. Does this mean that developers working 
on nautilus components will have to have eel-devel installed?

> Index: libnautilus/nautilus-idle-queue.c
> ===================================================================
> RCS file: /cvs/gnome/nautilus/libnautilus/nautilus-idle-queue.c,v
> retrieving revision 1.3
> diff -u -r1.3 nautilus-idle-queue.c
> --- libnautilus/nautilus-idle-queue.c	2001/03/08 19:53:29	1.3
> +++ libnautilus/nautilus-idle-queue.c	2001/08/22 22:08:05
> @@ -26,6 +26,7 @@
>  #include <config.h>
>  #include "nautilus-idle-queue.h"
>
> +#include <eel/eel-glib-extensions.h>
>  #include <gtk/gtkmain.h>
>
>  struct NautilusIdleQueue {
> @@ -70,7 +71,7 @@
>  			}
>  		}
>
> -		g_list_free (functions);
> +		eel_g_list_free_deep (functions);
>  	}
>  	queue->in_idle = FALSE;
>
> @@ -130,7 +131,7 @@
>  		}
>  	}
>  	
> -	g_list_free (queue->functions);
> +	eel_g_list_free_deep (queue->functions);
>
>  	if (queue->idle_id != 0) {
>  		gtk_idle_remove (queue->idle_id);

In both cases, there's a loop just before the g_list_free statement that 
could just have a g_free added to it, then you wouldn't need to include 
eel, I think. So maybe we should go that way.

> -CONTROL_CENTER_REQUIRED=1.3

If you remove this from configure.in, you should also remove it from 
nautilus.spec.in to be polite to those who use that.

> Index: libnautilus-private/nautilus-file-changes-queue.c
> ===================================================================
> RCS file: /cvs/gnome/nautilus/libnautilus-
> private/nautilus-file-changes-queue.c,v
> retrieving revision 1.19
> diff -u -r1.19 nautilus-file-changes-queue.c
> --- libnautilus-private/nautilus-file-changes-queue.c	2001/05/11 01:30:30	
> 1.19
> +++ libnautilus-private/nautilus-file-changes-queue.c	2001/08/22 22:08:06
> @@ -547,5 +547,6 @@
>  		}
>  		change->from_uri = NULL;
>  		change->to_uri = NULL;
> +		g_free (change);
>  	}	
>  }

I would not bother nulling out fields in a struct that we're about to free.

> Index: libnautilus-private/nautilus-font-factory.c
> ===================================================================
> RCS file: /cvs/gnome/nautilus/libnautilus-private/nautilus-font-factory.c,
> v
> retrieving revision 1.16
> diff -u -r1.16 nautilus-font-factory.c
> --- libnautilus-private/nautilus-font-factory.c	2001/05/04 10:17:53	1.16
> +++ libnautilus-private/nautilus-font-factory.c	2001/08/22 22:08:06
> @@ -25,6 +25,7 @@
>  #include <config.h>
>  #include "nautilus-font-factory.h"
>
> +#include <gdk/gdkprivate.h>
>  #include "nautilus-global-preferences.h"
>  #include <eel/eel-gtk-macros.h>
>  #include <eel/eel-string.h>

What is this change about exactly? It's not mentioned in the change log 
entry.

> Index: src/nautilus-sidebar.c
> ===================================================================
> RCS file: /cvs/gnome/nautilus/src/nautilus-sidebar.c,v
> retrieving revision 1.185
> diff -u -r1.185 nautilus-sidebar.c
> --- src/nautilus-sidebar.c	2001/07/20 17:36:34	1.185
> +++ src/nautilus-sidebar.c	2001/08/22 22:08:09
> @@ -315,6 +315,11 @@
>  		nautilus_file_unref (sidebar->details->file);
>  	}
>
> +	/* If the title tab hasn't got a parent we have to free it 
> explicitly */
> +	if (GTK_WIDGET (sidebar->details->title_tab)->parent == NULL) {
> +		gtk_object_unref (GTK_OBJECT (sidebar->details->title_tab));
> +	}
> +	
>  	g_free (sidebar->details->uri);
>  	g_free (sidebar->details->default_background_color);
>  	g_free (sidebar->details->default_background_image);

I think you can just do an unconditional gtk_object_sink here instead, no?

> Index: src/file-manager/fm-properties-window.c

The changes in this file look like a separate feature to me.

    -- Darin