I looked into it some more, and I think I have a handle on the
problem now.

pygtk_cell_data_func_marshal() is leaking a gobject reference on its
cell_layout argument.

If the cell_layout argument has no pygobject wrapper on entry to the
function, then the call to pygobject_new(cell_layout) would cause a
new one to be created.  pygobject_new() just calls
pygobject_new_full() with a sink argument of TRUE, which causes the
sink function to be called on the object (only if a wrapper didn't
exist yet and had to be created).  The sink function, (sink_gtkobject(),
in this case), checks if the object is floating and if it is (which it was),
does the cannonical g_object_ref;gtk_object_sink.  As a result, a gobject
reference was inadvertently being given to the marshal function, which
should not have had a reference.  (The marshal *should* have had a python
reference to the pygobject -- which it did -- and the pygobject should
have had a gobject reference to the wrapped object -- which it did.
The sink was resulting in an extra unclaimed gobject reference.)

This seems to be a generic problem among all of the marshal functions
in pygtk.  Many of them are passed gobjects, which are passed to
pygobject_new to get a wrapper.  It doesn't look as if any of these
should result in a gobject reference being given to the caller.

I've worked around this in my copy of pygtk by patching it as follows:

Expose pygobject_new_full in the interface.  In every marshal function,
change every pygobject_new call to a pygobject_new_full call, with the
sink argument FALSE.

I don't know if this is the "right" solution, but it seems reasonable.
And it cleared up the leak with no apparent adverse effects.

phil dumont

The cell_layout argument to the marshal function still has
a floating reference because gtk_cell_view_menu_item_new()
creates the cell_layout (a CellView) at the beginning, and
doesn't add the CellView to its MenuItem parent until the
end of the function.  (The MenuItem is the one that's
supposed to -- and indeed does -- sink the floating
reference.)  But in between, it calls gtk_widget_size_request()
on the cell_view, which eventually leads to the CellView
with the still-floating reference being passed to the
marshal function.

Another way to have fixed the problem would be to make
gtk_cell_view_menu_item_new() create the MenuItem and
add the CellView to it immediately after the CellView
is created (and, more to the point, before the size
request).  Then the floating reference would have been
removed by the parent before we got the marshal, and when
the marshal wrapped the cell_layout and pygobject_new tried
to sink it, the sink function would have done nothing, because
there was no floating reference.  I've tried this too, and it
works fine.  But it doesn't seem like the right solution.  It
seems more like a wrapper bug than a gtk bug, and should be
fixed in the wrapper.

The bug is in gtk+, not pygtk.

You made a very interesting and deep analysis, and I thank for that, but with some minor flaws.  It is perfectly normal for pygobject_new to add one ref to the GObject.  It is necessary because the wrapper has to own a reference to the GObject.  The GObject will be unreffed again when the corresponding PyObject is collected.

gtk_cell_view_menu_item_new() has this code:
{
[...]
  cell_view = gtk_cell_view_new ();
[...]
  item = gtk_menu_item_new ();
  gtk_container_add (GTK_CONTAINER (item), cell_view);
}

That is, it creates a cell_view, adds it to a container, but forgets to unref it.  It works most of the time because of the floating reference that ensures C programmers are allowed to be lazy.  The assumption is broken, as you explained, and thus a GObject ref leak is created, since gtk_cell_view_menu_item_new doesn't cleanup correctly.  The correct solution is to add a g_object_unref(cell_view) to the end of gtk_cell_view_menu_item_new().  That way it works correctly whether or not a floating reference exists.

If adding the cell_view immediately to the icon helps, then we should probably
do that. But I don't fully see how it can make a difference:

scenario 1:

1. cell view is created with floating ref
2. python marshaller refs and sinks it
3. container refs and sinks it

end result: cell view has refcount of 2, not floating

scenario 2:

1. cell view is created iwth a floating ref
2. container refs and sinks it
3. python marshaller refs and sinks it

end result: cell view has a refcount of 2, not floating

See my last recommendation.  gtk_cell_view_menu_item_new() creates an object reference, but doesn't unref it in the end.  gtk_container_add only steals one reference in case that reference is in the floating state; this is true for most cases, but not in this case.

Adding a g_object_unref(cell_view) to gtk_cell_view_menu_item_new() correctly fixes the leak.

PS: floating references are a complete mess IMHO.

Hm... maybe adding g_object_unref(cell_view) doesn't fix it for all cases..

Created attachment 67980 [details] [review]
one way to fix it

One way to fix it, although i'm not so sure anymore the bug isn't in pygtk...

A more detailed description of what happens (without my patch):

scenario 1:

1. cell view is created with floating ref [ref_count=1, floating=1]
2. python marshaller refs it [ref_count=2, floating=1]
3. python marshaller refs and sinks it [ref_count=2, floating=0]
4. container refs and sinks it [ref_count=3, floating=0] (since floating was 0, sink doesn't do anything)

end result: cell view has refcount of 3, not floating

scenario 2 (changing order):

1. cell view is created with floating ref [ref_count=1, floating=1]
2. container refs and sinks it [ref_count=1, floating=0]
3. python marshaller refs it [ref_count=2, floating=0]
4. [skipped by pygtk because floating=0] python marshaller refs and sinks it

end result: cell view has a refcount of 2, not floating

So changing the order definitely helps, like Phil said.

In any case, I don't think pygtk is doing anything wrong.  It creates a normal ref for its own private use, then converts any floating references to normal references, since floating references cause too many problems in language bindings (as you can see in this instance).

Created attachment 67981 [details] [review]
fix by changing order

Looks to me like the pyhton marshaller is reffing once too often.
It should ref and sink, not ref, ref, and sink.

Anyway, I'll add the reordering fix now.

PyGTK does one ref, to be owned by the python wrapper; the next ref and sink cancel each other out, and are only used to convert a floating reference--not owned by pygtk--into a normal reference.

To be honest I'm not sure about this myself, but it has worked this way since forever and I'd rather not risk changing it.

Fixed in both branches now.