Re: [Evolution-hackers] Possible memory leak

2008-01-15 Thread Jose Dapena Paz 

El lun, 14-01-2008 a las 14:33 +0100, Philip Van Hoof escribió:

> I think this needs to be something like this:
> 
> GSList *item = cc->status_stack;
> cc->status_stack = g_slist_remove_link(cc->status_stack, item);
> g_slist_free (item);

Or just use g_slist_delete_link:

cc->status_stack = g_slist_delete_link (cc->status_stack, cc->status_stack);

AFAIK it frees the list node.

-- 
Jose Dapena Paz <[EMAIL PROTECTED]>
Igalia
___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers

Re: [Evolution-hackers] Possible memory leak

2008-01-15 Thread Philip Van Hoof 

On Tue, 2008-01-15 at 11:55 +0100, Jose Dapena Paz wrote:
> El lun, 14-01-2008 a las 14:33 +0100, Philip Van Hoof escribió:
> 
> > I think this needs to be something like this:
> > 
> > GSList *item = cc->status_stack;
> > cc->status_stack = g_slist_remove_link(cc->status_stack, item);
> > g_slist_free (item);
> 
> Or just use g_slist_delete_link:
> 
> cc->status_stack = g_slist_delete_link (cc->status_stack, cc->status_stack);

> AFAIK it frees the list node.

This is what got committed, indeed.

-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be




___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers

Re: [Evolution-hackers] Possible memory leak

2008-01-14 Thread Matthew Barnes 
On Mon, 2008-01-14 at 14:33 +0100, Philip Van Hoof wrote:
> I think I've found the leak ...
> 
> g_slist_remove_link:
> 
> Removes an element from a GSList, without freeing the element. The
> removed element's next link is set to NULL, so that it becomes a
> self-contained list with one element.
> 
> Notice the "without freeing the element"
> 
> Yet 
> 
> void
> camel_operation_end (CamelOperation *cc)
> {
>   ...
> 
>   g_free(s);
>   cc->status_stack = g_slist_remove_link(cc->status_stack,
>cc->status_stack);
>   UNLOCK();
> 
>   if (msg) {
>   cc->status(cc, msg, sofar, oftotal, cc->status_data);
>   g_free(msg);
>   }
> }


g_slist_delete_link() frees the GSList node, whereas
g_slist_remove_link() just disconnects the node from the list.

If I'm reading this right, the preceding logic frees the list node
contents, so I think we just need to use g_slist_delete_link() in place
of g_slist_remove_link().  One line change.

void
camel_operation_end (CamelOperation *cc)
{
...
s = cc->status_stack->data;
...
g_free (s);
cc->status_stack = g_slist_delete_link (
cc->status_stack, cc->status_stack);
...
}

___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers

Re: [Evolution-hackers] Possible memory leak

2008-01-14 Thread Philip Van Hoof 

I think I've found the leak ...

g_slist_remove_link:

Removes an element from a GSList, without freeing the element. The
removed element's next link is set to NULL, so that it becomes a
self-contained list with one element.

Notice the "without freeing the element"

Yet 

void
camel_operation_end (CamelOperation *cc)
{
...

g_free(s);
cc->status_stack = g_slist_remove_link(cc->status_stack,
 cc->status_stack);
UNLOCK();

if (msg) {
cc->status(cc, msg, sofar, oftotal, cc->status_data);
g_free(msg);
}
}

I think this needs to be something like this:

GSList *item = cc->status_stack;
cc->status_stack = g_slist_remove_link(cc->status_stack, item);
g_slist_free (item);


Can somebody with GSList know-how acknowledge this?



On Mon, 2008-01-14 at 02:28 +0100, Philip Van Hoof wrote:
> On Mon, 2008-01-14 at 01:47 +0100, Philip Van Hoof wrote:
> 
> > I have this memory analysis tool that I'm willing to believe that tells
> > me this line in camel-folder.c causes 381 times a leaked allocation of
> > in total 5.95 kb. (I opened one folder of 1000 items). It seems to be
> > the g_slice_alloc (which does a malloc since I disabled gslice) of the
> > g_hash_node_new.
> 
> I found it, this was my own mistake in camel-lite's adaptations.
> 
> I have found another leak in camel-operation.c:
> 
> The cc->status_stack = g_slist_prepend(cc->status_stack, s); in
> camel_operation_start seems to be leaked each time it's called.
> 
> I have 23 calls leaking 184 bytes. Various callers of
> camel_operation_start (all of them, it seems) seem to create this leak.
> 
> My version of camel-operation.c adds two fields (a total and a current
> position in stead of a percentage, because my requirements where to give
> accurate to-the-byte progress info, not rounded percentages). Those
> changes are closely reviewed once more and don't seem to be causing this
> leak.
> 
> ps. I attached the differences that I have. I basically replaced "pc"
> with a "sofar" and a "oftotal".
> 
> 
> ___
> Evolution-hackers mailing list
> Evolution-hackers@gnome.org
> http://mail.gnome.org/mailman/listinfo/evolution-hackers
-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be




___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers

Re: [Evolution-hackers] Possible memory leak

2008-01-13 Thread Philip Van Hoof 

On Mon, 2008-01-14 at 01:47 +0100, Philip Van Hoof wrote:

> I have this memory analysis tool that I'm willing to believe that tells
> me this line in camel-folder.c causes 381 times a leaked allocation of
> in total 5.95 kb. (I opened one folder of 1000 items). It seems to be
> the g_slice_alloc (which does a malloc since I disabled gslice) of the
> g_hash_node_new.

I found it, this was my own mistake in camel-lite's adaptations.

I have found another leak in camel-operation.c:

