Circular module dependency throws InternalError for unresolvable export when export* is present

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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

a year ago
Created attachment 8840455 [details] [diff] [review]
bug1341298.patch

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

a year ago
The patch applies on top of bug 1341411.
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+

Comment 5

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c1e74895212a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

11 months ago
See Also: → bug 1340304
You need to log in before you can comment on or make changes to this bug.