Created attachment 157541 [details] [review]
[jsapi] Export prototype in GJS_DEFINE_PROTO

When creating a new prototype using the GJS_DEFINE_PROTO macros,
save a static reference to the JSObject* representing the created
prototype. Previously only a reference to the JSClass structure was
saved, which is basically just a vtable without any actual state.
This is necessary to be able to create new instances of that prototype
without calling the provided constructor.

Created attachment 157542 [details] [review]
[cairo] Send in prototype to JS_NewObject

Send in a reference to the prototype to JS_NewObject, this
makes sure that the created object contains all properties from
the prototypes. This makes it possible to call Cairo.Context methods
on wrappers created from API created through introspection.

Review of attachment 157541 [details] [review]:

::: gjs/jsapi-util.h
@@ +144,3 @@
     return JS_TRUE; \
 } \
+static JSObject * cname##_prototype; \

This isn't gc-rooted... the way we do it elsewhere in gjs iirc is to save it in JS, not in C. That is, just get the prototype by looking up imports.cairo.Context.prototype or whatever.

(In reply to comment #3)
> Review of attachment 157541 [details] [review]:
> 
> ::: gjs/jsapi-util.h
> @@ +144,3 @@
>      return JS_TRUE; \
>  } \
> +static JSObject * cname##_prototype; \
> 
> This isn't gc-rooted... the way we do it elsewhere in gjs iirc is to save it in
> JS, not in C. That is, just get the prototype by looking up
> imports.cairo.Context.prototype or whatever.

You could think of it as an optimization I guess, to avoid having to do the JS->C. Adding the prototype w/ JS_AddRoot is easy, I'm just not sure how a prototype object is finalized.

Basically you want the prototype to be gc'd if there are no instances of the object anymore and the module is unloaded (not something we support yet, but we should eventually probably)

I think the C variable with AddRoot would make it "resident" (impossible to unload).

Created attachment 349270 [details] [review]
tests: Ensure a Cairo context is fully created in GI

According to the bug report, if you created a Cairo context through a GI
function such as Gdk.cairo_create(), it would be an empty wrapper. At some
point this was fixed, I suspect in SpiderMonkey itself, by associating the
prototype object with the JSClass and having JS_NewObject() look for it.

However, this behaviour may have changed in SpiderMonkey 45, so we should
add a test to make sure it doesn't regress.

This currently works, so the above patch just adds a test.

BUT! The test fails on SpiderMonkey 45 because they've made a change to JS_NewObject(); it doesn't look for the appropriate prototype object anymore. Instead, you're supposed to use JS_NewObjectWithGivenProto() and pass it explicitly. So I will rebase the patches and update them anyway.

I think it's OK to have the prototypes permanently rooted since we don't have loadable or unloadable modules. SpiderMonkey itself sticks them in slots on the global object, which we could also do, I think.

Review of attachment 349270 [details] [review]:

++

Comment on attachment 349270 [details] [review]
tests: Ensure a Cairo context is fully created in GI

Attachment 349270 [details] pushed as 23fb49b - tests: Ensure a Cairo context is fully created in GI

Created attachment 349490 [details] [review]
Export prototype in GJS_DEFINE_PROTO

When creating a new prototype using the GJS_DEFINE_PROTO macros,
save a static reference to the JSObject* representing the created
prototype. Previously only a reference to the JSClass structure was
saved, which is basically just a vtable without any actual state.
This is necessary to be able to create new instances of that prototype
without calling the provided constructor.

Created attachment 349491 [details] [review]
Send in prototype to JS_NewObject

Send in a reference to the prototype to JS_NewObject, this
makes sure that the created object contains all properties from
the prototypes. This makes it possible to call Cairo.Context methods
on wrappers created from API created through introspection.

Created attachment 349492 [details] [review]
global: Remove GJS_GLOBAL_SLOT_KEEP_ALIVE

We don't have a keep-alive object anymore so we don't need a slot to keep
it in.

I'm working on another patch to stick the prototypes in global object slots, but here are updates of Johan's patches.

Review of attachment 349490 [details] [review]:

++

Review of attachment 349491 [details] [review]:

++

Review of attachment 349492 [details] [review]:

++

Attachment 349490 [details] pushed as 3141fc0 - Export prototype in GJS_DEFINE_PROTO
Attachment 349491 [details] pushed as bc8f143 - Send in prototype to JS_NewObject
Attachment 349492 [details] pushed as 0744d13 - global: Remove GJS_GLOBAL_SLOT_KEEP_ALIVE

