Created attachment 349974 [details] [review]
enumeration: Remove call to JS_SetParent()

It was only necessary when objects were constructed with
JS_ConstructObject(), but we use JS_NewObject() now which already sets
the parent of the object to be the global object. In addition,
JS_SetParent() will be removed in SpiderMonkey 45.

Created attachment 349975 [details] [review]
boxed: Undefined value for field property

For boxed instances, the fields are implemented as properties with a
getter and setter. In SpiderMonkey 45, those properties are required to
have a value of undefined, rather than the (arbitrarily chosen) value of
null that we previously had.

Created attachment 349976 [details] [review]
js: Fix use of mozilla::Maybe API

Creating a mozilla::Maybe from another mozilla::Maybe is fast, so we can
dispense with the reference type, allowing a much more readable use of
mozilla::Some() or mozilla::Nothing() inline in function calls.

We still do have to specify the type of the mozilla::Maybe explicitly
when it is JS::MutableHandle and we are creating one from a JS::Rooted or
JS::Handle, otherwise the compiler will think it is JS::Rooted* or
JS::Handle*.

Created attachment 349977 [details] [review]
jsapi-class: Remove unnecessary default

This would have to change to nullptr in SpiderMonkey 45 since JS::Rooted
now treats nullptr differently from NULL. But instead we can just remove
it since it is already the default initial value for JS::RootedObject.

Created attachment 349978 [details] [review]
js: Use JS_NewPlainObject()

Instead of JS_NewObject(context, NULL) passing NULL for the JSClass
pointer, use JS_NewPlainObject() when we want a blank object to fill in
with properties. JS_NewPlainObject() is the same as "{}" or "new
Object()" in Javascript.

Created attachment 349979 [details] [review]
context: Use JS::AutoSaveExceptionState

JS_SaveExceptionState() seems to be broken in SpiderMonkey 45, but in any
case we can work around it by using the more modern RAII version. Since we
are using JS::RootedString here we can also get rid of the trick of
stuffing the string into one of the JS::CallArgs' slots to root it.

Created attachment 349980 [details] [review]
js: Don't pass global object to JS_NewObject functions

This is optional, and will become disallowed in SpiderMonkey 45. No
change in functionality, because leaving it out already causes JS to get
the global object internally.

Created attachment 349981 [details] [review]
js: Split compilation from execution in some places

In a few places, we need to split up compilation from execution in order
to be able to use new scoping rules for JS evaluation.

In SpiderMonkey 45, you cannot pass in a custom global object anymore to
JS::Evaluate(). Instead, you can either use the context's global object,
or pass in a "scope chain". The scope chain implicitly has the global
object at the end. In our case, we can pass in a scope chain of one object
to get the previous behaviour of gjs_eval_with_scope().

There is also a scope chain variant of JS::Evaluate(), but it does not
take a UTF-8 char * buffer.

When evaluating a script in another compartment, we need to use
JS::CloneAndExecuteScript() since we can't pass in the compartment's global
object either. This also requires a precompiled JSScript.

We have to duplicate the compile options and stripping the shebang line
in gjs_context_eval_file_in_compartment(), and change
gjs_strip_unix_shebang() to take an unsigned length. Not great, but I
think the alternative would cause even more churn.

Created attachment 349982 [details] [review]
js: Evaluate on global object where possible

There are a few places where it makes sense to switch from using
gjs_eval_with_scope() to plain old evaluation using the global object as
the scope. In SpiderMonkey 45, the semantics of evaluation scope change
quite drastically to match ES6 standards, so the more we can port away
from it without consequence, the less will be affected after the switch.

We previously evaluated typed-in scripts in the scope of the "this" object
that was passed to Console.interact(), which would have been the Console
module object. That is a strange thing to do. Instead, evaluate typed-in
scripts in the scope of the global object.

Same thing for gjs_run_script_in_coverage_compartment(). Previously the
scope was the CoverageStatistics singleton, which was unnecessary; instead
we evaluate using the coverage compartment's global object.

