Don't throw an error in GetModuleNamespace for errored modules

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: anba, Assigned: khyperia)

Tracking

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

https://github.com/tc39/ecma262/pull/1249 updated the spec to simply remove the erroneous assertion.
Assignee: nobody → khyperia
Status: NEW → ASSIGNED
Attachment #9004279 - Flags: review?(jcoppeard)
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 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.
Attachment #9004279 - Flags: review?(jcoppeard) → feedback+
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.
Attachment #9004603 - Flags: review?(jcoppeard)
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.
Attachment #9004603 - Flags: review?(jcoppeard) → review+
Attachment #9004279 - Flags: review+
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(khyperia)
Attachment #9004279 - Attachment is obsolete: true
Flags: needinfo?(khyperia)
Attachment #9004603 - Attachment is obsolete: true
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`
Attachment #9004689 - Flags: review?(jcoppeard)
Attachment #9004689 - Flags: review?(jcoppeard) → review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/432ffee53720
Don't throw an error in GetModuleNamespace for errored modules. r=jonco
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/432ffee53720
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.