Created attachment 349585 [details] [review]
jsapi-util: Return JSObject from create_proto

The return value is a prototype object, so there's no use in returning
JS::Value here only to convert it to an object in the calling code.

Created attachment 349586 [details] [review]
jsapi-util: GJS_DEFINE_PROTO stores protos in global slots

This refactors the GJS_DEFINE_PROTO family of macros to store the created
prototype objects in slots on the global object. We split up the
gjs_WHATEVER_create_proto() function into two functions: one that actually
creates the prototype or returns it if already found in a global slot,
called gjs_WHATEVER_ensure_proto(); and one that defines a constructor,
accessible from JS, as a property on a module object, called
gjs_WHATEVER_define_constructor(). (gjs_WHATEVER_create_proto() was kind of
a misnomer since it returned the constructor, not the prototype, and so we
do a corresponding rename in cairo-image-surface.cpp, for clarity.)

gjs_WHATEVER_define_constructor() is not necessary for every class; we only
need to call it if we actually want to define the constructor as a property
on a module object. Therefore we split it out into its own macro,
GJS_EXPORT_PROTO. This macro takes a "js_name" parameter, which is the name
of the property to define. Previously, the JS name was passed to the
gjs_WHATEVER_create_proto() function. It's better to keep that definition
together with the rest of the information about the class, fixed at compile
time.

We also add two macros, GJS_DEFINE_PROTO_WITH_PARENT and
GJS_DEFINE_PROTO_ABSTRACT_WITH_PARENT, which also move the definition of
the parent to compile time rather than runtime, and keep it in the same
place as the rest of each class's macro definition. Similarly, previously
we had to pass the parent prototype object to gjs_WHATEVER_create_proto().

This mostly affects the cairo module.

Review of attachment 349585 [details] [review]:

++

Review of attachment 349586 [details] [review]:

Feel free to push with these two nitpicks fixed.

::: gjs/jsapi-util.h
@@ +288,3 @@
+ * GJS_EXPORT_PROTO:
+ * @cn: The name of the prototype, separated by _
+        return false;                                                          \

Nitpick: the arguments in the macro are called cname and js_name.

