After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 639254 - invalid refcounting
invalid refcounting
Status: RESOLVED FIXED
Product: libgee
Classification: Platform
Component: general
0.6.x
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
: 640551 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-01-11 21:32 UTC by Daniel Svensson
Modified: 2011-01-28 17:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
full example application that produces the problem (861 bytes, application/x-compressed-tar)
2011-01-11 21:32 UTC, Daniel Svensson
  Details
Don't leak memory when clearing or finalizing a LinkedList (1.22 KB, patch)
2011-01-27 20:22 UTC, Travis Reitter
none Details | Review

Description Daniel Svensson 2011-01-11 21:32:40 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
Comment 1 Travis Reitter 2011-01-25 19:49:45 UTC
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).
Comment 2 Travis Reitter 2011-01-25 19:54:52 UTC
(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)
Comment 3 Travis Reitter 2011-01-25 23:36:04 UTC
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
Comment 4 Travis Reitter 2011-01-27 19:00:30 UTC
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.
Comment 5 Travis Reitter 2011-01-27 19:03:26 UTC
(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).
Comment 6 Travis Reitter 2011-01-27 20:00:43 UTC
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).
Comment 7 Travis Reitter 2011-01-27 20:22:12 UTC
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.
Comment 8 Travis Reitter 2011-01-27 20:46:49 UTC
*** Bug 640551 has been marked as a duplicate of this bug. ***
Comment 9 Jürg Billeter 2011-01-28 17:33:07 UTC
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.