Re: [Evolution-hackers] Memory leak in camel-imap-message-cache.c
On Mon, 2008-01-14 at 16:02 +0530, Srinivasa Ragavan wrote: > Look at the free_part function being called at > g_hash_table_foreach (cache->parts, free_part, cache); > > It frees the key. Aha. Okay thanks for the clarification. I wonder, wouldn't it be better to use the standard infrastructure of GHashTable to clearup keys? The full version of the GHashTable's constructor has a callback function pointer parameter that'll be used for freeing up keys like this. I of course also wonder howcome my memory analysis tool pointed me to this memory leak. Although it might be explained by one CamelMessage reference still being open when the exit(0) took place. > On Mon, 2008-01-14 at 02:58 +0100, Philip Van Hoof wrote: > > Hi there, > > > > The cache->parts = g_hash_table_new (g_str_hash, g_str_equal); of > > camel-imap-message-cache.c does not free its keys (it's the default way > > of creating a hashtable, so there's no freeup function provided for the > > keys). > > > > Yet at (or around) line 118 we see this: > > > > g_hash_table_insert (cache->parts, g_strdup (uid), subparts); > > > > That uid is string-copied. So either the hashtable needs a freeup > > function or the string should not be copied or .. this is wrong. > > > > Because I don't know how important the string copying is, what do you > > guys think we should do here? > > > > > -- 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] Memory leak in camel-imap-message-cache.c
Look at the free_part function being called at g_hash_table_foreach (cache->parts, free_part, cache); It frees the key. -Srini. On Mon, 2008-01-14 at 02:58 +0100, Philip Van Hoof wrote: > Hi there, > > The cache->parts = g_hash_table_new (g_str_hash, g_str_equal); of > camel-imap-message-cache.c does not free its keys (it's the default way > of creating a hashtable, so there's no freeup function provided for the > keys). > > Yet at (or around) line 118 we see this: > > g_hash_table_insert (cache->parts, g_strdup (uid), subparts); > > That uid is string-copied. So either the hashtable needs a freeup > function or the string should not be copied or .. this is wrong. > > Because I don't know how important the string copying is, what do you > guys think we should do here? > > ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Possible memory leak
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
[Evolution-hackers] Patch: memoryleak fix in CamelOperation. Was: Possible memory leak
Please review
http://bugzilla.gnome.org/show_bug.cgi?id=509370
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);
> }
> }
>
> 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
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] Memory leak in camel-imap-message-cache.c
On Mon, 2008-01-14 at 11:45 +0100, Philip Van Hoof wrote: > On Mon, 2008-01-14 at 16:02 +0530, Srinivasa Ragavan wrote: > > Look at the free_part function being called at > > g_hash_table_foreach (cache->parts, free_part, cache); > > > > It frees the key. > > Aha. Okay thanks for the clarification. > > I wonder, wouldn't it be better to use the standard infrastructure of > GHashTable to clearup keys? The full version of the GHashTable's > constructor has a callback function pointer parameter that'll be used > for freeing up keys like this. I think it needs to be done that way. But just a clean up work IMO. > I of course also wonder howcome my memory analysis tool pointed me to > this memory leak. Although it might be explained by one CamelMessage > reference still being open when the exit(0) took place. > I use totalview to analyze memory. I dont see that from it. May be at some specific scenarios. -Srini. ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
[Evolution-hackers] Evolution 2.21.5 , Evolution-Data-Server 2.21.5 , GtkHTML3.17.5 and Evolution-Exchange 2.21.5 released
Hi All, The Evolution Team is pleased to announce the release of Evolution 2.21.5. You can download the following : http://ftp.acc.umu.se/pub/gnome/sources/gtkhtml/3.17/gtkhtml-3.17.5.tar.bz2 http://ftp.acc.umu.se/pub/gnome/sources/evolution-data-server/2.21/evolution-data-server-2.21.5.tar.bz2 http://ftp.acc.umu.se/pub/gnome/sources/evolution/2.21/evolution-2.21.5.tar.bz2 http://ftp.acc.umu.se/pub/gnome/sources/evolution-exchange/2.21/evolution-exchange-2.21.5.tar.bz2 Upgrade Notes : Evolution 2.21.x is the unstable series of 2.22 development. (Note: Evolution/EDS/Exchange versions are synced with the GNOME versions) RELNOTE: If you have created labels/tags using Evolution 2.21.4. You need to delete them and recreate using 2.21.5. We have changes the way labels are stored now. It should be final now. What is New ? = Evolution: == New in 2.21.5: Mail errors are now non-intrusive (Srinivasa Ragavan) RTL fixes - Mail and Addressbook (Djihed Afifi) Message Tagging (aka custom labels) Improvements (Milan Crha) Bug fixes: #211353: Allow categories to be assigned to emails (Milan Crha) #219197: Quit application when session dies (Milan Crha) #270605: Skip disabled accounts. (Suman Manjunath) #300336: Added checkbox for "Enable Search Folders" option (Milan Crha) #300336: Ensure vfolder is running. (Milan Crha) #309432: Fix message headers for RTL languages. (Djihed Afifi) #317823: Bump GtkHTML requirement to 3.17.5 (Matthew Barnes) #317823: Save inline pictures embedded into HTML Mails (Milan Crha) #327965: Fixed multiple password prompts in an exchange account (Sushma Rai) #329692: Get the content size of the MIME part (Jean-Christophe BEGUE) #333695: Print attenddes/roles in the print view (Milan Crha) #339813: Setting new option 'e_date_edit_set_twodigit_year_can_future' to FALSE (Milan Crha) #348638: Remove pre-edit buffer cleanly in day view (Mayank Jain) #350932: Enable the use of scrollable tabs in the mail-preferences dialog. (Gert Kulyk) #362638: Overhaul the message passing API (Matthew Barnes) #364642: New option in Composer tab to preset Request Read Receipt in composer (Milan Crha) #375580: Use ISO-8859-1 encoding to store contacts in iPod (João Vale) #448441: Disable "OK" and "Edit Full" buttons if no source is selected. Also set always book from combo, do not use the new created (Milan Crha) #457842: Do not call edit/start editing of the event when double clicked on the same component as is actually editing (Milan Crha) #474118: Check for the right type of store and invoke appropriate functions (Bharath Acharya) #476264: Add mnemonic_widget for default junk plugin (Andre Klapper) #488213: Fix locks when displaying e-mail with large tiff drawing attached (Milan Crha) #492188: Use the new Tangoized icons instead of deprecated icons from gnome-icon-theme. (Michael Monreal) #492702: Just disable the dbus message part of mail notification if dbus isn't there. Also remove new-mail-notify plugin (Srinivasa Ragavan) #496301: Clean up the schema (Martin Meyer) #496402: Do not synchronize blocked bodies from pidgin (Milan Crha) #497914: backport changes from the copy/pasted code in imap-headers plugin (Gilles Dartiguelongue) #498095: Fix mnemonic issues (David Turner) #499145: Follow RFC 3798 to send return receipts. (Colin Leroy) #502303: Plugins configure widgets are not freed correctly in plugin-mamager (Milan Crha) #502783: Restore message states (read receipt/priority) when opening from a draft ([EMAIL PROTECTED]) #502914: Do not write NULL into gconf (Milan Crha) #503954: Accept custom domain names while setting up account (Nyall Dawson) #504480: Increases the height of the ETaskBar to eliminate the constant resizing (Milan Crha) #504541: Add --with[out]-help option to make it possible to skip building and installing user documentation (Matthew Barnes) #506772: Not-NULL check for a string array before finding its length (Christian Krause) #506814: Add the signal only if the view is present (Srinivasa Ragavan) #507067: Can't save pictures embedded in HTML Mail when picture = link (Milan Crha) #507311: Submit bugs to the "BugBuddyBugs" Bugzilla component (Matthew Barnes) #507359: #507363: Also check if toplevel widget has non-NULL window property (Milan Crha) #508282: Mark the window title for translation. (Changwoo Ryu) #508678: Included missing header glib/gi18n.h (Bharath Acharya) #508731: Have a safe default, if the values from gconf isn't so nice. (Srinivasa Ragavan) Updated Translations: Amitakhya Phukan (as) Espen Stefansen (nb) Jorge Gonzalez (es)