The cc->status_stack = g_slist_prepend(cc->status_stack, s); in
camel_operation_start seems to be leaked each time it's called.

I have 23 calls leaking 184 bytes. Various callers of
camel_operation_start (all of them, it seems) seem to create this leak.

My version of camel-operation.c adds two fields (a total and a current
position in stead of a percentage, because my requirements where to give
accurate to-the-byte progress info, not rounded percentages). Those
changes are closely reviewed once more and don't seem to be causing this
leak.

ps. I attached the differences that I have. I basically replaced "pc"
with a "sofar" and a "oftotal".


-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be
--- camel-operation.c	2007-12-23 18:12:33.0 +0100
+++ /home/pvanhoof/repos/tinymail/trunk/libtinymail-camel/camel-lite/camel/camel-operation.c	2007-11-15 10:42:00.0 +0100
@@ -46,7 +46,7 @@
 struct _status_stack {
 	guint32 flags;
 	char *msg;
-	int pc;/* last pc reported */
+	int sofar, oftotal;
 	unsigned int stamp;		/* last stamp reported */
 };
 
@@ -514,12 +514,14 @@
 	s = g_malloc0(sizeof(*s));
 	s->msg = msg;
 	s->flags = 0;
+	s->sofar = 0;
+	s->oftotal = 100;
 	cc->lastreport = s;
 	cc->status_stack = g_slist_prepend(cc->status_stack, s);
 
 	UNLOCK();
 
-	cc->status(cc, msg, CAMEL_OPERATION_START, cc->status_data);
+	cc->status(cc, msg, CAMEL_OPERATION_START, 100, cc->status_data);
 
 	d(printf("start '%s'\n", msg, pc));
 }
@@ -578,17 +580,17 @@
 /**
  * camel_operation_progress:
  * @cc: Operation to report to.
- * @pc: Percent complete, 0 to 100.
+ * @sofar: Amount complete
+ * @oftotal: Max amount (or, amount when completed)
  *
  * Report progress on the current operation.  If @cc is NULL, then the
- * currently registered operation is used.  @pc reports the current
- * percentage of completion, which should be in the range of 0 to 100.
+ * currently registered operation is used.  @
  *
  * If the total percentage is not know, then use
  * camel_operation_progress_count().
  **/
 void
-camel_operation_progress (CamelOperation *cc, int pc)
+camel_operation_progress (CamelOperation *cc, int sofar, int oftotal)
 {
 	unsigned int now;
 	struct _status_stack *s;
@@ -608,7 +610,8 @@
 	}
 
 	s = cc->status_stack->data;
-	s->pc = pc;
+	s->sofar = sofar;
+	s->oftotal = oftotal;
 
 	/* Transient messages dont start updating till 4 seconds after
 	   they started, then they update every second */
@@ -632,7 +635,7 @@
 	UNLOCK();
 
 	if (cc) {
-		cc->status(cc, msg, pc, cc->status_data);
+		cc->status(cc, msg, sofar, oftotal, cc->status_data);
 		g_free(msg);
 	}
 }
@@ -646,7 +649,7 @@
 void
 camel_operation_progress_count (CamelOperation *cc, int sofar)
 {
-	camel_operation_progress(cc, sofar);
+	camel_operation_progress(cc, sofar, 100);
 }
 
 /**
@@ -664,7 +667,8 @@
 	struct _status_stack *s, *p;
 	unsigned int now;
 	char *msg = NULL;
-	int pc = 0;
+	int sofar = 0;
+	int oftotal = 100;
 
 	if (cc == NULL)
 		cc = co_getcc();
@@ -693,13 +697,15 @@
 if (p->flags & CAMEL_OPERATION_TRANSIENT) {
 	if (p->stamp + CAMEL_OPERATION_TRANSIENT_DELAY < now) {
 		msg = g_strdup(p->msg);
-		pc = p->pc;
+		sofar = p->sofar;
+		oftotal = p->oftotal;
 		cc->lastreport = p;
 		break;
 	}
 } else {
 	msg = g_strdup(p->msg);
-	pc = p->pc;
+	sofar = p->sofar;
+	oftotal = p->oftotal;
 	cc->lastreport = p;
 	break;
 }
@@ -709,7 +715,8 @@
 		g_free(s->msg);
 	} else {
 		msg = s->msg;
-		pc = CAMEL_OPERATION_END;
+		sofar = CAMEL_OPERATION_END;
+		oftotal = 100;
 		cc->lastreport = s;
 	}
 	g_free(s);
@@ -718,7 +725,7 @@
 	UNLOCK();
 
 	if (msg) {
-		cc->status(cc, msg, pc, cc->status_data);
+		cc->status(cc, msg, sofar, oftotal, cc->status_data);
 		g_free(msg);
 	}
 }
___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers

[Evolution-hackers] Possible memory leak

2008-01-13 Thread Philip Van Hoof 

Hi there,

I have this memory analysis tool that I'm willing to believe that tells
me this line in camel-folder.c causes 381 times a leaked allocation of
in total 5.95 kb. (I opened one folder of 1000 items). It seems to be
the g_slice_alloc (which does a malloc since I disabled gslice) of the
g_hash_node_new.

camel-folder.c:2255 (last line of camel_folder_change_info_add_uid)

g_hash_table_insert(p->uid_stored, olduid, info->uid_added);

I'm not immediately seeing it myself (it looks like p->uid_stored is
destroyed in the finalise indeed). But just FYI ... it might be
interesting to monitor this one.

If each new mail causes a GHashNode leak .. that might be quite a lot of
memory in Evolution.

ps. The caller for the camelfolderchange was imap_update_summary, but I
already checked that the change instance is finalised and that seems to
be the case indeed.


-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be




___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers