Closed
Bug 1341298
Opened 7 years ago
Closed 7 years ago
Circular module dependency throws InternalError for unresolvable export when export* is present
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
5.70 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
main.js --- import "./m1.js" --- m1.js: --- export {} from "./m2.js" export* from "./empty.js" --- m2.js: --- export {x} from "./m1.js" --- empty.js: --- // an empty file --- STR: Run with `mozjs -m ./main.js` Expected: Throws SyntaxError Actual: Throws InternalError: --- shell/ModuleLoader.js:25:9 InternalError: module record in unexpected state Stack: Reflect.Loader<@shell/ModuleLoader.js:25:9 ---
Assignee | ||
Comment 1•7 years ago
|
||
When circular module dependencies are present, ModuleResolveExport may resolve modules which aren't yet instantiated, so we need to lower the expected module state from MODULE_STATE_INSTANTIATED to MODULE_STATE_PARSED. Lowering the expected module state led to a null-pointer crash in js/src/jit-test/tests/modules/bug-1287410.js, because the module-environment slot is null when the module isn't yet instantiated. Therefore I've changed ModuleEnvironmentObject::createImportBinding() to call ModuleObject::initialEnvironment() instead of ModuleObject::environment(). This solved the crash, but the test case no longer threw the expected InternalError. To get this right again, I've added an additional module state check in ModuleDeclarationInstantiation before calling CreateImportBinding.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8840455 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 2•7 years ago
|
||
The patch applies on top of bug 1341411.
Comment 3•7 years ago
|
||
Comment on attachment 8840455 [details] [diff] [review] bug1341298.patch Review of attachment 8840455 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Module.js @@ +205,5 @@ > > function RecordInstantationFailure(module) > { > + // Set the module's state to 'failed' to indicate a failed module > + // instantiation and reset the environment slot to 'undefined'. Thanks for fixing the comment.
Attachment #8840455 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 4•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=164805ab8b74b65be04851dc81f778da785ce3d0
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e74895212a Relax expected module state when resolving modules and circular module dependencies are present. r=jonco
Keywords: checkin-needed
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c1e74895212a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•