GNOME Bugzilla – Bug 784196
Switch to SpiderMonkey 52
Last modified: 2017-07-17 23:31:48 UTC
Patches to switch to SpiderMonkey 52. See also the wip/ptomato/mozjs52 branch. These patches will only be committed all at once when they have all been reviewed, so as not to leave master in an intermediate state. It's a bit difficult to compile SpiderMonkey 52 right now. Here is the latest stable tarball: https://queue.taskcluster.net/v1/task/F-L3MRcnR4CH-gh5PKfR-g/runs/0/artifacts/public/build/mozjs-52.2.1.tar.bz2 (but this link is not permanent.) Mozilla has not yet published an official ESR SpiderMonkey release. In the next few days I will work on making it easier to get and compile SpiderMonkey 52, in JHbuild, Continuous, and the org.gnome.Platform runtime.
Created attachment 354439 [details] [review] build: Build with mozj45 Changes necessary to header and pkgconfig file names.
Created attachment 354440 [details] [review] js: new JS_Enumerate API You now have to pass into JS_Enumerate() a rooted JS::IdVector for it to fill in, rather than it returning a JSIdArray.
Created attachment 354441 [details] [review] js: Switch from JS::NullPtr() to nullptr JS::NullPtr() is gone, and instead now functions that took JS::NullPtr() as a null handle value take nullptr_t, so we should use C++ nullptr.
Created attachment 354442 [details] [review] js: Global object is implicit in many functions In many function calls where you had to pass in a global object, it is now implicit and will use the global for the current scope.
Created attachment 354443 [details] [review] js: JSCLASS_IMPLEMENTS_BARRIERS is now implicit In https://bugzilla.mozilla.org/show_bug.cgi?id=1088214 the JSCLASS_IMPLEMENTS_BARRIERS flag was removed, simply because it's now required for all JSClass implementations.
Created attachment 354444 [details] [review] js: Weak pointer callback API change Weak pointer callbacks now come in two flavours: per-zone and per-compartment. We do everything in one compartment, and don't care about zones, so we should use the per-compartment flavour.
Created attachment 354445 [details] [review] js: setProperty operations with triple result state Property setting callbacks now get a JS::ObjectOpResult on which they have to call the succeed() method or one if the fail() methods. This is required in order to implement an awkward part of the ES6 spec where it is possible for a property setting operation to fail despite not throwing an exception. This replaces what used to be "strict mode" and JSStrictPropertyOp.
Created attachment 354446 [details] [review] js: New JS_IsArrayObject() API Now JS_IsArrayObject() distinguishes between errors (returning false) and the object not being an array (returning true but setting the passed-in bool ref to false.)
Created attachment 354447 [details] [review] js: Rename JS_InternString to JS_AtomizeAndPinString This was simply renamed in SpiderMonkey 45.
Created attachment 354448 [details] [review] importer: API change in enumerate operation Now an enumerate operation distinguishes between whether only the enumerable properties should be enumerated, or all of them, by passing in a boolean parameter. In the case of the importer object, we don't care, so we ignore the parameter.
Created attachment 354449 [details] [review] js: Set JSPROP_RESOLVING when defining properties JS_DefineProperty() can now call into a class's resolve hook, so when pre-defining properties on classes with resolve hooks we have to include JSPROP_RESOLVING in the flags so that the resolve hook is not called.
Created attachment 354450 [details] [review] modules/console: Update to js::PrintError from upstream The existing code doesn't compile with SpiderMonkey 45, so we copy the corresponding code from SpiderMonkey 45's js::PrintError() function.
Created attachment 354451 [details] [review] js: Various API changes for SpiderMonkey 45 A few minor changes that were only in one place.
Created attachment 354452 [details] [review] coverage: Misc Javascript-side API changes Debugger.Script.getOffsetLine() which we previously used in code coverage is gone now, replaced by Debugger.Script.getOffsetLocation() which gives more information. We don't currently take advantage of the extra information. ArrowExpression was renamed to ArrowFunctionExpression.
Created attachment 354453 [details] [review] importer: Seal import with JSPropertyDescriptor directly Previously we "sealed" the import by redefining the property on the importer object so that it was undeleteable. Now we can do this in a more direct way by getting and modifying the property's descriptor. In order to make this more convenient and faster we thread the jsid through the call stack so we don't have to convert it to and from a string.
Created attachment 354454 [details] [review] js: Update obsolete comments These comments were previously correct, but have become incorrect in SpiderMonkey 45.
Created attachment 354455 [details] [review] js: Adapt to new JS::TraceEdge<T> API Replacing the old JS_CallFooTracer() API is a new C++ JS::TraceEdge<T>() function. It works the same, but is templated. The old API will be going away in SpiderMonkey 52.
Created attachment 354456 [details] [review] docs: Overview of SpiderMonkey 45 features in NEWS Distilled from Mozilla's documentation.
Created attachment 354457 [details] [review] build: Build with mozjs52
Created attachment 354458 [details] [review] Windows: Build against SpiderMonkey 52 This is done so that people can also test Windows builds for gjs that is based on SpiderMonkey 52. As the upstream libffi is not very well maintained for Windows/MSVC, we are also switching to use the Centricular fork for libffi[1], which will also be the case for the upcoming stable releases for GLib (i.e. GObject) and GObject-Introspection. [1]: https://github.com/centricular/libffi
Created attachment 354459 [details] [review] js: New JSClass struct layout Instead of the various operation hooks being members of JSClass directly, now JSClass contains a pointer to a JSClassOps struct. The JSClassOps instead contains the function pointers to the operation hooks. For importer.cpp, we still use the internal js::Class instead of JSClass. This also contains a pointer to another struct, JSObjectOps, which we still need for our internal lazy enumerate hook.
Created attachment 354460 [details] [review] js: Add JSCLASS_FOREGROUND_FINALIZE flag For all classes with a finalize hook, SpiderMonkey 52 requires that they either have JSCLASS_FOREGROUND_FINALIZE or JSCLASS_BACKGROUND_FINALIZE set. Previously, only the BACKGROUND flag existed and FOREGROUND was implicit.
Created attachment 354461 [details] [review] js: Adapt to options changes JS::RuntimeOptions and JS::ContextOptions have merged into one JS::ContextOptions class. JS::CompartmentOptions, on the other hand, has split into "creation options" (can be set only at construct time) and "behaviors" (can be changed at run time.) The DontReportUncaught option is removed and is now activated unconditionally.
Created attachment 354462 [details] [review] js: Report warnings and errors with encoding JS_ReportWarning() and JS_ReportError() are changed in SpiderMonkey 52 to specify their character encodings. Therefore, we use JS_ReportWarningUTF8() and JS_ReportErrorUTF8(), except in a few cases where we use the ASCII version because the argument is a string literal with only ASCII characters and no format codes.
Created attachment 354463 [details] [review] tests: Refactor to avoid error reporter Error reporter callbacks and JS_ReportPendingException() are going away in SpiderMonkey 52. We used these to determine the message from a thrown exception. Luckily, it's quite easy to do so without an error reporter callback: simply create an error report from the exception object which you can get from JS_GetPendingException().
Created attachment 354464 [details] [review] js: Replace error reporter callbacks JS_SetErrorReporter() goes away in SpiderMonkey 52, and is replaced by JS::SetWarningReporter(). Errors should now be reported manually after each call into JS code. (We don't actually have to change much, since we already used the DontReportUncaught option and reported uncaught exceptions manually already. This is now the only choice.)
Created attachment 354465 [details] [review] js: Replace JSRuntime APIs that now take JSContext Many APIs that adapted to the removal of JSRuntime can be trivially replaced with JSContext without any other code changes. JS_AbortIfWrongThread() also takes JSContext instead of JSRuntime, but we only called it to check that we were creating a JSContext on the correct thread. Before creating the JSContext, there is no point in calling it, so remove it.
Created attachment 354466 [details] [review] js: Remove JSRuntime JSRuntime is merged into JSContext in SpiderMonkey 52. We reorganize things a bit to reflect the new situation. The files runtime.cpp and runtime.h are renamed to engine.cpp and engine.h, and will contain everything having to do with the SpiderMonkey engine: initializing the JSContext, and any callbacks that the engine requires embedders to set.
Created attachment 354467 [details] [review] js: Adapt to misc API changes in SpiderMonkey 52 - JS::InitSelfHostedCode() is now required to be called before the first global object is created. - js::GCMethods<T>::initial was moved from the friend API to the public API and renamed JS::GCPolicy<T>::initial. - JSPropertyDescriptor was renamed JS::PropertyDescriptor. - JS::CloneAndExecuteScript() now takes a return value parameter, even though we don't use it in this case. - js::DumpHeapComplete() was renamed to js::DumpHeap() and takes a JSContext parameter instead of JSRuntime.
Created attachment 354468 [details] [review] js: Unbarriered read while in weak ptr callback Inside the weak pointer callback, we only need to compare the pointer to nullptr. Since we don't actually use the pointer's location, no read barrier is needed. Previously this was not a problem, but SpiderMonkey 52 now asserts that the heap is not active when exposing a pointer to active JS through a read barrier, so the weak pointer callback will crash if we try to use a read barrier.
Created attachment 354469 [details] [review] js: Allow access to modules' lexical scope For compatibility with pre-ES6 code, we allow imported modules to access members of the lexical scope (i.e. variables defined with 'const' or 'let') as if they were properties, because that is how SpiderMonkey previously implemented 'let' and 'const'. When such properties are accessed, however, we log a warning that tells people to fix their code. Hopefully such uses will become rare and we can remove this compatibility workaround.
Created attachment 354470 [details] [review] docs: Overview of SpiderMonkey 52 features in NEWS Distilled from Mozilla's documentation.
Created attachment 354535 [details] [review] js: Remove JSRuntime JSRuntime is merged into JSContext in SpiderMonkey 52. We reorganize things a bit to reflect the new situation. The files runtime.cpp and runtime.h are renamed to engine.cpp and engine.h, and will contain everything having to do with the SpiderMonkey engine: initializing the JSContext, and any callbacks that the engine requires embedders to set.
Created attachment 354536 [details] [review] js: Adapt to misc API changes in SpiderMonkey 52 - JS::InitSelfHostedCode() is now required to be called before the first global object is created. - js::GCMethods<T>::initial was moved from the friend API to the public API and renamed JS::GCPolicy<T>::initial. - JSPropertyDescriptor was renamed JS::PropertyDescriptor. - JS::CloneAndExecuteScript() now takes a return value parameter, even though we don't use it in this case. - js::DumpHeapComplete() was renamed to js::DumpHeap() and takes a JSContext parameter instead of JSRuntime.
Review of attachment 354469 [details] [review]: Executing the script twice in two different compartments is not going to work, per bug 781429.
Created attachment 354605 [details] [review] js: New JSClass struct layout Instead of the various operation hooks being members of JSClass directly, now JSClass contains a pointer to a JSClassOps struct. The JSClassOps instead contains the function pointers to the operation hooks. For importer.cpp, we still use the internal js::Class instead of JSClass. This also contains a pointer to another struct, JSObjectOps, which we still need for our internal lazy enumerate hook.
Created attachment 354606 [details] [review] WIP - js: Allow access to modules' lexical scope For compatibility with pre-ES6 code, we allow imported modules to access members of the lexical scope (i.e. variables defined with 'const' or 'let') as if they were properties, because that is how SpiderMonkey previously implemented 'let' and 'const'. When such properties are accessed, however, we log a warning that tells people to fix their code. Hopefully such uses will become rare and we can remove this compatibility workaround. FIXME: This causes all sorts of unexpected effects with modules, since they are executed in a different compartment. I'm not sure this is a great idea.
Review of attachment 354606 [details] [review]: Marking needs-work since it is still failing some tests, but this is the best I've been able to come up with so far.
Created attachment 354721 [details] [review] js: Allow access to modules' lexical scope For compatibility with pre-ES6 code, we allow imported modules to access members of the lexical scope (i.e. variables defined with 'const' or 'let') as if they were properties, because that is how SpiderMonkey previously implemented 'let' and 'const'. When such properties are accessed, however, we log a warning that tells people to fix their code. Hopefully such uses will become rare and we can remove this compatibility workaround.
Comment on attachment 354606 [details] [review] WIP - js: Allow access to modules' lexical scope The problem is solved now with a patch from upstream mozjs.
Created attachment 354997 [details] [review] js: Various API changes for SpiderMonkey 45 A few minor changes that were only in one place.
Created attachment 354998 [details] [review] js: Replace JSRuntime APIs that now take JSContext Many APIs that adapted to the removal of JSRuntime can be trivially replaced with JSContext without any other code changes. JS_AbortIfWrongThread() also takes JSContext instead of JSRuntime, but we only called it to check that we were creating a JSContext on the correct thread. Before creating the JSContext, there is no point in calling it, so remove it.
Created attachment 354999 [details] [review] js: Adapt to misc API changes in SpiderMonkey 52 - JS::InitSelfHostedCode() is now required to be called before the first global object is created. - js::GCMethods<T>::initial was moved from the friend API to the public API and renamed JS::GCPolicy<T>::initial. - JSPropertyDescriptor was renamed JS::PropertyDescriptor. - JS::CloneAndExecuteScript() now takes a return value parameter, even though we don't use it in this case.
Review of attachment 354439 [details] [review]: OK
Review of attachment 354440 [details] [review]: OK
Review of attachment 354441 [details] [review]: Nice
Review of attachment 354442 [details] [review]: Nice
Review of attachment 354443 [details] [review]: OK
Review of attachment 354444 [details] [review]: OK
Review of attachment 354445 [details] [review]: Looks good
Review of attachment 354446 [details] [review]: OK
Review of attachment 354447 [details] [review]: OK
Review of attachment 354448 [details] [review]: Sure
Review of attachment 354449 [details] [review]: OK
Review of attachment 354450 [details] [review]: OK
Review of attachment 354997 [details] [review]: Looks good
Review of attachment 354452 [details] [review]: OK
Review of attachment 354453 [details] [review]: Nice
Review of attachment 354454 [details] [review]: ++
Review of attachment 354455 [details] [review]: Looks good
Review of attachment 354456 [details] [review]: Thanks for writing these down!
Review of attachment 354457 [details] [review]: OK
Review of attachment 354458 [details] [review]: Sure
Review of attachment 354460 [details] [review]: ++
Review of attachment 354461 [details] [review]: ++
Review of attachment 354462 [details] [review]: ::: gjs/jsapi-util-error.cpp @@ +85,2 @@ if (!gjs_string_from_utf8(context, s, -1, error_args[0])) { + JS_ReportErrorASCII(context, "Failed to copy exception string"); Any reason not to always use the UTF8 version of the method? The commit message mentions that these strings have no format specifiers, but that does not make them any less UTF8, no?
Review of attachment 354463 [details] [review]: Looks good
Review of attachment 354464 [details] [review]: ::: modules/console.cpp @@ +180,3 @@ + } + + JS::RootedString stack_trace(m_cx); Is this needed here? It's already called above. Or is it to clear any pending exceptions from the snippet above constructing the stack trace?
Review of attachment 354468 [details] [review]: ++
Review of attachment 354470 [details] [review]: Yay! ::: NEWS @@ +90,3 @@ + + Object.entries() method (proposed in ES2017) + + Atomics, SharedArrayBuffer, and WebAssembly are disabled by default, but + can be enabled if you compile mozjs yourself. Nit: all the other bullet points have no trailing period
Review of attachment 354998 [details] [review]: ++
Review of attachment 354535 [details] [review]: Nice cleanup
Review of attachment 354605 [details] [review]: ++
Review of attachment 354721 [details] [review]: OK
Review of attachment 354999 [details] [review]: ++
Created attachment 355220 [details] [review] js: Report warnings and errors with encoding JS_ReportWarning() and JS_ReportError() are changed in SpiderMonkey 52 to specify their character encodings. Therefore, we use JS_ReportWarningUTF8() and JS_ReportErrorUTF8(), except in a few cases where we use the ASCII version because the argument is a string literal with only ASCII characters and no format codes.
Created attachment 355221 [details] [review] js: Replace error reporter callbacks JS_SetErrorReporter() goes away in SpiderMonkey 52, and is replaced by JS::SetWarningReporter(). Errors should now be reported manually after each call into JS code. (We don't actually have to change much, since we already used the DontReportUncaught option and reported uncaught exceptions manually already. This is now the only choice.)
Thanks for all the reviews. Looks like we will be able to get this merged in time for 1.49.4 next week! (In reply to Cosimo Cecchi from comment #66) > Review of attachment 354462 [details] [review] [review]: > > ::: gjs/jsapi-util-error.cpp > @@ +85,2 @@ > if (!gjs_string_from_utf8(context, s, -1, error_args[0])) { > + JS_ReportErrorASCII(context, "Failed to copy exception string"); > > Any reason not to always use the UTF8 version of the method? The commit > message mentions that these strings have no format specifiers, but that does > not make them any less UTF8, no? I thought the UTF8 would require a conversion since JSStrings are stored internally as char16_t or Latin-1, but I looked at the source and it isn't converted to a JSString, so UTF8 is fine. (In reply to Cosimo Cecchi from comment #68) > Review of attachment 354464 [details] [review] [review]: > Is this needed here? It's already called above. Or is it to clear any > pending exceptions from the snippet above constructing the stack trace? It was there in the SpiderMonkey code that I adapted, but it's really not needed. Also, I realized that the code did not print the stack trace at all! I refactored the stack trace formatting out to a separate function, since it's used later in the rejected promise handler code. (In reply to Cosimo Cecchi from comment #70) > Review of attachment 354470 [details] [review] [review]: > ::: NEWS > @@ +90,3 @@ > + + Object.entries() method (proposed in ES2017) > + + Atomics, SharedArrayBuffer, and WebAssembly are disabled by default, > but > + can be enabled if you compile mozjs yourself. > > Nit: all the other bullet points have no trailing period Pushed to the branch, but I won't bother re-attaching it.
Review of attachment 355220 [details] [review]: OK
Review of attachment 355221 [details] [review]: Looks good now
Thanks for the additional reviews! I'm adding bug 784598 as a blocker to this one, since if I merge the mozjs52 branch right now, then jhbuild, continuous, and the GNOME runtimes will all break.
All patches were committed, but git-bz crapped out on me.