Created attachment 288018 [details] [review]
GjsDBusImplementation: unref the invocation passed to method_call()

Review of attachment 288018 [details] [review]:

It would be good to update the glib documentation to reflect this.

Review of attachment 288018 [details] [review]:

This seems wrong to me. g_dbus_method_invocation_return_value is the thing that unrefs it.

Isn't g_dbus_method_invocation_return_value / return_bus_error supposed to be the thing that does the unref? It's an error to forget to call that.

(In reply to comment #4)
> Isn't g_dbus_method_invocation_return_value / return_bus_error supposed to be
> the thing that does the unref? It's an error to forget to call that.

But it doesn't anymore, because we correctly protect it from eating a value that can be potentially owned by JS only.

Huh? Where do we do that?

(In reply to comment #6)
> Huh? Where do we do that?

In function.cpp, we recognize that the instance ownership transfer is EVERYTHING, so we reference the argument one extra time (so that the C function will not eat our reference)

The return_value invocation is:

 * g_dbus_method_invocation_return_value:
 * @invocation: (transfer full): A #GDBusMethodInvocation.
 * @parameters: (allow-none): A #GVariant tuple with out parameters 

Since the ref-eating is in the annotation, language bindings will balance it out and the object won't get an extra ref.

You could say that this annotation is wrong - but it does seem to me that the extra reference count is meant to be a convenience for C programmers - if you were writing this strictly for language bindings, there would be no point in doing it - so it's probably better annotated away.

(In reply to comment #8)
> The return_value invocation is:
> 
>  * g_dbus_method_invocation_return_value:
>  * @invocation: (transfer full): A #GDBusMethodInvocation.
>  * @parameters: (allow-none): A #GVariant tuple with out parameters 
> 
> Since the ref-eating is in the annotation, language bindings will balance it
> out and the object won't get an extra ref.
                                        ^^^^^
                                        unref

As I understand it, from the C side, it's supposed to work like this:

  Get a GDBusMethodInvocation in your method_call with a refcount of 1.
  You somehow hang onto it, and then call g_dbus_method_invocation_return_value, which eats the ref and destroys it.

With GJS added, it looks like this:

  libgjs-private gets a GDBusMethodInvocation with a refcount of 1. It emits a signal.
  To handle the signal, GJS creates a proxy wrapper for the object, which bumps the refcount up to 2 and adds a toggle ref.
  JS code shoves the GDBusMethodInvocation around, and eventually ends up calling return_value. This doesn't add any refs since it's (transfer full) and bumps the proxy wrapper refcount back to 1, which activates the toggle ref and unroots the object.
  GC eventually collects the method invocation.

So why is your patch needed, Owen?

(In reply to comment #10)
> As I understand it, from the C side, it's supposed to work like this:
> 
>   Get a GDBusMethodInvocation in your method_call with a refcount of 1.
>   You somehow hang onto it, and then call
> g_dbus_method_invocation_return_value, which eats the ref and destroys it.
> 
> With GJS added, it looks like this:
> 
>   libgjs-private gets a GDBusMethodInvocation with a refcount of 1. It emits a
> signal.
>   To handle the signal, GJS creates a proxy wrapper for the object, which bumps
> the refcount up to 2 and adds a toggle ref.
>   JS code shoves the GDBusMethodInvocation around, and eventually ends up
> calling return_value. This doesn't add any refs since it's (transfer full) and
> bumps the proxy wrapper refcount back to 1, which activates the toggle ref and
> unroots the object.

Your analysis is wrong here. gjs will add one ref exactly because it is (transfer full). So the proxy wrapper will have a refcount of 3 at the beginning of the return_value() call, and 2 at the end (and that keeps the object rooted forever).

>   GC eventually collects the method invocation.
> 
> So why is your patch needed, Owen?

Created attachment 288171 [details] [review]
GjsDBusImplementation: unref the invocation passed to method_call()

Here's a version with an updated commit message that describes what is
going on better and references a GLib bug I filed that updates the
GDBus documentation/annotation to clarify what is going on.

===

In C code, the reference passed to the method_call() virtual function
would be passed to the return_value/error function that is eventually
called. But since these functions are called from JS, and the calling
convention is normalized, we need to unref reference passed to
method_call ourselves. See GLib bug 738259 for a more complete discussion.

Review of attachment 288171 [details] [review]:

OK.

Attachment 288171 [details] pushed as 152c679 - GjsDBusImplementation: unref the invocation passed to method_call()