@@ +291,3 @@
+ *
+ * Defines a function gjs_<cn>_define_constructor(JSContext *, HandleObject)
+    gjs_set_global_slot(cx, GJS_GLOBAL_SLOT_PROTOTYPE_##cname,                 \

Isn't that called gjs_<cname>_ensure_proto()?

Comment on attachment 349585 [details] [review]
jsapi-util: Return JSObject from create_proto

Attachment 349585 [details] pushed as 06e37d7 - jsapi-util: Return JSObject from create_proto

Created attachment 349651 [details] [review]
jsapi-util: GJS_DEFINE_PROTO stores protos in global slots

This refactors the GJS_DEFINE_PROTO family of macros to store the created
prototype objects in slots on the global object. We split up the
gjs_WHATEVER_create_proto() function into two functions: one that actually
creates the prototype or returns it if already found in a global slot,
called gjs_WHATEVER_ensure_proto(); and one that defines a constructor,
accessible from JS, as a property on a module object, called
gjs_WHATEVER_define_constructor(). (gjs_WHATEVER_create_proto() was kind of
a misnomer since it returned the constructor, not the prototype, and so we
do a corresponding rename in cairo-image-surface.cpp, for clarity.)

gjs_WHATEVER_define_constructor() is not necessary for every class; we only
need to call it if we actually want to define the constructor as a property
on a module object. Therefore we split it out into its own macro,
GJS_EXPORT_PROTO. This macro takes a "js_name" parameter, which is the name
of the property to define. Previously, the JS name was passed to the
gjs_WHATEVER_create_proto() function. It's better to keep that definition
together with the rest of the information about the class, fixed at compile
time.

We also add two macros, GJS_DEFINE_PROTO_WITH_PARENT and
GJS_DEFINE_PROTO_ABSTRACT_WITH_PARENT, which also move the definition of
the parent to compile time rather than runtime, and keep it in the same
place as the rest of each class's macro definition. Similarly, previously
we had to pass the parent prototype object to gjs_WHATEVER_create_proto().

This mostly affects the cairo module.

Created attachment 349652 [details] [review]
jsapi-dynamic-class: Use JS_LinkConstructorAndPrototype()

Small cleanup; there is a function in the JSAPI that does exactly this,
automatically with the right flags.

Created attachment 349653 [details] [review]
jsapi-util: Add static function spec to GJS_DEFINE_PROTO

This allows the GJS_DEFINE_PROTO_* family of macros to have a JSFunctionSpec
array for functions defined on the constructor, rather than on the
prototype: i.e. static functions.

We clean up an incongruity in cairo-image-surface.cpp where you had to pass
the constructor into gjs_cairo_image_surface_init() in order to get the
static functions defined.

There's not much difference in the new version of "GJS_DEFINE_PROTO stores protos in global slots", just a few cleanups.

I still need to test this on my mozjs45 branch.

Review of attachment 349651 [details] [review]:

It's a bit unfortunate that you need to add separate declarations for all the ensure_proto implementations, and that you have to enumerate all the values manually in GjsGlobalSlot, but I don't have a better approach to offer.

Review of attachment 349652 [details] [review]:

Nice

Review of attachment 349653 [details] [review]:

Again, a bit unfortunate that you have to add empty arrays for every case here, but not sure if there's a better approach.

Comment on attachment 349652 [details] [review]
jsapi-dynamic-class: Use JS_LinkConstructorAndPrototype()

Attachment 349652 [details] pushed as bd36b0b - jsapi-dynamic-class: Use JS_LinkConstructorAndPrototype()

Created attachment 349704 [details] [review]
importer: Add a JSClass constant

We use a js::Class for the importer rather than a JSClass, but it is
easier to include a JSClass constant as well so that we don't have to
keep casting back to JSClass with js::Jsvalify().

Created attachment 349705 [details] [review]
WIP - js: Create objects with JS_NewObjectWithGivenProto()

Starting in SpiderMonkey 45 we have to pass in the prototype object to
all our instances of native classes. This is a sample of how that work
looks when it's done.

FIXME - ByteArray already does something similar, but can be accomplished
in less code using these macros.

This last patch is an example of what the cleanups would look like when applied to the other native classes. I checked on my mozjs45 branch and this is indeed required for SpiderMonkey 45; I have it down to only one failing test now!

I still have to do repo.cpp and byteArray.cpp, and I might still do some refactoring on "GJS_DEFINE_PROTO stores protos in global slots"; it might be if we pass in the module object to JS_InitClass then we don't even have to have gjs_WHATEVER_define_constructor() at all.

I do have an alternative to the macro hell; we could refactor it to use classes rather than macros. It would not solve the problem of having to add each class to the GjsGlobalSlot enum, though.

Review of attachment 349704 [details] [review]:

OK

(In reply to Philip Chimento from comment #33)
> This last patch is an example of what the cleanups would look like when
> applied to the other native classes. I checked on my mozjs45 branch and this
> is indeed required for SpiderMonkey 45; I have it down to only one failing
> test now!
> 
> I still have to do repo.cpp and byteArray.cpp, and I might still do some
> refactoring on "GJS_DEFINE_PROTO stores protos in global slots"; it might be
> if we pass in the module object to JS_InitClass then we don't even have to
> have gjs_WHATEVER_define_constructor() at all.

Sounds like a good plan, and I like the cleanup quite a bit.
 
> I do have an alternative to the macro hell; we could refactor it to use
> classes rather than macros. It would not solve the problem of having to add
> each class to the GjsGlobalSlot enum, though.

I don't particularly mind the macros -- they're not such an uncommon paradigm for this kind of things (see e.g. GObject).

Comment on attachment 349704 [details] [review]
importer: Add a JSClass constant

Attachment 349704 [details] pushed as 7a0866c - importer: Add a JSClass constant

Created attachment 349763 [details] [review]
jsapi-util: Split off class stuff into jsapi-class.h

I've been wishing I had done this for three days now; jsapi-util.h is far
too long, and it's no fun to recompile the entire program every time one
of the macros is changed. The class macros plus the dynamic class functions
make a fairly independent unit, so this seems logical.

Still refactoring, but I split off this patch: it's driving me crazy to recompile the entire GJS every time :-)

Review of attachment 349763 [details] [review]:

Looks good.

::: gjs/jsapi-class.h
@@ +23,3 @@
+
+#ifndef GJS_JSAPI_CLASS_H
+#define GJS_JSAPI_CLASS_H

As an aside, any reason not to start using #pragma once? Or is that not compatible with Visual Studio?

(In reply to Cosimo Cecchi from comment #39)
> As an aside, any reason not to start using #pragma once? Or is that not
> compatible with Visual Studio?

I don't know for sure, but it often doesn't support nice nonstandard extensions.

Comment on attachment 349763 [details] [review]
jsapi-util: Split off class stuff into jsapi-class.h

Attachment 349763 [details] pushed as aa60fc5 - jsapi-util: Split off class stuff into jsapi-class.h

Created attachment 349846 [details] [review]
jsapi-class: Get rid of "proto_name" argument

There's no need to have a "proto_name" that's different from the class's
name. Getting rid of this distinction will allow us to refactor things so
that JS_InitClass() defines the correct constructor directly on the global
object or the module object.

This is a small behavioural API break, since it changes the output of
toString() on the various Cairo objects, but I would say that people
should not be parsing the output of toString(). (I know we do in our tests,
but that's terrible.)

Created attachment 349847 [details] [review]
jsapi-class: GJS_DEFINE_PROTO stores protos in global slots

This refactors the GJS_DEFINE_PROTO family of macros to store the created
prototype objects in slots on the global object. We split up the
gjs_WHATEVER_create_proto() function into two functions: one that must be
called at least once for each type to define the class, store the prototype
in a global slot, and define the constructor as a property on a passed-in
module or global object, gjs_WHATEVER_define_proto(); and one that
retrieves the prototype from its global slot, gjs_WHATEVER_get_proto().

We also add two macros, GJS_DEFINE_PROTO_WITH_PARENT and
GJS_DEFINE_PROTO_ABSTRACT_WITH_PARENT, which also move the definition of
the parent to compile time rather than runtime, and keep it in the same
place as the rest of each class's macro definition. Similarly, previously
we had to pass the parent prototype object to gjs_WHATEVER_create_proto().

This mostly affects the cairo module.

This newest patch is, I think, a better version of the previous attempt. We don't need an extra GJS_EXPORT_PROTO macro. Instead, we directly pass in the global object or module object so that the constructor is defined as part of JS_InitClass().

Created attachment 349848 [details] [review]
jsapi-util: Add static function spec to GJS_DEFINE_PROTO

This allows the GJS_DEFINE_PROTO_* family of macros to have a JSFunctionSpec
array for functions defined on the constructor, rather than on the
prototype: i.e. static functions.

We clean up an incongruity in cairo-image-surface.cpp where you had to pass
the constructor into gjs_cairo_image_surface_init() in order to get the
static functions defined.

No change in this patch, just a rebase of the previous version.

Review of attachment 349847 [details] [review]:

::: gjs/jsapi-class.h
@@ +227,3 @@
     JS::RootedId class_name(cx,                                              \
         gjs_intern_string_to_id(cx, gjs_##cname##_class.name));              \
+    if (ctor != nullptr) {                                                   \

There is one problem still with this patch: this line warns that "ctor" will never be null when you pass a constructor function. I seem to be having trouble to get _Pragma("GCC diagnostic ignored -Waddress") to ignore it, which may be because of the macro definition.

Or I could do something ridiculous like if (strcmp(#ctor, "nullptr")) but that's ridiculous.

Created attachment 349883 [details] [review]
js: Create objects with JS_NewObjectWithGivenProto()

Starting in SpiderMonkey 45 we have to pass in the prototype object to
all our instances of native classes.

Since we only need the _get_proto and _define_proto functions for the
native classes, and not the JSClass like we do for gtype and cairo, we
split them out into a separate macro family,
GJS_DEFINE_PROTO_FUNCS[_WITH_PARENT].

We also make a few amendments to the macro: g_error instead of throw an
exception when JS_InitClass fails, since that's what the code we are
replacing did; and a debug message on success. We have to change the debug
topic to GJS_DEBUG_CONTEXT in order to avoid having to pass in yet another
parameter to the macro just for debugging.

Review of attachment 349847 [details] [review]:

::: gjs/jsapi-class.h
@@ +227,3 @@
     JS::RootedId class_name(cx,                                              \
         gjs_intern_string_to_id(cx, gjs_##cname##_class.name));              \
+    if (ctor != nullptr) {                                                   \

Maybe the best solution would be to skip the check, and look up the constructor function, even in the case where we could optimize by assuming it's equal to the prototype object.

Review of attachment 349846 [details] [review]:

Looks good.

Review of attachment 349847 [details] [review]:

::: gjs/jsapi-class.h
@@ +227,3 @@
     JS::RootedId class_name(cx,                                              \
         gjs_intern_string_to_id(cx, gjs_##cname##_class.name));              \
+    if (ctor != nullptr) {                                                   \

I think that's the best approach here; it's not like this function is called in a hot path anyway, right?

Review of attachment 349848 [details] [review]:

I think I complained before about the fact that this introduces another array that has a default empty value most of the times, but if there's no way around it...

Review of attachment 349883 [details] [review]:

++

Comment on attachment 349846 [details] [review]
jsapi-class: Get rid of "proto_name" argument

Attachment 349846 [details] pushed as dcc82c3 - jsapi-class: Get rid of "proto_name" argument

Created attachment 349966 [details] [review]
jsapi-class: GJS_DEFINE_PROTO stores protos in global slots

This refactors the GJS_DEFINE_PROTO family of macros to store the created
prototype objects in slots on the global object. We split up the
gjs_WHATEVER_create_proto() function into two functions: one that must be
called at least once for each type to define the class, store the prototype
in a global slot, and define the constructor as a property on a passed-in
module or global object, gjs_WHATEVER_define_proto(); and one that
retrieves the prototype from its global slot, gjs_WHATEVER_get_proto().

We also add two macros, GJS_DEFINE_PROTO_WITH_PARENT and
GJS_DEFINE_PROTO_ABSTRACT_WITH_PARENT, which also move the definition of
the parent to compile time rather than runtime, and keep it in the same
place as the rest of each class's macro definition. Similarly, previously
we had to pass the parent prototype object to gjs_WHATEVER_create_proto().

This mostly affects the cairo module.

(In reply to Cosimo Cecchi from comment #52)
> Review of attachment 349848 [details] [review] [review]:
> 
> I think I complained before about the fact that this introduces another
> array that has a default empty value most of the times, but if there's no
> way around it...

There are a few ways around it: 1) we could just not do the refactor, it's quite minor. 2) we could make the refactor bigger and move away from macros...

I went ahead and pushed it with the approach that you acked in the previous review.

Attachment 349848 [details] pushed as 4b8dcca - jsapi-util: Add static function spec to GJS_DEFINE_PROTO
Attachment 349883 [details] pushed as d794f31 - js: Create objects with JS_NewObjectWithGivenProto()
Attachment 349966 [details] pushed as 8807af7 - jsapi-class: GJS_DEFINE_PROTO stores protos in global slots

This patch seems to introduce an assertion in some existing gobject bindings which have worked fine til now:

https://bugzilla.redhat.com/show_bug.cgi?id=1524125

Gjs:ERROR:gjs/byteArray.cpp:824:JSObject* gjs_byte_array_get_proto(JSContext*): assertion failed: (((void) "gjs_" "byte_array" "_define_proto() must be called before " "gjs_" "byte_array" "_get_proto()", !v_proto.isUndefined()))

This is when calling from C back into gobject code.

Is some change required to gis code or is it supposed to be internal?

This is supposed to be internal, so what you found sounds like a bug in GJS.

Would you mind opening a bug on gitlab.gnome.org/GNOME/gjs with a minimal example or a link to the failing JS code? Thanks!

Trying to create an account on gitlab.gnome.org results in a 500
server error, so I couldn't do that.

However I have posted the affected code here (it is generated which
is why it's quite long):

http://oirase.annexia.org/tmp/gobject/src/session.c
(in the function event_callback where it calls g_signal_emit)

(In reply to Richard Jones from comment #60)
> Trying to create an account on gitlab.gnome.org results in a 500
> server error, so I couldn't do that.

Could you try again please? You can also log in with a GitHub or several other kinds of account, if you have one. I won't be able to track this problem here.

After spending another 5 minutes doing the stupid "captcha", I
still got a 500 error.  Is there not some other forum where one
can file bugs?

Please join #sysadmin or #gnome-hackers on IRC and ask csoriano or av about the login problems. This seems like a pretty serious problem with the GitLab instance. That said, I haven't seen any service outages on gitlab.gnome.org myself, so I wonder if there's a problem with account creation specifically.

Bug submitted here:
https://gitlab.gnome.org/GNOME/gjs/issues/39