Bug 195574 – [GLib] Returning G_TYPE_OBJECT from a method does not work

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Marcelo Lopes]

Created attachment 364290 [details]
jscobj.c: Example code to reproduce the issue

Found a problem similar to bug 195206

When a method is installed with jsc_class_add_method(), and the
return type is specified as G_TYPE_OBJECT, the reference count of
the returned object is decreased, which may delete the object.

I made an example based on the one Adrian attached to the bug
mentioned above:

This example adds an 'id' function to the JS object, which returns
a reference to itself. After it is called, the object is deleted
and the call to getPath fails.

Tested on WebKit version 2.23.92

[Carlos Garcia Campos]

Yes, handling of wrapped objects is also wrong in methods. I'm looking at it.

[Carlos Garcia Campos]

Ok, it's not actually wrong, you are supposed to return a new reference, the same way that methods returning a string are expected to return a new allocated string. I know it's confusing and needs to be documented, but functions, methods and constructors are all transfer full. In the case of constructors the ownership is taken the JSCClass. I'll add some comments to your test case to clarify things.

[Carlos Garcia Campos]

Comment on attachment 364290 [details]
jscobj.c: Example code to reproduce the issue

>/*
> * Build with:
> *    gcc -o jscobj jscobj.c $(pkg-config javascriptcoregtk-4.0 gio-2.0 --cflags --libs)
> */
>
>#include <gio/gio.h>
>#include <jsc/jsc.h>
>
>static GFile *js_GFile_constructor(const char *path) {
>  GFile* ret =  g_file_new_for_path(path);
>  g_object_ref(ret);
>  return ret;
>}

You should not ref again here, the object ownership is transferred to the JSCClass.

>static GFile *js_GFile_id(GFile* self) {
>  return self;
>}

And here you need to g_object_ref(), the object will be unrefed after being converted to a JSCValue.

>static void setup_jsc_context(JSCContext *ctx) {
>  JSCClass *klass =
>      jsc_context_register_class(ctx, "GFile" /* name */, NULL /* parent */,
>                                 NULL /* vtable */, NULL /* destroy_notify */);

You should pass g_object_unref as destroy_notify, since the instance is owned by the JSCClass.

>  g_autoptr(JSCValue) ctor = jsc_class_add_constructor(
>      klass, NULL /* name: same as class */, G_CALLBACK(js_GFile_constructor),
>      NULL /* user_data */, NULL /* user_data destroy_notify */,
>      G_TYPE_OBJECT /* return type */, 1 /* n_params */, G_TYPE_STRING);
>  jsc_class_add_method(klass, "getPath" /* name */, G_CALLBACK(g_file_get_path),
>                       NULL /* user_data */,
>                       NULL /* user_data destroy_notify */,
>                       G_TYPE_STRING /* return type */, 0 /* n_params */);
>  jsc_class_add_method(klass, "id" /* name */, G_CALLBACK(js_GFile_id),
>                       NULL /* user_data */,
>                       NULL /* user_data destroy_notify */,
>                       G_TYPE_OBJECT /* return type */, 0 /* n_params */);
>
>  jsc_context_set_value(ctx, jsc_class_get_name(klass) /* name */,
>                        ctor /* value */);
>}
>
>int main(int argc, char *argv[]) {
>  g_autoptr(JSCContext) ctx = jsc_context_new();
>  setup_jsc_context(ctx);
>
>  g_autoptr(JSCValue) result =
>      jsc_context_evaluate(ctx, "(new GFile('.')).getPath()", -1);
>  g_autofree char *result_string = jsc_value_to_string(result);
>  g_print("Path: %s\n", result_string);
>
>  result =
>      jsc_context_evaluate(ctx, "var file = new GFile('.'); var file_copy = file.id(); file.getPath()", -1);
>  g_assert(jsc_value_is_string(result));
>
>  result_string = jsc_value_to_string(result);
>  g_print("Path after copy: %s\n", result_string);
>
>  return 0;
>}

