Circular module graphs instantiate too early #2629

domenic · 2017-05-04T16:01:15Z

@jonco3 found this in https://bugzilla.mozilla.org/show_bug.cgi?id=1361988. Given the module graph

      test2
        ^
        |
  .-> test1 --.
  |           |
  '-- test3 <-'

(with test1 as the root)

it appears we instantiate subgraphs too early, causing assertion failures in HostResolveImportedModule.

We probably need to move all instantiation to the "top level", instead of doing it on subgraphs as we go.

I don't believe this is fixed by any of the in-flight PRs (#2595, #2604, #2625).

The text was updated successfully, but these errors were encountered:

domenic · 2017-05-04T16:10:03Z

@jonco3 points out in IRC that instantiate was originally top-level, but that was changed for reasons discussed in #1545. Instead he's thinking maybe we can solve this by marking modules that are part of a cycle and not instantiating them eagarly, using the ancestor list.

There are several different ways things can go wrong with <script type=module>. In order from earliest to latest, for a single module script, they are: - Fetching failures - Parsing failures - Invalid module specifiers - Instantiation failures - Evaluation failures This tweaks the way that these errors interact, to ensure that fetching failures are treated one way, causing the <script>'s "error" event to fire, and the other failures are uniformly treated as errors running the script, causing the global's "error" event to fire. This also makes it clear that when fetching descendant module scripts, you can bail out early if one of them fails to fetch or has previously been discovered to be errored. Evaluation failures are particularly tricky for dependencies, as usually ModuleEvaluation() is a no-op if it has happened previously (either successfully or not). This is discussed in more depth in tc39/ecma262#862. To work around this, we need to store the evaluation error, if one occurs, similar to what we already do for instantiation errors. Fixes #2567. However, there are still problems with this setup, which may need further infrastructure changes; see: - #2595 (comment) - #2629 - #2630 But for now the improvement given by this commit is enough to merge it.

This addresses tc39#862 by ensuring that repeated calls to ModuleDeclarationInstantiation() and ModuleEvaluation(), for Source Text Module Records, rethrow any exceptions they previously threw, instead of silently succeeding. This allows host environments to do top-down instantiation/evaluation of module graphs, instead of having to do bottom-up instantiation/evaluation in order to record individual failures and thus prevent future instantiation/evaluation. In the process, this helps formalize some of the invariants previously stated in a vague way, such as "ModuleDeclarationInstantiation must have completed successfully", replacing them instead with an explicit [[Status]] field whose contents can be asserted against. For background on the trouble caused by the previous approach of silent success, see: - whatwg/html#1545 - tc39#862 - whatwg/html#2629

This builds on tc39/ecma262#916, in which the JavaScript specification remembers errors during module instantiation and evaluation. That allows us to stop our error-prone approach of doing bottom-up instantiation (which currently fails for cyclic graphs; see #2629), and obviates the consequent need to propagate errors (which is also buggy; see #2630). Finally, it makes certain edge cases during evaluation nicer; see #2595 (comment). For background on why we originally went with a bottom-up approach, see #1545. At the time we didn't seem to believe changing the JavaScript spec was an option, but it's become increasingly clear it's the most reasonable one. Closes #2629. Closes #2630.

This closes #862 by ensuring that repeated calls to ModuleDeclarationInstantiation() and ModuleEvaluation(), for Source Text Module Records, rethrow any exceptions they previously threw, instead of silently succeeding. This clarifies how host environments can do top-down instantiation/evaluation of module graphs, instead of having to do bottom-up instantiation/evaluation in order to record individual failures and thus prevent future instantiation/evaluation. For background on the trouble caused by the previous approach of silent success, see: - whatwg/html#1545 - #862 - whatwg/html#2629 - whatwg/html#2595 (comment) In the process, this helps formalize some of the invariants previously stated in a vague way, such as "ModuleDeclarationInstantiation must have completed successfully", replacing them instead with an explicit [[Status]] field whose contents can be asserted against. It also clarifies the relationship of Module Records that are not Source Text Module Records with respect to these invariants; in brief, they are expected to be leaves in the graph, with no descendants. Finally, it also updates the machinery involved in module instantiation and evaluation, first by renaming the ModuleDeclarationInstantion() and ModuleEvaluation() abstract methods to Instantiate() and Evaluate(), and also by documenting all of the abstract operations and methods involved. This includes non-normative prose containing example Source Text Module Record graphs and how they are processed. Closes #916.

