Created attachment 234675 [details] [review]
Proposed patch.

I'm not sure if this patch is correct, since g_binding_set_property calls g_value_get_object, not g_value_dup_object. On the other hand, GI_TRANSFER_EVERYTHING is documented as implying that the caller owns the ref, which is what happens when setting the property, in that the ref is not transferred away from the caller, so the caller still owns it.

Is there a particular bug you're experiencing that you're trying to fix with this patch?

I see that GObject-2.0.gir lists the property as being transfer-ownership="none", but that seems like a giscanner bug; it's not really even *possible* for an object, boxed, or string property to be (transfer none), because g_object_get_property() will ref/copy it internally.

gobject-introspection has tests to make sure that "Transfer: container" and "Transfer: none" work, but there are no tests of "Transfer: full" (or any notes suggesting that the explicit "Transfer: none" was redundant) so it looks like the author of those tests thought that the default would have been "Transfer: full" without the annotations, suggesting again that the scanner is outputting the wrong default value. (And, by extension, that all bindings are either ignoring property transfer information, or leaking objects.)

(In reply to comment #2)
> Is there a particular bug you're experiencing that you're trying to fix with
> this patch?

I was trying to fix bug 675726, since I'm seeing another instance of it.

> I see that GObject-2.0.gir lists the property as being
> transfer-ownership="none", but that seems like a giscanner bug; it's not really
> even *possible* for an object, boxed, or string property to be (transfer none),
> because g_object_get_property() will ref/copy it internally.
> 
> gobject-introspection has tests to make sure that "Transfer: container" and
> "Transfer: none" work, but there are no tests of "Transfer: full" (or any notes
> suggesting that the explicit "Transfer: none" was redundant) so it looks like
> the author of those tests thought that the default would have been "Transfer:
> full" without the annotations, suggesting again that the scanner is outputting
> the wrong default value. (And, by extension, that all bindings are either
> ignoring property transfer information, or leaking objects.)

Scanning gtk+, the properties that I've looked at do behave similarly to gbinding's object property (ie, either ref or weakref the object when setting and call g_value_set_object when getting).

So I agree that making transfer full the default makes sense, at least in theory (it might lead to crashes if there are properties anywhere that behave oddly and aren't annotated).

In pygobject's case, refs are being leaked partly because of this.

The docs for g_object_get_property say:

"In general, a copy is made of the property contents and the caller is responsible for freeing the memory by calling g_value_unset()."

http://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#g-object-get-property

which pygi is currently not doing when looking at introspection based properties. However, there is also some amount of ambiguity here. If this bug gets fixed, should we continue to ignore the advice of the docs and only use marshaling ownership rules of introspection?

(In reply to comment #4)
> However, there is also some amount of ambiguity here. If this bug
> gets fixed, should we continue to ignore the advice of the docs and only use
> marshaling ownership rules of introspection?

*Theoretically*, you could have a transfer-none object-valued property, if the get_property() implementation did:

  case PROP_WHATEVER:
      g_value_set_object (value, priv->my_object_prop);
      /* Undo the ref added by the GValue */
      g_object_unref (priv->my_object_prop);
      break;

This is beyond pathological though. Object and variant properties should be assumed to be transfer-full; if they claim not to be it's almost certainly because of this bug.

String properties are unavoidably transfer-full, so you should just ignore annotations there. (And the scanner should warn about them.)

Boxed properties can be transfer-full or transfer-container; if they claim to be transfer-none then you should assume transfer-container for G_TYPE_HASH_TABLE and G_TYPE_POINTER_ARRAY, and transfer-full for everything else. (Assuming transfer-container might end up leaking in some cases, but that's better than assuming transfer-full and crashing in other cases.)

Pointer properties can be transfer-none, transfer-container, or transfer-full, and tend to be transfer-none by default (since the GValue itself does not do any reffing or copying in this case). So for pointer properties, just obey the annotation.

This sounds fine. The ambiguity I'm talking about arises as follows:

Caller of get_property:

    GValue value = { 0, };
    g_value_init (&value, G_PARAM_SPEC_VALUE_TYPE (pspec));
    g_object_get_property (obj, pspec->name, &value);

"value" should generally hold a new reference to the objects property. Proceed as follows:

1. If transfer is full, the marshaller might steal the new reference from the GValue.

    PyObject *pyobj = marshal_object_from_gvalue (val, transfer);
    value.unset (); // this would then break things

2. An alternate is to always have the marshaller make a copy/ref by forcing (transfer none) then rely on "unset" to cleanup the results of g_object_get_property.

    PyObject *pyobj = marshal_object_from_gvalue (val, GI_TRANSFER_NOTHING);
    value.unset ();

Note I went with option 2 from comment #6
https://git.gnome.org/browse/pygobject/commit/?id=04816f741

This means we ignore transfer annotations on properties and use g_object_get_property() as you would in C. I don't believe users of g_object_get_property() should ever have to do anything beyond calling g_value_unset() to ensure the memory is free'd.

So what we do is:

1. call g_object_get_property()
2. get the boxed/object/string/whatever out of the gvalue
3. copy this pointer based on type annotations to a Python native or wrapped value.
4. call g_value_unset()

This in combination with some other re-work fixed a bunch of leaks:
https://bugzilla.gnome.org/show_bug.cgi?id=726999#c7

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