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 638542 - Reference counting bug in function invocation path
Reference counting bug in function invocation path
Status: RESOLVED NOTABUG
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2011-01-02 22:50 UTC by Giovanni Campagna
Modified: 2011-01-04 12:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GObject: always increase the reference count when wrapping (1.31 KB, patch)
2011-01-02 22:58 UTC, Giovanni Campagna
none Details | Review

Description Giovanni Campagna 2011-01-02 22:50:13 UTC
After a function is invoked, all arguments with (transfer full) are released. This includes GInitiallyUnowned objects which were newly created (thus floating) by the call. But the reference count of these objects when unreffed is 1, because inside the JSObject constructor g_object_ref_sink only clears the floating flag, and g_object_add_toggle_ref is paired with g_object_unref.

This does not happen with <constructor>s because for some weird reason they're marked (transfer none).

Test case:
IBus = imports.gi.IBus
bus = new IBus.Bus
list = bus.list_engines()
Comment 1 Giovanni Campagna 2011-01-02 22:58:34 UTC
Created attachment 177379 [details] [review]
GObject: always increase the reference count when wrapping

Previously, floating objects did not have the ref count increased,
so they were finalized at the end of the call if (transfer full),
resulting in a segmentation fault.
Comment 2 Owen Taylor 2011-01-03 16:04:38 UTC
We've previously determined that using (transfer none) is the correct way to do this, and you can find examples in the shell annotations, etc. The meaning of g_object_ref_sink() is basically:

 - Is there a floating reference I can adopt?
   - If so, give it to me
   - Otherwise take a new reference

And when a function returns a floating reference, for the caller to adopt the floating reference should be though of as independent of any transfer of reference involved.

We can't support both (transfer none) and (transfer full) because (transfer full) would mean that the caller is returning a new reference count on the returned object which *also* happens to be floating.

So this is not a GJS bug and IBus needs to be fixed.
Comment 3 Giovanni Campagna 2011-01-03 16:23:36 UTC
(In reply to comment #2)
> We've previously determined that using (transfer none) is the correct way to do
> this, and you can find examples in the shell annotations, etc.

So you're saying (transfer none) if floating, and (transfer full) if not?
What if sometimes returns a floating object and sometimes not (singletons, weird ->constructor returning an existing object, early signals clearing the floating flag, g_object_force_floating, factory methods that accept any GType)?

> The meaning of
> g_object_ref_sink() is basically:
> 
>  - Is there a floating reference I can adopt?
>    - If so, give it to me
>    - Otherwise take a new reference
> 
> And when a function returns a floating reference, for the caller to adopt the
> floating reference should be though of as independent of any transfer of
> reference involved.

In fact, my patch always adopts the reference, and since, as you say, adopting should be independent of transfer, it always takes a new reference on the object.

> 
> We can't support both (transfer none) and (transfer full) because (transfer
> full) would mean that the caller is returning a new reference count on the
> returned object which *also* happens to be floating.

Which is true. Nothing else is holding the floating reference, so it is a new reference.
We clear the floating flag it because floating refs are useless outside of C, but we should treat it like a normal reference.

> 
> So this is not a GJS bug and IBus needs to be fixed.

Sorry, but I disagree.
A new object is a new reference, that someone must take care of.
Comment 4 Havoc Pennington 2011-01-03 16:50:36 UTC
Here's an alternative explanation of it.

The "floating" reference is floating because *nobody owns it*. The floating ref can be removed by anyone, but is owned by no-one. By convention, a "container" type object that "holds" the floating object, would normally remove it.

Therefore, the floating reference cannot be transferred, because there's no ownership.

This is why calling unref() on an object with only a floating reference is incorrect, you can only unref if you own a ref. On a floating object, you do not.
The floating ref is floating with no owner.

If you have a function that sometimes returns a floating object and sometimes not, then that's fine. The function should be defined to either always return a non-floating ref (transfer full) or never return a non-floating ref (transfer none). The floating ref is not relevant.

