1420420 - Update module implementation to latest spec regarding instantiation error handling

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jon Coppeard (:jonco)]

The module spec changed again to not remember instantiation errors:

https://github.com/tc39/ecma262/pull/1006
https://github.com/whatwg/html/pull/2991

This fixes more problems with module graph fetching error handling.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1420420-dont-record-instantiation-errors

Update implementation in line with https://github.com/tc39/ecma262/pull/1006 so we don't record errors while instantiating modules.  This removes the module 'errored' state and the JS::IsModuleErrored and JS::GetModuleError APIs.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1420420-dom-changes

Update module error handling in line with https://github.com/whatwg/html/pull/2991.  This removes ModuleScript::IsErrored and gives ModuleScript two optional errors fields: ParseError and ErrorToRethrow.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1420420-disable-tests

This patch sets up some expected test failures with these changes.  This is because the tests don't match the latest spec (see https://github.com/w3c/web-platform-tests/issues/7747 / bug 1420925).

Specifically the tests expect repeated instantiation failure for a module to always throw the same error object, but according to the spec that doesn't happen any more.

Comment 4

[James Graham [:jgraham]]

Comment on attachment 8932459 [details] [diff] [review]
bug1420420-disable-tests

Review of attachment 8932459 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, although another option would be to just fix the tests here, and the two-way sync will deal with upstreaming your changes.

Comment 5

[André Bargull [:anba]]

Comment on attachment 8932457 [details] [diff] [review]
bug1420420-dont-record-instantiation-errors

Review of attachment 8932457 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't yet reviewed the patch in detail, but r- for now because the current patch asserts in this test case:
---
let moduleRepo = {};
setModuleResolveHook(function(module, specifier) {
    if (specifier in moduleRepo)
        return moduleRepo[specifier];
    throw "Module '" + specifier + "' not found";
});

moduleRepo["good"] = parseModule(`export let x`);

moduleRepo["y1"] = parseModule(`export let y`);
moduleRepo["y2"] = parseModule(`export let y`);
moduleRepo["bad"] = parseModule(`export* from "y1"; export* from "y2";`);

moduleRepo["a"] = parseModule(`import {x} from "good"; import {y} from "bad";`);

let b = moduleRepo["b"] = parseModule(`import "a";`);
let c = moduleRepo["c"] = parseModule(`import "a";`);

try { b.declarationInstantiation(); } catch (e) { print("caught:", e); }
try { c.declarationInstantiation(); } catch (e) { print("caught:", e); }
---

Asserts with:
---
0x0000000000592bd8 in js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::putNewInfallible<JS::Handle<jsid>&, js::IndirectBindingMap::Binding>(jsid const&, JS::Handle<jsid>&, js::IndirectBindingMap::Binding&&) (l=..., this=0x7ffff3f344c0)
    at /home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/js/HashTable.h:1838
1838            MOZ_ASSERT(!lookup(l).found());
---

Stacktrace:
---
#0  0x0000000000592bd8 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (l=..., this=0x7ffff3f344c0)
    at /home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/js/HashTable.h:1838
#1  0x0000000000592bd8 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (l=..., this=0x7ffff3f344c0)
    at /home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/js/HashTable.h:1857
#2  0x0000000000592bd8 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (v=<unknown type in /home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/js/src/shell/js, CU 0xe2a2a9, DIE 0x107713c>, k=<synthetischer Zeiger>, this=0x7ffff3f344c0)
    at /home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/js/HashTable.h:253
#3  0x0000000000592bd8 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (this=0x7ffff3f344c0, cx=cx@entry=0x7ffff5616000, name=..., name@entry=$jsid("x"), environment=..., 
    environment@entry=(js::ModuleEnvironmentObject * const) 0x7ffff4299080 [object ModuleEnvironmentObject] delegate, localName=..., localName@entry=$jsid("x"))
    at /home/andre/hg/mozilla-inbound/js/src/builtin/ModuleObject.cpp:349
#4  0x0000000000b1e667 in js::ModuleEnvironmentObject::createImportBinding(JSContext*, JS::Handle<JSAtom*>, JS::Handle<js::ModuleObject*>, JS::Handle<JSAtom*>) (this=(js::ModuleEnvironmentObject * const) 0x7ffff428d190 [object ModuleEnvironmentObject] delegate, cx=0x7ffff5616000, importName=..., 
    importName@entry="x", module=..., 
    module@entry=(js::ModuleObject * const) 0x7ffff428c1a0 [object Module], localName=..., 
    localName@entry="x") at /home/andre/hg/mozilla-inbound/js/src/vm/EnvironmentObject.cpp:496
