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

domenic · 2017-05-13T00:12:04Z

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.

/cc @whatwg/modules

Tagging "do not merge yet" pending ES spec resolution; however, as noted in that PR, if that becomes difficult we have some workarounds we can apply.

This CL adds a layout test demonstrating the currently broken cycle handling, which currently fails on ToT. If a cyclic module graph node contained other edges that are not part of the cycle, the graph does not load reliably. (To be more specific, the graph does not load unless the other edges complete loading before we instantiate the cycle nodes). We expect to have the root cause fixed in the spec change PR: whatwg/html#2674 Bug: 594639 Change-Id: I3cf8a640a73083ad612c96fc4bbd0f01e00f6e46 Reviewed-on: https://chromium-review.googlesource.com/535413 Cr-Commit-Position: refs/heads/master@{#479534} WPT-Export-Revision: 1d8720afaeeb738690321b52bc88960ecb3b36f3

This CL adds a layout test demonstrating the currently broken cycle handling, which currently fails on ToT. If a cyclic module graph node contained other edges that are not part of the cycle, the graph does not load reliably. (To be more specific, the graph does not load unless the other edges complete loading before we instantiate the cycle nodes). We expect to have the root cause fixed in the spec change PR: whatwg/html#2674 Bug: 594639 Change-Id: I3cf8a640a73083ad612c96fc4bbd0f01e00f6e46 Reviewed-on: https://chromium-review.googlesource.com/535413 Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#479551}

domenic · 2017-06-19T15:58:14Z

source

-    </div>
-   </li>
+   <li><p>Assert: <var>resolved module script</var>'s <span
+   data-x="concept-module-script-module-record">module record</span> is not null.</p>


Should assert it's not "is errored"

domenic · 2017-06-19T15:59:00Z

source

-
-       <li><p>Set <var>script</var>'s <span data-x="concept-module-script-parse-error">parse
-       error</span> to <var>error</var>.</p></li>
+       <li><p><span data-x="concept-module-script-set-parse-error">Set the parse error</span> of


"Parse error" is no longer accurate since this stores dependency errors which might be instantiation errors etc. It's still "errors that allow us to short-circuit and not evaluate"

WIP This change is to be introduced in whatwg html spec PR: whatwg/html#2674 Bug: 594639, 727299, whatwg/html#2674 Change-Id: I7354e00820dd222f30b4a4eba1d3cf8c1b319798 Reviewed-on: https://chromium-review.googlesource.com/540960 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#480758} WPT-Export-Revision: 83acd8a77ee7cf68e0f7063f8d7f87aecf876334

…ule script. This CL updates Blink's implementation of https://html.spec.whatwg.org/#creating-a-module-script , namely ModuleScript::Create to validate its record.[[RequestedModules]] specifiers. This change is to be introduced in whatwg html spec PR: whatwg/html#2674 Bug: 594639, 727299, whatwg/html#2674 Change-Id: I6f45ab3909b356376884b82539e0d16b538c35bb Reviewed-on: https://chromium-review.googlesource.com/538324 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#480712}

This CL updates spec text references to https://html.spec.whatwg.org/#run-a-module-script. This change is to be introduced in whatwg html spec PR: whatwg/html#2674 The actual implementation behavior change will be introduced in separate CLs. Bug: 594639, 727299, whatwg/html#2674 Change-Id: Ia658aed3bd2c37814aa1950c7cf4fec8b1c4bb1d Reviewed-on: https://chromium-review.googlesource.com/541178 Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#480761}

This CL updates the following spec concepts: "#concept-module-script-is-errored" - Null record is now also treated as an error "#concept-module-script-pre-instantiation-error" - Renamed from module script's error. "#concept-module-script-module-record" - Implement "set its [[HostDefined]] field to undefined" step. "#concept-module-script-set-pre-instantiation-error" - Don't set script's state to errored. This change is to be introduced in whatwg html spec PR: whatwg/html#2674 The actual implementation behavior change will be introduced in separate CLs. Bug: 594639, 727299, whatwg/html#2674 Change-Id: Ie9734344030f99bcdaa3d595f8dbec87d44a7c92 Reviewed-on: https://chromium-review.googlesource.com/541097 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#482213}