This builds on tc39/ecma262#916, in which the JavaScript specification remembers errors during module instantiation and evaluation. That allows us to stop our error-prone approach of doing bottom-up instantiation (which currently fails for cyclic graphs; see #2629), and obviates the consequent need to propagate errors (which is also buggy; see #2630). Finally, it makes certain edge cases during evaluation nicer; see #2595 (comment). For background on why we originally went with a bottom-up approach, see #1545. At the time we didn't seem to believe changing the JavaScript spec was an option, but it's become increasingly clear it's the most reasonable one. Closes #2629. Closes #2630.

This builds on tc39/ecma262#916, in which the JavaScript specification remembers errors during module instantiation and evaluation. That allows us to stop our error-prone approach of doing bottom-up instantiation (which currently fails for cyclic graphs; see #2629), and obviates the consequent need to propagate errors (which is also buggy; see #2630). Finally, it makes certain edge cases during evaluation nicer; see #2595 (comment). For background on why we originally went with a bottom-up approach, see #1545. At the time we didn't seem to believe changing the JavaScript spec was an option, but since then we've learned that doing so is the most reasonable way to go. There may be more work to do here in certain error-related edge cases, or to make potential optimizations more obvious. But now at least the setup is implementable and reasonable. Closes #2629. Closes #2630.

There are several different ways things can go wrong with <script type=module>. In order from earliest to latest, for a single module script, they are: - Fetching failures - Parsing failures - Invalid module specifiers - Instantiation failures - Evaluation failures This tweaks the way that these errors interact, to ensure that fetching failures are treated one way, causing the <script>'s "error" event to fire, and the other failures are uniformly treated as errors running the script, causing the global's "error" event to fire. This also makes it clear that when fetching descendant module scripts, you can bail out early if one of them fails to fetch or has previously been discovered to be errored. Evaluation failures are particularly tricky for dependencies, as usually ModuleEvaluation() is a no-op if it has happened previously (either successfully or not). This is discussed in more depth in tc39/ecma262#862. To work around this, we need to store the evaluation error, if one occurs, similar to what we already do for instantiation errors. Fixes whatwg#2567. However, there are still problems with this setup, which may need further infrastructure changes; see: - whatwg#2595 (comment) - whatwg#2629 - whatwg#2630 But for now the improvement given by this commit is enough to merge it.

Several of the cases that were handled as errors are actually impossible to hit. Instead of throwing errors in these impossible situations, we can just assert that they are impossible. This also cleans up "resolve a module specifier" and the module map to be clear that they operate on URL records, not on absolute URL strings. Note that the remaining error case, of a null result in the module map, may go away, depending on how we fix whatwg#2629 and whatwg#2630. It currently occurs because of our strategy of calling ModuleDeclarationInstantiation() even when we know it will fail, in order to propagate errors, but that strategy is seeming increasingly unwise in the face of those bugs.

This builds on tc39/ecma262#916, in which the JavaScript specification remembers errors during module instantiation and evaluation. That allows us to stop our error-prone approach of doing bottom-up instantiation (which currently fails for cyclic graphs; see whatwg#2629), and obviates the consequent need to propagate errors (which is also buggy; see whatwg#2630). Finally, it makes certain edge cases during evaluation nicer; see whatwg#2595 (comment). For background on why we originally went with a bottom-up approach, see whatwg#1545. At the time we didn't seem to believe changing the JavaScript spec was an option, but since then we've learned that doing so is the most reasonable way to go. There may be more work to do here in certain error-related edge cases, or to make potential optimizations more obvious. But now at least the setup is implementable and reasonable. Closes whatwg#2629. Closes whatwg#2630.

domenic added the topic: script label May 4, 2017

domenic self-assigned this May 4, 2017

This was referenced May 4, 2017

Don't handle impossible errors in HostResolveImportedModule #2625

Merged

Importing modules which failed evaluation previously tc39/ecma262#862

Closed

domenic closed this as completed in 616df18 May 12, 2017

domenic reopened this May 12, 2017

domenic mentioned this issue May 13, 2017

Store and rethrow module instantiation/evaluation errors tc39/ecma262#916

Closed

domenic mentioned this issue May 13, 2017

Rely on the JavaScript spec to handle more module errors #2674

Merged

domenic closed this as completed in #2674 Aug 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circular module graphs instantiate too early #2629

Circular module graphs instantiate too early #2629

domenic commented May 4, 2017 •

edited

Loading

domenic commented May 4, 2017

Circular module graphs instantiate too early #2629

Circular module graphs instantiate too early #2629

Comments

domenic commented May 4, 2017 • edited Loading

domenic commented May 4, 2017

domenic commented May 4, 2017 •

edited

Loading