#5  0x0000000000c392f9 in intrinsic_CreateImportBinding(JSContext*, unsigned int, JS::Value*) (cx=0x7ffff5616000, argc=<optimised out>, vp=<optimised out>)
    at /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:2056
...
---

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1420420-dont-record-instantiation-errors v2

Well spotted!

We allow re-instantiation of previously failed modules now so we can't assert that environment bindings do not already present as we did previously.

I added your testcase as a test.

Comment 7

[André Bargull [:anba]]

Comment on attachment 8932886 [details] [diff] [review]
bug1420420-dont-record-instantiation-errors v2

Review of attachment 8932886 [details] [diff] [review]:
-----------------------------------------------------------------

This slightly modified test case also asserts:
---
let moduleRepo = {};
setModuleResolveHook(function(module, specifier) {
    if (specifier in moduleRepo)
        return moduleRepo[specifier];
    throw "Module '" + specifier + "' not found";
});

moduleRepo["good"] = parseModule(`export let x`);

moduleRepo["y1"] = parseModule(`export let y`);
moduleRepo["y2"] = parseModule(`export let y`);
moduleRepo["bad"] = parseModule(`export* from "y1"; export* from "y2";`);

moduleRepo["a"] = parseModule(`import* as ns from "good"; import {y} from "bad";`);

let b = moduleRepo["b"] = parseModule(`import "a";`);
let c = moduleRepo["c"] = parseModule(`import "a";`);

try { b.declarationInstantiation(); } catch (e) { print("caught:", e); }
try { c.declarationInstantiation(); } catch (e) { print("caught:", e); }
---

Asserts with:
---
Assertion failure: environment->getSlot(shape->slot()).isMagic(JS_UNINITIALIZED_LEXICAL), at /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:2075
---

Stack trace:
---
#0  0x0000000000c514f7 in intrinsic_CreateNamespaceBinding(JSContext*, unsigned int, JS::Value*) (cx=0x7ffff5616000, argc=<optimised out>, vp=<optimised out>)
    at /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:2075
#1  0x00003a65815ef421 in  ()
...
---

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1420420-dont-record-instantiation-errors v3

I updated the patch to remove the assert when creating namespace bindings too.

Comment 9

[André Bargull [:anba]]

Comment on attachment 8932950 [details] [diff] [review]
bug1420420-dont-record-instantiation-errors v3

Review of attachment 8932950 [details] [diff] [review]:
-----------------------------------------------------------------

Only r- because of the |undefined| error value issue. Fixing that issue probably requires either adding a new MODULE_STATUS_EVALUATED_ERROR state or, which could be simpler and avoids some extra non-spec steps, wrapping the thrown value in an object. Basically something like this (untested!):
---
diff --git a/js/src/builtin/Module.js b/js/src/builtin/Module.js
--- a/js/src/builtin/Module.js
+++ b/js/src/builtin/Module.js
@@ -489,12 +489,9 @@ function RecordModuleEvaluationError(mod
            "Non-module passed to RecordModuleEvaluationError");
 
-    if (module.status === MODULE_STATUS_EVALUATED) {
-        assert(SameValue(GetModuleEvaluationError(module), error),
-               "Attempt to set different error on evaluated module");
+    if (module.status === MODULE_STATUS_EVALUATED)
         return;
-    }
 
     ModuleSetStatus(module, MODULE_STATUS_EVALUATED);
-    UnsafeSetReservedSlot(module, MODULE_OBJECT_EVALUATION_ERROR_SLOT, error);
+    UnsafeSetReservedSlot(module, MODULE_OBJECT_EVALUATION_ERROR_SLOT, {value: error});
 }
 
@@ -535,6 +532,4 @@ function ModuleEvaluate()
         assert(module.status === MODULE_STATUS_EVALUATED,
                "Bad module status after failed evaluation");
-        assert(SameValue(GetModuleEvaluationError(module), error),
-               "Module has different error set after failed evaluation");
 
         throw error;
@@ -562,5 +557,5 @@ function InnerModuleEvaluation(module, s
         if (typeof(error) === "undefined")
             return index;
-        throw error;
+        throw error.value;
     }
---

