I can confirm it. It happens in Vala 0.5.4 (the one shipped with Ubuntu) and Vala 0.7.4.

I attach a sample code. The output of this code is:

References: 1d
References: 2d
References: 3d
References: 3d

But it should be

References: 1d
References: 2d
References: 3d
References: 2d

Created attachment 139734 [details]
Sample code which triggers the bug.

This code contains the sample commented in comment #1

Created attachment 139738 [details]
the same test with Gee and ArrayList

This is the same test but using Gee and ArrayList. This works as expected, so there is really a bug in Glib's List.

I tested the test code from #2 in Vala 0.7.5 and still fails.

afaik, there is nothing that can be done, because List doesn't do refcounting itself. As you said, you should use Gee.

perhaps reimplementing functions that remove elements from the list could be a solution though.

Marc-Andre: but, as you can see in the examples I uploaded, it is doing refcounting, but only when adding an element, not when overwritting. That's the bug. It should do it in both cases.

(In reply to comment #7)
> Marc-Andre: but, as you can see in the examples I uploaded, it is doing
> refcounting, but only when adding an element, not when overwritting. That's the
> bug. It should do it in both cases.

Adding a ref is easy, that's the way you pass refcounted references to functions that have "owned". On the other hand, remove() is different, because you don't know if the reference is actually removed.

Also, remove_all() is even worst. You don't know how many times the reference has been removed.

Well, I am not sure, but it's interesting to discuss it, as I have been bitten by this in the past.

(In reply to comment #6)
> perhaps reimplementing functions that remove elements from the list could be a
> solution though.

Yes; the following modification to the List binding fixes this example:


		[ReturnsModifiedPointer ()]
		[CCode (cname = "g_list_remove")]
		public unowned G _remove (G data);

		[ReturnsModifiedPointer ()]
		[CCode (cname = "_vala_list_remove")]
		public unowned G remove (G data)
		{
			foreach (var v in this) {
				if (v == data) {
					Object vo = v as Object;
					if (vo != null) {
						vo.unref ();
					}
					break;
				}
			}

			return _remove (data);
		}

Limitations:
1. Only works for contained Objects, not types with custom unref functions. 
2. Generates compiler warnings, since I had to change the return type of both functions from void to unowned G.

Similar things could be done with the other removal functions.

Regarding limitation 1, it seems to me that we should be able to simply write "var mine = (owned) v;" inside the loop so that it gets destroyed when it goes out of scope; however, that does not seem to happen (the generated C code deals with gpointers and seems unaware of the true type of the elements).

Regarding limitation 2, it should be possible to fix this by allowing (requiring) a void method to return a value of the appropriate type if it is marked as ReturnsModifiedPointer ().

An alternative approach might be to let the compiler handle all this, by introducing various new attributes:

		[ReturnsModifiedPointer ()]
		[RemovesFirstMatch ()]
		public void remove (G data);

The only problem is, how does the compiler now how to iterate over the elements? Well, the best way might be to require the type (i.e. GLib.List) to have an iterator() function (which also supports foreach). Currently foreach support for List is hardcoded, but it should be possible to implement this in vapi instead, if we allow custom class/struct definitions inside vapi (see my comment 3 in bug 622261).

Note also that List and SList are already special to automatically unref their contents when they get destroyed. I've entered bug 625772 for all the glib containers that don't even do this.

*** Bug 624249 has been marked as a duplicate of this bug. ***

(In reply to comment #9)
> Yes; the following modification to the List binding fixes this example:
>         [ReturnsModifiedPointer ()]
>         [CCode (cname = "g_list_remove")]
>         public unowned G _remove (G data);
> 
>         [ReturnsModifiedPointer ()]
>         [CCode (cname = "_vala_list_remove")]
>         public unowned G remove (G data)
>         {
>             foreach (var v in this) {
>                 if (v == data) {
>                     Object vo = v as Object;
>                     if (vo != null) {
>                         vo.unref ();
>                     }
>                     break;
>                 }
>             }
> 
>             return _remove (data);
>         }

That looks similar to binding enhancement I proposed in https://bugzilla.gnome.org/review?bug=624249&attachment=166328

(In reply to comment #12)
> That looks similar to binding enhancement I proposed in
> https://bugzilla.gnome.org/review?bug=624249&attachment=166328

We should really handle this correctly without requiring calls to separate methods. Probably requires special handling in code generator to handle this for all cases.

This is not a Vala bug but a GLib bug.

If you check at GList, it hasn't a GDestroyNotify at initialization, like GHashTable, no g_list_remove() returns data to handle by you automatically, then is your responsability as a programmer to know how to handle data, in this case each time you call GLib.List.remove() on ref counted objects, you should decrease it by calling unref() explicitly in Object classes or the one provided by your bindings.

So if GLib adds a new GLib.List<Object>() specialized handler for GObject classes, taking care about ref/unref, we just take care on using GLib.List<G> on Objects and use Gee.ArrayList<G> instead.ç

Use GLib.List<G> as returning value where your class takes care about your objects not returned GLib.List<G> one.

If you create a GLib.List<G> as return value, creating one using append(), as described, advice your user to unref all returned objects using GLib.List<G>.foreach() before get out of context.

For public API you may want to use GLib.Queue(), it returns removed objects, then it can be unref by Vala automatically.