GNOME Bugzilla – Bug 777962
Switch to SpiderMonkey 38
Last modified: 2017-02-15 04:06:44 UTC
This bug will hold all the patches that can only be committed all at once, after the switch.
Created attachment 344601 [details] [review] build: Build with mozjs38 Requires updating our includes. The function used from OldDebugAPI.h was simply moved into jsapi.h, and some functions were separated out into js/Conversions.h.
Created attachment 344602 [details] [review] docs: Overview of new JS features in NEWS Distilled from SpiderMonkey's documentation.
Created attachment 344603 [details] [review] js: AutoValueVector's [] operator is rooted Previously, the [] operator of JS::AutoValueVector would give you a raw pointer, and if you wanted a rooted value you had to call the handleAt() method. In SpiderMonkey 38, that method is removed, and the [] operator gives you a rooted value.
Created attachment 344604 [details] [review] js: Adapt to new JS_NewObject() API JS_NewObject() does not take a prototype parameter anymore, that has been split into a different function, JS_NewObjectWithGivenProto(). Passing JS::NullPtr() as prototype means "Look in the global object for a prototype that matches the given class" and that is now the only possible behaviour for JS_NewObject(). Both JS_NewObject() and JS_NewObjectWithGivenProto() have had their global object parameter made optional, so in the cases where we were passing JS::NullPtr() there, then we simply leave it out.
Created attachment 344605 [details] [review] js: Adapt to new JS_DefinePropertyById() API Like JS_DefineProperty() could already, JS_DefinePropertyById() is now overloaded to take several types as the property value. This allows us to pass rooted objects directly in, in order to avoid creating temporary JS::Values.
Created attachment 344606 [details] [review] js: Rename JS_CallHeapFooTracer() The various flavours of JS_CallHeapFooTracer() are now called JS_CallFooTracer() in SpiderMonkey 38.
Created attachment 344607 [details] [review] js: Discontinue JSClass stubs JSClass now simply takes NULL to indicate that an operation should default to a stub function.
Created attachment 344608 [details] [review] js: Adapt to new JS_SetErrorReporter() API JS_SetErrorReporter() now acts on a JSRuntime, not a JSContext. (JSRuntime and JSContext will be merged in a future version of SpiderMonkey anyway.)
Created attachment 344609 [details] [review] constructor-proxy: Adapt to new js::DirectProxyHandler API Callability is no longer indicated with the bizarre setDefaultClass() method, but instead by overriding the isCallable() and isConstructor() methods; and some signatures have changed.
Created attachment 344610 [details] [review] runtime: Adapt to new JSFinalizeCallback JSFinalizeCallback now allows a user-data parameter, so we can get rid of some fiddling with runtime private data.
Created attachment 344611 [details] [review] js: Adapt to new mozilla::Maybe API mozilla::Maybe now has a bool operator so we don't have to compare against .empty(), and we can construct values with mozilla::Some(). The API is still fairly verbose and will be improved even further in later SpiderMonkey releases.
Created attachment 344612 [details] [review] js: Adapt to new signature of resolve operation JSNewResolveOp and JSResolveOp are merged in SpiderMonkey 38. There is only one signature, so no need to distinguish the two with the JSCLASS_NEW_RESOLVE flag. Converting from the old JSNewResolveOp callback to the new JSResolveOp callback is a matter of making sure that *resolved is set whenever the callback returns true. (The equivalent of *resolved = true before this change was objp.set(obj) on the passed-in MutableHandleObject.)
Created attachment 344613 [details] [review] js: Accommodate both Latin-1 and two-byte strings Starting with SpiderMonkey 38, strings can be stored internally in the JS engine as either Latin-1 or two bytes per character (sort-of-UTF-16). When operating on the characters directly, we must check JS_StringHasLatin1Chars() and use the appropriate accessors depending on how they are stored. Additionally, since the string's characters are still owned by the string and may get moved around during garbage collection, we cannot use any JSAPI functions that might trigger a GC while maintaining a pointer to the string's characters. To enforce this, we must construct a JS::AutoCheckCannotGC object, which will cause a crash if a GC starts while it is in scope.
Created attachment 344614 [details] [review] js: JSNative accessors in JS_DefineProperty() In JS_DefineProperty(), the getter and setter parameters must now be of type JSNative. We have already switched to JSNative accessors everywhere, but there are a few macros that must change in order to ensure everything is cast to the right type.
Created attachment 344615 [details] [review] js: Fix misc APIs for mozjs38 This commit adapts to a few miscellaneous API changes in mozjs38 that don't fit anywhere else.
Created attachment 344616 [details] [review] js: Various SpiderMonkey 38 improvements Here are two places where SpiderMonkey 38 offers a better API than the one we were using. The third spot, we could use JS::CallArgs::requireAtLeast(), but there is an issue with the visibility of that symbol in the shared library. Add a link to the Mozilla bug for that.
Created attachment 344617 [details] [review] WIP - object: Update weak pointers after GC The only place where we currently maintain a weak pointer is object.cpp, where a GObject is associated with its JS wrapper object. In SpiderMonkey 38, we have to call JS_UpdateWeakPointerAfterGC on weak pointers, every garbage collection cycle, in case the garbage collector moves them. This is one way to do it, another way might be to add a separate callback for each object with JS_AddWeakPointerCallback() instead of one callback that processes all the weak pointers it knows about. FIXME: The weak pointer list might have to have a lock.
Created attachment 344618 [details] [review] WIP - importer: Switch to eager enumeration Enumeration of just the keys, without defining properties, has been removed from the public API in SpiderMonkey 38. In practice, Firefox was not using lazy enumeration, so they removed it. That is bad news for our importer object, since it was nice to be able to enumerate the keys without actually reading and executing all the code in the modules. This changes the enumerate hook on the importer to import all the modules immediately. FIXME: This still has some problems. If a module throws an exception during import, then what should the value of the defined property be?
Created attachment 344619 [details] [review] WIP - importer: Switch back to lazy enumeration Lazy enumeration is still available through the jsfriend API in js::Class, which is really JSClass but with some extra fields that are masked in the public version. In addition, JSNewEnumerateOp has changed somewhat, compared with SpiderMonkey 31. Instead of the enumerate callback being called incrementally for each property, now it is just called once the first time an object's properties are enumerated, and you have to fill in the passed-in JS::AutoIdVector with any names of lazy properties. FIXME: This changes the behaviour, since the AutoIdVector already contains any enumerable property names that the JS engine already knows about, before the enumerate callback is called! So we now get searchPath, byteArray, etc. when we enumerate the importer.
All of these patches, except for "js: Various SpiderMonkey 38 improvements", have to be committed at the same time, or else GJS won't build. Most of them are trivial adaptations to API changes. The final three patches need more careful scrutiny. I believe "WIP - object: Update weak pointers after GC" is correct, though it touches that gnarly toggle-ref code, so I can't be sure :-) The final two, "WIP - importer: Switch to eager enumeration" and "WIP - importer: Switch back to lazy enumeration" are two approaches to solve the same problem, which is that object enumeration was totally refactored in SpiderMonkey, and you can't do lazy enumeration anymore through the public API. There are a few approaches we could take: - Switch to eager enumeration (commit patch 1 of 2). This would make enumerating the importer object really expensive but I doubt people do it very often anyway. Needs a bit of work on edge cases. - Use the jsfriendapi to keep lazy enumeration that _almost_ works the same (squash patch 2 into patch 1). Needs a bit of work on edge cases. - Do some hack like define the properties to null in the enumerate hook, and then override the get hook to really import the module if the value was null (but I already talked with Cosimo about this option and he didn't like it.) - Change the importer to a C++ Proxy object, which would also use the jsfriendapi. - Rebase and fix up Jasper's "wip/imports-rewrite" branch; it uses the bootstrap system which I already fixed up for bug 777724. I've done some work on this, see the "wip/ptomato/jasper-imports" branch. It doesn't pass all the tests, not sure how much more work it needs. I'm hoping that ES6 modules will land in SpiderMonkey 59, so I'm hesitant to do _too_ much refactoring of the importer if we have to rewrite it all again by next year.
(In reply to Philip Chimento from comment #19) > Created attachment 344619 [details] [review] [review] > WIP - importer: Switch back to lazy enumeration > > ... > > FIXME: This changes the behaviour, since the AutoIdVector already > contains any enumerable property names that the JS engine already knows > about, before the enumerate callback is called! So we now get searchPath, > byteArray, etc. when we enumerate the importer. Can we mark these as not enumerable? I feel like at least searchPath should be.
Created attachment 345014 [details] [review] js: Various SpiderMonkey 38 improvements Here are three places where SpiderMonkey 38 offers a better API than the one we were using. The fourth spot, we could use JS::CallArgs::requireAtLeast(), but there is an issue with the visibility of that symbol in the shared library. Add a link to the Mozilla bug for that.
(In reply to Giovanni Campagna from comment #21) > (In reply to Philip Chimento from comment #19) > > Created attachment 344619 [details] [review] [review] [review] > > WIP - importer: Switch back to lazy enumeration > > > > ... > > > > FIXME: This changes the behaviour, since the AutoIdVector already > > contains any enumerable property names that the JS engine already knows > > about, before the enumerate callback is called! So we now get searchPath, > > byteArray, etc. when we enumerate the importer. > > Can we mark these as not enumerable? I feel like at least searchPath should > be. It's explicitly marked enumerable, and has been that way since the initial commit; but it wasn't enumerable in practice because the enumerate callback wouldn't have returned it, so I don't see a problem in changing that. However, if we set native modules like byteArray to be non-enumerable, that's an inconsistency between JS modules and native ones. A pre-existing one, sure, but we should think twice before doing it or not doing it.
Created attachment 345510 [details] [review] jsapi-util-root: Re-enable test SpiderMonkey 31's garbage collector was not smart enough to collect this object right after it was unrooted. But SpiderMonkey 38's is.
Review of attachment 344601 [details] [review]: OK
Review of attachment 344602 [details] [review]: Thanks for doing this! Only a small comment. ::: NEWS @@ +524,1 @@ +Version 0.1 This is unrelated.
Review of attachment 344603 [details] [review]: OK
Review of attachment 344604 [details] [review]: Looks good.
Review of attachment 344605 [details] [review]: Looks good.
Review of attachment 344606 [details] [review]: OK
Review of attachment 344607 [details] [review]: OK
Review of attachment 344608 [details] [review]: OK
Review of attachment 344609 [details] [review]: OK
Review of attachment 344610 [details] [review]: OK
Review of attachment 344611 [details] [review]: OK
Review of attachment 344612 [details] [review]: ::: gi/ns.cpp @@ +109,3 @@ + /* we defined the property in this object */ + g_base_info_unref(info); + *resolved = true; Should this not be *resolved = defined? (Otherwise defined is also unused.) ::: gi/object.cpp @@ +614,2 @@ ret = true; + *resolved = false; /* technically we shouldn't touch this if returning false? */ Yeah, maybe it's also possible to do early returns instead? ::: gi/repo.cpp @@ +176,2 @@ if (priv == NULL) /* we are the prototype, or have the wrong class */ + return false; This was returning true with objp not set before. So should it not return true with *resolved=false?
Review of attachment 344613 [details] [review]: ::: gjs/byteArray.cpp @@ +624,3 @@ + "LATIN1", /* from_encoding */ + NULL, /* bytes read */ + JS_GetLatin1StringCharsAndLength(context, nogc, str, &len); Optional: it would be nice if you factored the g_convert() into a single call. (You could store the from_encoding into another variable.) ::: gjs/jsapi-util-string.cpp @@ +194,3 @@ + JS::AutoCheckCannotGC nogc; + + if (JS_StringHasLatin1Chars(value.toString())) { Optional: would be nice to factor out the following block into a separate function.
Review of attachment 344614 [details] [review]: Looks good.
Review of attachment 344615 [details] [review]: OK
Created attachment 345681 [details] [review] WIP - js: Update weak pointers after GC There are two places where we maintain weak pointers to GC things: one is in object.cpp, where a GObject is associated with its JS wrapper object. The other is in gtype.cpp, where each GType is associated with its wrapper object. In SpiderMonkey 38, we have to call JS_UpdateWeakPointerAfterGC() on weak pointers, every garbage collection cycle, in case the garbage collector moves them. FIXME: This is one way to do it, another way might be to add a separate callback for each object with JS_AddWeakPointerCallback() instead of one callback per file that processes all the weak pointers it knows about. FIXME: The GType weak pointers might be better off not stored as GType qdata, instead have a map<GType, JS::Heap<JSObject*>*>
Created attachment 345682 [details] [review] WIP - importer: Switch to eager enumeration Enumeration of just the keys, without defining properties, has been removed from the public API in SpiderMonkey 38. In practice, Firefox was not using lazy enumeration, so they removed it. That is bad news for our importer object, since it was nice to be able to enumerate the keys without actually reading and executing all the code in the modules. This changes the enumerate hook on the importer to import all the modules immediately. FIXME: This still has some problems. If a module throws an exception during import, then what should the value of the defined property be?
Created attachment 345683 [details] [review] WIP - importer: Switch back to lazy enumeration Lazy enumeration is still available through the jsfriend API in js::Class, which is really JSClass but with some extra fields that are masked in the public version. In addition, JSNewEnumerateOp has changed somewhat, compared with SpiderMonkey 31. Instead of the enumerate callback being called incrementally for each property, now it is just called once the first time an object's properties are enumerated, and you have to fill in the passed-in JS::AutoIdVector with any names of lazy properties. FIXME: This changes the behaviour, since the AutoIdVector already contains any enumerable property names that the JS engine already knows about, before the enumerate callback is called! So we now get searchPath, byteArray, etc. when we enumerate the importer.
Review of attachment 345014 [details] [review]: Looks good.
Review of attachment 345510 [details] [review]: OK
Review of attachment 345681 [details] [review]: ::: gi/gtype.cpp @@ +77,3 @@ + if (!weak_pointer_callback) + JS_AddWeakPointerCallback(JS_GetRuntime(cx), update_gtype_weak_pointers, + else Need to set weak_pointer_callback = true here @@ +170,3 @@ + ensure_weak_pointer_callback(context); + g_type_set_qdata(gtype, gjs_get_gtype_wrapper_quark(), heap_wrapper); + weak_pointer_list.insert(gtype); We discussed this in person -- would be nice to experiment with a rooted structure for the GType wrappers instead of tracking weak pointers. ::: gi/object.cpp @@ +1333,3 @@ + if (!weak_pointer_callback) + JS_AddWeakPointerCallback(JS_GetRuntime(cx), + * may also cause it to be erased.) Also need to set weak_pointer_callback = true here.
Review of attachment 345682 [details] [review]: ::: gjs/importer.cpp @@ +732,3 @@ + JS::AutoIdVector module_props(context); + JS::RootedValue v_prop_name(context); + load_module_elements(context, object, module_props, init_path); init_path will be leaked when you return false below.
Review of attachment 345683 [details] [review]: Nice, I like the cleanup. ::: gjs/importer.cpp @@ +56,3 @@ +extern js::Class gjs_importer_extended_class; +// FIXME make JSClass const in macro +static JSClass *gjs_importer_class = (JSClass *)(&gjs_importer_extended_class); You mentioned you wanted to fix this as part of this patch?
(In reply to Cosimo Cecchi from comment #37) > Review of attachment 344613 [details] [review] [review]: > > ::: gjs/byteArray.cpp > @@ +624,3 @@ > + "LATIN1", /* from_encoding */ > + NULL, /* bytes read */ > + JS_GetLatin1StringCharsAndLength(context, nogc, str, > &len); > > Optional: it would be nice if you factored the g_convert() into a single > call. (You could store the from_encoding into another variable.) I don't think that's possible, since the two types for the "chars" array (JSLatin1Char * == unsigned char *, and char16_t *) may have different alignment requirements. I could declare the array as char16_t* which will have wider alignment, but it seems more readable to me the way it was.
Created attachment 345687 [details] [review] js: Adapt to new signature of resolve operation JSNewResolveOp and JSResolveOp are merged in SpiderMonkey 38. There is only one signature, so no need to distinguish the two with the JSCLASS_NEW_RESOLVE flag. Converting from the old JSNewResolveOp callback to the new JSResolveOp callback is a matter of making sure that *resolved is set whenever the callback returns true. (The equivalent of *resolved = true before this change was objp.set(obj) on the passed-in MutableHandleObject.)
Created attachment 345688 [details] [review] js: Accommodate both Latin-1 and two-byte strings Starting with SpiderMonkey 38, strings can be stored internally in the JS engine as either Latin-1 or two bytes per character (sort-of-UTF-16). When operating on the characters directly, we must check JS_StringHasLatin1Chars() and use the appropriate accessors depending on how they are stored. Additionally, since the string's characters are still owned by the string and may get moved around during garbage collection, we cannot use any JSAPI functions that might trigger a GC while maintaining a pointer to the string's characters. To enforce this, we must construct a JS::AutoCheckCannotGC object, which will cause a crash if a GC starts while it is in scope.
(In reply to Cosimo Cecchi from comment #26) > Review of attachment 344602 [details] [review] [review]: > > Thanks for doing this! Only a small comment. > > ::: NEWS > @@ +524,1 @@ > +Version 0.1 > > This is unrelated. I'll fix that on the branch, but I won't bother to attach a new patch if that's OK with you.
Created attachment 345689 [details] [review] WIP - js: Update weak pointers after GC There are two places where we maintain weak pointers to GC things: one is in object.cpp, where a GObject is associated with its JS wrapper object. The other is in gtype.cpp, where each GType is associated with its wrapper object. In SpiderMonkey 38, we have to call JS_UpdateWeakPointerAfterGC() on weak pointers, every garbage collection cycle, in case the garbage collector moves them. FIXME: This is one way to do it, another way might be to add a separate callback for each object with JS_AddWeakPointerCallback() instead of one callback per file that processes all the weak pointers it knows about. FIXME: The GType weak pointers might be better off not stored as GType qdata, instead have a map<GType, JS::Heap<JSObject*>*>
Created attachment 345690 [details] [review] WIP - importer: Switch to eager enumeration Enumeration of just the keys, without defining properties, has been removed from the public API in SpiderMonkey 38. In practice, Firefox was not using lazy enumeration, so they removed it. That is bad news for our importer object, since it was nice to be able to enumerate the keys without actually reading and executing all the code in the modules. This changes the enumerate hook on the importer to import all the modules immediately. FIXME: This still has some problems. If a module throws an exception during import, then what should the value of the defined property be?
Created attachment 345691 [details] [review] WIP - importer: Switch back to lazy enumeration Lazy enumeration is still available through the jsfriend API in js::Class, which is really JSClass but with some extra fields that are masked in the public version. In addition, JSNewEnumerateOp has changed somewhat, compared with SpiderMonkey 31. Instead of the enumerate callback being called incrementally for each property, now it is just called once the first time an object's properties are enumerated, and you have to fill in the passed-in JS::AutoIdVector with any names of lazy properties. FIXME: This changes the behaviour, since the AutoIdVector already contains any enumerable property names that the JS engine already knows about, before the enumerate callback is called! So we now get searchPath, byteArray, etc. when we enumerate the importer.
(In reply to Cosimo Cecchi from comment #45) > Review of attachment 345681 [details] [review] [review]: > We discussed this in person -- would be nice to experiment with a rooted > structure for the GType wrappers instead of tracking weak pointers. I've attached new patches that addressed all the comments except for the above one. I will have to delay investigating that until later, or possibly tomorrow.
(In reply to Philip Chimento from comment #55) > (In reply to Cosimo Cecchi from comment #45) > > Review of attachment 345681 [details] [review] [review] [review]: > > > We discussed this in person -- would be nice to experiment with a rooted > > structure for the GType wrappers instead of tracking weak pointers. > > I've attached new patches that addressed all the comments except for the > above one. I will have to delay investigating that until later, or possibly > tomorrow. I realized later that we can't do that. It would be a leak, because the JS gtype object would own the C++ struct, which would root the JS gtype object.
Review of attachment 345687 [details] [review]: Looks good
Review of attachment 345688 [details] [review]: Looks good
Review of attachment 345689 [details] [review]: OK
Review of attachment 345690 [details] [review]: OK
Review of attachment 345691 [details] [review]: Looks good
Hooray! Amended the "WIP" commit messages on the three remaining patches and pushed everything. I will make the change in jhbuild and gnome-continuous as well. Attachment 344601 [details] pushed as 1adf901 - build: Build with mozjs38 Attachment 344602 [details] pushed as 452e1fd - docs: Overview of new JS features in NEWS Attachment 344603 [details] pushed as c057122 - js: AutoValueVector's [] operator is rooted Attachment 344604 [details] pushed as 894eafe - js: Adapt to new JS_NewObject() API Attachment 344605 [details] pushed as 82e5e11 - js: Adapt to new JS_DefinePropertyById() API Attachment 344606 [details] pushed as 0a08a75 - js: Rename JS_CallHeapFooTracer() Attachment 344607 [details] pushed as 4a087ff - js: Discontinue JSClass stubs Attachment 344608 [details] pushed as fb6fcc0 - js: Adapt to new JS_SetErrorReporter() API Attachment 344609 [details] pushed as 12114dd - constructor-proxy: Adapt to new js::DirectProxyHandler API Attachment 344610 [details] pushed as c735ea8 - runtime: Adapt to new JSFinalizeCallback Attachment 344611 [details] pushed as 9ae4ebe - js: Adapt to new mozilla::Maybe API Attachment 344614 [details] pushed as f7f2837 - js: JSNative accessors in JS_DefineProperty() Attachment 344615 [details] pushed as 4858889 - js: Fix misc APIs for mozjs38 Attachment 345014 [details] pushed as 9ccae32 - js: Various SpiderMonkey 38 improvements Attachment 345510 [details] pushed as dd62f4a - jsapi-util-root: Re-enable test Attachment 345687 [details] pushed as 8a0021f - js: Adapt to new signature of resolve operation Attachment 345688 [details] pushed as aa743ce - js: Accommodate both Latin-1 and two-byte strings