[Carlos Garcia Campos]

Created attachment 364525 [details]
Patch

[Marcelo Lopes]

Thanks!

Your comments and the tests on the patch helped to clarify how to handle the lifetime of the objects created and also how to correctly implement a function that creates and returns a new native object (factory function).

[Adrian Perez]

Comment on attachment 364525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364525&action=review

Informally reviewing… There is a couple of places where I think the wording
could be improved to avoid ambiguity. Otherwise the added text is a welcome
improvement 👍

> Source/JavaScriptCore/API/glib/JSCClass.cpp:593
> + * passed to jsc_context_register_class().

Using “freed with” here does not make 100% clear that the GDestroyNotify 
supplied by the user of the API is responsible to free the value. How about
writing:

  Note  that the value returned by @callback is adopted by @jsc_class, and
  must be freed by the #GDestroyNotify passed to jsc_context_register_class().

WDYT?

> Source/JavaScriptCore/API/glib/JSCValue.cpp:594
> + * When @instance is provided, @jsc_class must be provided too. @jscClass takes ownership of

@jscClass → @jsc_class

> Source/JavaScriptCore/API/glib/JSCValue.cpp:595
> + * @instance that will be freed with the #GDestroyNotify passed to jsc_context_register_class().

How about the same as above?

  …@instance must be freed by the #GDestroyNotify passed to jsc_context_register_class().

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #6)
> Comment on attachment 364525 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364525&action=review
> 
> Informally reviewing…

Thanks!

> There is a couple of places where I think the wording
> could be improved to avoid ambiguity. Otherwise the added text is a welcome
> improvement 👍
> 
> > Source/JavaScriptCore/API/glib/JSCClass.cpp:593
> > + * passed to jsc_context_register_class().
> 
> Using “freed with” here does not make 100% clear that the GDestroyNotify 
> supplied by the user of the API is responsible to free the value.

No? What do you understand then?

> How about
> writing:
> 
>   Note  that the value returned by @callback is adopted by @jsc_class, and
>   must be freed by the #GDestroyNotify passed to
> jsc_context_register_class().
> 
> WDYT?

I'm not sure, the "must" here sounds to me like the user should free the instance, while it will be freed by the JSCClass using the destroy function passed on class registration.

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:594
> > + * When @instance is provided, @jsc_class must be provided too. @jscClass takes ownership of
> 
> @jscClass → @jsc_class

Oops

> > Source/JavaScriptCore/API/glib/JSCValue.cpp:595
> > + * @instance that will be freed with the #GDestroyNotify passed to jsc_context_register_class().
> 
> How about the same as above?
> 
>   …@instance must be freed by the #GDestroyNotify passed to
> jsc_context_register_class().

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #7)
> (In reply to Adrian Perez from comment #6)
> > Comment on attachment 364525 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=364525&action=review
> > 
> > Informally reviewing…
> 
> Thanks!
> 
> > There is a couple of places where I think the wording
> > could be improved to avoid ambiguity. Otherwise the added text is a welcome
> > improvement 👍
> > 
> > > Source/JavaScriptCore/API/glib/JSCClass.cpp:593
> > > + * passed to jsc_context_register_class().
> > 
> > Using “freed with” here does not make 100% clear that the GDestroyNotify 
> > supplied by the user of the API is responsible to free the value.
> 
> No? What do you understand then?

Using “freed with” can be parsed as “freed [alongside] with”, which to me
means that the GDestroyNotify will be called, but the freeing will happen
automatically, after the GDestroyNotify callback returns (i.e. the callback
notifies that the item will be freed, but does not need itself to do the
freeing).

If you just change the “with” preposition and use “by” instead, as in “freed
by the GDestroyNotify callback”, then it's clear that freeing has to be done
“by” the callback itself.

> > How about
> > writing:
> > 
> >   Note  that the value returned by @callback is adopted by @jsc_class, and
> >   must be freed by the #GDestroyNotify passed to
> > jsc_context_register_class().
> > 
> > WDYT?
> 
> I'm not sure, the "must" here sounds to me like the user should free the
> instance, while it will be freed by the JSCClass using the destroy function
> passed on class registration.

But the user has to “tell” how to free the data after all by passing a suitable
GDestroyNotify that will free the data… so yes, the user “has to” control how
the data will be freed.

Maybe it is even clearer by writing instead:

  Note that the value returned by @callback is adopted by @jsc_class,
  and the #GDestroyNotify passed to jsc_context_register_class() is
  responsible for disposing of it.
 
Does this sound any better to you?

[Carlos Garcia Campos]

Created attachment 364658 [details]
Patch

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #9)
> Created attachment 364658 [details]
> Patch

I think the wording now leaves less chances of confusion, thanks.
Patch LGTM now :)