We cannot change the importer (our imported module objects are the scope)
or gjs_context_eval() (it's public API and gnome-shell uses it to evaluate
extension code, so we don't want to change what might end up on the global
object.)

Created attachment 349983 [details] [review]
js: Module exports use ES6 scope rules

We "import" modules by executing them and taking their global scope as the
module object. In ES6, variables declared with "let" and "const" do not end
up in the global scope any longer. Instead, they end up in the "global
lexical scope", which is a different object. Unfortunately, this means
breaking the way many modules export their variables, but if you want
a symbol to be exported, you have to declare it with "var" or place it
explicitly on the global object some other way.

Review of attachment 349974 [details] [review]:

++

Review of attachment 349975 [details] [review]:

++

Review of attachment 349976 [details] [review]:

++

Review of attachment 349977 [details] [review]:

++

Review of attachment 349978 [details] [review]:

++

Review of attachment 349979 [details] [review]:

++

Review of attachment 349980 [details] [review]:

++

Review of attachment 349981 [details] [review]:

Looks good.

Review of attachment 349982 [details] [review]:

++

Review of attachment 349983 [details] [review]:

Ugh, that will break quite a lot of other applications... Could we expose the "global lexical scope" members on the global scope as well, or work around this more generally instead? Otherwise this looks good.

Attachment 349974 [details] pushed as 5f058b6 - enumeration: Remove call to JS_SetParent()
Attachment 349975 [details] pushed as 6d0522c - boxed: Undefined value for field property
Attachment 349976 [details] pushed as 2e8c4f0 - js: Fix use of mozilla::Maybe API
Attachment 349977 [details] pushed as 726c230 - jsapi-class: Remove unnecessary default
Attachment 349978 [details] pushed as 93a13dc - js: Use JS_NewPlainObject()
Attachment 349979 [details] pushed as 56e8665 - context: Use JS::AutoSaveExceptionState
Attachment 349980 [details] pushed as ac3eb9c - js: Don't pass global object to JS_NewObject functions
Attachment 349981 [details] pushed as 663a100 - js: Split compilation from execution in some places
Attachment 349982 [details] pushed as 113ba8d - js: Evaluate on global object where possible

(In reply to Cosimo Cecchi from comment #20)
> Review of attachment 349983 [details] [review] [review]:
> 
> Ugh, that will break quite a lot of other applications... Could we expose
> the "global lexical scope" members on the global scope as well, or work
> around this more generally instead? Otherwise this looks good.

Yes, this is going to be a huge pain.

Apparently you can access the global lexical scope from JSAPI [1] but I haven't managed to do so yet. The only way I've figured out how to access it so far, is by using a function which has a comment above it saying "Don't use this function" [2]. I will try to figure this out more systematically soon.

There's also the question of what to do when we get the scope object; do we simply copy the defined members to the module object? or emit some warning so that maintainers will clean up their code?

[1] http://searchfox.org/mozilla-central/source/js/src/jsapi.h#1575
[2] http://searchfox.org/mozilla-central/source/js/src/jsfriendapi.h#2800

OK, I've got some progress on this. Here's a test program [1] that imports a "module" consisting of the following code:

    var a = 1;
    let b = 2;
    const c = 3;

Currently in our GJS importer all three symbols would be available on the imported module object. Following ES6 rules in mozjs45, only "a" should show up, and the other two would appear on a lexical scope object that you can't normally access from JS code.

I've found two ways of doing it. The first method is more straightforward but you have to create a new global object to import the module, and do the import in that global object's compartment. I'm not certain how well that will play with actually using the module in JS code that runs in the main compartment.

The second method uses js::ExecuteInGlobalAndReturnScope() which does not require switching compartments, but as I mentioned before comes with a caution not to use it, for whatever reason.

Now that we have access to the lexical scope object, the question is what to do with it. Ideally we would keep compatibility with existing code for now, while still warning clients to change their code for the future. Because any workaround we implement now to make the lexical scope members available, will certainly not continue to work under ES6 modules.

One way would be to print warnings if members show up on the lexical scope object after importing, but even with heuristics such as "don't warn about 'const Something = imports.something'" and "don't warn about members starting with an underscore" I really don't like this idea, because it actually causes more false positives for clients who _are_ trying to write correct ES6.

Another way would be to make a new class for module objects, with a resolve hook that looks for unresolved properties on the lexical scope object, and resolves them but prints a warning. This would be a lot of extra code but might be a lot cleaner anyway because you get no false positives; you only get a warning if you expected something declared with let/const to be exported, and you prove you expected it by trying to access it.

[1] https://gist.github.com/ptomato/8d112afe44f2b10972b92f4c2ae9fea0

Created attachment 350844 [details] [review]
build: Autodetect SpiderMonkey's debug mode

We currently check whether JS_DEBUG was defined in js-config.h in order to
decide whether to define DEBUG. (It's a long-standing bug in SpiderMonkey
headers that you have to define DEBUG before including them if SpiderMonkey
is in debug mode.)

That will not be possible in SpiderMonkey 52. In an effort to mitigate the
bug, it will error out if you include js-config.h without defining DEBUG
when you are supposed to (or having defined DEBUG when you are not supposed
to.) So, our method of checking will not work anymore.

Instead, run a configure-time check that generates an #error if JS_DEBUG is
defined in js-config.h, and define a symbol in config.h. This check will
also work in the SpiderMonkey 52 case, since including js-config.h will
also error out if JS_DEBUG is defined.

Created attachment 350922 [details] [review]
closure: Remove pointer to runtime

This isn't used at all! Noticed because JSRuntime is going away in
SpiderMonkey 52.

Created attachment 350923 [details] [review]
context: Use GThread to determine owner thread

The thread facilities are going away in SpiderMonkey 52, so we use GThread
to determine a context's owner thread in order to be able to check it
later. (g_thread_self() will still work if GLib didn't create the thread.)

Created attachment 350924 [details] [review]
coverage: Root using context, not runtime

JSRuntime is going away in SpiderMonkey 52, so we should stop using the
constructor of JS::Rooted<T> that takes JSRuntime instead of JSContext.
There were a few instances where it was used.

Created attachment 350925 [details] [review]
jsapi-constructor-proxy: Inherit from js::Wrapper

js::DirectProxyHandler and js::Wrapper are supposedly the same, except
that js::Wrapper can expose the object it's wrapping to C++ code. We don't
use that functionality. However, js::DirectProxyHandler is being removed
in SpiderMonkey 52 in favour of js::Wrapper, so it makes sense to switch.

Created attachment 350926 [details] [review]
system: Switch from JS::CallReceiver to JS::CallArgs

JS::CallReceiver is going away in SpiderMonkey 52. It was a version of
JS::CallArgs that didn't care about the number of arguments passed to the
function. We already use JS::CallArgs everywhere else, so this is not a
problem.

Created attachment 350927 [details] [review]
coverage: Root using context, not runtime

JSRuntime is going away in SpiderMonkey 52, so we should stop using the
constructor of JS::Rooted<T> that takes JSRuntime instead of JSContext.
There were a few instances where it was used.

Review of attachment 350844 [details] [review]:

OK

Review of attachment 350922 [details] [review]:

Nice

Review of attachment 350923 [details] [review]:

OK

Review of attachment 350925 [details] [review]:

Sure

Review of attachment 350926 [details] [review]:

Sure

Review of attachment 350927 [details] [review]:

OK

Attachment 350844 [details] pushed as 8a0bba5 - build: Autodetect SpiderMonkey's debug mode
Attachment 350922 [details] pushed as 2337884 - closure: Remove pointer to runtime
Attachment 350923 [details] pushed as 9aa4116 - context: Use GThread to determine owner thread
Attachment 350925 [details] pushed as 0982d99 - jsapi-constructor-proxy: Inherit from js::Wrapper
Attachment 350926 [details] pushed as 99dd8db - system: Switch from JS::CallReceiver to JS::CallArgs
Attachment 350927 [details] pushed as aff4b2d - coverage: Root using context, not runtime

Created attachment 353555 [details] [review]
console: Refactor read-eval-print loop

This makes the REPL look more like the code from the SpiderMonkey shell
in mozjs52. This is because in SpiderMonkey 52, error reporter callbacks
and JS_ReportPendingException() are going away. We can anticipate this
change by introducing an AutoReportException class which prints any
pending exception when it goes out of scope.

This code still uses JS_ReportPendingException(), since that saves us
from doing many things manually, but that will change when switching to
SpiderMonkey 52 which adds APIs to do the things that
JS_ReportPendingException() would have done, such as print a stack trace.

Created attachment 353593 [details] [review]
coverage: Drop support for legacy comprehensions

In SpiderMonkey 52 these will be considered syntax errors. Only the form
of array comprehensions with the expression at the end will be supported.
Switch the regression tests to test the new form rather than the old
form, and add support for the 'ComprehensionIf' Reflect.parse node type.

Created attachment 353594 [details] [review]
js: Stop using flags argument to String.replace()

This argument (e.g., str.replace('foo', 'bar', 'g')) was nonstandard and
is being removed in SpiderMonkey 52. Instead, we use a regular expression
literal, e.g. str.replace(/foo/g, 'bar').

In addition, tweak the test assertions in testGtk to test that the
provided children are neither null nor undefined, because more tests
should have failed on this.

Created attachment 353595 [details] [review]
tests: Root using context, not runtime

JSRuntime is going away in SpiderMonkey 52, so we should stop using the
constructor of JS::Rooted<T> that takes JSRuntime instead of JSContext.
There were a few instances I missed in the previous commit that fixed
this.

Review of attachment 353593 [details] [review]:

OK

Review of attachment 353594 [details] [review]:

OK

Review of attachment 353595 [details] [review]:

OK

Review of attachment 353555 [details] [review]:

::: modules/console.cpp
@@ +151,3 @@
+        JSErrorReport *report = JS_ErrorFromException(m_cx, exn);
+        if (!report) {
+    explicit AutoReportException(JSContext *cx) : m_cx(cx) {}

Should we really be guarding for this here? The documentation only mentions JS_ErrorFromException() returning nullptr when there the object passed-in is not an exception, which should never be the case here (and we don't guard for OOM elsewhere)

@@ +206,3 @@
+
+    JS::RootedValue result(cx);
+                           const char      *bytes,

You could simplify this in a single condition instead of a nested if.

@@ +223,3 @@
+    display_str = gjs_value_debug_string(cx, result);
+    if (display_str) {
+    if (!JS::Evaluate(cx, global, options, bytes, length, &result)) {

Any reason not to use g_print()?

Review of attachment 353555 [details] [review]:

::: modules/console.cpp
@@ +151,3 @@
+        JSErrorReport *report = JS_ErrorFromException(m_cx, exn);
+        if (!report) {
+    explicit AutoReportException(JSContext *cx) : m_cx(cx) {}

I did this because the SpiderMonkey code that I copied it from did it as well... However, the source (http://searchfox.org/mozilla-central/source/js/src/jsexn.cpp#405) does show that it returns nullptr after recovering from OOM as well.

It's true that we don't care about OOM elsewhere though; I'll remove it if you feel strongly about it.

Created attachment 353651 [details] [review]
console: Refactor read-eval-print loop

This makes the REPL look more like the code from the SpiderMonkey shell
in mozjs52. This is because in SpiderMonkey 52, error reporter callbacks
and JS_ReportPendingException() are going away. We can anticipate this
change by introducing an AutoReportException class which prints any
pending exception when it goes out of scope.

This code still uses JS_ReportPendingException(), since that saves us
from doing many things manually, but that will change when switching to
SpiderMonkey 52 which adds APIs to do the things that
JS_ReportPendingException() would have done, such as print a stack trace.

An update on the lexical scope problem.

> Now that we have access to the lexical scope object, the question is what to
> do with it. Ideally we would keep compatibility with existing code for now,
> while still warning clients to change their code for the future. Because any
> workaround we implement now to make the lexical scope members available,
> will certainly not continue to work under ES6 modules.
> 
> One way would be to print warnings if members show up on the lexical scope
> object after importing, but even with heuristics such as "don't warn about
> 'const Something = imports.something'" and "don't warn about members
> starting with an underscore" I really don't like this idea, because it
> actually causes more false positives for clients who _are_ trying to write
> correct ES6.
> 
> Another way would be to make a new class for module objects, with a resolve
> hook that looks for unresolved properties on the lexical scope object, and
> resolves them but prints a warning. This would be a lot of extra code but
> might be a lot cleaner anyway because you get no false positives; you only
> get a warning if you expected something declared with let/const to be
> exported, and you prove you expected it by trying to access it.

This latter idea is what I'm trying to do. I have a patch on the wip/ptomato/mozjs52 branch, but it doesn't keep full compatibility with the existing code. I need to figure out how to deal with the global object first. Unfortunately I discovered that this "module" doesn't cover all the features we need:

>     var a = 1;
>     let b = 2;
>     const c = 3;

Instead we need to consider this "module":

    var a = 1;
    let b = 2;
    const c = 3;
    this.d = 4;

In the current code, "this" (which at the toplevel scope refers to the global object) refers to the module object that is in the process of being imported. This is used e.g. in imports.cairo which does Lang.copyProperties(imports.cairoNative, this); and also extensively in the overrides modules.

Neither of the methods that I found for getting access to the lexical scope object [1] will allow the module code to be executed with the correct global object. The first method, with JS_GlobalLexicalScope(), requires the module object to also be a global object. In that case, the module object must run in its own compartment, and won't have an "imports" object available to it, among other problems. This is potentially solvable with cross-compartment wrappers, but means a huge amount of refactoring that will only become obsolete when we get ES6 modules.

The second method, js::ExecuteInGlobalAndReturnScope(), avoids that problem but does not allow a scope chain which is how we allow properties of 'this' to appear on the module object but still have access to global properties such as 'imports'. So, in the above "module" example, d would not be exported.

I have two ideas on how to proceed, but neither of them are very good.

1. Drop support for 'this' being the module object, and instead expose a global 'module' object to module code that is only present while the module is being imported. This is an API break, since modules that use 'this' will need to be updated.

2. Import the module twice: once as normal, and then import it once again using js::ExecuteInGlobalAndReturnScope() to get at the lexical scope object. This does not require breaking API but it does assume that executing the module twice will not have any side effects. The second import will also have the side effect of defining any properties that the module adds to 'this' on the _global_ global object, so they will be exposed outside the module.

Any suggestions?

[1] https://gist.github.com/ptomato/8d112afe44f2b10972b92f4c2ae9fea0

(In reply to Philip Chimento from comment #48)
I eventually went for this solution:

> 2. Import the module twice: once as normal, and then import it once again
> using js::ExecuteInGlobalAndReturnScope() to get at the lexical scope
> object. This does not require breaking API but it does assume that executing
> the module twice will not have any side effects. The second import will also
> have the side effect of defining any properties that the module adds to
> 'this' on the _global_ global object, so they will be exposed outside the
> module.

I made it so that a second global object is created for the second import, which required a lot of refactoring; but I think the refactor was positive even if we later remove the workaround. This still does have the drawback that any outside effect caused by the import -- for example, writing a file -- will still happen twice.

I will add the refactors here, and soon open a new bug for the patches on the wip/ptomato/mozjs52 branch -- it's time.

Created attachment 354434 [details] [review]
js: Use a special object for modules

This introduces a new GjsModule class for imported module objects. The
functionality is unchanged, but we will use this as a basis for making
bindings from modules' lexical scopes (variables declared with 'let' and
'const') available for compatibility. In SpiderMonkey 45, these will stop
being exported as properties, as per ES6 standard.

The function GjsModule::evaluate_import() contains duplicated code with
gjs_eval_with_scope(), but that will change when we have to import the
lexical scope separately.

Created attachment 354435 [details] [review]
js: Use autoptr in gjs_object_require_property()

We now use GjsAutoJSChar for the overload of
gjs_object_require_property() that deals with string properties. This
converts existing callers to use GjsAutoJSChar, and in fact fixes a few
instances where the string was freed with the wrong function.

Created attachment 354436 [details] [review]
jsapi-util-string: Remove useless length calculation

This was left over from using JS_EncodeStringToBuffer() in past code. It
is not used now, so it's unnecessary to calculate it.

Created attachment 354437 [details] [review]
js: Refactor global object creation

In order to more easily create global objects, we refactor code that
deals with them into gjs/global.cpp and gjs/global.h. Previously global
objects were set up partly in gjs_context_constructed() and partly in
gjs_init_context_standard(); we disentangle that code and move everything
dealing with setting up the GjsContext into gjs_context_constructed(),
while global object setup moves to gjs_create_global_object().

Since global objects all share the same root importer, we must split the
setup into two. Creating the root importer is the responsibility of
gjs_context_constructed(), and it requires that the first global has been
created. After creating the root importer, the global object is finished
with gjs_define_global_properties().

In the case of global objects beyond the first, such as the global object
for the coverage compartment, gjs_define_global_properties() will also
wrap the root importer in a cross-compartment wrapper so that the new
global can access it.

(In reply to Philip Chimento from comment #49)
> (In reply to Philip Chimento from comment #48)
> I eventually went for this solution:
> 
> > 2. Import the module twice: once as normal, and then import it once again
> > using js::ExecuteInGlobalAndReturnScope() to get at the lexical scope
> > object. This does not require breaking API but it does assume that executing
> > the module twice will not have any side effects. The second import will also
> > have the side effect of defining any properties that the module adds to
> > 'this' on the _global_ global object, so they will be exposed outside the
> > module.
> 
> I made it so that a second global object is created for the second import,
> which required a lot of refactoring; but I think the refactor was positive
> even if we later remove the workaround. This still does have the drawback
> that any outside effect caused by the import -- for example, writing a file
> -- will still happen twice.

This turns out to be very bad when the module registers a GType, which is an outside effect. The second import will fail. Two imports is therefore a dead end.

Review of attachment 354435 [details] [review]:

Looks good to me.

Review of attachment 354436 [details] [review]:

OK

Review of attachment 354434 [details] [review]:

Looks good to me.

Review of attachment 354434 [details] [review]:

I'd like to get the module import fully working on the other side, on the wip/ptomato/mozjs52 branch, before I commit this one. I wrote some more tests yesterday evening that at least should be added to this patch.

Attachment 354435 [details] pushed as 6916213 - js: Use autoptr in gjs_object_require_property()
Attachment 354436 [details] pushed as 0963b42 - jsapi-util-string: Remove useless length calculation

Created attachment 354592 [details] [review]
js: Use a special object for modules

This introduces a new GjsModule class for imported module objects. The
functionality is unchanged, but we will use this as a basis for making
bindings from modules' lexical scopes (variables declared with 'let' and
'const') available for compatibility. In SpiderMonkey 45, these will stop
being exported as properties, as per ES6 standard.

The function GjsModule::evaluate_import() contains duplicated code with
gjs_eval_with_scope(), but that will change when we have to import the
lexical scope separately.

Created attachment 354593 [details] [review]
js: Refactor global object creation

In order to more easily create global objects, we refactor code that
deals with them into gjs/global.cpp and gjs/global.h. Previously global
objects were set up partly in gjs_context_constructed() and partly in
gjs_init_context_standard(); we disentangle that code and move everything
dealing with setting up the GjsContext into gjs_context_constructed(),
while global object setup moves to gjs_create_global_object().

Since global objects all share the same root importer, we must split the
setup into two. Creating the root importer is the responsibility of
gjs_context_constructed(), and it requires that the first global has been
created. After creating the root importer, the global object is finished
with gjs_define_global_properties().

In the case of global objects beyond the first, such as the global object
for the coverage compartment, gjs_define_global_properties() will also
wrap the root importer in a cross-compartment wrapper so that the new
global can access it.

I had a conversation with Shu-yu Guo in #jsapi about the module imports problem, which resulted in https://bugzilla.mozilla.org/show_bug.cgi?id=1377016 and it looks like we can get the needed fix into SpiderMonkey ESR52.

It looks like the problem is solved with the patch on that Mozilla bug, fixing JS_ExtensibleLexicalEnvironment().

Cosimo, here's an overview of the remaining patches:

"js: Module exports use ES6 scope rules"
"js: Use a special object for modules" - Now that the lexical environment problem is solved, these two can be re-examined with the intention of committing them.

"console: Refactor read-eval-print loop" - needs re-review, I fixed up the comments from the previous round.

"js: Refactor global object creation" - This one is not required anymore, but I think it is a good refactor anyway, so worth a review.

Review of attachment 353651 [details] [review]:

::: modules/console.cpp
@@ +151,3 @@
+        JSErrorReport *report = JS_ErrorFromException(m_cx, exn);
+        if (!report) {
+    explicit AutoReportException(JSContext *cx) : m_cx(cx) {}

This looks overly defensive to me -- do we guard for OOM elsewhere in GJS?

@@ +159,3 @@
+        MOZ_ASSERT(!JSREPORT_IS_WARNING(report->flags));
+
+            return;

Should you not call gjs_console_print_error() here?

Review of attachment 353651 [details] [review]:

Thanks for the review.

::: modules/console.cpp
@@ +151,3 @@
+        JSErrorReport *report = JS_ErrorFromException(m_cx, exn);
+        if (!report) {
+    explicit AutoReportException(JSContext *cx) : m_cx(cx) {}

I copied this from the equivalent SpiderMonkey code; I guess it would be more GLib-ish to not try to recover, and instead do something like g_assert(((void) "Out of memory initializing ErrorReport", report));

@@ +159,3 @@
+        MOZ_ASSERT(!JSREPORT_IS_WARNING(report->flags));
+
+            return;

That's what I do after JS_ReportPendingException() gets removed in mozjs52 and the "message" argument is removed from the reporter callback. But for now, JS_ReportPendingException() calls gjs_console_print_error() and supplies the "message" argument. If we call gjs_console_print_error() directly then we have to extract the "message" which is a bunch more code.

Created attachment 355181 [details] [review]
console: Refactor read-eval-print loop

This makes the REPL look more like the code from the SpiderMonkey shell
in mozjs52. This is because in SpiderMonkey 52, error reporter callbacks
and JS_ReportPendingException() are going away. We can anticipate this
change by introducing an AutoReportException class which prints any
pending exception when it goes out of scope.

This code still uses JS_ReportPendingException(), since that saves us
from doing many things manually, but that will change when switching to
SpiderMonkey 52 which adds APIs to do the things that
JS_ReportPendingException() would have done, such as print a stack trace.

Created attachment 355182 [details] [review]
console: Refactor read-eval-print loop

This makes the REPL look more like the code from the SpiderMonkey shell
in mozjs52. This is because in SpiderMonkey 52, error reporter callbacks
and JS_ReportPendingException() are going away. We can anticipate this
change by introducing an AutoReportException class which prints any
pending exception when it goes out of scope.

This code still uses JS_ReportPendingException(), since that saves us
from doing many things manually, but that will change when switching to
SpiderMonkey 52 which adds APIs to do the things that
JS_ReportPendingException() would have done, such as print a stack trace.

Review of attachment 355182 [details] [review]:

Looks good

Review of attachment 354592 [details] [review]:

Looks good with a trivial comment below.

::: gjs/importer.cpp
@@ +187,3 @@
 }
 
+/* Make the property we set in gjs_module_import() permament;

permament -> permanent

Review of attachment 354593 [details] [review]:

Nice refactor, it looks good to me.

Attachment 349983 [details] pushed as fcac2be - js: Module exports use ES6 scope rules
Attachment 354592 [details] pushed as 7e88bd2 - js: Use a special object for modules
Attachment 354593 [details] pushed as 2a38e4a - js: Refactor global object creation
Attachment 355182 [details] pushed as 86a309a - console: Refactor read-eval-print loop