Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error cases of <script type=module> #2595

Merged
merged 1 commit into from
May 12, 2017
Merged

Fix error cases of <script type=module> #2595

merged 1 commit into from
May 12, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 26, 2017

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 (and still firing the <script>'s "load" event).

Fixes #2567.


I have a separate document that walks through 7 scenarios both for a module and for its dependent, and shows that they give the expected results.

Tests at https://codereview.chromium.org/2845983002


Preview of this, #2604, and #2625 together at https://dl.dropboxusercontent.com/u/20140634/modules-all-better/index.html

/cc @whatwg/modules

@domenic
Copy link
Member Author

domenic commented Apr 27, 2017

Upon reviewing outstanding emails it seems there is another case I have not really taken care of here, which is this:

  • A depends on B, C, D
  • B and C are already in the map, as errored module scripts (not null)
  • We would like the spec to not require fetching/waiting for D

However with both the current spec and the current draft in this PR, waiting for D is required, because you are only allowed to abort "fetch the descendants of a module script" early if you get back null, not if you get back an errored module script.

This should be a pretty simple fix.

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.
@domenic
Copy link
Member Author

domenic commented May 3, 2017

I've updated this to also store evaluation errors, so they are treated more uniformly. See tc39/ecma262#862 (comment). I will also update the doc.

@domenic
Copy link
Member Author

domenic commented May 4, 2017

Further discussion with @GeorgNeis: this fix for storing evaluation errors is not great. It stores evaluation errors the same as it stores instantiation errors, so they prevent all future evaluation. Whereas intuitively, I think it would be better if stored evaluation errors "made the import statement throw", but still executed code before that point. (However: since import statements are hoisted, the only "code before that point" that could possibly matter is other import statements.) Example:

(This example has been edited to be actually correct; please disregard any earlier version you got in your email.)

// top.js
import "./dep.js";
// dep.js
import "./throws.js";
// dep2.js
console.log("dep2.js");
// top2.js
import "./dep2.js";
import "./throws.js";
// throws.js
throw new Error("boo");
<script type=module src=top.js></script>
<script type=module src=top2.js></script>

Expected:

<exception thrown: boo>

dep2.js
<exception thrown: boo>

Actual:

<exception thrown: boo>

<exception thrown: boo>

That is, top2.js never even evaluates, and thus neither does dep2.js, whereas we'd expect it to evaluate, and then (figuratively at least) have import "./throws.js" re-throw the error.

I'm not sure how to accomplish these semantics without surgery to ECMAScript, having it remember and re-throw the error as discussed in tc39/ecma262#862.

I think for now we should continue to land this PR and tests stemming from it, but note this as a potential future improvement.

Copy link

@GeorgNeis GeorgNeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubberstamp lgtm to unblock larger revisions

@domenic domenic merged commit 1157631 into master May 12, 2017
@domenic domenic deleted the module-error-cases branch May 12, 2017 16:50
domenic added a commit that referenced this pull request May 13, 2017
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.
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 9, 2017
…updates

This CL updates Blink's impl of #internal-module-script-graph-fetching-procedure
and #fetch-the-descendants-of-and-instantiate-a-module-script to match
recent spec change: whatwg/html#2595 .

Tests: external/wpt/html/semantics/scripting-1/the-script-element/module/specifier-error.html
Bug: 727299, 594639
Change-Id: I022b3b380b408a6d5c75a59d161aea4fe2868f48
Reviewed-on: https://chromium-review.googlesource.com/528724
Commit-Queue: Kouhei Ueno <[email protected]>
Cr-Commit-Position: refs/heads/master@{#478195}
WPT-Export-Revision: 3daf3d44e29dea96cf38336f8db32884a9928db5
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 9, 2017
…updates

This CL updates Blink's impl of #internal-module-script-graph-fetching-procedure
and #fetch-the-descendants-of-and-instantiate-a-module-script to match
recent spec change: whatwg/html#2595 .

Tests: external/wpt/html/semantics/scripting-1/the-script-element/module/specifier-error.html
Bug: 727299, 594639
Change-Id: I022b3b380b408a6d5c75a59d161aea4fe2868f48
Reviewed-on: https://chromium-review.googlesource.com/528724
Commit-Queue: Kouhei Ueno <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Cr-Commit-Position: refs/heads/master@{#478219}
WPT-Export-Revision: a1fd48a745a56045e25c3365a69f3fce0358074f
MXEBot pushed a commit to mirror/chromium that referenced this pull request Jun 10, 2017
…ion error

This CL updates Blink's impl of #creating-a-module-script to match
recent spec change: whatwg/html#2595 .

Before this CL, ModuleScript::Create, the Blink's impl of #creating-a-module-script reported the compilation error to console.
After this CL, ModuleScript::Create records module script compilation error
to ModuleScript::error_, and stops reporting the error to console.

Tests: external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-*.html
Bug: 727299, 594639
Change-Id: Iad166780aa93db491abe1c5185fb2da07b61b8f0
Reviewed-on: https://chromium-review.googlesource.com/526555
Commit-Queue: Kouhei Ueno <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/master@{#478207}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 12, 2017
…updates

This CL updates Blink's impl of #internal-module-script-graph-fetching-procedure
and #fetch-the-descendants-of-and-instantiate-a-module-script to match
recent spec change: whatwg/html#2595 .

Tests: external/wpt/html/semantics/scripting-1/the-script-element/module/specifier-error.html
Bug: 727299, 594639
Change-Id: I022b3b380b408a6d5c75a59d161aea4fe2868f48
Reviewed-on: https://chromium-review.googlesource.com/528724
Commit-Queue: Kouhei Ueno <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Cr-Commit-Position: refs/heads/master@{#478219}
WPT-Export-Revision: a1fd48a745a56045e25c3365a69f3fce0358074f
yzyz pushed a commit to yzyz/chromium that referenced this pull request Jun 12, 2017
…updates

This CL updates Blink's impl of #internal-module-script-graph-fetching-procedure
and #fetch-the-descendants-of-and-instantiate-a-module-script to match
recent spec change: whatwg/html#2595 .

Tests: external/wpt/html/semantics/scripting-1/the-script-element/module/specifier-error.html
Bug: 727299, 594639
Change-Id: I022b3b380b408a6d5c75a59d161aea4fe2868f48
Reviewed-on: https://chromium-review.googlesource.com/528724
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Hiroki Nakagawa <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Cr-Commit-Position: refs/heads/master@{#478609}
domenic added a commit that referenced this pull request Jun 16, 2017
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.
domenic added a commit that referenced this pull request Jun 19, 2017
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.
bterlson pushed a commit to tc39/ecma262 that referenced this pull request Aug 2, 2017
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.
domenic added a commit that referenced this pull request Aug 3, 2017
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.
domenic added a commit that referenced this pull request Aug 3, 2017
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.
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
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.
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants