Module script errors maybe shouldn't propagate to descendant scripts so much #2630

domenic · 2017-05-04T16:22:42Z

Discovered by @nyaxt. Given the graph

A.js -> 404.js
       -> B.js -> C.js

(working from https://dl.dropboxusercontent.com/u/20140634/modules-all-better/index.html which has various fixes applied from in-progress PRs)

If 404.js comes back first, then during "fetch the descendants of and instantiate a module script", we proceed forward to do ModuleDeclarationInstantion(), which fails because it can't find 404.js. But then we propagate this error, marking B.js and C.js as errored.
- This is not good, because it means a future <script type="module" src="B.js"> will auto-fail, even if there's nothing wrong with that subgraph.
If B.js/C.js comes back first, they get marked as instantiated, but then that gets overridden with the error, same as above.

At first glance it seems that the mechanism of "instantiate anyway then propagate errors" is not working well, at least for fetch failures.

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

nyaxt · 2017-05-04T16:28:59Z

"that gets overridden with the error, same as above."

I don't think this (the latter black bullet) will happen, because https://html.spec.whatwg.org/multipage/webappapis.html#uninstantiated-inclusive-descendant-module-scripts will filter out module scripts not in "uninstantiated" state.

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.

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 #2629 and #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 #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

domenic mentioned this issue May 4, 2017

Don't handle impossible errors in HostResolveImportedModule #2625

Merged

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

Module script errors maybe shouldn't propagate to descendant scripts so much #2630

Module script errors maybe shouldn't propagate to descendant scripts so much #2630

domenic commented May 4, 2017

nyaxt commented May 4, 2017 •

edited

Loading

Module script errors maybe shouldn't propagate to descendant scripts so much #2630

Module script errors maybe shouldn't propagate to descendant scripts so much #2630

Comments

domenic commented May 4, 2017

nyaxt commented May 4, 2017 • edited Loading

nyaxt commented May 4, 2017 •

edited

Loading