This CL exposes v8::Module::Status() as ScriptModule::Status() Bug: 594639, 727299, whatwg/html#2674 Change-Id: If515936fe72122343b05c1bd71f86559725d80cc Reviewed-on: https://chromium-review.googlesource.com/554531 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#483354}

…eld's [[Value]]. This CL exposes v8::Module::GetException() as ScriptModule::ErrorCompletion(). This CL also updates ModulatorImpl::GetError(ModuleScript*) impl to match the latest "#concept-module-script-error" concept. ModulatorImpl change will be covered by external/wpt/html/semantics/scripting-1/the-script-element/instantiation-error* once the entire module infra is updated. Bug: 594639, 727299, whatwg/html#2674 , tc39/ecma262#916 Change-Id: I4d4847c2031f7456e659984b00542fdb08c68dd9 Reviewed-on: https://chromium-review.googlesource.com/554633 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#483521}

WIP This change is to be introduced in whatwg html spec PR: whatwg/html#2674 Bug: 594639, 727299, whatwg/html#2674 Change-Id: I7354e00820dd222f30b4a4eba1d3cf8c1b319798 Reviewed-on: https://chromium-review.googlesource.com/540960 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#483894} WPT-Export-Revision: 2f138be2c08faca6c2eadd74c4b9d63adb8e75b5

This CL implements ModuleScript::RecordStatus(), which returns module script's record's [[Status]]. Which will replace usage of ModuleScript::state_ after we switch to the new #internal-module-script-graph-fetching procedure proposed in the linked PRs. The call to ScriptModule::Status routed via Modulator to allow mock implementation in Modulator infra unit tests. This codepath will be tested in modulator infra unit tests after the #internal-module-script-graph-fetching update. Bug: 594639, 727299, whatwg/html#2674 , tc39/ecma262#916 Change-Id: I2ca193a8c779945c9ae9b08c2b5f55de41fb1a3c Reviewed-on: https://chromium-review.googlesource.com/558724 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#483902}

This CL introduces ModuleScript::HasInstantiated(), which implements spec concept: "#concept-module-script-has-instantiated" This will replace usage of ModuleScript::state_ comparison to kInstantiated after we switch to the new #internal-module-script-graph-fetching procedure proposed in the linked PRs. This codepath will be tested in modulator infra unit tests after the #internal-module-script-graph-fetching update. Bug: 594639, 727299, whatwg/html#2674 , tc39/ecma262#916 Change-Id: Icdcc21cf00b1e47a35e747d85dd5af7907dc5a2a Reviewed-on: https://chromium-review.googlesource.com/558166 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#483904}

…ed via GetError Before this CL, ExecuteModule reported error acquired via ModuleScript::GetErrorInternal. This is not always correct after 40485cc. ModuleScript::GetErrorInternal only refers to preinstantiation errors, and we may have errors stored inside module records as [[ErrorCompletion]] field's [[Value]]. This CL corrects ExecuteModule to report the appropriate error by using error acquired by ModulatorImpl::GetError, which correctly implements the "#concept-module-script-error" spec concept. This is tested by wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-*.html. However the wpt tests don't pass until we have updated Bug: 594639, 727299, whatwg/html#2674 , tc39/ecma262#916 Change-Id: I97de9762bfad58e1f1b71bedf524761e53d43158 Reviewed-on: https://chromium-review.googlesource.com/558725 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#483908}

…TreeLinker The spec concept "module script's state" is removed from latest HTML spec. Remove its usage outside ModuleTreeLinker. Bonus: Introduce pretty-printer of ModuleScript. Bug: 594639, 727299, whatwg/html#2674 Change-Id: Ic01d729d4c2bfa546da9138e7a8248b997060c87 Reviewed-on: https://chromium-review.googlesource.com/558567 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#483930}

This CL updates ModuleScript::IsErrored so that it would also consider its record's [[Status]]. This behavior change will be tested in wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-*.html once we update ModuleTreeLinker to latest #internal-module-script-graph-fetching procedure Bug: 594639, 727299, whatwg/html#2674 , tc39/ecma262#916 Change-Id: I575001a44ca1cc2ad5c9208c93c0b628a09086dd Reviewed-on: https://chromium-review.googlesource.com/558825 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#483945}