::: js/src/builtin/Module.js
@@ +173,4 @@
>  
> +function IsResolvedBinding(resolution)
> +{
> +    assert(resolution === "ambiguous" || typeof(resolution) === "object",

Nit: Other code in this file doesn't parenthesise the |typeof| operand.

Also:
typeof(null) === "object": It's not a bug, it's a feature! :-)

@@ +174,5 @@
> +function IsResolvedBinding(resolution)
> +{
> +    assert(resolution === "ambiguous" || typeof(resolution) === "object",
> +           "Bad module resolution result");
> +    return resolution !== null && typeof(resolution) == "object";

Nit: /==/===/

@@ +189,3 @@
>             "Bad module state in GetModuleNamespace");
>  
>      // Step 3

The actual step 3 is missing.

@@ +232,5 @@
>      assert(IsModule(module), "Non-module passed to GetModuleEnvironment");
>  
>      // Check for a previous failed attempt to instantiate this module. This can
>      // only happen due to a bug in the module loader.
> +    if (module.error)

/error/evaluationError/ ?

And similar to the issue noted in ModuleObject::hadEvaluationError(), |evaluationError| can be a falsy value.

@@ +264,5 @@
>      }
>      return false;
>  }
>  
> +function HandleModuleInstantiationFailure(module)

Do you think it's useful/necessary to explain why |m.[[Environment]]| is not reset in our implementation?