If you have a function that sometimes returns a non-floating ref and sometimes does not return a non-floating ref, that function is broken. There aren't any annotations for this, for good reason. It's impossible to use such a function correctly because the caller doesn't know whether it now owns a ref.

Anyway this has been true of floating refs for many years, it isn't a gjs/gobject-introspection specific thing.
Comment 5 Havoc Pennington 2011-01-03 16:54:18 UTC
> So you're saying (transfer none) if floating, and (transfer full) if not?

So this is not right; the rule is transfer none if there's no non-floating ref that passes to the caller, and transfer full if there is a non-floating ref transferred. The floating ref is not relevant in any way and does not show up in annotations. Just ignore it.

> In fact, my patch always adopts the reference

The word "adopt" is misleading imo. The floating ref can't be adopted. What you can do is take a new ref, and remove the floating ref. Which is what g_object_ref_sink() does (conceptually). In this case you aren't adopting the floating ref, you are taking a new ref and removing the floating ref which had no owner.

Maybe semantics but the point is, nobody can ever own the floating ref and it can never be transferred. Pretend it does not exist.
Comment 6 Owen Taylor 2011-01-03 16:56:00 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > We've previously determined that using (transfer none) is the correct way to do
> > this, and you can find examples in the shell annotations, etc.
> 
> So you're saying (transfer none) if floating, and (transfer full) if not?
> What if sometimes returns a floating object and sometimes not (singletons,
> weird ->constructor returning an existing object, early signals clearing the
> floating flag, g_object_force_floating, factory methods that accept any GType)?

Let's look at the "early signals clearing the floating flag" case.

 - Create object
 - Emit a signal
 - JS handler (say) ref-sinks the object and adopts the floating reference
 - return the object
 - Caller assumes that it has been given a reference

Oops, reference is owned by JS and object is finalized at next GC. If you are going to expose a floating object to the external world and keep on using it, you must take a non-floating reference to the object, either by calling g_object_ref_sink() and adopting the reference yourself or using g_object_ref().

In the return value case, if you really want to make a function return floating and non-floating objects, it should use (transfer full) and return an extra reference count, so a reference count of 2 in the floating case. But it's typically better to simply not to do that. Floating objects should be returned only by "constructor-like" functions that are returning a new object that is naturally adopted by the caller.

> > The meaning of
> > g_object_ref_sink() is basically:
> > 
> >  - Is there a floating reference I can adopt?
> >    - If so, give it to me
> >    - Otherwise take a new reference
> > 
> > And when a function returns a floating reference, for the caller to adopt the
> > floating reference should be though of as independent of any transfer of
> > reference involved.
> 
> In fact, my patch always adopts the reference, and since, as you say, adopting
> should be independent of transfer, it always takes a new reference on the
> object.

(transfer full) plus returning a floating object means that if I adopt the floating reference I need to *also* unref the initial reference.

> > 
> > We can't support both (transfer none) and (transfer full) because (transfer
> > full) would mean that the caller is returning a new reference count on the
> > returned object which *also* happens to be floating.
> 
> Which is true. Nothing else is holding the floating reference, so it is a new
> reference.
> We clear the floating flag it because floating refs are useless outside of C,
> but we should treat it like a normal reference.

Yes, it would be possible to define (transfer full) to require checking whether the object is floating or not and doing different things, but this is really not consistent with the design of floating objects and the underlying memory management patterns is not a legitimate memory management pattern in GObject code.

g_object_ref_sink()

should be thought of as a variant of g_object_ref() that also adopts a floating reference. If you return objects so that your caller needs to do:

 if (g_object_is_floating(obj))
    g_object_ref_sink_object();
 else
    /* Don't do anything */;

then, among other things, people using your function from C will get it wrong and leak or double free.
Comment 7 Giovanni Campagna 2011-01-04 12:24:03 UTC
I understand all your comments now, and I see that "(transfer none)" does not mean "I keep a reference on it", rather it means "You should not unref it" (which is valid for floating references as well).
I will file a bug on IBus, thanks!