[Michael Catanzaro]

Comment on attachment 364658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364658&action=review

> Source/JavaScriptCore/API/glib/JSCClass.cpp:735
> + * Note that the value returned by @callback must be fully transferred. In case of boxed types, you could use

fully transferred -> transfer full

could -> should?

(Ditto for copypaste below)

> Source/JavaScriptCore/API/glib/JSCClass.cpp:738
> + * with jsc_value_new_object() that receives the copy as instance parameter.

as an instance

(Ditto for copypaste below)

> Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:1628
> +static GString* getGStringCopyWillRaise(GString* str)
> +{
> +    return static_cast<GString*>(g_boxed_copy(G_TYPE_GSTRING, str));
> +}

I don't understand, why does this cause a JS exception?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #11)
> Comment on attachment 364658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364658&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCClass.cpp:735
> > + * Note that the value returned by @callback must be fully transferred. In case of boxed types, you could use
> 
> fully transferred -> transfer full

Ok.

> could -> should?

You could, not necessarily should. It depends on the boxed type, if it's refcounted, you could use the actual boxed type and return a ref, because the same pointer will be returned in the end. If the boxed type is not refcounted and you don't want to make a copy, you should use G_TYPE_POINTER instead to ensure the same pointer is returned. If you really want to make a copy, then you should use JSC_TYPE_VALUE and use jsc_value_new_object() passing the copy as the instance parameter. I'll clarify it better.

> (Ditto for copypaste below)
> 
> > Source/JavaScriptCore/API/glib/JSCClass.cpp:738
> > + * with jsc_value_new_object() that receives the copy as instance parameter.
> 
> as an instance

No, it's *the* instance parameter in:

JSCValue* jsc_value_new_object(JSCContext* context, gpointer instance, JSCClass* jscClass)

> (Ditto for copypaste below)
> 
> > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:1628
> > +static GString* getGStringCopyWillRaise(GString* str)
> > +{
> > +    return static_cast<GString*>(g_boxed_copy(G_TYPE_GSTRING, str));
> > +}
> 
> I don't understand, why does this cause a JS exception?

Because we are returning a pointer to a new boxed type that doesn't have a JSValue associated. Functions returning object, boxed or pointer should return a pointer of a previously wrapped object, either with jsc_value_new_object() or jsc_value_constructor_call() or new Foo called by JavaScript. That's why the documention says that if you really want to make a copy of the boxed type you should use JSC_TYPE_VALUE and return the value created with jsc_value_new_object(). That's what the other test case does.

[Carlos Garcia Campos]

Committed r243283: <https://trac.webkit.org/changeset/243283>

[Radar WebKit Bug Importer]

<rdar://problem/49101273>

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #12)
> No, it's *the* instance parameter in:

@instance

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #15)
> (In reply to Carlos Garcia Campos from comment #12)
> > No, it's *the* instance parameter in:
> 
> @instance

No, because it's not a parameter of that function.