GNOME Bugzilla – Bug 742249
spidermonkey 31 prep
Last modified: 2016-11-11 02:59:09 UTC
JS_ARGV, JS_SET_RVAL are gone in spidermonkey 31, and JS_THIS will be going away in the future
Created attachment 293630 [details] [review] Port to CallReceiver/CallArgs
Created attachment 293631 [details] [review] boxed: Remove usage of TinyId's These are removed in spidermonkey 31
Giovanni did suggest on IRC trying to bypass the internal property lookups since we now have 2 hashtables after the TinyID removal, I still haven't been able to work out if that is possible, seems to me though that the JSClass PropertyOp hooks would fire after the internal lookup.
Review of attachment 293630 [details] [review]: ::: gjs/compat.h @@ -75,3 @@ * GJS_NATIVE_CONSTRUCTOR_VARIABLES: * Declare variables necessary for the constructor; should - * be at the very top. Any reason this went away? ::: gjs/jsapi-util.cpp @@ +1068,2 @@ /** + * gjs_parse_call_args: Did you mean to change this?
Review of attachment 293631 [details] [review]: I'll see if there's a better solution than this. I'm quite certain you can have native getters/setters like this without a name lookup at runtime. ::: gi/boxed.cpp @@ +572,3 @@ + + out: + return NULL; Weird indentation.
Created attachment 293676 [details] [review] boxed: Remove usage of TinyId's These are removed in spidermonkey 31 fixed Jasper's comment
Created attachment 293677 [details] [review] Port to CallReceiver/CallArgs JS_ARGV, JS_SET_RVAL are gone in spidermonkey 31, and JS_THIS will be going away in the future
Created attachment 293679 [details] [review] Build with -std=c++11 as required by spidermonkey 31
Attachment 293677 [details] pushed as 31a1e86 - Port to CallReceiver/CallArgs
*** Bug 751252 has been marked as a duplicate of this bug. ***
They appear to be syncing mozjs releases with firefox ESR releases so expect version 38 eventually. https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Releases
Tim, are the other two patches still ready for review? Any interest in resurrecting this?
Cosimo, they should be, though haven't checked if they still apply to master. The exact rooting port is pretty massive, I had hoped to semi-automate it using clang, but that never really worked out. Mozilla did it incrementally, none of their tools will even work for us now we have to deal with it all of once. I probably don't have the spare cycles to work on the complete port by myself, but if we could get a few people working on it, maybe it could be knocked off in a weekend or so?
Current release is https://people.mozilla.org/~sfink/mozjs-45.0.2.tar.bz2 if this is still interest in this.
I've been taking a look at switching to GC rooting. This will change the API of pretty much everything in jsapi-util.h. How does this affect other modules that use libgjs's internals such as gnome-shell? Do we need an API bump of libgjs and gjs's .pc files?
Created attachment 335826 [details] [review] byteArray: Remove further usage of TinyId This was probably not doing anything in the first place, but will be ignored starting with mozjs31.
Created attachment 335827 [details] [review] build: Require GTK 3.20 Our Gtk.WidgetClass now requires gtk_widget_class_set_css_name(), which was first available in 3.20.
Created attachment 335828 [details] [review] build: Require C++11 This was already required in practice, as the mozjs24 headers included C++11 features. Probably this was not noticed because GCC did not warn about that, but Clang does.
Created attachment 335829 [details] [review] build: Suppress jsapi.h warnings in Clang It requires slightly different macros to suppress the warnings from jsapi.h in Clang.
Created attachment 335830 [details] [review] build: Avoid compiler warnings This gets rid of one possible free() of an uninitialized variable; renames some variables that shadowed local variables in outer blocks; removes some unused variables; adds arguments to invocations of varargs macros without any varargs, since this is disallowed; and puts extra braces around initializers where the compiler complains about it.
Created attachment 335831 [details] [review] byteArray: Fix bug with length > maxint32 This is extremely unlikely to happen, but I noticed it while making some other changes.
Created attachment 335832 [details] [review] js: Discontinue usage of JSBool In mozjs31, JSBool and its values JS_TRUE and JS_FALSE are discontinued in favor of regular C++ bool, true, and false. In order to ease porting to mozjs31, we switch to C++ booleans everywhere. Almost everywhere, that is. In some cases bool *, or function pointers with bool return types, will not automatically be cast. We therefore leave a few instances of JSBool in the code. These will be removed when the actual port to mozjs31 happens. Fixes a few formatting glitches in lines of code that were touched anyway.
Created attachment 335833 [details] [review] js: Prefer bool to gboolean Since we removed JSBool, JS_TRUE, and JS_FALSE in the previous commit, we now also make usage of gboolean, TRUE, and FALSE consistent. In some places, JSBool and gboolean were used interchangeably. Here, we take the approach of using C++ bool, true, and false everywhere, except when required by the API, e.g. for the return type of idle functions. In a few instances, TRUE and FALSE were used in comments in JS code; here we change those to true and false to avoid confusion. Fixes a few formatting glitches in lines of code that were touched anyway.
Created attachment 335834 [details] [review] doc: Update JS::Value memory notes This information was out of date, not corresponding to how JS::Value worked in mozjs24. This updates the information and also abridges it, since it is explained fairly well on the MDN wiki now; we don't need to maintain a separate document about it that will just get out of date.
Created attachment 335835 [details] [review] js: Replace all jsval by JS::Value JS::Value is the new-style API. Nowadays jsval is simply a typedef for JS::Value for convenience, but since we will be updating everything to the JS::RootedValue, JS::HandleValue style API in mozjs31, we may as well go through and weed out all usage of jsval in favor of JS::Value for clarity.
Created attachment 335836 [details] [review] js: Remove use of JSVAL_* macros Macros such as JSVAL_IS_STRING, JSVAL_TO_BOOLEAN, OBJECT_TO_JSVAL, etc., are removed in mozjs31. Instead, use the equivalent methods of JS::Value. This would be fairly straightforward except for JSVAL_IS_OBJECT, JSVAL_TO_OBJECT, and OBJECT_TO_JSVAL. Contrary to what the names imply, these macros deal with values that are either objects or null. The newer API's isObject(), toObject(), and JS::ObjectValue() do not include null. Strictly speaking, all uses of JSVAL_TO_OBJECT should be replaced by the isObjectOrNull() method, and likewise for the other OBJECT macros. (JSVAL_IS_OBJECT had already been removed from the API in an earlier SpiderMonkey release and was implemented as a shim in compat.h; this shim is now removed.) However, there were many cases where - it was already ruled out that a value was null; - it was already ruled out that a JSObject pointer was null; - or, in my opinion the existing code erroneously allowed a null value. In these cases, I have replaced the OBJECT macros with the equivalent Object methods. A few strange undocumented macros were also used; JSVAL_IS_TRACEABLE seems to be equivalent to JS::Value::isGCThing().
Created attachment 335837 [details] [review] compat: Remove JS_GetGlobalObject define As long as we are switching to new SpiderMonkey API, we should get rid of this define that mimics removed API, since we wrote a function to replace it.
Created attachment 335838 [details] [review] compat: Remove JS_NewNumberValue shim This shim was not necessary as it checked for failure of a function that can't fail according to the SpiderMonkey documentation. Instead switch to the newer JS::NumberValue() API. This allows simplifying the code in a few places since a point is now removed where it was necessary to check for errors.
Created attachment 335839 [details] [review] class: Deprecate gjs_new_object_for_constructor() According to the comments, in mozjs 1.8.5, JS_NewObjectForConstructor() did not work with our dynamic classes. Therefore a gjs_new_object_for_constructor() shim was created that copied the JS_NewObjectForConstructor() code from mozjs 1.9. Now that we are on mozjs 24, we can get rid of this shim. Unfortunately we can't remove the function altogether because it's public API in libgjs.
Created attachment 335840 [details] [review] compat: Move native constructor macros These macros are no longer protected by a jsapi.h version check, so they should be in jsapi-util.h with the other macros, rather than in compat.h.
Review of attachment 293679 [details] [review]: In my opinion better to do a configure-time check for C++11, I've attached a different patch that does this.
Here are 15 patches switching away from APIs that will disappear in mozjs31, plus some various cleanups to make things easier (for example, getting rid of Clang warnings was important so I could see any new warnings in the code as I changed it.) I have not tackled the exact rooting port yet, but this does fix pretty much everything other than that that can be fixed while still targeting mozjs24. For ease of review, building, and testing, these patches plus Tim's TinyId removal can be found on the wip/ptomato/mozjs31prep branch. Likewise, there is a wip/ptomato/mozjs31 branch with some commits on top of that for post-switch changes.
The patches break ompiling gnome-shell: shell-js.cpp: In function ‘gboolean shell_js_add_extension_importer(const char*, const char*, const char*, GError**)’: shell-js.cpp:49:52: error: ‘JS_GetGlobalObject’ was not declared in this scope JS_GetGlobalObject(context), ^ shell-js.cpp:64:38: error: ‘JSVAL_IS_OBJECT’ was not declared in this scope if (!JSVAL_IS_OBJECT (target_object)) ^ But I guess gnome-shell developers will need to fix that on their side?
I forgot those were of course part of the API too. I'll push new patches later that don't remove those shims (and remove them instead on the post-31 update branch)
Created attachment 335894 [details] [review] js: Remove use of JSVAL_* macros Macros such as JSVAL_IS_STRING, JSVAL_TO_BOOLEAN, OBJECT_TO_JSVAL, etc., are removed in mozjs31. Instead, use the equivalent methods of JS::Value. This would be fairly straightforward except for JSVAL_IS_OBJECT, JSVAL_TO_OBJECT, and OBJECT_TO_JSVAL. Contrary to what the names imply, these macros deal with values that are either objects or null. The newer API's isObject(), toObject(), and JS::ObjectValue() do not include null. Strictly speaking, all uses of JSVAL_TO_OBJECT should be replaced by the isObjectOrNull() method, and likewise for the other OBJECT macros. (JSVAL_IS_OBJECT had already been removed from the API in an earlier SpiderMonkey release and was implemented as a shim in compat.h; this shim is now removed.) However, there were many cases where - it was already ruled out that a value was null; - it was already ruled out that a JSObject pointer was null; - or, in my opinion the existing code erroneously allowed a null value. In these cases, I have replaced the OBJECT macros with the equivalent Object methods. A few strange undocumented macros were also used; JSVAL_IS_TRACEABLE seems to be equivalent to JS::Value::isGCThing().
Created attachment 335895 [details] [review] compat: Remove use of JS_GetGlobalObject define As long as we are switching to new SpiderMonkey API, we should get rid of all usage this define that mimics removed API, since we wrote a function to replace it. Unfortunately we must leave the define itself, since it's part of the libgjs API, but we can mark it deprecated.
Created attachment 335896 [details] [review] compat: Remove use of JS_NewNumberValue shim This shim was not necessary as it checked for failure of a function that can't fail according to the SpiderMonkey documentation. Instead switch to the newer JS::NumberValue() API. This allows simplifying the code in a few places since a point is now removed where it was necessary to check for errors. Unfortunately we can't remove the shim itself as it's part of the libgjs API, but we can add a note that it's deprecated.
Created attachment 335897 [details] [review] class: Deprecate gjs_new_object_for_constructor() According to the comments, in mozjs 1.8.5, JS_NewObjectForConstructor() did not work with our dynamic classes. Therefore a gjs_new_object_for_constructor() shim was created that copied the JS_NewObjectForConstructor() code from mozjs 1.9. Now that we are on mozjs 24, we can get rid of this shim. Unfortunately we can't remove the function altogether because it's public API in libgjs.
Created attachment 335898 [details] [review] compat: Move native constructor macros These macros are no longer protected by a jsapi.h version check, so they should be in jsapi-util.h with the other macros, rather than in compat.h.
Review of attachment 335826 [details] [review]: Looks good to me.
Review of attachment 335827 [details] [review]: Sure.
Review of attachment 335828 [details] [review]: Sure.
Review of attachment 335829 [details] [review]: OK
Review of attachment 335830 [details] [review]: Nice
Review of attachment 335831 [details] [review]: Nice catch.
Review of attachment 335832 [details] [review]: I trust that this is just an automated search/replace.
Review of attachment 335833 [details] [review]: I trust that this is also an automated search/replace.
Review of attachment 335834 [details] [review]: Sure!
Review of attachment 335835 [details] [review]: I trust that this is also an automated search/replace.
Review of attachment 335895 [details] [review]: Looks good.
Review of attachment 335896 [details] [review]: Looks good.
Review of attachment 335897 [details] [review]: Sure
Review of attachment 335898 [details] [review]: OK
Review of attachment 335894 [details] [review]: ::: gi/arg.cpp @@ +2260,2 @@ if (arg->v_pointer == NULL) { + *value_p = JS::UndefinedValue(); Should be JS::NullValue().
Review of attachment 293676 [details] [review]: Just one nitpick. ::: gi/boxed.cpp @@ +877,3 @@ gboolean result; + result = JS_DefineProperty(context, proto, field_name, Bad indentation
Philip, most of the patches look good to me, or were too big/mechanical for a proper review. I however found what I think is a genuine bug. In the meantime, I released a new "stable" gjs 1.46.0 and branched it for GNOME 3.22, so we can start pushing this to master right away.
Attachment 335826 [details] pushed as 9c831a8 - byteArray: Remove further usage of TinyId Attachment 335827 [details] pushed as 62e255b - build: Require GTK 3.20 Attachment 335828 [details] pushed as 4e627cb - build: Require C++11 Attachment 335829 [details] pushed as 8f4936a - build: Suppress jsapi.h warnings in Clang Attachment 335830 [details] pushed as 3924a6b - build: Avoid compiler warnings Attachment 335831 [details] pushed as 17a296a - byteArray: Fix bug with length > maxint32 Attachment 335834 [details] pushed as b68a2e3 - doc: Update JS::Value memory notes
Created attachment 335965 [details] [review] boxed: Remove usage of TinyId's These are removed in spidermonkey 31
Created attachment 335966 [details] [review] js: Discontinue usage of JSBool In mozjs31, JSBool and its values JS_TRUE and JS_FALSE are discontinued in favor of regular C++ bool, true, and false. In order to ease porting to mozjs31, we switch to C++ booleans everywhere. Almost everywhere, that is. In some cases bool *, or function pointers with bool return types, will not automatically be cast. We therefore leave a few instances of JSBool in the code. These will be removed when the actual port to mozjs31 happens. Fixes a few formatting glitches in lines of code that were touched anyway.
Created attachment 335967 [details] [review] js: Prefer bool to gboolean Since we removed JSBool, JS_TRUE, and JS_FALSE in the previous commit, we now also make usage of gboolean, TRUE, and FALSE consistent. In some places, JSBool and gboolean were used interchangeably. Here, we take the approach of using C++ bool, true, and false everywhere, except when required by the API, e.g. for the return type of idle functions. In a few instances, TRUE and FALSE were used in comments in JS code; here we change those to true and false to avoid confusion. Fixes a few formatting glitches in lines of code that were touched anyway.
Created attachment 335968 [details] [review] js: Replace all jsval by JS::Value JS::Value is the new-style API. Nowadays jsval is simply a typedef for JS::Value for convenience, but since we will be updating everything to the JS::RootedValue, JS::HandleValue style API in mozjs31, we may as well go through and weed out all usage of jsval in favor of JS::Value for clarity.
Created attachment 335969 [details] [review] js: Remove use of JSVAL_* macros Macros such as JSVAL_IS_STRING, JSVAL_TO_BOOLEAN, OBJECT_TO_JSVAL, etc., are removed in mozjs31. Instead, use the equivalent methods of JS::Value. This would be fairly straightforward except for JSVAL_IS_OBJECT, JSVAL_TO_OBJECT, and OBJECT_TO_JSVAL. Contrary to what the names imply, these macros deal with values that are either objects or null. The newer API's isObject(), toObject(), and JS::ObjectValue() do not include null. Strictly speaking, all uses of JSVAL_TO_OBJECT should be replaced by the isObjectOrNull() method, and likewise for the other OBJECT macros. (JSVAL_IS_OBJECT had already been removed from the API in an earlier SpiderMonkey release and was implemented as a shim in compat.h; this shim is now removed.) However, there were many cases where - it was already ruled out that a value was null; - it was already ruled out that a JSObject pointer was null; - or, in my opinion the existing code erroneously allowed a null value. In these cases, I have replaced the OBJECT macros with the equivalent Object methods. A few strange undocumented macros were also used; JSVAL_IS_TRACEABLE seems to be equivalent to JS::Value::isGCThing().
Created attachment 335970 [details] [review] compat: Remove use of JS_GetGlobalObject define As long as we are switching to new SpiderMonkey API, we should get rid of all usage this define that mimics removed API, since we wrote a function to replace it. Unfortunately we must leave the define itself, since it's part of the libgjs API, but we can mark it deprecated.
Created attachment 335971 [details] [review] compat: Remove use of JS_NewNumberValue shim This shim was not necessary as it checked for failure of a function that can't fail according to the SpiderMonkey documentation. Instead switch to the newer JS::NumberValue() API. This allows simplifying the code in a few places since a point is now removed where it was necessary to check for errors. Unfortunately we can't remove the shim itself as it's part of the libgjs API, but we can add a note that it's deprecated.
Created attachment 335972 [details] [review] class: Deprecate gjs_new_object_for_constructor() According to the comments, in mozjs 1.8.5, JS_NewObjectForConstructor() did not work with our dynamic classes. Therefore a gjs_new_object_for_constructor() shim was created that copied the JS_NewObjectForConstructor() code from mozjs 1.9. Now that we are on mozjs 24, we can get rid of this shim. Unfortunately we can't remove the function altogether because it's public API in libgjs.
Created attachment 335973 [details] [review] compat: Move native constructor macros These macros are no longer protected by a jsapi.h version check, so they should be in jsapi-util.h with the other macros, rather than in compat.h.
Yo, Cosimo! Thanks for the reviews. (In reply to Cosimo Cecchi from comment #46) > Review of attachment 335832 [details] [review] [review]: > > I trust that this is just an automated search/replace. Yes, these three were automated although I looked at each case to make sure it made sense. However, now that I think of it, I'm not sure that the JSBool/gboolean/bool replacements are able to be pushed right now, since they change the APIs to take bool instead of JSBool/gboolean (which are both typedef'd to int, I think). 1) Do you know if that is an API break for libgjs? 2) At what point can we actually break API for libgjs and has it ever been done before, how does gnome-shell cope with it etc.? (In reply to Cosimo Cecchi from comment #56) > Philip, most of the patches look good to me, or were too big/mechanical for > a proper review. I however found what I think is a genuine bug. > > In the meantime, I released a new "stable" gjs 1.46.0 and branched it for > GNOME 3.22, so we can start pushing this to master right away. I pushed to master the commits that you accepted that came before the big bool replace. The other stuff you flagged is fixed up now, thanks. I can try to reshuffle things so that some of the other patches come before the bool replace if we need to think about those for a while longer.
Review of attachment 335965 [details] [review]: Looks good
Review of attachment 335966 [details] [review]: OK
Review of attachment 335967 [details] [review]: OK
Review of attachment 335968 [details] [review]: OK
Review of attachment 335969 [details] [review]: Looks good now.
Review of attachment 335970 [details] [review]: Looks good.
Review of attachment 335971 [details] [review]: Looks good.
Review of attachment 335972 [details] [review]: OK
Review of attachment 335973 [details] [review]: OK
Hi, I've got a quick request, as your most of the way to mozjs38, would it be possible to target that as well? I ask this because fedora has 6 different versions of mozjs, and currently the only other user of mozjs31 is 0AD, and they have patches to move to mozjs38, where mongodb is currently. Basically, its a bit of a nightmare for the distro's to patch 5 different "unsupported" versions of mozjs, so its not handled well.
(In reply to Philip Chimento from comment #67) > Yes, these three were automated although I looked at each case to make sure > it made sense. However, now that I think of it, I'm not sure that the > JSBool/gboolean/bool replacements are able to be pushed right now, since > they change the APIs to take bool instead of JSBool/gboolean (which are both > typedef'd to int, I think). > > 1) Do you know if that is an API break for libgjs? > 2) At what point can we actually break API for libgjs and has it ever been > done before, how does gnome-shell cope with it etc.? I guess it is an API break for libgjs, and I'm not sure if it has been done before. Colin/Giovanni, do you know? In any event, gnome-shell could be changed to require the new version of the API; perhaps a safer way would be to introduce a couple of compatibility defines for the old symbols? > (In reply to Cosimo Cecchi from comment #56) > > Philip, most of the patches look good to me, or were too big/mechanical for > > a proper review. I however found what I think is a genuine bug. > > > > In the meantime, I released a new "stable" gjs 1.46.0 and branched it for > > GNOME 3.22, so we can start pushing this to master right away. > > I pushed to master the commits that you accepted that came before the big > bool replace. The other stuff you flagged is fixed up now, thanks. I can try > to reshuffle things so that some of the other patches come before the bool > replace if we need to think about those for a while longer. Sounds good; feel free to rework the patchset that way if you have the chance. I marked those patches as a-c-n because they technically look OK, but let's wait for Colin or Giovanni's opinion before actually pushing them.
I think it's fine to break libgjs API, because the only known project using it is gnome-shell. In fact, we should actually reduce the libgjs API surface to essentially "make a JS context, run this code". Anything else is unnecessary and should be done with gobject-introspection. What is not ok to break is the JS interface, but as far as I can see none of the patches affect that. (I only cursorily looked at the patches in the branch, not here, but they all make sense to me) The bool replace also is a good idea in general because <stdbool.h> has been a thing for 17 years now, and we should use it even in C code.
Attachment 335965 [details] pushed as fed55aa - boxed: Remove usage of TinyId's Attachment 335968 [details] pushed as dcfc291 - js: Replace all jsval by JS::Value Attachment 335969 [details] pushed as a152bed - js: Remove use of JSVAL_* macros Attachment 335970 [details] pushed as 0c0ec76 - compat: Remove use of JS_GetGlobalObject define Attachment 335971 [details] pushed as b4ce35d - compat: Remove use of JS_NewNumberValue shim Attachment 335972 [details] pushed as 2a67a87 - class: Deprecate gjs_new_object_for_constructor() Attachment 335973 [details] pushed as 941be05 - compat: Move native constructor macros
Created attachment 336046 [details] [review] js: Discontinue usage of JSBool In mozjs31, JSBool and its values JS_TRUE and JS_FALSE are discontinued in favor of regular C++ bool, true, and false. In order to ease porting to mozjs31, we switch to C++ booleans everywhere. Almost everywhere, that is. In some cases bool *, or function pointers with bool return types, will not automatically be cast. We therefore leave a few instances of JSBool in the code. These will be removed when the actual port to mozjs31 happens. Fixes a few formatting glitches in lines of code that were touched anyway.
Created attachment 336047 [details] [review] js: Prefer bool to gboolean Since we removed JSBool, JS_TRUE, and JS_FALSE in the previous commit, we now also make usage of gboolean, TRUE, and FALSE consistent. In some places, JSBool and gboolean were used interchangeably. Here, we take the approach of using C++ bool, true, and false everywhere, except when required by the API, e.g. for the return type of idle functions. In a few instances, TRUE and FALSE were used in comments in JS code; here we change those to true and false to avoid confusion. Fixes a few formatting glitches in lines of code that were touched anyway.
(In reply to jeremy.linton from comment #77) > Hi, I've got a quick request, as your most of the way to mozjs38, would it > be possible to target that as well? I ask this because fedora has 6 > different versions of mozjs, and currently the only other user of mozjs31 is > 0AD, and they have patches to move to mozjs38, where mongodb is currently. > > Basically, its a bit of a nightmare for the distro's to patch 5 different > "unsupported" versions of mozjs, so its not handled well. I do plan to keep on going until reaching the latest version mozjs45, but I think the amount of work, and risk of regressions, is prohibitive enough that I'd prefer not to make the leap all at once. (In reply to Cosimo Cecchi from comment #78) > In any event, gnome-shell could be changed to require the new version of the > API; perhaps a safer way would be to introduce a couple of compatibility > defines for the old symbols? There's no possibility for that in this case, as there are no new symbols; the return types and argument types of existing functions would be changed. In any case, I've held off pushing those patches until we can figure out what to do vis-a-vis gnome-shell (and eos-desktop, which as a gnome-shell fork also uses libgjs.) (In reply to Giovanni Campagna from comment #79) > In fact, we should actually reduce the libgjs API surface to essentially > "make a JS context, run this code". > Anything else is unnecessary and should be done with gobject-introspection. Is that speculation, or if I did that would the gnome-shell developers be cheering?
(In reply to Philip Chimento from comment #83) > (In reply to Giovanni Campagna from comment #79) > > In fact, we should actually reduce the libgjs API surface to essentially > > "make a JS context, run this code". > > Anything else is unnecessary and should be done with gobject-introspection. > > Is that speculation, or if I did that would the gnome-shell developers be > cheering? (I'm not a gnome-shell dev any more, but...) gnome-shell does not seem to use any internal GJS API, it just loads the code and runs it. It does use the raw SpiderMonkey API to define the extension importer in a very hackish way. A native gjs module to define importers on arbitrary JS objects would be cleaner code, and I'm sure the g-s devs would be happy to drop the cpp code that uses libmozjs.
Created attachment 336245 [details] [review] shell-js: Remove usage of deprecated API This removes all usage of SpiderMonkey API that is deprecated in mozjs24 and will be removed in mozjs31.
The above is a patch for gnome-shell that removes all usage of the APIs that the previous patches have been dealing with in GJS. It compiles properly, but I have not been able to test it because my Fedora box is tied up with other things right now. Cosimo, Giovanni, I also tried compiling it with the bool commits, but they effectively turn the GJS headers into C++ headers. So for that, everything that includes <gjs/gjs.h> would have to be a C++ file. Or else we'd have to include <stdbool.h> in our C++ header to keep it compatible with C. I have no idea if C++ bool is guaranteed to be the same width as C _Bool, though. (In reply to Giovanni Campagna from comment #84) > A native gjs module to define importers on arbitrary JS objects would be > cleaner code, and I'm sure the g-s devs would be happy to drop the cpp code > that uses libmozjs. Do you know what necessitated the hack in the first place? In any case, I could definitely see something like GjsPrivate.define_importer().
(In reply to Philip Chimento from comment #86) > The above is a patch for gnome-shell that removes all usage of the APIs that > the previous patches have been dealing with in GJS. It compiles properly, > but I have not been able to test it because my Fedora box is tied up with > other things right now. The patch seems mechanical so it should be fine. I don't have a jhbuild testing setup either, unfortunately. > Cosimo, Giovanni, I also tried compiling it with the bool commits, but they > effectively turn the GJS headers into C++ headers. So for that, everything > that includes <gjs/gjs.h> would have to be a C++ file. Or else we'd have to > include <stdbool.h> in our C++ header to keep it compatible with C. I have > no idea if C++ bool is guaranteed to be the same width as C _Bool, though. I would expect it is, but I don't know for sure. (As an hint to that, gcc defines _Bool to bool in C++) We could static_assert(sizeof(bool) == 1), I expect it to hold on all useful architectures, even if not mandated by the C/C++ standards for the usual bureaucratic reasons. Also, I don't see the problem of including stdbool.h from gjs.h. It's a standard C header and it's supported by all interesting compilers, given that gjs on windows is not a thing yet. > (In reply to Giovanni Campagna from comment #84) > > A native gjs module to define importers on arbitrary JS objects would be > > cleaner code, and I'm sure the g-s devs would be happy to drop the cpp code > > that uses libmozjs. > > Do you know what necessitated the hack in the first place? In any case, I > could definitely see something like GjsPrivate.define_importer(). The extension system imports extensions JS files from private directory, and then gives each extension an "imports" object so they can load their own JS files without namespace conflicts with global modules.
Created attachment 336395 [details] [review] shell-js: Remove usage of deprecated API This removes all usage of SpiderMonkey API that is deprecated in mozjs24 and will be removed in mozjs31.
At Sam Spilsbury's suggestion I changed ObjectOrNull to Object in the gnome-shell patch, because it makes no sense to define an importer property on null.
Created attachment 336405 [details] [review] js: Discontinue usage of JSBool In mozjs31, JSBool and its values JS_TRUE and JS_FALSE are discontinued in favor of regular C++ bool, true, and false. In order to ease porting to mozjs31, we switch to C++ booleans everywhere. Almost everywhere, that is. In some cases bool *, or function pointers with bool return types, will not automatically be cast. We therefore leave a few instances of JSBool in the code. These will be removed when the actual port to mozjs31 happens. Fixes a few formatting glitches in lines of code that were touched anyway.
Created attachment 336406 [details] [review] js: Prefer bool to gboolean Since we removed JSBool, JS_TRUE, and JS_FALSE in the previous commit, we now also make usage of gboolean, TRUE, and FALSE consistent. In some places, JSBool and gboolean were used interchangeably. Here, we take the approach of using C++ bool, true, and false everywhere, except when required by the API, e.g. for the return type of idle functions. In a few instances, TRUE and FALSE were used in comments in JS code; here we change those to true and false to avoid confusion. Fixes a few formatting glitches in lines of code that were touched anyway.
Here are the boolean patches again with <stdbool.h> included in installed headers.
Review of attachment 336405 [details] [review]: Let's try this with the stdbool include.
Review of attachment 336406 [details] [review]: OK.
(In reply to Philip Chimento from comment #88) > Created attachment 336395 [details] [review] [review] > shell-js: Remove usage of deprecated API > > This removes all usage of SpiderMonkey API that is deprecated in mozjs24 > and will be removed in mozjs31. We should get a gnome-shell reviewer for this one... Florian?
Review of attachment 336395 [details] [review]: Sure, LGTM
Comment on attachment 336395 [details] [review] shell-js: Remove usage of deprecated API Attachment 336395 [details] pushed as f00826f - shell-js: Remove usage of deprecated API
Attachment 336405 [details] pushed as e26191a - js: Discontinue usage of JSBool Attachment 336406 [details] pushed as ea52a3d - js: Prefer bool to gboolean
OK, I'll keep going with the GC rooting. I might be able to have a first patch ready by the end of today, though it will be a more difficult review than these patches so far. What (if anything) should we do about reducing GJS's API surface, then?
Created attachment 336577 [details] [review] arg: Extract variable This is for readability and saves a few calls of val.toObject(). It will become convenient when we root the return value of toObject().
Created attachment 336578 [details] [review] js: Don't cast JS::HandleObject to void * If we want the JSObject pointer in order to print it out, we should get it with .get().
Created attachment 336579 [details] [review] js: Replace JS_Add*Root with Rooted where trivial Instead of using JS_AddFooRoot and JS_RemoveFooRoot on a value within the same scope, we should use JS::Rooted which is the mozjs31-style API. Besides being RAII and more readable, it will be required when mozjs API functions start taking JS::Handle parameters in mozjs31. This commit makes that change in places where it doesn't propagate into the API.
Created attachment 336580 [details] [review] js: Rooted objects in private access functions Starting with the functions defined in jsapi-util.h's GJS_DEFINE_PRIV_FROM_JS macro, we convert the JSObject pointers there to JS::HandleObjects, in order to be able to pass them to JS_GetInstancePrivate() in mozjs31 which will only accept HandleObjects. We work backwards from there, in a huge cascade, changing JSObject* to JS::HandleObject in all function argument types that call into those functions, and rooting the objects with JS::RootedObject where they are created. This is mostly straightforward but object_init_list in object.cpp needed some extra attention. Normally objects are rooted for the duration of a scope. In this case, we used a GSList to keep track of the objects past the end of the scope, so they could be fetched again in the GObject's construct() vfunc. With the normal rooting, that does not work because the objects can be moved or GC'd after the original scope ends. Instead we have to use JS_AddNamedObjectRoot() and JS_RemoveObjectRoot(). This can be changed to a nicer API in mozjs31, JS::PersistentRooted.
Here's four more patches. The first three are pretty simple. I'm not 100% sure about the one removing the void * casts. The last one is pretty large, and not mechanical at all, so it won't be an easy review.
I did not review patch 4, but I noticed one thing: we now potentially pass NULL to RootedObject, because a number of places call .toObjectOrNull. Is RootedObject NULL safe? Or should we check for JS null beforehand? I also see a number of places where we probably are mishandling null in the existing code, and we should check that.
It seems to be OK, here are a few examples from the Mozilla codebase: https://dxr.mozilla.org/mozilla-central/rev/9baec74b3db1bf005c66ae2f50bafbdb02c3be38/js/src/builtin/Object.cpp#34 https://dxr.mozilla.org/mozilla-central/rev/9baec74b3db1bf005c66ae2f50bafbdb02c3be38/js/src/builtin/Object.cpp#467 In fact I think when you declare JS::RootedObject obj(cx) without an initial value, you get a rooted NULL JSObject * pointer, but I can't find the link where I read that now.
*** Bug 770894 has been marked as a duplicate of this bug. ***
Review of attachment 336577 [details] [review]: Looks good
Review of attachment 336578 [details] [review]: Looks good
Review of attachment 336579 [details] [review]: Looks good to me.
Attachment 336577 [details] pushed as 951f7f7 - arg: Extract variable Attachment 336578 [details] pushed as 9fbf77b - js: Don't cast JS::HandleObject to void * Attachment 336579 [details] pushed as 9fb00a1 - js: Replace JS_Add*Root with Rooted where trivial
Created attachment 336715 [details] [review] js: Replace JS_ValueTo...() with JS::To...() Where ... is Boolean, Number, ECMAUint32 -> Uint32, ECMAInt32 -> Int32, and Int16. These can be replaced one-for-one, though the new int32 APIs do not always take the slow route of converting via a double. Unlike JS_ValueToBoolean(), JS::ToBoolean() can't fail, so we can get rid of a couple of error paths.
Created attachment 336750 [details] [review] js: Deprecate GJS rooted value arrays There is now JS API for this, JS::AutoValueVector, which achieves the same thing more safely through RAII, and in addition avoids the alignment problems that might result from casting the char * pointer inside GArray to another type. The GjsRootedArray type was not actually used anymore inside GJS, but gjs_root_value_locations() and friends still were, so these are replaced with JS::AutoValueVector.
Review of attachment 336580 [details] [review]: ::: gi/object.cpp @@ +2976,2 @@ GType gtype; + g_autofree char *signal_name = NULL; Do we have a requirement on a new enough glib to have g_autofree?
Review of attachment 336715 [details] [review]: Looks good to me.
(In reply to Cosimo Cecchi from comment #114) > Review of attachment 336580 [details] [review] [review]: > > ::: gi/object.cpp > @@ +2976,2 @@ > GType gtype; > + g_autofree char *signal_name = NULL; > > Do we have a requirement on a new enough glib to have g_autofree? Or is there any point of using g_autofree instead of std::unique_ptr<char>?
Review of attachment 336750 [details] [review]: One comment, but looks good otherwise. ::: test/gjs-tests.cpp @@ -93,3 @@ - -static void -gjstest_test_func_gjs_jsapi_util_array(void) Why are you removing this test? Usually tests for deprecated APIs are removed at the same time the API itself is, no?
Comment on attachment 336715 [details] [review] js: Replace JS_ValueTo...() with JS::To...() Attachment 336715 [details] pushed as 2377ea1 - js: Replace JS_ValueTo...() with JS::To...()
Created attachment 336850 [details] [review] js: Replace JS_ValueToInt32 and gjs_value_to_int64 Previously GJS behaved differently than the JavaScript language itself when marshalling arguments in and out of GObject introspection functions. GJS always used JS_ValueToInt32(), a deprecated function that dated from before the standardization of JavaScript, and had a hand-rolled 64-bit version to match the nonstandard behaviour, gjs_value_to_int64(). This commit replaces those with JS::ToInt32() and JS::ToInt64(), respectively. Notable differences between the old and new behaviour are: - Floating-point numbers ending in 0.5 are rounded differently when marshalled into 32 or 64-bit signed integers. Previously they were rounded to floor(x + 0.5), now they are rounded to signum(x) * floor(abs(x)) as per the ECMA standard: http://www.ecma-international.org/ecma-262/7.0/index.html#sec-toint32 Note that previously, GJS rounded according to the standard when converting to *unsigned* integers! - Objects whose number value is NaN (e.g, arrays of strings), would previously fail to convert, throwing a TypeError. Now they convert to 0 when marshalled into 32 or 64-bit signed integers. Note that the new behaviour is the behaviour you got all along when using pure JavaScript, without any GObject introspection: gjs> let a = new Int32Array(2); gjs> a[0] = 10.5; 10.5 gjs> a[1] = ['this', 'is', 'fine']; this,is,fine gjs> a[0] 10 gjs> a[1] 0 Finally, this comments out a test altogether which relied on the fail-to-convert behaviour. There is not really any way to cause that behaviour on demand anymore, looking at http://www.ecma-international.org/ecma-262/7.0/index.html#sec-type-conversion until we upgrade to mozjs38, where we can make a Symbol fail to convert.
Created attachment 336851 [details] [review] js: Rooted objects in private access functions Starting with the functions defined in jsapi-util.h's GJS_DEFINE_PRIV_FROM_JS macro, we convert the JSObject pointers there to JS::HandleObjects, in order to be able to pass them to JS_GetInstancePrivate() in mozjs31 which will only accept HandleObjects. We work backwards from there, in a huge cascade, changing JSObject* to JS::HandleObject in all function argument types that call into those functions, and rooting the objects with JS::RootedObject where they are created. This is mostly straightforward but object_init_list in object.cpp needed some extra attention. Normally objects are rooted for the duration of a scope. In this case, we used a GSList to keep track of the objects past the end of the scope, so they could be fetched again in the GObject's construct() vfunc. With the normal rooting, that does not work because the objects can be moved or GC'd after the original scope ends. Instead we have to use JS_AddNamedObjectRoot() and JS_RemoveObjectRoot(). This can be changed to a nicer API in mozjs31, JS::PersistentRooted.
(In reply to Giovanni Campagna from comment #116) > (In reply to Cosimo Cecchi from comment #114) > > Review of attachment 336580 [details] [review] [review] [review]: > > > > ::: gi/object.cpp > > @@ +2976,2 @@ > > GType gtype; > > + g_autofree char *signal_name = NULL; > > > > Do we have a requirement on a new enough glib to have g_autofree? > > Or is there any point of using g_autofree instead of std::unique_ptr<char>? It's a bit awkward to use std::unique_ptr in this case because you can't pass a unique_ptr's address into another function to be filled, like a C-style out argument; but I prefer the idea of using an actual C++ language feature since this is C++ after all. Patch updated. (In reply to Cosimo Cecchi from comment #117) > Review of attachment 336750 [details] [review] [review]: > > One comment, but looks good otherwise. > > ::: test/gjs-tests.cpp > @@ -93,3 @@ > - > -static void > -gjstest_test_func_gjs_jsapi_util_array(void) > > Why are you removing this test? Usually tests for deprecated APIs are > removed at the same time the API itself is, no? I considered the test potentially harmful, because these APIs could cause alignment violations and should not be used at all. And indeed they were not even used anywhere.
(In reply to Philip Chimento from comment #121) > (In reply to Giovanni Campagna from comment #116) > > It's a bit awkward to use std::unique_ptr in this case because you can't > pass a unique_ptr's address into another function to be filled, like a > C-style out argument; but I prefer the idea of using an actual C++ language > feature since this is C++ after all. Patch updated. I think you can, if you declare the function to take std::unique_ptr<>&, and then std::move whatever you have into the out argument. But I have not tried it.
(In reply to Giovanni Campagna from comment #122) > (In reply to Philip Chimento from comment #121) > > (In reply to Giovanni Campagna from comment #116) > > > > It's a bit awkward to use std::unique_ptr in this case because you can't > > pass a unique_ptr's address into another function to be filled, like a > > C-style out argument; but I prefer the idea of using an actual C++ language > > feature since this is C++ after all. Patch updated. > > I think you can, if you declare the function to take std::unique_ptr<>&, and > then std::move whatever you have into the out argument. But I have not tried > it. That sounds correct. But, the function is public API, see also the bug I just opened...
Created attachment 336852 [details] [review] util: Root gjs_build_string_array() This was an easy target and also gets rid of more alignment problems with GArray. JS_NewArrayObject will take a JS::HandleValueArray parameter in mozjs31.
Review of attachment 336850 [details] [review]: Makes sense and looks good. Thanks for the great commit message!
Review of attachment 336851 [details] [review]: Looks good now.
Review of attachment 336852 [details] [review]: Looks good.
Review of attachment 336750 [details] [review]: OK
Attachment 336750 [details] pushed as bceeb34 - js: Deprecate GJS rooted value arrays Attachment 336850 [details] pushed as 26f6101 - js: Replace JS_ValueToInt32 and gjs_value_to_int64 Attachment 336851 [details] pushed as 2af44fa - js: Rooted objects in private access functions Attachment 336852 [details] pushed as 4eb3164 - util: Root gjs_build_string_array()
Created attachment 336960 [details] [review] byte array: Root gjs_value_to_{gsize,byte}() More easy targets for converting to exact GC rooting.
Review of attachment 336960 [details] [review]: Looks good to me.
Comment on attachment 336960 [details] [review] byte array: Root gjs_value_to_{gsize,byte}() Attachment 336960 [details] pushed as 98a44d3 - byte array: Root gjs_value_to_{gsize,byte}()
Created attachment 337036 [details] [review] js: Use JSCLASS_NO_OPTIONAL_MEMBERS In mozjs31, some members of JSClass really become optional. Until then, use this macro; it's easily deletable when we switch, clearer to read, and safer because if an extra value gets in the initializer it will cause a compile error.
Created attachment 337128 [details] [review] js: Use JS_FS and JS_PS macros The layout of the JSFunctionSpec and JSPropertySpec changes in between SpiderMonkey releases, so using these macros makes our code easier to port to subsequent releases. It's also safer since we don't have to cast all the function pointers to JSNative, though sadly that means we do need to reintroduce JSBool in many places until we switch to mozjs31. We don't convert JSFunctionSpecs yet because there are no JS_FS macros for JSPropertyOp-style entries. These are removed, because broken, in a subsequent SpiderMonkey release, though I'm not sure if it's 31: https://bugzilla.mozilla.org/show_bug.cgi?id=992977 So, in a subsequent commit we will switch to JSNative property accessors. Removing the casts has exposed a bug in imports._gi.add_interface() which probably crashes if you try to call it from JS. This bug is not fixed with this commit.
Created attachment 337129 [details] [review] js: Add macros for 'this' and private data This operation is done over and over again inside most JSNative functions, and SpiderMonkey code often uses convenience macros like this. These will come in handy even more when we switch to JSNative property accessors in a following commit. In doing so we get rid of the args.thisv().toObjectOrNull() idiom which I'm pretty sure is wrong, except in function_call() which is fine to have anything as the 'this' object.
Review of attachment 337036 [details] [review]: Looks good
Review of attachment 337128 [details] [review]: Just one comment, but feel free to push if that's intended. ::: gi/object.cpp @@ +3054,3 @@ + JS_FS("register_type", gjs_register_type, 4, GJS_MODULE_PROP_FLAGS), + // FIXME this function will be totally broken if you try to use it from JS + JS_FS("add_interface", (JSNative)gjs_add_interface, 2, GJS_MODULE_PROP_FLAGS), Is the fact that you still need to cast the function to JSNative connected to the FIXME? It looks like this is the only function left that needs a cast.
(In reply to Philip Chimento from comment #135) > Created attachment 337129 [details] [review] [review] > js: Add macros for 'this' and private data > > This operation is done over and over again inside most JSNative > functions, and SpiderMonkey code often uses convenience macros like this. > These will come in handy even more when we switch to JSNative property > accessors in a following commit. > > In doing so we get rid of the args.thisv().toObjectOrNull() idiom which > I'm pretty sure is wrong, except in function_call() which is fine to have > anything as the 'this' object. It's not a new bug, but pretty much all GJS_GET_PRIV calls should be GET_PRIV_WITH_TYPECHECK_AND_THROW, otherwise you can get gjs to mis-cast the private structure if you pass an object of a different type and crash. The exceptions to the rule are: 1) JSClass function pointers (you need to insist a lot with the JS engine to get it to call a function on a different JSClass object, so not throwing the error is OK) 2) places where it's legal to have multiple types (ie, the current usages of priv_from_js_with_typecheck) 3) static helpers where we can ensure typechecking already happened (The current code plays fast and loose with this rule because most functions are GI functions, that have their own checking in function.cpp and arg.cpp. But JSNatives need the typecheck)
Review of attachment 337129 [details] [review]: ::: gi/function.cpp @@ +1299,3 @@ JS::CallArgs js_argv = JS::CallArgsFromVp (js_argc, vp); + /* Don't want to typecheck js_argv.thisv() here, since it's legal for it to + * be anything */ Don't you want to check thisv().isObjectOrNull()? Also, do we have any GI function that is ok with null as the instance argument? For comparison, most DOM and JS builtin functions do the equivalent of ::ToObject(), which means they accept strings and numbers (converted to the appropriate object) and reject undefined or null.
Review of attachment 337129 [details] [review]: Looks good. Feel free to push if the answer to below is "no". ::: modules/cairo-context.cpp @@ -53,3 @@ #define _GJS_CAIRO_CONTEXT_DEFINE_FUNC0(method, cfunc) \ _GJS_CAIRO_CONTEXT_DEFINE_FUNC_BEGIN(method) \ - cr = gjs_cairo_context_get_context(context, obj); \ Can't you remove this function altogether now?
(In reply to Cosimo Cecchi from comment #137) > Review of attachment 337128 [details] [review] [review]: > > Just one comment, but feel free to push if that's intended. > > ::: gi/object.cpp > @@ +3054,3 @@ > + JS_FS("register_type", gjs_register_type, 4, GJS_MODULE_PROP_FLAGS), > + // FIXME this function will be totally broken if you try to use it from > JS > + JS_FS("add_interface", (JSNative)gjs_add_interface, 2, > GJS_MODULE_PROP_FLAGS), > > Is the fact that you still need to cast the function to JSNative connected > to the FIXME? It looks like this is the only function left that needs a cast. Yes. The function needs a cast because it has completely the wrong number and type of parameters. This was already the case but was hidden because of the cast. My guess is someone did some refactoring but didn't realize this function was a JSNative implementation, and due to the casts being used everywhere the compiler didn't notice either. I still need to do some git log digging to find out where and why this broke. In any case, I will push this patch and the other one that had no comments.
(In reply to Giovanni Campagna from comment #138) > It's not a new bug, but pretty much all GJS_GET_PRIV calls should be > GET_PRIV_WITH_TYPECHECK_AND_THROW, otherwise you can get gjs to mis-cast the > private structure if you pass an object of a different type and crash. Do you think I should make that change as part of this patch? (In reply to Giovanni Campagna from comment #139) > Review of attachment 337129 [details] [review] [review]: > > ::: gi/function.cpp > @@ +1299,3 @@ > JS::CallArgs js_argv = JS::CallArgsFromVp (js_argc, vp); > + /* Don't want to typecheck js_argv.thisv() here, since it's legal for > it to > + * be anything */ > > Don't you want to check thisv().isObjectOrNull()? From what I could tell, it needs to be able to be an integer as well, so that you can do new SomeErrorDomain({message: blah}). Although I admit I didn't spend any time digging into why that works with toObject()! > Also, do we have any GI function that is ok with null as the instance > argument? Not that I know of. > For comparison, most DOM and JS builtin functions do the equivalent of > ::ToObject(), which means they accept strings and numbers (converted to the > appropriate object) and reject undefined or null. It sounds reasonable to use JS::ToObject() in the macros then, instead of isObject()/toObject(). I saw a lot of Mozilla code uses JS_ComputeThis() but that's not public API. (In reply to Cosimo Cecchi from comment #140) > Review of attachment 337129 [details] [review] [review]: > > ::: modules/cairo-context.cpp > @@ -53,3 @@ > #define _GJS_CAIRO_CONTEXT_DEFINE_FUNC0(method, cfunc) \ > _GJS_CAIRO_CONTEXT_DEFINE_FUNC_BEGIN(method) \ > - cr = gjs_cairo_context_get_context(context, obj); \ > > Can't you remove this function altogether now? It's still used in context_to_g_argument().
(In reply to Philip Chimento from comment #142) > (In reply to Giovanni Campagna from comment #138) > > It's not a new bug, but pretty much all GJS_GET_PRIV calls should be > > GET_PRIV_WITH_TYPECHECK_AND_THROW, otherwise you can get gjs to mis-cast the > > private structure if you pass an object of a different type and crash. > > Do you think I should make that change as part of this patch? Not sure, but I think we should make this change in the patch or immediately after. Doing both at the same time seems to make sense and reduces churn. > (In reply to Giovanni Campagna from comment #139) > > Review of attachment 337129 [details] [review] [review] [review]: > > > > ::: gi/function.cpp > > @@ +1299,3 @@ > > JS::CallArgs js_argv = JS::CallArgsFromVp (js_argc, vp); > > + /* Don't want to typecheck js_argv.thisv() here, since it's legal for > > it to > > + * be anything */ > > > > Don't you want to check thisv().isObjectOrNull()? > > From what I could tell, it needs to be able to be an integer as well, so > that you can do new SomeErrorDomain({message: blah}). Although I admit I > didn't spend any time digging into why that works with toObject()! Mh? "new SomeErrorDomain" goes into the GError constructor in gi/gerror.cpp, not in function_call. function_call is only for gi functions. And I would expect .toObject() to return garbage if not isObject(), remembering the jsval layout on old mozjs versions.
(In reply to Philip Chimento from comment #142) > (In reply to Giovanni Campagna from comment #139) > > For comparison, most DOM and JS builtin functions do the equivalent of > > ::ToObject(), which means they accept strings and numbers (converted to the > > appropriate object) and reject undefined or null. > > It sounds reasonable to use JS::ToObject() in the macros then, instead of > isObject()/toObject(). I saw a lot of Mozilla code uses JS_ComputeThis() but > that's not public API. I got this wrong; JS_ComputeThis() is public API but will be removed. Instead we have JS::CallArgs::computeThis(), so I went ahead and used that. (In reply to Giovanni Campagna from comment #143) > (In reply to Philip Chimento from comment #142) > > (In reply to Giovanni Campagna from comment #139) > > > Review of attachment 337129 [details] [review] [review] [review] [review]: > > > > > > ::: gi/function.cpp > > > @@ +1299,3 @@ > > > JS::CallArgs js_argv = JS::CallArgsFromVp (js_argc, vp); > > > + /* Don't want to typecheck js_argv.thisv() here, since it's legal for > > > it to > > > + * be anything */ > > > > > > Don't you want to check thisv().isObjectOrNull()? > > > > From what I could tell, it needs to be able to be an integer as well, so > > that you can do new SomeErrorDomain({message: blah}). Although I admit I > > didn't spend any time digging into why that works with toObject()! > > Mh? "new SomeErrorDomain" goes into the GError constructor in gi/gerror.cpp, > not in function_call. function_call is only for gi functions. > And I would expect .toObject() to return garbage if not isObject(), > remembering the jsval layout on old mozjs versions. I got this wrong as well. Not sure why it was failing before, but when I tried it again with the new code using JS::CallArgs::computeThis(), no problem.
Created attachment 337547 [details] [review] js: Add macros for 'this' and private data This adds two macros, GJS_GET_THIS() to get a function's 'this' value as a rooted object (and if it was not an object, to box it into one), and GJS_GET_PRIV() to get our private C data from the 'this' object. This operation is done over and over again inside most JSNative functions, and SpiderMonkey code often uses convenience macros like this. These will come in handy even more when we switch to JSNative property accessors in a following commit. At the same time we make things more typesafe by doing a typecheck on each 'this' object in the native methods as part of GJS_GET_PRIV() and throwing an error if it is not the correct type. This gets rid of the args.thisv().toObjectOrNull() idiom which I'm pretty sure is wrong.
Created attachment 337548 [details] [review] js: Switch to JSNative property accessors Per https://bugzilla.mozilla.org/show_bug.cgi?id=992977, JSPropertyOp-style property accessors are going to be removed in a future version of SpiderMonkey. Instead, we use JSNative property accessors. This allows us to use the JS_PSG and JS_PSGS macros in our JSPropertySpec arrays, making things safer by eliminating the callback casts.
Created attachment 337680 [details] [review] doc: Refer to JS::PersistentRooted Instead of explaining JS_Add*Root and JS_Remove*Root, talk about JS::PersistentRooted<T> instead. This would be the post-SpiderMonkey 31 solution, and JS_Add*Root should only be used sparingly.
Created attachment 337681 [details] [review] js: Root gjs_string_from_utf8() Starting with gjs_string_from_utf8(), we convert the JS::Value out-arg to JS::MutableHandleValue. Working backwards from there in a huge cascade, we change JS::Value * to JS::MutableHandleValue in many functions that call gjs_string_from_utf8(), and root the values with JS::RootedValue where they are created. This is mostly straightforward, though requires replacing a few instances of a stack-allocated JS::Value array created with g_newa(), with JS::AutoValueVector instead. In the course of moving to more RAII classes such as JS::Rooted<T>, we can get rid of some more gotos in favour of returns.
Created attachment 337682 [details] [review] js: Remove most of JS_Add*Root and JS_Remove*Root For rooting regular stack-allocated GC things, we should use JS::Rooted<T> instead of JS_Add*Root and JS_Remove*Root. This requires changing a few more function signatures to take JS::MutableHandleValue. In a few cases, it is possible to encapsulate argc, argv, rval in a JS::CallArgs& in cases where we know the function's caller is going to have a JS::CallArgs anyway. A few uses of JS_Add*Root and JS_Remove*Root remain, but those will be removed in mozjs31 in favour of JS::PersistentRooted.
Created attachment 337889 [details] [review] jsapi-util: Rooting-safe gjs_parse_call_args() This removes the old unused gjs_parse_args(), and moves gjs_parse_call_args() into a new header file jsapi-util-args.h. In order to ensure that JSObjects unpacked from gjs_parse_call_args() land in GC roots, we need to add some type safety to its variable arguments. Instead of accepting a JSObject** pointer for the "o" format character, it must accept a JS::MutableHandleObject. We can do this (and make the whole thing type safe as well) by using C++ templates instead of C varargs. Now we can issue a compile-time error when an unknown type is passed in as a return location for an argument, in particular JSObject**. This also fixes some undefined behaviour: the old signature of gjs_parse_call_args() placed the varargs parameter right after the JS::CallArgs& parameter, and using a reference parameter in va_start() is undefined. Here's a good explanation of what can go wrong: http://stackoverflow.com/a/222288/172999
Created attachment 337890 [details] [review] tests: Consolidate fixture code gjs-tests.cpp used a similar test fixture to gjs-test-call-args.cpp, so we consolidate the two into gjs-test-utils.h.
Created attachment 337891 [details] [review] test: Fix use after free Noticed this while writing the previous commit, so cleaned it up.
Created attachment 337964 [details] [review] test: Fix use after free Noticed this while working on the unit test fixtures, so cleaned it up.
Created attachment 337965 [details] [review] jsapi-util: Rooting-safe gjs_parse_call_args() This removes the old unused gjs_parse_args(), and moves gjs_parse_call_args() into new files jsapi-util-args.cpp and jsapi-util-args.h. In order to ensure that JSObjects unpacked from gjs_parse_call_args() land in GC roots, we need to add some type safety to its variable arguments. Instead of accepting a JSObject** pointer for the "o" format character, it must accept a JS::MutableHandleObject. We can do this (and make the whole thing type safe as well) by using C++ templates instead of C varargs. Now we can issue a compile-time error when an unknown type is passed in as a return location for an argument, in particular JSObject**. This also fixes some undefined behaviour: the old signature of gjs_parse_call_args() placed the varargs parameter right after the JS::CallArgs& parameter, and using a reference parameter in va_start() is undefined. Here's a good explanation of what can go wrong: http://stackoverflow.com/a/222288/172999
Created attachment 337966 [details] [review] tests: Consolidate fixture code gjs-tests.cpp used a similar test fixture to gjs-test-call-args.cpp, so we consolidate the two into gjs-test-utils.h.
Created attachment 337991 [details] [review] WIP - js: Root gjs_object_get_property_const() Starting with gjs_object_get_property_const() and working backwards, we cause another cascade of GC rooting changes. One disadvantage of JS::MutableHandle is that it's not so easy to make an optional out parameter for a function. Previously, for example, a JSObject** parameter could be NULL to indicate the caller wasn't interested in the return value. e.g., if (out_obj != NULL) { do_some_op(); *out_obj = something; } Now we have a few options. SpiderMonkey doesn't really have any examples of this in the JSAPI so it's hard to tell what is most idiomatic and from asking on IRC it seems that they are pretty equivalent: - Use JS::MutableHandle* - should be OK but not really done in SM - Use mozilla::Maybe<JS::MutableHandle> https://dxr.mozilla.org/mozilla-beta/source/mfbt/Maybe.h - Use overloading; define a function prototype with and without the parameter - Pass in an explicitly ignored object, which is what I've done in this patch (grep for "ignored".) Ignored object seems to be the least invasive route but before committing this patch we should pick one of the above.
Review of attachment 337547 [details] [review]: Looks good to me.
Review of attachment 337548 [details] [review]: Looks good to me.
Review of attachment 337680 [details] [review]: Sure
Review of attachment 337681 [details] [review]: Looks good -- some comments below that are mostly cosmetic. ::: gi/arg.cpp @@ +2461,2 @@ case GI_TYPE_TAG_UINT64: + value_p.set(JS::NumberValue(arg->v_uint64)); Why not setNumber? ::: gi/boxed.cpp @@ +898,3 @@ if (!_gjs_proxy_to_string_func(context, obj, "boxed", (GIBaseInfo*)priv->info, + priv->gtype, priv->gboxed, rec.rval())) + return false; Return _gjs_proxy_to_string_func() directly? ::: gi/ns.cpp @@ +139,1 @@ + if (!gjs_string_from_utf8(context, priv->gi_namespace, -1, args.rval())) return gjs_string_from_utf8()? ::: gi/union.cpp @@ +291,3 @@ if (!_gjs_proxy_to_string_func(context, obj, "union", (GIBaseInfo*)priv->info, + priv->gtype, priv->gboxed, rec.rval())) + return false; Can you return _gjs_proxy_to_string_func() directly? ::: gi/value.cpp @@ +815,3 @@ int v; v = g_value_get_int(gvalue); + value_p.set(JS::NumberValue(v)); You can't use setNumber() here? @@ +989,3 @@ g_value_transform(gvalue, &int_value); v = g_value_get_int(&int_value); + value_p.set(JS::NumberValue(v)); Ditto.
Review of attachment 337682 [details] [review]: Looks good to me.
Review of attachment 337964 [details] [review]: Sure
Review of attachment 337965 [details] [review]: Looks mostly good to me. I did not look at all the tests in detail, but I am glad that we are now testing that behavior much more thoroughly :) ::: gjs/jsapi-util-args.cpp @@ +31,3 @@ + +/* This preserves the previous behaviour of gjs_parse_args(), but maybe we want + * to use JS::ToBoolean instead? */ I would not mind... What is the difference in behavior? @@ +118,3 @@ + if (nullable) + throw g_strdup("Invalid format string combination ?u"); + if (!value.isNumber() || !JS::ToNumber(cx, value, &num)) Why not use JS::ToUint32() here? ::: gjs/jsapi-util-args.h @@ +38,3 @@ +void assign(JSContext *, const char, bool, JS::HandleValue, uint32_t *); +void assign(JSContext *, const char, bool, JS::HandleValue, int64_t *); +void assign(JSContext *, const char, bool, JS::HandleValue, double *); Extra whitespace
Review of attachment 337966 [details] [review]: Nice cleanup!
Comment on attachment 337964 [details] [review] test: Fix use after free Attachment 337964 [details] pushed as caf4d4b - test: Fix use after free
Created attachment 338062 [details] [review] js: Root gjs_string_from_utf8() Starting with gjs_string_from_utf8(), we convert the JS::Value out-arg to JS::MutableHandleValue. Working backwards from there in a huge cascade, we change JS::Value * to JS::MutableHandleValue in many functions that call gjs_string_from_utf8(), and root the values with JS::RootedValue where they are created. This is mostly straightforward, though requires replacing a few instances of a stack-allocated JS::Value array created with g_newa(), with JS::AutoValueVector instead. In the course of moving to more RAII classes such as JS::Rooted<T>, we can get rid of some more gotos in favour of returns.
Created attachment 338063 [details] [review] jsapi-util: Rooting-safe gjs_parse_call_args() This removes the old unused gjs_parse_args(), and moves gjs_parse_call_args() into new files jsapi-util-args.cpp and jsapi-util-args.h. In order to ensure that JSObjects unpacked from gjs_parse_call_args() land in GC roots, we need to add some type safety to its variable arguments. Instead of accepting a JSObject** pointer for the "o" format character, it must accept a JS::MutableHandleObject. We can do this (and make the whole thing type safe as well) by using C++ templates instead of C varargs. Now we can issue a compile-time error when an unknown type is passed in as a return location for an argument, in particular JSObject**. This also fixes some undefined behaviour: the old signature of gjs_parse_call_args() placed the varargs parameter right after the JS::CallArgs& parameter, and using a reference parameter in va_start() is undefined. Here's a good explanation of what can go wrong: http://stackoverflow.com/a/222288/172999
(In reply to Cosimo Cecchi from comment #160) > Review of attachment 337681 [details] [review] [review]: > > Looks good -- some comments below that are mostly cosmetic. > > ::: gi/arg.cpp > @@ +2461,2 @@ > case GI_TYPE_TAG_UINT64: > + value_p.set(JS::NumberValue(arg->v_uint64)); > > Why not setNumber? setNumber doesn't have an overload for 64-bit ints, so the compiler doesn't know whether to implicitly cast it to a uint32_t or double. > ::: gi/value.cpp > @@ +815,3 @@ > int v; > v = g_value_get_int(gvalue); > + value_p.set(JS::NumberValue(v)); > > You can't use setNumber() here? > > @@ +989,3 @@ > g_value_transform(gvalue, &int_value); > v = g_value_get_int(&int_value); > + value_p.set(JS::NumberValue(v)); > > Ditto. Same story for int here. (In reply to Cosimo Cecchi from comment #163) > Review of attachment 337965 [details] [review] [review]: > > Looks mostly good to me. I did not look at all the tests in detail, but I am > glad that we are now testing that behavior much more thoroughly :) > > ::: gjs/jsapi-util-args.cpp > @@ +31,3 @@ > + > +/* This preserves the previous behaviour of gjs_parse_args(), but maybe we > want > + * to use JS::ToBoolean instead? */ > > I would not mind... What is the difference in behavior? JS::ToBoolean is defined in the ES standard [1], basically it converts 3 and "foo" to a true boolean value. The current code will throw an exception if passed anything that's not a boolean (isBoolean() makes sure of that.) We could also do something similar in the equivalent of this code for object types, using JS_ValueToObject. > @@ +118,3 @@ > + if (nullable) > + throw g_strdup("Invalid format string combination ?u"); > + if (!value.isNumber() || !JS::ToNumber(cx, value, &num)) > > Why not use JS::ToUint32() here? Basically, because the previous code didn't either :-) The range check is nice though. JS::ToUint32 will give you the value modulo 2^32. Fixed the return false/return true stuff, and the whitespace glitch. Here are new versions of the above two patches. I couldn't commit the other two accepted patches yet because they don't apply without these. [1] http://www.ecma-international.org/ecma-262/6.0/#sec-toboolean
Created attachment 338066 [details] [review] jsapi-util: Rooting-safe gjs_parse_call_args() This removes the old unused gjs_parse_args(), and moves gjs_parse_call_args() into new files jsapi-util-args.cpp and jsapi-util-args.h. In order to ensure that JSObjects unpacked from gjs_parse_call_args() land in GC roots, we need to add some type safety to its variable arguments. Instead of accepting a JSObject** pointer for the "o" format character, it must accept a JS::MutableHandleObject. We can do this (and make the whole thing type safe as well) by using C++ templates instead of C varargs. Now we can issue a compile-time error when an unknown type is passed in as a return location for an argument, in particular JSObject**. This also fixes some undefined behaviour: the old signature of gjs_parse_call_args() placed the varargs parameter right after the JS::CallArgs& parameter, and using a reference parameter in va_start() is undefined. Here's a good explanation of what can go wrong: http://stackoverflow.com/a/222288/172999
I reworked the gjs_parse_call_args() patch to put everything in the header file after all, as inline functions. Online opinions seem to be split on whether this is better or not, but I was running into a lot of trouble getting the OSX linker to include the symbols from jsapi-util-args.o in libgjs.so.
Review of attachment 338062 [details] [review]: Feel free to commit with this fixed. ::: gi/object.cpp @@ +1780,3 @@ } if (!_gjs_proxy_to_string_func(context, obj, "object", (GIBaseInfo*)priv->info, return _gjs_proxy_to_string_func()
Created attachment 338129 [details] [review] tests: Consolidate fixture code gjs-tests.cpp used a similar test fixture to gjs-test-call-args.cpp, so we consolidate the two into gjs-test-utils.cpp.
I reworked the "Consolidate fixture code" patch to add gjs-test-utils.cpp because I want to put some more functions in there later.
Review of attachment 338066 [details] [review]: Looks good!
Review of attachment 338129 [details] [review]: Looks good.
Comment on attachment 338062 [details] [review] js: Root gjs_string_from_utf8() Attachment 338062 [details] pushed as c2b5c3d - js: Root gjs_string_from_utf8()
Comment on attachment 337682 [details] [review] js: Remove most of JS_Add*Root and JS_Remove*Root Attachment 337682 [details] pushed as 16c6e14 - js: Remove most of JS_Add*Root and JS_Remove*Root
Attachment 338066 [details] pushed as 11d96aa - jsapi-util: Rooting-safe gjs_parse_call_args() Attachment 338129 [details] pushed as 469c722 - tests: Consolidate fixture code
Created attachment 338135 [details] [review] js: Root gjs_object_get_property_const() Starting with gjs_object_get_property_const() and working backwards, we cause another cascade of GC rooting changes. One disadvantage of JS::MutableHandle is that it's not so easy to make an optional out parameter for a function. Previously, for example, a JSObject** parameter could be NULL to indicate the caller wasn't interested in the return value. e.g., if (out_obj != NULL) { do_some_op(); *out_obj = something; } Now we have a few options. For those optional parameters that eventually get passed as the "constructor" out parameter of gjs_init_class_dynamic(), the constructor is going to need to be created anyhow. So there it's best to pass an ignored handle in. In gjs_context_get_frame_info(), where we can avoid doing some work by indicating that we don't need the particular out parameters, we use mozilla::Maybe. This has an unwieldy API in mozjs24 but is improved to be much more convenient in later releases. (If this were C++17, we could use std::optional instead.)
Created attachment 338136 [details] [review] context: Store global object as JS::Heap "GC things" that are part of structures stored on the heap can't be rooted with JS::Rooted, as those objects must be rooted and unrooted in LIFO order for performance reasons. In this case, we need to root the global object with JS_AddRoot and JS_RemoveRoot. It also must be stored inside a JS::Heap because it is a member of a struct that is allocated on the heap. The JS::Heap keeps track of when the garbage collector moves the object, but it does not actually root the object. See this page for more information: http://trac.wildfiregames.com/wiki/JSRootingGuide
Created attachment 338137 [details] [review] js: Root importer.cpp Starting out with define_meta_properties() in importer.cpp, we do another cascade of GC rooting changes.
Created attachment 338138 [details] [review] js: Root importer define_meta_properties() Starting out with define_meta_properties() in importer.cpp, we do another cascade of GC rooting changes.
Created attachment 338139 [details] [review] importer: Root misc functions This uses rooting for everything else in importer.cpp that would cause a compile error in mozjs31.
Created attachment 338140 [details] [review] js: Root importer define_meta_properties() Starting out with define_meta_properties() in importer.cpp, we do another cascade of GC rooting changes.
Review of attachment 338135 [details] [review]: Looks good.
Review of attachment 338136 [details] [review]: Looks good.
Review of attachment 338139 [details] [review]: Looks good.
Review of attachment 338140 [details] [review]: Looks good.
Created attachment 338238 [details] [review] js: Root create_proto functions This changes the gjs_whatever_create_proto() functions generated by the GJS_DEFINE_PROTO macros in jsapi-util.h to be correctly rooted, letting changes to those function prototypes cascade everywhere else.
Attachment 338135 [details] pushed as 5826de6 - js: Root gjs_object_get_property_const() Attachment 338136 [details] pushed as 099fabc - context: Store global object as JS::Heap Attachment 338139 [details] pushed as f3dd1d3 - importer: Root misc functions Attachment 338140 [details] pushed as 95cdfc2 - js: Root importer define_meta_properties()
Created attachment 338240 [details] [review] js: Root create_proto functions This changes the gjs_whatever_create_proto() functions generated by the GJS_DEFINE_PROTO macros in jsapi-util.h to be correctly rooted, letting changes to those function prototypes cascade everywhere else.
Review of attachment 338240 [details] [review]: Looks good to me.
Comment on attachment 338240 [details] [review] js: Root create_proto functions Attachment 338240 [details] pushed as c78a706 - js: Root create_proto functions
Created attachment 338374 [details] [review] js: Root gjs_object_require_property() Starting out with gjs_object_require_property(), we do another cascade of GC rooting changes. This requires making some changes to the private structs of Boxed and Fundamental. Since they contain members of type jsid, which are subject to garbage collection, those members must be traced. JSClass provides a trace callback for this purpose.
Created attachment 338461 [details] [review] js: Add convenience gjs_object_require_property_value This adds several functions in an overloaded gjs_object_require_property_value() family, as well as a gjs_object_require_converted_property_value() that can be overloaded when the opportunity arises. One operation that is done many times across the codebase is to call gjs_object_require_property() and then typecheck the JS::Value that it returns, throwing a custom exception if the property was the wrong type. We introduce gjs_object_require_property_value() which takes a pointer to whatever type we expect in the JS::Value, and takes care of all the rooting and exception throwing. Basically it calls the equivalent of isWhateverType() and toWhateverType() while skipping JSString * in favour of returning a UTF-8 encoded string. For not only type-checking but also converting a JS::Value of the wrong type to the requested type, there is gjs_object_require_converted_property_value(). Currently in the codebase this operation is only done on one type, uint32, but it can be overloaded with more types later. This reduces a lot of code duplication that would otherwise all have to be converted to rooting by hand in the next commit.
Review of attachment 338374 [details] [review]: Looks good to me.
Review of attachment 338461 [details] [review]: Feel free to push with the below fixed. ::: gi/repo.cpp @@ +638,1 @@ JS::RootedValue ns_obj(context); This looks unused now.
Attachment 338374 [details] pushed as 04de94e - js: Root gjs_object_require_property() Attachment 338461 [details] pushed as 1a5cd77 - js: Add convenience gjs_object_require_property_value
Created attachment 338465 [details] [review] coverage: Root misc functions This uses rooting for everything else in coverage.cpp that would cause a compile error in mozjs31. GjsCoveragePrivate contains a JSObject allocated on the heap, which we must wrap in a JS::Heap<JSObject *> and trace, but this is fairly simple as there already was a tracer present.
Review of attachment 338465 [details] [review]: Looks good to me.
Comment on attachment 338465 [details] [review] coverage: Root misc functions Attachment 338465 [details] pushed as e1593a8 - coverage: Root misc functions
Created attachment 338470 [details] [review] js: Remove the concept of a "context global" The idea of a global object associated with a JSContext used to be a thing, but was already deprecated in mozjs24. We continued to use it by peeking into a private API, but that is going away in mozjs31 as well. The object that we were really trying to get, when calling that private API, was actually the same object as returned by gjs_get_import_global(): the object created in gjs_init_context_standard() as the GjsContext's global object. Therefore we can simply remove gjs_get_global_object(), the wrapper for the aforementioned private API, and call gjs_get_import_global() instead. The one problem with this was that the object was not referenced by the GjsContext anymore by the time the GjsContext's dispose function ran, and it was needed in order to dispose other stuff, so this would crash. It previously worked because the object still existed as the context global, you just couldn't reach it from the GjsContext. Now when we tried to reach it at dispose time, we just got a null pointer. This was due to my misunderstanding, in a previous commit, how a JS::Heap was supposed to work. Now instead of rooting the global object in GjsContext, we trace it, which allows it to remain alive until GjsContext is completely gone and then eventually be garbage-collected by JS.
Created attachment 338471 [details] [review] jsapi-util: Use CallArgs in gjs_throw_abstract_constructor_error() JS_CALLEE(), with which you could figure out the function being called from a vp array, is gone in mozjs31. Instead, use JS::CallArgs::callee(), meaning that we have to pass JS::CallArgs into gjs_throw_abstract_constructor_error() instead of the vp array.
Review of attachment 338470 [details] [review]: Nice, thanks for the great commit message.
Review of attachment 338471 [details] [review]: Looks good.
Attachment 338470 [details] pushed as a92321f - js: Remove the concept of a "context global" Attachment 338471 [details] pushed as 4c7810c - jsapi-util: Use CallArgs in gjs_throw_abstract_constructor_error()
Created attachment 338554 [details] [review] jsapi-util: Remove gjs_move_exception() This was only used in one place in function.cpp, where the source context and destination context were the same, making the gjs_move_exception() effectively a no-op. It seems this was left over from when imports were done in a different context.
Created attachment 338555 [details] [review] jsapi-util: Root several utility functions This adds GC rooting to the signatures of: - gjs_define_string_array() - gjs_log_exception_full() - gjs_value_debug_string() - gjs_call_function_value() - gjs_eval_with_scope() All fairly simple cases that do not really cascade into other code.
Created attachment 338556 [details] [review] jsapi-util: Root misc functions This uses rooting for everything else in jsapi-util.cpp that would cause a compile error in mozjs31.
Review of attachment 338554 [details] [review]: Nice.
Review of attachment 338555 [details] [review]: Looks good to me.
Review of attachment 338556 [details] [review]: OK
Attachment 338554 [details] pushed as 033effc - jsapi-util: Remove gjs_move_exception() Attachment 338555 [details] pushed as 6788c72 - jsapi-util: Root several utility functions Attachment 338556 [details] pushed as 6cbab89 - jsapi-util: Root misc functions
Created attachment 338572 [details] [review] js: Root gjs_init_dynamic_class() Starting in jsapi-dynamic-class.cpp and working backwards, we cause another cascade of GC rooting changes.
Review of attachment 338572 [details] [review]: Looks good.
Comment on attachment 338572 [details] [review] js: Root gjs_init_dynamic_class() Attachment 338572 [details] pushed as 7e1f366 - js: Root gjs_init_dynamic_class()
Created attachment 338637 [details] [review] jsapi-util-error: Root misc functions This converts everything else in jsapi-util-error.cpp to use exact rooting, that would otherwise cause a compile error in mozjs31.
Review of attachment 338637 [details] [review]: ::: gjs/jsapi-util-error.cpp @@ +88,3 @@ + if (!gjs_object_require_property_value(context, global, "global object", + error_name, &constructor)) { Does this not recurse into gjs_throw() (which then recurses into this function) when it fails?
(In reply to Cosimo Cecchi from comment #218) > Review of attachment 338637 [details] [review] [review]: > > ::: gjs/jsapi-util-error.cpp > @@ +88,3 @@ > > + if (!gjs_object_require_property_value(context, global, "global object", > + error_name, &constructor)) { > > Does this not recurse into gjs_throw() (which then recurses into this > function) when it fails? You are right, good catch.
Created attachment 338643 [details] [review] jsapi-util-error: Root misc functions This converts everything else in jsapi-util-error.cpp to use exact rooting, that would otherwise cause a compile error in mozjs31.
Created attachment 338644 [details] [review] jsapi-util-string: Misc mozjs31 changes This converts to exact rooting everywhere that would otherwise cause a compile error in mozjs31. Also adds a cast in one place since jschar changes signedness from mozjs24 to mozjs31. This cast does nothing right now, but neither does it harm anything.
Created attachment 338645 [details] [review] stack: Root misc functions This converts everything else in stack.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31.
Review of attachment 338643 [details] [review]: Looks good now.
Review of attachment 338644 [details] [review]: Looks good.
Review of attachment 338645 [details] [review]: Looks good.
Attachment 338643 [details] pushed as be417a4 - jsapi-util-error: Root misc functions Attachment 338644 [details] pushed as c27577b - jsapi-util-string: Misc mozjs31 changes Attachment 338645 [details] pushed as 3da7185 - stack: Root misc functions
Created attachment 338655 [details] [review] js: Root throw_invalid_argument() in arg.cpp Starting in throw_invalid_argument() and working backwards, we cause another cascade of GC rooting changes. This, by necessity, adds a couple of unnecessary roots in gjs_invoke_c_function() which is quite unfortunate. (Eventually the vp array comes from a JS::CallArgs somewhere, which is rooted, so we should not have to re-root any of its values.) In mozjs31, we will be able to remove these again, because it introduces JS::HandleValueArray for passing around references to arrays of rooted values.
Created attachment 338656 [details] [review] arg: Root misc functions This converts everything else in arg.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31.
Created attachment 338657 [details] [review] boxed: Root misc functions This converts everything else in boxed.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31.
Review of attachment 338655 [details] [review]: Feel free to push with these two fixed ::: gi/arg.cpp @@ +371,3 @@ result = g_hash_table_new(g_str_hash, g_str_equal); + JS::RootedValue key_js(context), val_js(context); Indentation looks off ::: gi/function.cpp @@ +948,3 @@ in_value)) { failed = true; break; You can remove this break here
Review of attachment 338656 [details] [review]: Looks good to me.
Review of attachment 338657 [details] [review]: Looks good to me.
(In reply to Cosimo Cecchi from comment #230) > Review of attachment 338655 [details] [review] [review]: > ::: gi/arg.cpp > @@ +371,3 @@ > result = g_hash_table_new(g_str_hash, g_str_equal); > > + JS::RootedValue key_js(context), val_js(context); > > Indentation looks off Indeed, some of the existing indentation was 3 spaces... I'll fix that up
Attachment 338655 [details] pushed as 6864b3f - js: Root throw_invalid_argument() in arg.cpp Attachment 338656 [details] pushed as 5dc229d - arg: Root misc functions Attachment 338657 [details] pushed as 55b287e - boxed: Root misc functions
Created attachment 338658 [details] [review] enum: Root misc functions This converts everything else in enumeration.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31. This has effects on the functions in enumeration.h, but all of the code that called them was already converted, so no changes needed.
Review of attachment 338658 [details] [review]: Looks good.
Comment on attachment 338658 [details] [review] enum: Root misc functions Attachment 338658 [details] pushed as c3cd1d9 - enum: Root misc functions
Created attachment 338661 [details] [review] js: Root gjs_define_function() Starting from gjs_define_function() in function.cpp and working outwards, we cause another cascade of GC rooting changes.
Created attachment 338662 [details] [review] function: Store trampoline pointer in JS::Heap In order to keep tracking the trampoline->js_function when the garbage collector moves it, it needs to be stored in a JS::Heap. The way to do this in future SpiderMonkeys is to use JS::PersistentRooted, but that is tricky here since sometimes we want to have the pointer (if is_vfunc) and sometimes we don't. After some conversation on #jsapi I believe the correct way is to manually add a root to the value. I'm not entirely sure though... (We cannot use a tracer here, as we did in previous commits, because this isn't tied to the lifetime of a JS object.)
Created attachment 338663 [details] [review] function: Root misc functions This converts everything else in function.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31.
Review of attachment 338661 [details] [review]: Looks good.
Review of attachment 338662 [details] [review]: OK
Review of attachment 338663 [details] [review]: Looks good.
Attachment 338661 [details] pushed as 9abc94a - js: Root gjs_define_function() Attachment 338662 [details] pushed as d8bb804 - function: Store trampoline pointer in JS::Heap Attachment 338663 [details] pushed as 1219ccd - function: Root misc functions
Created attachment 338754 [details] [review] function: Use whole value array In mozjs31, it will not be possible to do pointer fiddling with the AutoValueVector passed into JS_CallFunctionValue(), because that function will take the whole vector instead of a vp array plus number of args. Therefore, we restructure gjs_callback_closure() so that vfuncs pop the first element of the vector back into the this_object root, instead of manipulating the pointer passed to JS_CallFunctionValue().
Created attachment 338755 [details] [review] js: Root misc function, keep-alive, ns This converts everything else in function.cpp, keep-alive.cpp, and ns.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31. (I did function.cpp in an earlier commit, but missed one spot.)
Review of attachment 338754 [details] [review]: Looks good.
Review of attachment 338755 [details] [review]: Sure
Attachment 338754 [details] pushed as aa8dbec - function: Use whole value array Attachment 338755 [details] pushed as a2c4ec3 - js: Root misc function, keep-alive, ns
Created attachment 339001 [details] [review] object: Use JS::Heap for GObject-JSObject weak ref This will be necessary in mozjs31 because the garbage collector could move the object around, which should be tracked by JS::Heap. Unlike other uses of JS::Heap, we do not want to either trace or root the object, because it is supposed to be a weak reference.
Created attachment 339002 [details] [review] object: Root misc functions This converts everything else in object.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31.
Giovanni, if you have some spare time, maybe you could look at the toggle ref commit (attachment 339001 [details] [review]) as well?
Created attachment 339005 [details] [review] js: Root misc fundamental, param, repo, union This converts everything in fundamental.cpp, param.cpp, repo.cpp, and union.cpp to use exact rooting, that would otherwise have caused a compile error in mozjs31.
Created attachment 339006 [details] [review] js: Root gjs_value_to_g_value() Converting gjs_value_to_g_value() and gjs_value_to_g_value_no_copy() to use exact rooting has a small cascade effect into object.cpp.
Review of attachment 339001 [details] [review]: Makes sense, except for some comments. I'm surprised we did not need more changes. ::: gi/object.cpp @@ +1985,3 @@ +{ + gpointer data = g_object_get_qdata(gobj, gjs_object_priv_quark()); + if (G_UNLIKELY(data == NULL)) { Is this really UNLIKELY? (failing an unlikely branch is an almost guaranteed instruction cache miss, which is not good) @@ +2019,3 @@ +poison_js_obj(GObject *gobj) +{ + ensure_heap_wrapper(gobj)->set((JSObject *) -1); What happens when the JS::Heap is called with -1? From reading the mozjs38 header code, it tries to inform the GC about the pointer, and I'd expect it to crash. How about calling .setToCrashOnTouch()/.isSetToCrashOnTouch()? Is it not available in mozjs31?
(In reply to Giovanni Campagna from comment #255) > Review of attachment 339001 [details] [review] [review]: > > Makes sense, except for some comments. I'm surprised we did not need more > changes. Me too, though we may well need more later - I have only tested this in mozjs24 as I am still working through all the API changes to get GJS to compile with mozjs31! > ::: gi/object.cpp > @@ +1985,3 @@ > +{ > + gpointer data = g_object_get_qdata(gobj, gjs_object_priv_quark()); > + if (G_UNLIKELY(data == NULL)) { > > Is this really UNLIKELY? > > (failing an unlikely branch is an almost guaranteed instruction cache miss, > which is not good) You're probably right. It's likely enough that it will happen at least once per object, I'll remove it. > @@ +2019,3 @@ > +poison_js_obj(GObject *gobj) > +{ > + ensure_heap_wrapper(gobj)->set((JSObject *) -1); > > What happens when the JS::Heap is called with -1? > > From reading the mozjs38 header code, it tries to inform the GC about the > pointer, and I'd expect it to crash. > > How about calling .setToCrashOnTouch()/.isSetToCrashOnTouch()? Is it not > available in mozjs31? Indeed, I was planning to use those and that's the reason I split out the poison_js_obj function. It's not available in mozjs24 yet, so I can't commit it at this point. In mozjs31 it seems that there's a bug in it; the compiler chokes on isSetToCrashOnTouch() when instantiating the template of that method for JSObject* because they forgot to cast the enum to T. https://dxr.mozilla.org/mozilla-esr31/source/js/public/RootingAPI.h#256 I can in any case use the crashOnTouch value of 1 instead of the previous sentinel value of -1 here. I don't think there would be any real difference between the two.
Created attachment 339083 [details] [review] object: Use JS::Heap for GObject-JSObject weak ref This will be necessary in mozjs31 because the garbage collector could move the object around, which should be tracked by JS::Heap. Unlike other uses of JS::Heap, we do not want to either trace or root the object, because it is supposed to be a weak reference.
Created attachment 339091 [details] [review] cairo: Root gjs_cairo_image_surface_init() Using exact rooting in gjs_cairo_image_surface_init() has a small cascade effect into cairo.cpp.
Created attachment 339092 [details] [review] js: Root misc functions This ports the rest of the main codebase to use exact GC rooting for everything that would otherwise have caused a compile error in mozjs31. There were only a few modifications per file still to do so I stuck them all in one commit.
Created attachment 339093 [details] [review] test: Root misc functions in tests This converts any code from the tests to use exact GC rooting that would otherwise have caused a compile error in mozjs31.
Created attachment 339096 [details] [review] js: Root misc functions This ports the rest of the main codebase to use exact GC rooting for everything that would otherwise have caused a compile error in mozjs31. There were only a few modifications per file still to do so I stuck them all in one commit.
Review of attachment 339002 [details] [review]: Looks good to me.
(In reply to Philip Chimento from comment #256) > (In reply to Giovanni Campagna from comment #255) > > How about calling .setToCrashOnTouch()/.isSetToCrashOnTouch()? Is it not > > available in mozjs31? > > Indeed, I was planning to use those and that's the reason I split out the > poison_js_obj function. It's not available in mozjs24 yet, so I can't commit > it at this point. In mozjs31 it seems that there's a bug in it; the compiler > chokes on isSetToCrashOnTouch() when instantiating the template of that > method for JSObject* because they forgot to cast the enum to T. > > https://dxr.mozilla.org/mozilla-esr31/source/js/public/RootingAPI.h#256 > > I can in any case use the crashOnTouch value of 1 instead of the previous > sentinel value of -1 here. I don't think there would be any real difference > between the two. The Mozilla bug that I opened (https://bugzilla.mozilla.org/show_bug.cgi?id=1315122) had the opposite effect from what I intended: they quickly made a commit to remove the API altogether. So, I guess we should not switch to it :-P
Comment on attachment 339002 [details] [review] object: Root misc functions Attachment 339002 [details] pushed as 8dbd2ba - object: Root misc functions
Created attachment 339159 [details] [review] js: Don't pass JSPROP_READONLY to JS_PSG This is redundant, since you already can't set a property that doesn't have a setter. It will be illegal and caught at compile time in mozjs31.
Created attachment 339160 [details] [review] js: Root misc functions This ports the rest of the main codebase to use exact GC rooting for everything that would otherwise have caused a compile error in mozjs31. There were only a few modifications per file still to do so I stuck them all in one commit.
Review of attachment 339005 [details] [review]: Looks good to me.
Review of attachment 339006 [details] [review]: Looks good to me.
Review of attachment 339091 [details] [review]: Looks good.
Review of attachment 339093 [details] [review]: Looks good.
Review of attachment 339159 [details] [review]: OK
Review of attachment 339160 [details] [review]: Looks good to me.
Review of attachment 339083 [details] [review]: Looks good to me -- perhaps Giovanni wants to have another look though.
Review of attachment 339083 [details] [review]: ::: gi/object.cpp @@ +2020,3 @@ +poison_js_obj(GObject *gobj) +{ + /* COMPAT: Use JS::Heap::setToCrashOnTouch() in mozjs31 */ I should remove this comment now, in any case.
Attachment 339005 [details] pushed as 18b4117 - js: Root misc fundamental, param, repo, union Attachment 339006 [details] pushed as 8f3e64b - js: Root gjs_value_to_g_value() Attachment 339091 [details] pushed as 63aee3c - cairo: Root gjs_cairo_image_surface_init() Attachment 339093 [details] pushed as fbb3c18 - test: Root misc functions in tests Attachment 339159 [details] pushed as 19b2f46 - js: Don't pass JSPROP_READONLY to JS_PSG Attachment 339160 [details] pushed as cc03bc8 - js: Root misc functions
(In reply to Cosimo Cecchi from comment #273) > Review of attachment 339083 [details] [review] [review]: > > Looks good to me -- perhaps Giovanni wants to have another look though. Yeah looks good to me too.
Comment on attachment 339083 [details] [review] object: Use JS::Heap for GObject-JSObject weak ref Attachment 339083 [details] pushed as e22a7d8 - object: Use JS::Heap for GObject-JSObject weak ref
Well, that's that... I may still have some stragglers in my mozjs31 branch that can be backported to the current code, I'll check later. But otherwise I think this "spidermonkey 31 prep" bug can be closed! I'll open a new bug for the actual switch. Currently with mozjs31 everything compiles, and some of the tests even pass, but most importantly gjs-console will not start because it can't find the Console module. This is what I'll work on fixing next. You can check out my wip/ptomato/mozjs31 branch if you want to take a look.
Wow! I've been following this bug for a long time. Great work everyone, and especially Philip. How much do you reckon is left until the switch to mozjs31?
Gjs-console is critical as it needed at least for gnome-maps and Polari.
(In reply to Philip Chimento from comment #278) > Well, that's that... I may still have some stragglers in my mozjs31 branch > that can be backported to the current code, I'll check later. But otherwise > I think this "spidermonkey 31 prep" bug can be closed! > > I'll open a new bug for the actual switch. Great! Closing this bug then. Thanks a lot for all the patches, Philip!
Created attachment 339274 [details] [review] build: Remove usage of jsfriendapi.h The "friend API" is more unstable in between SpiderMonkey releases than the normal API, so we should avoid using it if possible. Since we only need it for this one numerical constant, we just stick the number in, as we were already doing lower down in the file.
Created attachment 339275 [details] [review] js: Leftover rooting changes Fixing some stragglers that were rooted only on my post-mozjs31-switch branch.
Created attachment 339276 [details] [review] importer: Overwrite property rather than change attrs JS_GetPropertyAttributes() and JS_SetPropertyAttributes() are going away in mozjs31. Previously seal_import() added the "permanent" flag in the property descriptor. Now, we simply overwrite the existing property with a new one with the "permanent" flag set. This has the same effect. Until mozjs31 we have to use JS_GetPropertyDescriptorById() which can return properties from higher up the prototype chain. However, on the importer object, there will not be any properties of the same name higher up the prototype chain, because we already defined the property in define_import().
(In reply to Philip Chimento from comment #278) > Well, that's that... I may still have some stragglers in my mozjs31 branch > that can be backported to the current code, I'll check later. I did, here are three more patches. These should really be the last :-) (In reply to Mattias Bengtsson from comment #279) > Wow! I've been following this bug for a long time. Great work everyone, and > especially Philip. How much do you reckon is left until the switch to > mozjs31? Thank you! I currently have some crashes using mozjs31 as noted above; once they are solved, I'm not sure whether I will keep running into more crashes or things will "just work". We should also decide whether the switch would be a 1.48 thing or we should wait until I have time to continue porting to mozjs38 and mozjs45. We should also factor in how much time would be left for application writers to give the new GJS a shakedown. (In reply to Hussam Al-Tayeb from comment #280) > Gjs-console is critical as it needed at least for gnome-maps and Polari. It's critical for everything that uses GJS, except for gnome-shell, and I doubt that gnome-shell would work on that branch either. Certainly, I would not consider the task done if it didn't work...
Review of attachment 339274 [details] [review]: Looks good.
Review of attachment 339275 [details] [review]: Looks good.
Review of attachment 339276 [details] [review]: OK
Attachment 339274 [details] pushed as 030dcf6 - build: Remove usage of jsfriendapi.h Attachment 339275 [details] pushed as 0046d37 - js: Leftover rooting changes Attachment 339276 [details] pushed as 685f9e4 - importer: Overwrite property rather than change attrs
I reopened https://bugzilla.gnome.org/show_bug.cgi?id=751252 for further discussion about switching to mozjs31. Please subscribe there to continue the discussion :-)
Created attachment 339298 [details] [review] function: Root retval of gjs_invoke_c_function() This previously escaped notice, but it will need to be rooted in mozjs31. Since a NULL pointer here was previously used to disable some code paths that were unnecessary if we didn't want the return value, we use a mozilla::Maybe instead.
One more straggler! And there's another patch coming tomorrow as I've discovered that you have to use JS_NewObjectWithGivenProto() in several places in mozjs31 where you could use JS_NewObject() instead in mozjs24.
Review of attachment 339298 [details] [review]: Looks good to me.
Comment on attachment 339298 [details] [review] function: Root retval of gjs_invoke_c_function() Attachment 339298 [details] pushed as 75dccac - function: Root retval of gjs_invoke_c_function()
Created attachment 339359 [details] [review] gtype: Specify GType object prototype explicitly Previously, JS_NewObject() was able to infer the prototype of the GType object because the prototype was there in the global namespace. JS_NewObject() has stopped doing this in mozjs31, so we need to explicitly specify the prototype using JS_NewObjectWithGivenProto(). (I'm not sure if this needs to be fixed up anywhere else. The tests all pass without it, and in the importer case, the tests were relying on the importer object _not_ having the prototype of Object.)
Review of attachment 339359 [details] [review]: Looks good to me.
Comment on attachment 339359 [details] [review] gtype: Specify GType object prototype explicitly Attachment 339359 [details] pushed as b7efc89 - gtype: Specify GType object prototype explicitly
Created attachment 339546 [details] [review] jsapi: Use same #defines as SpiderMonkey This pulls in js-config.h into jsapi-wrapper.h, because some of SpiderMonkey's #defines that are set at build time affect the public API. In addition, in mozjs24 and 31 at least, it looks like sometimes SpiderMonkey headers erroneously check for the presence of DEBUG instead of JS_DEBUG, so we should define DEBUG when JS_DEBUG is defined.
Created attachment 339547 [details] [review] js: Have an active request where needed This was surfaced by #defining DEBUG. In two places, we tried to create a JS::Rooted without being in a request, which is not allowed.
Created attachment 339548 [details] [review] function: Handle callbacks with no return value Previously this was handled by the else clause in this if statement, but since we switched to rooting, we call .toObjectOrNull() on the callback's return value. In the case of no return value, the JS return value is undefined. We could handle this lower down in the existing else clause, but this seems easier to understand.
Created attachment 339549 [details] [review] value: Pass NULL vp array if no closure arguments This used to work with an unrooted JS::Value* vp array, but now that we are using JS::AutoValueVector we can't index argv[0] if the size of the vector is 0. Pass NULL instead.
Found some more bugs, as I realized I was using SpiderMonkey debug mode improperly. The first of these four patches ensures that debug mode is used properly in the future.
Review of attachment 339546 [details] [review]: Ugh, a bit ugly but OK.
Review of attachment 339547 [details] [review]: Looks good to me.
Review of attachment 339548 [details] [review]: OK
Review of attachment 339549 [details] [review]: OK
Attachment 339546 [details] pushed as dd2a0c5 - jsapi: Use same #defines as SpiderMonkey Attachment 339547 [details] pushed as 1340fde - js: Have an active request where needed Attachment 339548 [details] pushed as a5f644f - function: Handle callbacks with no return value Attachment 339549 [details] pushed as e329af8 - value: Pass NULL vp array if no closure arguments
Created attachment 339564 [details] [review] js: Check for NULL in tracers It's possible to have a GC before the private data of these objects is allocated. In that case, there's nothing to trace, so if the private data member is NULL then just return from the trace callback.
Review of attachment 339564 [details] [review]: OK
Attachment 339564 [details] pushed as bb93b97 - js: Check for NULL in tracers
OK, I think that's really it now.