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
Yes, handling of wrapped objects is also wrong in methods. I'm looking at it.
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.
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; >}
Created attachment 364525 [details] Patch
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).
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().
(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().
(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?
Created attachment 364658 [details] Patch
(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 :)
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?
(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.
Committed r243283: <https://trac.webkit.org/changeset/243283>
<rdar://problem/49101273>
(In reply to Carlos Garcia Campos from comment #12) > No, it's *the* instance parameter in: @instance
(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.