I believe that current expected semantics for bindings is:

if transfer-ownership=full: take over a ref
if transfer-ownership=none: ref-sink and take over a ref

I like that it is rather simple.  IMO the gtk_action_get_proxies case is a bug in GtkAction implementation, it should sink the objects it stores

BTW: how do you suggest checking for 'floating' state above? Calling g_object_is_floating() during runtime, or having attribute in GI typelib?

(In reply to comment #1)

> IMO the gtk_action_get_proxies case is a bug
> in GtkAction implementation, it should sink the objects it stores

Yes, that's my feeling.  I'll see if I can do a patch.

https://bugzilla.gnome.org/show_bug.cgi?id=657367

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

(In reply to comment #2)
> (In reply to comment #1)
> 
> > IMO the gtk_action_get_proxies case is a bug
> > in GtkAction implementation, it should sink the objects it stores
> 
> Yes, that's my feeling.  I'll see if I can do a patch.

Colin, how do you feel about Torsten's initial request?

Or maybe we just need to explain properly how the transfer annotation should be written when returning floating instances. I could add something to http://live.gnome.org/GObjectIntrospection/Annotations#Transfer_Modes in that case and pass it to you for review.

When the output argument is annotated (transfer full), it means that callee *owns* the reference and passes it back to caller.  But floating reference is (by definition) not owned by anyone.  IMO that is why functions which return objects with only floating reference should be marked as (transfer none) -> they cannot transfer ownership to caller, because they simply don't have it.

IMO current GoI handling of floating refs is OK, although maybe a bit surprising at the first glance. I agree that the documentation is painfully missing...

(In reply to comment #5)
>
> Colin, how do you feel about Torsten's initial request?

We can't really change how we handle floating values incompatibly at this point, so I think it's a case of fixing functions like gtk_action_get_proxies() to either always return floating refs, or never return them.

> Or maybe we just need to explain properly how the transfer annotation should be
> written when returning floating instances. I could add something to
> http://live.gnome.org/GObjectIntrospection/Annotations#Transfer_Modes in that
> case and pass it to you for review.

Basically always (transfer none) if you make a new floating object.  That's by far the most common case.

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

IMO functions returning a new, floating reference should not be annotated with (transfer none). This would intuitively mean that the returned object is not owned by the caller and will only cause confusion when reading the docs.

Instead I'd propose a new annotation like (transfer floating) for functions that return a new, floating reference.

Adding a new alias floating for none would be fine. Should probably not extended the enum/girepository api though.

It might also make sense to expose this in the g-i API. Bindings might want to use this information to know that they get a new floating reference from such function, instead of relying on runtime checks.

I pushed a fix for this to gobject-introspection and updated the wiki page:


commit 699ad0f
Author: Johan Dahlin <johan@gnome.org>
Date:   2011-11-25 12:09:51 -0200

    Add a floating alias for none
    
    https://bugzilla.gnome.org/show_bug.cgi?id=657202

(In reply to comment #11)
> It might also make sense to expose this in the g-i API. Bindings might want to
> use this information to know that they get a new floating reference from such
> function, instead of relying on runtime checks.

If I understand it right, the proposed semantics for bindings would be:

1. if transfer-none:  g_object_ref() returned value and store
2. if transfer-floating: g_object_ref_sink() returned value and store
3. if transfer-full: store

I don't see any advantage from current scheme, which is basically:

1. transfer-none: g_object_ref_sink() returned value and store
2. transfer-full: store

.. given that floating refs are returned as transfer-none.

BTW I think that scheme originally proposed in this bug thread is
thread-unsafe, in case that some other thread sinks the object between
is_floating() and ref_sink() operations.  Currently valid scheme (either
ref_sink or nothing) is thread safe, given that ref_sink is atomic operation.

If transfer-floating is added as an alias for none, does this mean that it's only available inside the code but not inside the typelib/gir files? In that case it's rather useless IMHO, it should be available from the typelib/gir files too to be able to be used by bindings.


Also transfer-floating would not only be used for return values but also for functions that take ownership of a floating reference, e.g. gtk_container_add()/gtk_bin_add() and gst_bin_add().

ping?

(In reply to comment #14)
> If transfer-floating is added as an alias for none, does this mean that it's
> only available inside the code but not inside the typelib/gir files? In that
> case it's rather useless IMHO, it should be available from the typelib/gir
> files too to be able to be used by bindings.

So...what bindings do you see using this? 

> Also transfer-floating would not only be used for return values but also for
> functions that take ownership of a floating reference, e.g.
> gtk_container_add()/gtk_bin_add() and gst_bin_add().

But if we're saying that bindings should always ref_sink any floating values they see, it's not useful to add a (transfer floating) because by the time the object has gone out to a binding, it's not longer floating, and so you can't pass it to a different function which takes ownership.

Erm, bindings have to ref_sink any floating objects. So they need to be able to check this and ideally they can know this from the annotations instead of checking the flag on the object.

(In reply to comment #17)
> Erm, bindings have to ref_sink any floating objects. So they need to be able to
> check this and ideally they can know this from the annotations instead of
> checking the flag on the object.

Why ideally?  The annotations are just a set of flags too.  We'd be replacing:

if (g_object_is_floating(gobj)) {
  g_object_ref_sink(gobj);
}

with:

if (g_type_info_is_floating(info)) {
  g_object_ref_sink(gobj);
}

What's the compelling advantage?

I was thinking about bindings for static bindings for static languages. In that case it wouldn't be necessary to call anything at runtime to check if this object now needs special handling or not.

For dynamic bindings you're right, there's no advantage.

(In reply to comment #18)
> Why ideally?  The annotations are just a set of flags too.  We'd be replacing:
> 
> if (g_object_is_floating(gobj)) {
>   g_object_ref_sink(gobj);
> }

This might not be threadsafe, there is a race between test-for-floating and ref_sink calls.

> with:
> 
> if (g_type_info_is_floating(info)) {
>   g_object_ref_sink(gobj);
> }

I still believe that even this is not needed.  IMO bindings (both static and dynamic ones) don't need to inspect floating flag at all, they only have to act according to transfer flag.  So:

1) Binding 'consumes' transfer=none objects by _ref_sink(); this way, if the object was floating, it is not floating any more (and it is made in a thread-safe, atomic way).

2) Binding 'consumes' transfer=full values by *no action*; IMO the binding does not have to care whether the object has floating reference; all it knows is that the caller promised that the object already has one valid (i.e. non-floating) reference and this reference is given (transferred) to the caller (i.e. binding).

The end result is the same in both cases: binding owns one valid (non-floating) reference to the 'consumed' object, it can safely store the pointer to the object and when it is done with the object, it calls _unref() on it.

This is how Lua dynamic binding works and I still haven't met any problem with this scheme.

(In reply to comment #20)
> (In reply to comment #18)
> > Why ideally?  The annotations are just a set of flags too.  We'd be replacing:
> > 
> > if (g_object_is_floating(gobj)) {
> >   g_object_ref_sink(gobj);
> > }
> 
> This might not be threadsafe, there is a race between test-for-floating and
> ref_sink calls.

Would the race condition be a problem if this was only done directly after object creation?

GObject *gobj = g_object_new(...)
if (g_object_is_floating(gobj)) {
    g_object_ref_sink(gobj);
}

Generally nothing else would know about the object in order to cause a problem. Or are you talking about some sort of compiler optimization that would cause a problem?

> I still believe that even this is not needed.  IMO bindings (both static and
> dynamic ones) don't need to inspect floating flag at all, they only have to act
> according to transfer flag.  So:
> 
> 1) Binding 'consumes' transfer=none objects by _ref_sink(); this way, if the
> object was floating, it is not floating any more (and it is made in a
> thread-safe, atomic way).

This might work in casual usage but breaks down during some vfunc and signal callbacks. For instance in bug 687522, the developer was attempting to implement the Gtk.Action.create_tool_item vfunc. The issue is this vfunc specifies a return of transfer=none and the Python wrapper will free the GObject before it gets back to the caller. Furthermore, since we immediately sink the object being created, adding a ref to it before returning would then leak. What really needs to happen is the vfunc should be returning a floating reference.

A simpler but still problematic example with sinking floating refs is with bug 661359. In this case the developer was attaching a callback to the Gtk.CellRendererText "editing-started" signal (this is what started the thread on the gtk-devel-list). The problem is this signal is emitted before the "editable" widget passed to the callbacks is ever sunk by the gtk internals. So pygobject would sink the object and own the reference, once the callback and signal marshaling finish, the object will be finalized handing a bad object back to the gtk internals. The initial solution was to blindly add a new ref but this caused leaks in other areas of the code (bug 675726). The right solution is just to keep the object floating as it passes through the python callback and we simply add a safety ref during the lifetime of the python wrapper.

> 2) Binding 'consumes' transfer=full values by *no action*; IMO the binding does
> not have to care whether the object has floating reference; all it knows is
> that the caller promised that the object already has one valid (i.e.
> non-floating) reference and this reference is given (transferred) to the caller
> (i.e. binding).

Sure but it is still possible for something which is marked as transfer=full to return a floating reference. So it isn't necessarily a bad idea to sink it if floating, otherwise leave it alone and let the wrapper steal the ref.

> The end result is the same in both cases: binding owns one valid (non-floating)
> reference to the 'consumed' object, it can safely store the pointer to the
> object and when it is done with the object, it calls _unref() on it.
> 
> This is how Lua dynamic binding works and I still haven't met any problem with
> this scheme.

You might want try out the examples in bug 687522 and bug 661359 and see if they cause problems. I added a bunch of test helpers to GIMarshalingTests for automated testing of these vfunc situations. They can also double as signal callback tests if your marshaling code is shared. Hopefully they can be helpful to other language binding maintainers:
http://git.gnome.org/browse/gobject-introspection/commit/?id=1fb2d14693fc7e

Python tests which make use if them:
http://git.gnome.org/browse/pygobject/tree/tests/test_object_marshaling.py

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > Why ideally?  The annotations are just a set of flags too.  We'd be replacing:
> > > 
> > > if (g_object_is_floating(gobj)) {
> > >   g_object_ref_sink(gobj);
> > > }
> > 
> > This might not be threadsafe, there is a race between test-for-floating and
> > ref_sink calls.
> 
> Would the race condition be a problem if this was only done directly after
> object creation?
>
> GObject *gobj = g_object_new(...)
> if (g_object_is_floating(gobj)) {
>     g_object_ref_sink(gobj);
> }
> 
> Generally nothing else would know about the object in order to cause a problem.
> Or are you talking about some sort of compiler optimization that would cause a
> problem?
>

I don't think so.  I think that race can happen only in the case when binding sinks this way any generic object it receives (typically as an return/outarg of any called function).

> > I still believe that even this is not needed.  IMO bindings (both static and
> > dynamic ones) don't need to inspect floating flag at all, they only have to act
> > according to transfer flag.  So:
> > 
> > 1) Binding 'consumes' transfer=none objects by _ref_sink(); this way, if the
> > object was floating, it is not floating any more (and it is made in a
> > thread-safe, atomic way).
> 
> This might work in casual usage but breaks down during some vfunc and signal
> callbacks. <.. explanation snipped ..>

Hmm.  You are right. I wrote comments to this bug at the time when my binding did not support subclassing/vfunc overriding at all, so I was never hit by this problem.  Recently I implemented subclassing feature and realised exactly the problem you are talking about.  For now I solved it by adding an exception to the rule above: if the C->binding conversion happens for function input argument, use only plain g_object_ref() instead of g_object_ref_sink().  For now this solution works, although it makes my simple rule stated in comments above a bit more complex and ugly :-(

> > 2) Binding 'consumes' transfer=full values by *no action*; IMO the binding does
> > not have to care whether the object has floating reference; all it knows is
> > that the caller promised that the object already has one valid (i.e.
> > non-floating) reference and this reference is given (transferred) to the caller
> > (i.e. binding).
> 
> Sure but it is still possible for something which is marked as transfer=full to
> return a floating reference. So it isn't necessarily a bad idea to sink it if
> floating, otherwise leave it alone and let the wrapper steal the ref.

Yep.  You might sink the ref if you want.  My point is that you don't have to, because transfer=full indicates that the caller is giving you at least one non-floating ref, so binding does not have to bother with floating one at all.

> > This is how Lua dynamic binding works and I still haven't met any problem with
> > this scheme.
> 
> You might want try out the examples in bug 687522 and bug 661359 and see if
> they cause problems. I added a bunch of test helpers to GIMarshalingTests for
> automated testing of these vfunc situations. They can also double as signal
> callback tests if your marshaling code is shared. Hopefully they can be helpful
> to other language binding maintainers:
> http://git.gnome.org/browse/gobject-introspection/commit/?id=1fb2d14693fc7e
> 
> Python tests which make use if them:
> http://git.gnome.org/browse/pygobject/tree/tests/test_object_marshaling.py

Thanks a lot for the pointers, I'll definitely try this and add the tests to my testsuite.

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

hey, just wanted to mention that this bug was describrd as a blocker for bug #720960, which basically, leads to memleaks in GStreamer bindings in Vala.

ugh I meant bug #702960

I know I'm beating a dead horse here, but just for the record: I realized that the constructors of GtkWindow and its descendants present a case arguing against always setting transfer-ownership=full for GInitiallyUnowned descendants.  Their constructors return a non-floating object and do not transfer ownership (because gtk+ keeps an internal list of windows).  Hence, the typelib correctly states that for, say, gtk_window_new we have transfer-ownership=none.  But currently the typelib also states the same for, say, gtk_label_new.  So the typelib does not account for the semantic difference between these two kinds of constructors.  It does not tell me that

  l = gtk_label_new (...);
  g_object_unref (l);

is legal, whereas

  w = gtk_window_new (...);
  g_object_unref (w);

is not.

(In reply to comment #26)
> I know I'm beating a dead horse here, but just for the record: I realized that
> the constructors of GtkWindow and its descendants present a case arguing
> against always setting transfer-ownership=full for GInitiallyUnowned
> descendants.  Their constructors return a non-floating object and do not
> transfer ownership (because gtk+ keeps an internal list of windows).  Hence,
> the typelib correctly states that for, say, gtk_window_new we have
> transfer-ownership=none.  But currently the typelib also states the same for,
> say, gtk_label_new.  So the typelib does not account for the semantic
> difference between these two kinds of constructors.  It does not tell me that
> 
>   l = gtk_label_new (...);
>   g_object_unref (l);
> 
> is legal, whereas

It isn't legal to finalize a floating object.

>   w = gtk_window_new (...);
>   g_object_unref (w);
> 
> is not.

I've opened bug 742618 for adding "transfer floating/transfer floating-full" aliases for args and returns. See bug 742618, comment 3 for the details.

I'd appreciate any input.

argh.. I meant bug 742618, comment 2

(In reply to comment #27)
> It isn't legal to finalize a floating object.

Well, gtk+ does emit a warning when you finalize a floating widget, so my examples were badly chosen.  But as far as I can see, there is nothing in glib that frowns upon finalizing a floating object.

(In reply to comment #30)
> (In reply to comment #27)
> > It isn't legal to finalize a floating object.
> 
> Well, gtk+ does emit a warning when you finalize a floating widget, so my
> examples were badly chosen.  But as far as I can see, there is nothing in glib
> that frowns upon finalizing a floating object.

Ah, indeed. I didn't know that this was a gtk+ only thing. GStreamer/GstElement also doesn't warn in this case.

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #27)
> > > It isn't legal to finalize a floating object.
> > 
> > Well, gtk+ does emit a warning when you finalize a floating widget, so my
> > examples were badly chosen.  But as far as I can see, there is nothing in glib
> > that frowns upon finalizing a floating object.
> 
> Ah, indeed. I didn't know that this was a gtk+ only thing. GStreamer/GstElement
> also doesn't warn in this case.

And having such a warning will break quite some code.

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]

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

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gobject-introspection/issues/54.