@@ +297,5 @@
>      } catch (error) {
>          for (let i = 0; i < stack.length; i++) {
>              let m = stack[i];
> +            assert(m.status === MODULE_STATUS_INSTANTIATING,
> +                   "Expected instantating status during failed instantiation");

Typo: instantating -> instantiating

@@ +451,1 @@
>      assert(module.status === MODULE_STATUS_UNINSTANTIATED ||

Is the |MODULE_STATUS_UNINSTANTIATED| case still possible?

@@ +489,5 @@
> +           "Non-module passed to RecordModuleEvaluationError");
> +
> +    if (module.status === MODULE_STATUS_EVALUATED) {
> +        assert(SameValue(GetModuleEvaluationError(module), error),
> +               "Attempt to set different error on evaluated module");

---
let a = parseModule(`throw new Error`);
a.declarationInstantiation();
stackTest(function() {
    a.evaluation();
});
---

Leads to this assertion error:
---
Self-hosted JavaScript assertion info: "/home/andre/hg/mozilla-inbound/js/src/builtin/Module.js:493: Attempt to set different error on evaluated module"
Assertion failure: false, at /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:411
---

@@ +560,1 @@
>      {

Nit: { on same line as |if|

@@ +560,3 @@
>      {
> +        let error = GetModuleEvaluationError(module);
> +        if (typeof(error) === "undefined")

And another place where |undefined| as the thrown error value is a problem.

Nit: Other code in this file doesn't parenthesise the |typeof| operand.

::: js/src/builtin/ModuleObject.cpp
@@ +976,3 @@
>  {
> +    return status() == MODULE_STATUS_EVALUATED &&
> +           !getReservedSlot(EvaluationErrorSlot).isUndefined();

Kind of pre-existing:

The saved "error" can be any value, including |undefined|. So testing the slot against |undefined| is not correct when |undefined| itself was thrown.

---
let moduleRepo = {};
setModuleResolveHook(function(module, specifier) {
    if (specifier in moduleRepo)
        return moduleRepo[specifier];
    throw "Module '" + specifier + "' not found";
});

moduleRepo["a"] = parseModule(`throw undefined`);

let b = moduleRepo["b"] = parseModule(`import "a";`);
let c = moduleRepo["c"] = parseModule(`import "a";`);

b.declarationInstantiation();
c.declarationInstantiation();

try { b.evaluation() } catch (e) { print("caught:", e); }
try { c.evaluation() } catch (e) { print("caught:", e); }
---

Should print "caught: undefined" two times, but only prints it once with the current patch.


The behaviour before this patch was also buggy:
---
caught: undefined
caught: InternalError: module record has unexpected status
---

Comment 10

[Jon Coppeard (:jonco)]

(In reply to André Bargull [:anba] from comment #9)
Thanks for the thorough review!  TIL you can throw undefined.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: bug1420420-dont-record-instantiation-errors v4

I added a MODULE_STATUS_EVALUATED_ERROR state and addressed the other problems.

Comment 12

[André Bargull [:anba]]

Comment on attachment 8933320 [details] [diff] [review]
bug1420420-dont-record-instantiation-errors v4

Review of attachment 8933320 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!

::: js/src/builtin/Module.js
@@ +185,5 @@
>  {
>      // Step 1
>      assert(IsModule(module), "GetModuleNamespace called with non-module");
>  
> +    // Steps 2 & 3

Uber-nit: "Steps 2-3"

@@ +454,2 @@
>  {
> +    assert(/*module.status === MODULE_STATUS_UNINSTANTIATED ||*/

Can we remove the commented out code?

@@ +572,4 @@
>  
>      // Step 4
> +    if (module.status !== MODULE_STATUS_INSTANTIATED)
> +        DumpMessage("Bad module status in InnerModuleEvaluation: " + module.status);

Left-over debugging message?

::: js/src/vm/EnvironmentObject.cpp
@@ +497,1 @@
>          ReportOutOfMemory(cx);

Pre-existing: |ReportOutOfMemory(cx)| is not needed here, because IndirectBindingMap::put(...) already calls ReportOutOfMemory.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8932458 [details] [diff] [review]
bug1420420-dom-changes

Review of attachment 8932458 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/script/ScriptLoader.cpp
@@ +840,5 @@
>  {
>    LOG(("ScriptLoadRequest (%p): Check dependencies loaded", aRequest));
>  
>    RefPtr<ModuleScript> moduleScript = aRequest->mModuleScript;
> +  if (!moduleScript && moduleScript->HasParseError()) {

here you want ||

@@ +876,5 @@
>    }
>  }
>  
> +JS::Value
> +ScriptLoader::FindFirstParseError(ModuleLoadRequest* aRequest)

Don't we need to root this value?

@@ +908,5 @@
>  
>    ModuleScript* moduleScript = aRequest->mModuleScript;
>    MOZ_ASSERT(moduleScript);
> +
> +  JS::Value parseError = FindFirstParseError(aRequest);

same here.

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Andrea Marchesini [:baku] from comment #13)
> here you want ||

Ugh, yeah.

> Don't we need to root this value?

We don't strictly need to since nothing that can GC happens before the last use of this variable.  It would be more usual to root this but it ends up being a pain getting a JSContext at this point because we may not have a JSObject to pass to AutoJSAPI::Init.

Having said that, I'm going to run the rooting analysis on try before I push this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92ad856a4bae
Update module implementation to match latest spec regarding handling of instantiation errors r=anba r=baku r=jgraham

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing tests/jit-test/jit-test/tests/modules/bug-1402649.js after asserting:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7dc46a3a5c913dd9d251c3c58f26cc7d77df0bf

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=92ad856a4bae40df9faf7aba278cdfbf8b32edd5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=150213651&repo=mozilla-inbound

[task 2017-12-06T15:41:07.179Z] 15:41:07     INFO -  TEST-PASS | tests/jit-test/jit-test/tests/modules/bug-1402535.js | Success (code 0, args "--no-baseline --no-ion") [0.2 s]
[task 2017-12-06T15:41:07.181Z] 15:41:07     INFO -  {"action": "test_start", "pid": 944, "source": "jittests", "test": "modules/bug-1402535.js", "thread": "main", "time": 1512574866.943696}
[task 2017-12-06T15:41:07.182Z] 15:41:07     INFO -  {"action": "test_end", "extra": {"jitflags": "--no-baseline --no-ion"}, "message": "Success", "pid": 944, "source": "jittests", "status": "PASS", "test": "modules/bug-1402535.js", "thread": "main", "time": 1512574867.174552}
[task 2017-12-06T15:41:11.213Z] 15:41:11     INFO -  Self-hosted JavaScript assertion info: "/builds/worker/workspace/build/src/js/src/builtin/Module.js:78: New module status inconsistent with current status"
[task 2017-12-06T15:41:11.215Z] 15:41:11     INFO -  Assertion failure: false, at /builds/worker/workspace/build/src/js/src/vm/SelfHosting.cpp:411
[task 2017-12-06T15:41:11.215Z] 15:41:11     INFO -  Exit code: -11
[task 2017-12-06T15:41:11.215Z] 15:41:11     INFO -  FAIL - modules/bug-1402649.js

Comment 17

[Jon Coppeard (:jonco)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #16)
It turns out that under OOM conditions we can fail module instantiation even after a module has been marked as instantiated.  I'm going to update the assert to allow this and add a comment.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1ca2738093
Update module implementation to match latest spec regarding handling of instantiation errors r=anba r=baku r=jgraham

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7a1ca2738093

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/457b0fe91e0d
followup - sidestep a rooting hazard. r=me a=hazad-fix

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/457b0fe91e0d