Re: Problems with un-owned objects passed to closures in pygobject (gtk_cell_renderer_text_start_editing)

[Tristan Van Berkom <tvb gnome org>]

On Thu, Jan 17, 2013 at 9:42 AM, Simon Feltman <s feltman gmail com> wrote:
> Hi Everyone,
>
> I'm trying to figure out a complete solution for proper reference management
> of gobjects passed into python vfuncs and signal closures as "in" arguments.
> Currently pygobject will add a ref to gobject arguments marked as
> transfer-none during marshaling and additionally sink/ref the gobject when
> the python object wrapper is created around it. The initial added ref during
> marshaling is then leaked upon the closure finishing in most cases.
>
> The fix could simply be to not add the initial ref during marshaling (or we
> need to make sure it is released when the closure finishes). The problem is
> this behavior is relied upon when a floating ref is passed and is the
> explicit fix described in the following ticket (which un-fortunately causes
> a leak in the non-floating case):
> https://bugzilla.gnome.org/show_bug.cgi?id=661359
>
> The specific problem described in bug 661359 occurs when a python closure is
> connected to a GtkCellRendererTexts "editing-started" signal. During the
> start of editing, a GtkEntry is created and passed almost immediately to
> signal emission before any ownership takes place:
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellrenderer.c#n864
>
> This results in the following ref counting behavior of the GtkEntry as it is
> used by gtk_cell_area_activate_cell and gtk_cell_renderer_start_editing:
>
> 1 (floating) - gtk_cell_renderer_text_start_editing creates the GtkEntry
> 2 (floating) - g_signal_emit argument marshaling adds an additional ref
> during signal emission
> 3 (floating) - python closure marshaling adds an additional ref described
> above
> 3 (owned) - A PyGObject wraps the GtkEntry and sinks the floating ref
> * python callback is called and passed the wrapper
> 2 (owned) - The PyGObject wrapper is destroyed after the python callback
> finishes additionally freeing a gobject ref
> 1 (owned) - g_signal_emit argument marshaling completes and frees the ref it
> added
> 2 (owned) - gtk_cell_area_activate_cell sets the CellAreas edit widget which
> adds a ref
> 1 (owned) - cell editing finishes and removes the widget from the CellArea
>
> Given the above it seems cell editing will leak a new editor each time
> editing occurs even if no closures are connected to the "editing-started"
> signal (python or otherwise). The pygobject specific problems I mention
> could be solved by tracking incoming floating refs and re-floating them upon
> closure exit if the python ref is not stored anywhere (or by attempting to
> just keep it floating). However, the root cause of all this could simply be
> that cell editing needs better reference management of the editors being
> created. For instance, gtk_cell_renderer_text_start_editing creates the
> GtkEntry and stores it as an attribute in GtkCellRendererText.priv.entry
> without sinking the ref:
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellrenderertext.c#n2049
>
> I think this should be changed to immediately sink the ref and unref it
> during gtk_cell_renderer_text_editing_done. This would solve the python
> problems and fix the reference leak for the editor (and bug 661359 can be
> backed out). Additionally I think gtk_cell_area_set_edit_widget should be
> sinking the incoming editable widget instead of simply adding a ref:
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkcellarea.c#n3309
>
> As a general practice shouldn't refs be sunk when they are being assigned to
> a structured attribute especially when dealing with GInitiallyUnowned based
> objects?

Perhaps, but this would not solve your problem as I understand it.

  o Working around the problem by changing the way GTK+ code is written is
     not a viable long term solution afaict

  o Your issue as far as I can see is that a floating object gets passed as
     a signal argument, this can happen in a variety of situations,
     consider a construct where a code segment creates an object and
     passes it to another object immediately:

     ------------------------------
     BaseBall *ball =
           g_object_new (BASE_TYPE_BALL,
                                     "pitch-type", PITCH_TYPE_CURVE,
                                      NULL);

     pitcher_throw_ball (pitcher, ball);
     ------------------------------

     In this case the pitcher might own the ball for some time, or it
might in turn
     relinquish ownership and give the ball to the global Stadium * object.

 o Forcing the object to become floating again after the PyGObject is
finalized feels
    rather hacky, it sounds like you will leak the underlying object
in cases where
    ownership was actually intended (i.e. assignment to a PyObject member)

Rather, it sounds like PyGObject should not mandate sinking the
floating reference
until ownership is actually implied, i.e. by tying the lifecycle of
the said object to a member
of another PyGObject (a strong reference). Stack variables on the
other hand, such as
declared and assigned in PyGobject functions, or passed to them as
arguments, should
not interfere with the ownership of the underlying GObject.

Would that make more sense ?