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

Do not remember module instantiation errors. #1006

Merged
merged 3 commits into from
Oct 12, 2017

Conversation

GeorgNeis
Copy link
Contributor

A while ago, we changed the specification of modules such that instantiation and evaluation errors are recorded and then re-thrown by later instantiation/evaluation attempts. We have since realized that, in the case of instantiation errors, this was not a good idea, as it does not give us the kind of determinacy that we want.

The problem has to do with instantiating multiple "root" modules in a non-deterministic order, which happens e.g. due to multiple <script type=module ...> elements or multiple import() expressions. What we want to guarantee is at least "blame determinism", i.e., no matter
at which point an instantiation happens, if it fails then the error always originates from the same importing statement.

In cyclic graphs, having a failed instantiation record the error is in conflict with this goal. Consider this example:

<!-- index.html -->
<script type="module" src="./A.js"></script>
<script type="module" src="./B.js"></script>
// A.js
import "./B.js"
import {something} from "./empty.js"
// B.js
import "./A.js";
import {something_else} from "./empty.js"

If A gets instantiated first, both script loads result in an error blaming B's import of "something_else". If B gets instantiated first, both loads result in an error blaming A's import of "something".

Our solution is simply to not record instantiation errors but instead reset a module to "uninstantiated" if instantiation fails. In the example above, the load of A will then result in an error blaming B's import of "something_else", and the load of B will result in an error blaming A's import of "something" --- no matter which one gets instantiated first. (The error objects are not identical. While it's possible to enforce even this, it would require significant extra complexity, which
is why we decided against it.)

The treatment of evaluation errors remains unchanged: unlike instantiation, evaluation must not happen more than once, and is not idempotent, so we definitely need to record the error there. (See launchTheMissiles() in #862.)

Summary of the changes proposed in this PR:

Main changes:

  • Instantiation errors are no longer remembered.
  • Instantiation failure resets module to status "uninstantiated".
  • Since the only recorded errors are now evaluation errors, instantiation can treat errored modules like instantiated modules.

Minor changes include:

  • Reverted ResolveExport to what it was originally.
  • Renamed [[ErrorCompletion]] to [[EvaluationError]].
  • Replace "errored" status by "evaluated" with non-undefined
    [[EvaluationError]].
  • Moved [[Status]] and [[EvaluationError]] from Abstract MR to Source Text MR.
  • Renamed InnerModuleDeclarationInstantiation to InnerModuleInstantiation.

Related pull request for HTML: whatwg/html#2991

@allenwb
Copy link
Member

allenwb commented Sep 22, 2017

😀

@bterlson
Copy link
Member

Give me a little bit to read through the diff but the approach seems fine.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 11, 2017
…iation errors

Today, we are in somewhat stale state where new module tree fetching algorithm
[1][2] is partially applied. This CL (temporarily) disables an assert which
exists in the final algorithm, but doesn't hold today.

Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently
doesn't hold, as the instantiation may have failed for a module script node,
and current V8 implementation records the instantiation error as an error.
See the attached test case for an example.

[1] whatwg/html#2991
[2] tc39/ecma262#1006
[3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier)

Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html
Bug: 772750, 763597
Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 11, 2017
…iation errors

Today, we are in somewhat stale state where new module tree fetching algorithm
[1][2] is partially applied. This CL (temporarily) disables an assert which
exists in the final algorithm, but doesn't hold today.

Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently
doesn't hold, as the instantiation may have failed for a module script node,
and current V8 implementation records the instantiation error as an error.
See the attached test case for an example.

[1] whatwg/html#2991
[2] tc39/ecma262#1006
[3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier)

Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html
Bug: 772750, 763597
Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16
Reviewed-on: https://chromium-review.googlesource.com/708078
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507888}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 11, 2017
…iation errors

Today, we are in somewhat stale state where new module tree fetching algorithm
[1][2] is partially applied. This CL (temporarily) disables an assert which
exists in the final algorithm, but doesn't hold today.

Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently
doesn't hold, as the instantiation may have failed for a module script node,
and current V8 implementation records the instantiation error as an error.
See the attached test case for an example.

[1] whatwg/html#2991
[2] tc39/ecma262#1006
[3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier)

Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html
Bug: 772750, 763597
Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16
Reviewed-on: https://chromium-review.googlesource.com/708078
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507888}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Oct 12, 2017
…iation errors

Today, we are in somewhat stale state where new module tree fetching algorithm
[1][2] is partially applied. This CL (temporarily) disables an assert which
exists in the final algorithm, but doesn't hold today.

Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently
doesn't hold, as the instantiation may have failed for a module script node,
and current V8 implementation records the instantiation error as an error.
See the attached test case for an example.

[1] whatwg/html#2991
[2] tc39/ecma262#1006
[3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier)

Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html
Bug: 772750, 763597
Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16
Reviewed-on: https://chromium-review.googlesource.com/708078
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507888}
@bterlson
Copy link
Member

This looks good. Especially appreciate the updated copy in 15.2.1.16.6 Example Source Text Module Record Graphs. I introduced a merge conflict - will attempt to resolve soon unless someone beats me to it.

@bterlson bterlson force-pushed the modules-instantiation branch from 5408ef3 to 8bc7f46 Compare October 12, 2017 21:16
@bterlson
Copy link
Member

Alright I think I rebased correctly. @GeorgNeis @domenic could one of you take a quick look and make sure I didn't hose anything?

@domenic
Copy link
Member

domenic commented Oct 12, 2017

LGTM

@bterlson bterlson merged commit c0e07a8 into tc39:master Oct 12, 2017
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
…iation errors

Today, we are in somewhat stale state where new module tree fetching algorithm
[1][2] is partially applied. This CL (temporarily) disables an assert which
exists in the final algorithm, but doesn't hold today.

Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently
doesn't hold, as the instantiation may have failed for a module script node,
and current V8 implementation records the instantiation error as an error.
See the attached test case for an example.

[1] whatwg/html#2991
[2] tc39/ecma262#1006
[3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier)

Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html
Bug: 772750, 763597
Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16
Reviewed-on: https://chromium-review.googlesource.com/708078
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507888}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants