1476921 - Don't throw an error in GetModuleNamespace for errored modules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

https://github.com/tc39/ecma262/pull/1249 updated the spec to simply remove the erroneous assertion.

Comment 1

[Ashley Hauck [:khyperia]]

Attachment: Don't throw an error in GetModuleNamespace for errored modules.

Comment 2

[Ashley Hauck [:khyperia]]

I'm not sure how to add a test for this, advice would be appreciated. (Also, I'm not sure if the patch is the right thing to do, not sure if that throw should be deleted or kept - spec seems to imply delete, code comments imply keep).

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 9004279 [details] [diff] [review]
Don't throw an error in GetModuleNamespace for errored modules.

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

I think the spec is right and we can delete this.  Loading a module graph containing an errored module will still fail because we will re-throw the exception in the evaluation phase (InnerModuleEvaluation step 2).

To test this you could try and reproduce the example in https://github.com/tc39/ecma262/issues/1155.  If you look in jit-test/tests/modules/import-namespace.js  you will see code like:

let b = moduleRepo['b'] = parseModule(
    `import * as ns from 'a';
     export { ns };
     export var x = ns.a + ns.b;`
);

instantiateModule(b);
evaluateModule(b);

If you do a block like that for each of the modules in the example in the issue I think that will reproduce the problem.  Obviously we are expecting exceptions when we evaluate module a and module b here.

So this is basically r+ for the fix but I'd like to see test code.

Give me a shout if any of this doesn't make sense.

Comment 4

[Ashley Hauck [:khyperia]]

Attachment: Add tests to make sure GetModuleNamespace errors are correctly handled.

Comment 5

[Ashley Hauck [:khyperia]]

Comment on attachment 9004603 [details] [diff] [review]
Add tests to make sure GetModuleNamespace errors are correctly handled.

This case is a little simpler than the one in the github issue, and I'm not 100% sure what the ordering and requirements are here, but this test fails before the patch and succeeds after (and that's the only change between the two runs), so I'm calling it good.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 9004603 [details] [diff] [review]
Add tests to make sure GetModuleNamespace errors are correctly handled.

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

That looks good to me.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/873cd2e119b3
Don't throw an error in GetModuleNamespace for errored modules. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/25af0b19ec39
Add tests to make sure GetModuleNamespace errors are correctly handled. r=jonco

Comment 8

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1476921) for build bustage. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=196274288&repo=mozilla-inbound&lineNumber=78090
 /builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true
[task 2018-08-28T19:14:14.566Z] Stack:
[task 2018-08-28T19:14:14.566Z]   assertThrowsMyError@/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5
[task 2018-08-28T19:14:14.566Z]   @/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:35:1
[task 2018-08-28T19:14:14.566Z] Exit code: 3
[task 2018-08-28T19:14:14.566Z] FAIL - modules/bug1449153.js
[task 2018-08-28T19:14:14.566Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/modules/bug1449153.js | /builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true (code 3, args "--ion-eager --ion-offthread-compile=off") [0.0 s]
[task 2018-08-28T19:14:14.566Z] {"action": "test_start", "jitflags": "--ion-eager --ion-offthread-compile=off", "pid": 85299, "source": "jittests", "test": "modules/bug1449153.js", "thread": "main", "time": 1535483654.533368}
[task 2018-08-28T19:14:14.567Z] {"action": "test_end", "extra": {"jitflags": "--ion-eager --ion-offthread-compile=off", "pid": 85299}, "jitflags": "--ion-eager --ion-offthread-compile=off", "message": "/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true", "pid": 85299, "source": "jittests", "status": "FAIL", "test": "modules/bug1449153.js", "thread": "main", "time": 1535483654.56675}
[task 2018-08-28T19:14:14.567Z] INFO exit-status     : 3
[task 2018-08-28T19:14:14.567Z] INFO timed-out       : False
[task 2018-08-28T19:14:14.567Z] INFO stderr         2> /builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true
[task 2018-08-28T19:14:14.567Z] INFO stderr         2> Stack:
[task 2018-08-28T19:14:14.567Z] INFO stderr         2> assertThrowsMyError@/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5
[task 2018-08-28T19:14:14.567Z] INFO stderr         2> @/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:35:1
[task 2018-08-28T19:14:14.569Z] TEST-PASS | js/src/jit-test/tests/modules/bug1429031.js | Success (code 3, args "--no-baseline --no-ion") [0.0 s]
[task 2018-08-28T19:14:14.569Z] {"action": "test_start", "jitflags": "--no-baseline --no-ion", "pid": 85285, "source": "jittests", "test": "modules/bug1429031.js", "thread": "main", "time": 1535483654.521905}
[task 2018-08-28T19:14:14.569Z] {"action": "test_end", "extra": {"jitflags": "--no-baseline --no-ion", "pid": 85285}, "jitflags": "--no-baseline --no-ion", "message": "Success", "pid": 85285, "source": "jittests", "status": "PASS", "test": "modules/bug1429031.js", "thread": "main", "time": 1535483654.569505}
[task 2018-08-28T19:14:14.572Z] /builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true
[task 2018-08-28T19:14:14.572Z] Stack:
[task 2018-08-28T19:14:14.572Z]   assertThrowsMyError@/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5
[task 2018-08-28T19:14:14.573Z]   @/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:35:1
[task 2018-08-28T19:14:14.573Z] Exit code: 3
[task 2018-08-28T19:14:14.573Z] FAIL - modules/bug1449153.js
[task 2018-08-28T19:14:14.573Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/modules/bug1449153.js | /builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true (code 3, args "--baseline-eager") [0.0 s]
[task 2018-08-28T19:14:14.573Z] {"action": "test_start", "jitflags": "--baseline-eager", "pid": 85326, "source": "jittests", "test": "modules/bug1449153.js", "thread": "main", "time": 1535483654.542628}
[task 2018-08-28T19:14:14.573Z] {"action": "test_end", "extra": {"jitflags": "--baseline-eager", "pid": 85326}, "jitflags": "--baseline-eager", "message": "/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true", "pid": 85326, "source": "jittests", "status": "FAIL", "test": "modules/bug1449153.js", "thread": "main", "time": 1535483654.573318}
[task 2018-08-28T19:14:14.573Z] INFO exit-status     : 3
[task 2018-08-28T19:14:14.573Z] INFO timed-out       : False
[task 2018-08-28T19:14:14.573Z] INFO stderr         2> /builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true
[task 2018-08-28T19:14:14.574Z] INFO stderr         2> Stack:
[task 2018-08-28T19:14:14.574Z] INFO stderr         2> assertThrowsMyError@/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5
[task 2018-08-28T19:14:14.574Z] INFO stderr         2> @/builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:35:1
[task 2018-08-28T19:14:14.588Z] /builds/worker/workspace/build/src/js/src/jit-test/tests/modules/bug1449153.js:14:5 Error: Assertion failed: got false, expected true
[task 2018-08-28T19:14:14.588Z] Stack:

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=25af0b19ec39666d33efe54a7feca0e7cb35fb6c

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/211b070bac735ff9301f80af489a045a7f644b60

Comment 9

[Ashley Hauck [:khyperia]]

Attachment: Don't throw an error in GetModuleNamespace for errored modules.

Comment 10

[Ashley Hauck [:khyperia]]

Comment on attachment 9004689 [details] [diff] [review]
Don't throw an error in GetModuleNamespace for errored modules.

The new bit is in `js/src/jit-test/tests/modules/bug1449153.js`

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/432ffee53720
Don't throw an error in GetModuleNamespace for errored modules. r=jonco

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/432ffee53720