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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
---
Attached patch bug1341298.patchSplinter Review
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/c1e74895212a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
See Also: → 1340304
You need to log in before you can comment on or make changes to this bug.