This CL implements the changes introduced in whatwg html spec PR: whatwg/html#2674 Summary: - "final result" is now simply the "module script", instead of previous "descendant module script" which was used to report where error occurred. - "#fetch-the-descendants-of-a-module-script" assumes (actually asserts) that all module specifiers are valid, as now it is guaranteed that their failure is handled at ModuleScript::Create. - ModuleTreeLinker::NotifyOneDescendantFinished now waits for all descendants fetch to complete, even in error cases. -- This matches spec procedure precisely, but we can optimize here later while keeping the behavior. - ModuleScriptLinker now only triggers instantiate on module tree root. (iff ModuleGraphLevel == kTopLevelModuleFetch) -- V8 is now responsible for instantiating the descendant tree + keeping their instantiation status as module script's record's [[Status]] (accessible via ModuleScript::RecordStatus() in Blink) -- UninstantiatedInclusiveDescendants() is removed, as it is no longer used. With this change, Blink now conforms to all module script WPTs! Bug: 594639, 727299, whatwg/html#2674 Change-Id: I7354e00820dd222f30b4a4eba1d3cf8c1b319798 Reviewed-on: https://chromium-review.googlesource.com/540960 Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#483957}

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 allows us to write a HostResolveImportedModule that never throws, which is useful for reducing traversals on the ES side

- Properly propagate the *error*, not the *parse error*, during fetching - "Parse error" is not accurate anymore; it became "pre-instantiation error" - Can assert a stronger condition in HostResolveImportedModule

domenic · 2017-08-03T20:08:38Z

The ES spec change has landed! I've rebased and made some minor editorial tweaks in light of that.

Given that this has received significant review from @nyaxt and @hiroshige-g in the course of implementing it in Chrome, I will now merge this. It's not necessarily perfect yet; we have a few remaining cases we're unsure of around error handling behavior, and we think that the spec might be written using a slow algorithm that, which we could clean up so that implementers have to do less non-obvious optimizations. But this change represents a significant improvement from the status quo, and serves as a good foundation going forward.

This CL updates ResolveModuleCallback to reflect the HTML specification changes at whatwg/html#2674. Concretely, it replaces certain error handling cases with assertions that these errors can (no longer) happen. Bug: Change-Id: I66490625652c556b0e26218d9fc1015b694c680f Reviewed-on: https://chromium-review.googlesource.com/595973 Commit-Queue: Georg Neis <neis@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#492286}

domenic added do not merge yet Pull request must not be merged per rationale in comment topic: script labels May 13, 2017

domenic mentioned this pull request May 13, 2017

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

Closed

chromium-wpt-export-bot mentioned this pull request Jun 14, 2017

[ES6 modules] Add WPT for loading cyclic module graph w/ slow imports web-platform-tests/wpt#6228

Merged

domenic force-pushed the modules-with-help branch from 8b5896b to f094c77 Compare June 16, 2017 21:20

domenic commented Jun 19, 2017

View reviewed changes

domenic force-pushed the modules-with-help branch 2 times, most recently from c3a3609 to 5db819e Compare June 19, 2017 18:01

chromium-wpt-export-bot mentioned this pull request Jun 20, 2017

[ES6 modules] Update #internal-module-script-graph-fetching procedure web-platform-tests/wpt#6288

Closed

domenic added 4 commits August 3, 2017 15:27

Propagate errors ourselves, deterministically

98bf499

This allows us to write a HostResolveImportedModule that never throws, which is useful for reducing traversals on the ES side

Mostly-editorial fixes after conversation with neis

16b85f6

- Properly propagate the *error*, not the *parse error*, during fetching - "Parse error" is not accurate anymore; it became "pre-instantiation error" - Can assert a stronger condition in HostResolveImportedModule

Update for rename to the methods

395d7d8

domenic force-pushed the modules-with-help branch from 5db819e to 395d7d8 Compare August 3, 2017 19:30

Last-minute tweaks

a8357cd

domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Aug 3, 2017

domenic merged commit 2b408b6 into master Aug 3, 2017

domenic deleted the modules-with-help branch August 3, 2017 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

domenic commented May 13, 2017 •

edited

Loading

domenic Jun 19, 2017

domenic Jun 19, 2017

domenic commented Aug 3, 2017

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

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

Conversation

domenic commented May 13, 2017 • edited Loading

domenic Jun 19, 2017

Choose a reason for hiding this comment

domenic Jun 19, 2017

Choose a reason for hiding this comment

domenic commented Aug 3, 2017

domenic commented May 13, 2017 •

edited

Loading