Closed Bug 1420420 Opened 2 years ago Closed 2 years ago

Update module implementation to latest spec regarding instantiation error handling

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Priority: -- → P3
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.
Attachment #8932457 - Flags: review?(andrebargull)
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.
Attachment #8932458 - Flags: review?(amarchesini)
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.
Attachment #8932459 - Flags: review?(james)
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.
Attachment #8932459 - Flags: review?(james) → review+
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
...
---
Attachment #8932457 - Flags: review?(andrebargull) → review-
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.
Attachment #8932457 - Attachment is obsolete: true
Attachment #8932886 - Flags: review?(andrebargull)
Attachment #8932886 - Attachment is patch: true
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  ()
...
---
Attachment #8932886 - Flags: review?(andrebargull) → review-
I updated the patch to remove the assert when creating namespace bindings too.
Attachment #8932886 - Attachment is obsolete: true
Attachment #8932950 - Flags: review?(andrebargull)
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
---
Attachment #8932950 - Flags: review?(andrebargull) → review-
(In reply to André Bargull [:anba] from comment #9)
Thanks for the thorough review!  TIL you can throw undefined.
I added a MODULE_STATUS_EVALUATED_ERROR state and addressed the other problems.
Attachment #8932950 - Attachment is obsolete: true
Attachment #8933320 - Flags: review?(andrebargull)
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.
Attachment #8933320 - Flags: review?(andrebargull) → review+
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.
Attachment #8932458 - Flags: review?(amarchesini) → review+
(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.
Pushed by jcoppeard@mozilla.com:
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
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
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7a1ca2738093
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/457b0fe91e0d
followup - sidestep a rooting hazard. r=me a=hazad-fix
Duplicate of this bug: 1393962
Duplicate of this bug: 1340304
Duplicate of this bug: 1364128
You need to log in before you can comment on or make changes to this bug.