GNOME Bugzilla – Bug 639254
invalid refcounting
Last modified: 2011-01-28 17:33:07 UTC
Created attachment 178088 [details] full example application that produces the problem It seems like clearing a list in gee does not deref its elements, while remove_at call does. By filing the bug under Vala I have made the assumption that Gee.LinkedList.clear()'s nulling of head and tail should cause a deref of all elements. Gee.LinkedList.clear() source: http://git.gnome.org/browse/libgee/tree/gee/linkedlist.vala#n135 Here is a short test application: public static void test_clear () { GLib.debug ("-----clear------"); var thing = new Thing (); GLib.debug ("%d", thing.references ()); var list = new Gee.LinkedList<Thing>(); list.add(thing); GLib.debug ("%d", thing.references ()); list.clear (); GLib.debug ("%d", thing.references ()); } public static void test_remove () { GLib.debug ("-----remove-----"); var thing = new Thing (); GLib.debug ("%d", thing.references ()); var list = new Gee.LinkedList<Thing>(); list.add(thing); GLib.debug ("%d", thing.references ()); list.remove_at (0); GLib.debug ("%d", thing.references ()); } public static void main (string[] args) { test_clear (); test_remove (); } For full source (.c/.h/.vapi/.vala/Makefile) see the attached tarball. The output of the test application with valac 0.10.0 (latest stable in Ubuntu Natty), and libgee 0.6.0-1ubuntu2 is the following: ** (process:1377): DEBUG: test.vala:3: -----clear------ ** (process:1377): DEBUG: test.vala:5: 1 ** (process:1377): DEBUG: test.vala:10: 2 ** (process:1377): DEBUG: test.vala:12: 2 ** (process:1377): DEBUG: test.vala:17: -----remove----- ** (process:1377): DEBUG: test.vala:20: 1 ** (process:1377): DEBUG: test.vala:25: 2 ** (process:1377): DEBUG: test.vala:27: 1
I'm not sure this bug still exists - with this full program: =================================================== using Gee; public class Thing : Object { public uint references { get { return this.ref_count; } } } public static void test_clear () { GLib.debug ("-----clear------"); var thing = new Thing (); GLib.debug ("%u", thing.references); var list = new Gee.LinkedList<Thing>(); list.add(thing); GLib.debug ("%u", thing.references); list.clear (); GLib.debug ("%u", thing.references); } public static void test_remove () { GLib.debug ("-----remove-----"); var thing = new Thing (); GLib.debug ("%u", thing.references); var list = new Gee.LinkedList<Thing>(); list.add(thing); GLib.debug ("%u", thing.references); list.remove_at (0); GLib.debug ("%u", thing.references); } public static void main (string[] args) { test_clear (); test_remove (); } =================================================== I get the following output: ** (process:26594): DEBUG: test.vala:10: -----clear------ ** (process:26594): DEBUG: test.vala:12: 1 ** (process:26594): DEBUG: test.vala:17: 2 ** (process:26594): DEBUG: test.vala:19: 1 ** (process:26594): DEBUG: test.vala:24: -----remove----- ** (process:26594): DEBUG: test.vala:27: 1 ** (process:26594): DEBUG: test.vala:32: 2 ** (process:26594): DEBUG: test.vala:34: 1 This is with Vala 0.11.3.14-1eee and libgee 0.6 (from the source tarball).
(In reply to comment #1) ... > This is with Vala 0.11.3.14-1eee and libgee 0.6 (from the source tarball). And the same with Vala 0.11.5.3-fe751 and libgee 0.6 (from source tarball, recompiled with this newer valac)
There is definitely still some leaking going on, though (both with libgee 0.6 and libgee from master (0.7)). Valgrind reports it as happening on the add. See bug #640551
Actually, this still is a problem. I'm trying to work out a string leak in LinkedList.add() in a test case for Folks, and clear() fails to free the appropriate memory. So maybe something was fixed wrt the simple test case above, but there are clearly still issues. I've been scrubbing through the LinkedList Vala and generated C without much luck though.
(In reply to comment #4) > I've been scrubbing through the LinkedList Vala and generated C without much > luck though. The latest bit I've found is that this._head is often null by the time the LinkedList is finalized (which may mean it's getting set to NULL at the C level when it shouldn't be and/or its contents just aren't being freed at that time).
I've figured out the core problem. Here are LinkedList.Node's free and finalize functions in C: #define _gee_linked_list_node_free0(var) ((var == NULL) ? NULL : (var = (gee_linked_list_node_free (var), NULL))) static void gee_linked_list_node_free (GeeLinkedListNode* self) { _gee_linked_list_node_free0 (self->next); g_slice_free (GeeLinkedListNode, self); } static void gee_linked_list_finalize (GObject* obj) { GeeLinkedList * self; self = GEE_LINKED_LIST (obj); _gee_linked_list_node_free0 (self->priv->_head); G_OBJECT_CLASS (gee_linked_list_parent_class)->finalize (obj); } The actual data (self->data, which is LinkedList.Node.data in Vala) never gets destroyed when the Node list gets finalized. Normally it would, but Node takes its elements as owned and then fails to free them. In fact, it doesn't even have the data's free function in scope to call it. I tried adding this to the beginning of LinkedList.clear() and calling clear() from an otherwise-empty ~LinkedList(): /* simply setting this._head = null doesn't free its data contents */ for (weak Node<G> n = this._head; n != null; n = n.next) { this._remove_node (n); } since LinkedList.remove() normally does succeed in freeing the memory that would otherwise be leaked. However, it doesn't work here (I guess _gee_linked_list_node_free0 (self->priv->_head); or an equivalent is getting called before the LinkedList is getting destroyed).
Created attachment 179470 [details] [review] Don't leak memory when clearing or finalizing a LinkedList This fixes the problem I described in comment #6. Now we always remove all nodes when clear()ing and call clear() when finalizing (like HashSet). It means we don't need to have the Nodes duplicate data (especially non-Objects, where it's expensive) or even know how to destroy their data explicitly.
*** Bug 640551 has been marked as a duplicate of this bug. ***
commit fead26c06f5667b72cecd41ee4bcc81908dfc12a Author: Jürg Billeter <j@bitron.ch> Date: Fri Jan 28 18:28:49 2011 +0100 Fix memory leak in LinkedList.clear Based on patch by Travis Reitter, fixes bug 639254.