Fix module script error handling, again #2991

domenic · 2017-08-31T23:31:51Z

Recent investigation determined more problems with the module script
graph fetching algorithms with regard to error handling. Essentially,
they were not robust to network races during fetching. Certain errors
would be propagated nondeterministically; worse, Evaluate() could be
called on modules with errored dependencies, due to another module
script graph interleaving its instantiation between a graph's
instantiation and evaluation.

This revision avoids all these problems. The strategy is essentially:

Do not record instantiation errors of dependencies. This is mostly a
feature of a corresponding revision to the JavaScript specification;
we just no longer look for such errors. Instead, we call
Instantiate(), which now does a from-scratch re-instantiation attempt,
finding the appropriate error each time.
Do not record parse errors of dependencies. Instead, traverse the
graph right before instantiation to deterministically find the first
parse error.

To implement this strategy, a new "error to rethrow" field was
introduced to the script struct; its purpose is to store an error that
is to be rethrown during instantiation. (An alternate implementation
would have been to find another way of passing the error from "prepare a
script" and its associated script-fetching algorithms, to "execute a
script block" and its associated script-running algorithms. But the
script struct was a convenient place to keep this. This does have
impacts for error object identity across reuses of the same script.)

Note that with this strategy we no longer inspect the [[Status]] or
[[ErrorCompletion]] fields of JavaScript's Source Text Module Records;
instead, we can simply call Instantiate() or Evaluate() and deal with
any thrown errors. This is a much nicer encapsulation boundary.

/cc @whatwg/modules. On top of #2972, so "do not merge yet"; also needs some JS spec updates as well. @hiroshige-g and/or @nyaxt to review.

This will need some tests to exercise those cases we discovered, as well as the slightly-changed error identity behavior that comes from not recording errors as aggressively. We plan to write those as part of Blink updating to this.

Preview version at https://output-osunnkiurv.now.sh

hiroshige-g · 2017-09-05T22:56:44Z

source

-   <li><p>If <var>module script</var> <span data-x="concept-module-script-is-errored">is
-   errored</span> or <span data-x="concept-module-script-has-instantiated">has instantiated</span>,
-   asynchronously complete this algorithm with <var>module script</var>, and abort these
+   <li><p>If <var>module script</var>'s <span data-x="concept-script-record">record</span> is null,


Also we should check whether module script is null because we remove a null check from "internal module script graph fetching procedure"?.

Oh, I think that removal was unintentional; we still need to bail there for null. I will add it back.

hiroshige-g · 2017-09-05T23:24:23Z

source

-     script</span>, and all previous invocations have completed already. However, this is optimizing
-     for the rare failure case, so likely not worth the trouble.</p>
-    </div>
+    <p>If any of the invocations of the <span>internal module script graph fetching procedure</span>


I think we have to wait for all of the invocations to complete, even if any of the invocations completes with null, to ensure deterministic choice of error and to make the assertion "by now all module scripts in the graph rooted at moduleScript will have successfully been fetched" in Step 8.2 of "find the first parse error" to hold

If any of the invocations complete with null, we will get a fetch error, and bail out before "find the first parse error" is reached.

Fetch errors are not given any real "identity"; they are all just null. So at the spec level it doesn't matter if you wait or not; you still get null. In implementations, though, you may want to propagate more useful information. I wonder if we should write that in the spec.

I think this will not be too hard to change. We will just introduce some new struct, a "script fetching error", which contains a URL. Then instead of propagating null we can propagate that through the chain. And this step in particular will be about deterministically finding the right script fetching error.

Finally when we get to "execute the script block" we will check to see if the script's script is a script fetching error, and say that the UA may optionally use the URL in the script fetching error to give good diagnostic info (e.g. in the developer console). At this point we can also rename script's script -> script's result, which is slightly less confusing of a name.

WDYT?

If any of the invocations complete with null, we will get a fetch error, and bail out before "find the first parse error" is reached.

Oh, I see.
So this PR contains the following behavior change, right?

Example:

<script type="module" src="A.js" onload=... onerror=...> A.js: import B.js import 404.js B.js: <parse error>

Previously: fetch error (i.e. null) is handled in the same way as the preinstantiation error.
onload is called, because "fetch the descendants of a module script" returns NON-null.
The parse error in B.js is reported.

After this PR:
onerror is called, because "fetch the descendants of a module script" returns null.

I just want to clarify the behavior change, and currently I'm neutral about this behavior change.

Oh, interesting, I did not consider that behavior change. I think it is a good change though. I think it is nice to report errors in stages: first fetching, then parsing, then instantiation, then evaluation. I.e. an error in any of the previous stages will prevent you from seeing an error in the future stages.

We will just introduce some new struct, a "script fetching error", which contains a URL. Then instead of propagating null we can propagate that through the chain. And this step in particular will be about deterministically finding the right script fetching error.

This might be a good option (to be done separately).
In the classic script world, "script's script is null" and the fact that onerror is fired is sufficient, because there is only one URL that can fail to be fetched, which is no longer true in module graph loading.

Filed #3025 to track.

hiroshige-g · 2017-09-05T23:39:07Z

Using script's error in two different ways looks confusing for me.

If script's record is null, script's error must be a parse error of the script itself, which is only referenced in "find the first parse error".
If script's record is not null, script's error can be a parse error of the script or another script, an instantiation error found during Instantiate(). It is never referenced from "find the first parse error", and is used for store the error to be reported in "run a module script".

How about introducing a new field for Use Case 2, while keeping the field for Use Case 1 as parse error?

Separation of these two uses is ensured by relatively subtle aspects of the spec, i.e.

script's error is set to an instantiation error only if script's record is not null (and thus the instantiation error never referenced by "find the first parse error").
script A's error is set to a parse error of another script B only if script A's record is not null, because otherwise "find the first parse error" returns the script A's error (and thus the parse error referenced by "find the first parse error" is always the parse error of the script itself, not one propagated from another)

Previously, we could invoke the internal module script graph fetching procedure once for each combination of node and ancestor list, i.e. once for each node and for each path to the node. This could be up to O(2^n) for n nodes, as seen in https://gist.github.com/domenic/d877f8b2e6d6ae62a9c94f916739e4db. This change simplifies the model by using a single visited set per top-level fetch, which ensures that the internal module script graph fetching procedure is only invoked once for each node. This change is technically editorial, as it has no visible, testable effects. (It may subtly change the choice of error reported, but that was already nondeterministic, and being fixed in #2991.) It's intended to make it much easier for implementers to follow the standard as written without causing themselves massive slowdowns.

domenic · 2017-09-06T20:54:51Z

How about introducing a new field for Use Case 2, while keeping the field for Use Case 1 as parse error?

That is probably a good idea. I was being too clever by reusing the same field. I will try to change this.

domenic · 2017-09-08T01:17:37Z

Updated. New preview version at https://output-orhqepubzd.now.sh

hiroshige-g · 2017-09-08T19:56:15Z

source

-    text, in which case the script's <span data-x="concept-script-record">record</span> will
-    be null; or more general errors in dependencies of a <span>module script</span>. This
-    distinction is used by the algorithm for <span>finding the first parse error</span>.</p>
+    text (equal to the script's <span data-x="concept-script-parse-error">parse error</span>), or


[Optional] This can be a parse error propagated from another script. Maybe this distinction has less sense now (because we split error into parse error and error to rethrow) and thus we might be able to remove this paragraph.

Right, that was meant to be part of the second clause. Maybe it's best to just remove it, as you say.

hiroshige-g · 2017-09-08T20:56:52Z

source


    <ol>
-     <li><p>Set <var>script</var>'s <span data-x="concept-script-error">error</span> to
+     <li><p>Set <var>script</var>'s <span data-x="concept-script-parse-error">parse error</span> and
+     its <span data-x="concept-script-error-to-rethrow">error to rethrow</span> to


I feel it is clearer not to set a non-null error to error to rethrow of a module script in create a module script here and below, because it results in the following semantics (that I feel clearer):
error to rethrow is set by fetch the descendants of and instantiate a module script and used by run a module script.
In fact, in the case of module scripts error to rethrow is always overridden (by the same parse error value) in fetch the descendants of and instantiate a module script, and thus it is redundant to set error to rethrow here.

In the case of classic scripts, error to rethrow should be set when created (or if we want to omit setting of error to rethrow when creating a classic script as well, run a classic script should refer parse error instead of error to rethrow and error to rethrow would become module-only), because there can be no steps between creating and running where we can set error to rethrow.

I see, I forgot that it was redundant. Good idea; I will not set it (for module scripts).

domenic · 2017-09-14T05:04:03Z

Most recent tweaks applied; new preview version at https://output-osunnkiurv.now.sh . Will still wait on the TC39 spec patch though.

Updated OP with a new commit message; reminder to self to use that when merging.

1. This CL applies the spec change by whatwg/html#2971. Since [1], the code has already been behaving like the spec PR 2971, and this CL makes the code align with PR 2971 more completely, by removing AncestorList and adding spec comments. 2. This CL also flattens the code structure of ModuleTreeLinker, which is enabled by the spec PR 2971, to make the code simpler to make further optimizations easy. That is, instead of creating a ModuleTreeLinker for each module script (i.e. for each "fetch the descendants" call) in a module graph, this CL creates a single ModuleTreeLinker that corresponds to a top-level module graph script. Most of fetching-related classes/methods, including - ModuleTreeLinker::DependencyModuleClient - ModuleTreeLinker::FetchDescendants() - ModuleTreeLinker::NotifyOneDescendantFinished() are merged into the single method NotifyModuleLoadFinished() that implements the main body of "fetch the descendants" and "internal module script graph fetching procedure". This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL> directly, as we no longer have to share it across multiple ModuleTreeLinkers. Modulator::FetchTreeInternal() is removed as we no longer create ModuleTreeLinkers for subtree fetching via Modulator. 3. This CL applies a part of whatwg/html#2991, particularly introduces FindFirstParseError() that corresponds to "find the first parse error" and use it to find the error to be reported, instead of propagating errors based on Step 6.1 and 6.2 of "fetch the descendants". (*) In some subtle cases, this might cause behavior changes in error reporting, but these changes shouldn't be significant, because anyway the spec before PR 2991 (and thus the previous implementation) behaves nondeterministically in some similarly subtle cases. This CL is an intermediate step to apply spec PRs 2971 and 2991. This CL refactors largely ModuleTreeLinker with keeping the existing behavior mostly (except for (*)), and subsequent CLs will apply the behavior changes and remaining code structure changes introduced by PR 2991. Bug: 763597 Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d Reviewed-on: https://chromium-review.googlesource.com/583552 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#503034}

This reverts commit 5a168df. Reason for revert: find-it says this bloke the linux build Original change's description: > Flatten ModuleTreeLinker > > 1. This CL applies the spec change by > whatwg/html#2971. > Since [1], the code has already been behaving like the spec PR 2971, and this CL > makes the code align with PR 2971 more completely, by removing AncestorList > and adding spec comments. > > 2. This CL also flattens the code structure of ModuleTreeLinker, which is > enabled by the spec PR 2971, to make the code simpler to make further > optimizations easy. > That is, instead of creating a ModuleTreeLinker for each module script > (i.e. for each "fetch the descendants" call) in a module graph, this CL > creates a single ModuleTreeLinker that corresponds to a top-level module graph > script. > > Most of fetching-related classes/methods, including > - ModuleTreeLinker::DependencyModuleClient > - ModuleTreeLinker::FetchDescendants() > - ModuleTreeLinker::NotifyOneDescendantFinished() > are merged into the single method NotifyModuleLoadFinished() that implements > the main body of "fetch the descendants" and > "internal module script graph fetching procedure". > > This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL> > directly, as we no longer have to share it across multiple ModuleTreeLinkers. > Modulator::FetchTreeInternal() is removed as we no longer create > ModuleTreeLinkers for subtree fetching via Modulator. > > 3. This CL applies a part of > whatwg/html#2991, particularly introduces > FindFirstParseError() that corresponds to "find the first parse error" and > use it to find the error to be reported, instead of propagating errors > based on Step 6.1 and 6.2 of "fetch the descendants". > > (*) In some subtle cases, this might cause behavior changes in error reporting, > but these changes shouldn't be significant, because anyway the spec before > PR 2991 (and thus the previous implementation) behaves nondeterministically > in some similarly subtle cases. > > This CL is an intermediate step to apply spec PRs 2971 and 2991. > This CL refactors largely ModuleTreeLinker with keeping the existing behavior > mostly (except for (*)), and subsequent CLs will apply the behavior changes > and remaining code structure changes introduced by PR 2991. > > Bug: 763597 > Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d > Reviewed-on: https://chromium-review.googlesource.com/583552 > Commit-Queue: Hiroshige Hayashizaki <[email protected]> > Reviewed-by: Kouhei Ueno <[email protected]> > Cr-Commit-Position: refs/heads/master@{#503034} [email protected],[email protected],[email protected],[email protected] Change-Id: I297bca8d91c91f49e4ef1e46a01a1af90a7e67ef No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 763597 Reviewed-on: https://chromium-review.googlesource.com/674784 Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#503059}

This reverts commit 5a168df. Reason for revert: Seems to be breaking ModuleTreeLinkerTest.FetchTreeWith3Deps1Fail on: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20TSan%20Tests/builds/11696 Original change's description: > Flatten ModuleTreeLinker > > 1. This CL applies the spec change by > whatwg/html#2971. > Since [1], the code has already been behaving like the spec PR 2971, and this CL > makes the code align with PR 2971 more completely, by removing AncestorList > and adding spec comments. > > 2. This CL also flattens the code structure of ModuleTreeLinker, which is > enabled by the spec PR 2971, to make the code simpler to make further > optimizations easy. > That is, instead of creating a ModuleTreeLinker for each module script > (i.e. for each "fetch the descendants" call) in a module graph, this CL > creates a single ModuleTreeLinker that corresponds to a top-level module graph > script. > > Most of fetching-related classes/methods, including > - ModuleTreeLinker::DependencyModuleClient > - ModuleTreeLinker::FetchDescendants() > - ModuleTreeLinker::NotifyOneDescendantFinished() > are merged into the single method NotifyModuleLoadFinished() that implements > the main body of "fetch the descendants" and > "internal module script graph fetching procedure". > > This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL> > directly, as we no longer have to share it across multiple ModuleTreeLinkers. > Modulator::FetchTreeInternal() is removed as we no longer create > ModuleTreeLinkers for subtree fetching via Modulator. > > 3. This CL applies a part of > whatwg/html#2991, particularly introduces > FindFirstParseError() that corresponds to "find the first parse error" and > use it to find the error to be reported, instead of propagating errors > based on Step 6.1 and 6.2 of "fetch the descendants". > > (*) In some subtle cases, this might cause behavior changes in error reporting, > but these changes shouldn't be significant, because anyway the spec before > PR 2991 (and thus the previous implementation) behaves nondeterministically > in some similarly subtle cases. > > This CL is an intermediate step to apply spec PRs 2971 and 2991. > This CL refactors largely ModuleTreeLinker with keeping the existing behavior > mostly (except for (*)), and subsequent CLs will apply the behavior changes > and remaining code structure changes introduced by PR 2991. > > Bug: 763597 > Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d > Reviewed-on: https://chromium-review.googlesource.com/583552 > Commit-Queue: Hiroshige Hayashizaki <[email protected]> > Reviewed-by: Kouhei Ueno <[email protected]> > Cr-Commit-Position: refs/heads/master@{#503034} [email protected],[email protected],[email protected],[email protected] Change-Id: I84c8f8f443bdbb1d5fbe8cb52d2cb513c7890d2c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 763597 Reviewed-on: https://chromium-review.googlesource.com/674510 Reviewed-by: calamity <[email protected]> Commit-Queue: calamity <[email protected]> Cr-Commit-Position: refs/heads/master@{#503063}

This CL relands https://chromium-review.googlesource.com/c/chromium/src/+/583552 with a fix for test failure in release builds: Move FindFirstParseError() call out of DCHECK(). Original CL description: 1. This CL applies the spec change by whatwg/html#2971. Since [1], the code has already been behaving like the spec PR 2971, and this CL makes the code align with PR 2971 more completely, by removing AncestorList and adding spec comments. 2. This CL also flattens the code structure of ModuleTreeLinker, which is enabled by the spec PR 2971, to make the code simpler to make further optimizations easy. That is, instead of creating a ModuleTreeLinker for each module script (i.e. for each "fetch the descendants" call) in a module graph, this CL creates a single ModuleTreeLinker that corresponds to a top-level module graph script. Most of fetching-related classes/methods, including - ModuleTreeLinker::DependencyModuleClient - ModuleTreeLinker::FetchDescendants() - ModuleTreeLinker::NotifyOneDescendantFinished() are merged into the single method NotifyModuleLoadFinished() that implements the main body of "fetch the descendants" and "internal module script graph fetching procedure". This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL> directly, as we no longer have to share it across multiple ModuleTreeLinkers. Modulator::FetchTreeInternal() is removed as we no longer create ModuleTreeLinkers for subtree fetching via Modulator. 3. This CL applies a part of whatwg/html#2991, particularly introduces FindFirstParseError() that corresponds to "find the first parse error" and use it to find the error to be reported, instead of propagating errors based on Step 6.1 and 6.2 of "fetch the descendants". (*) In some subtle cases, this might cause behavior changes in error reporting, but these changes shouldn't be significant, because anyway the spec before PR 2991 (and thus the previous implementation) behaves nondeterministically in some similarly subtle cases. This CL is an intermediate step to apply spec PRs 2971 and 2991. This CL refactors largely ModuleTreeLinker with keeping the existing behavior mostly (except for (*)), and subsequent CLs will apply the behavior changes and remaining code structure changes introduced by PR 2991. Bug: 763597 Change-Id: I4136db0db4bc8a0cdc7f5c23b18787b405d8c98f Reviewed-on: https://chromium-review.googlesource.com/674748 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#503126}

The web platform tests testing module script errors are not deterministic until we have the new algorithm being proposed at HTML PRs below: - whatwg/html#2971 - whatwg/html#2991 This CL marks the test as flaky until we land the new algorithm. [email protected] Bug: 767093, 763597 Change-Id: I9ef17f2ae6dc51acbef4dbab1f7f7c7438d17eb7 Reviewed-on: https://chromium-review.googlesource.com/676403 Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#503304}

nyaxt · 2017-09-25T08:06:12Z

The diff lgtm.
After chat w/ @hiroshige-g, I don't have strong preference over splitting {fetch,compile} errors. I feel like not splitting would result in simpler implementation, but I'd defer to @hiroshige-g to make the call.

IsErrored() and HasInstantiated() are to be removed because they are removed from the HTML spec in whatwg/html#2991. This CL - Replaces IsErrored() calls in unit tests with checks for Instantiate() results or with HasEmptyRecord(), depending on whether instantiation errors or parse errors are tested, and - Removes EXPECT_FALSE(HasInstantiated()) from ModuleMapTest.cpp, because instantiation is already prohibited by NOTREACHED() in DummyModulator::InstantiateModule(). Bug: 763597 Change-Id: I272a03f3d5ce8ddf6ea645a3d0d1a1ae7543f2be Reviewed-on: https://chromium-review.googlesource.com/703483 Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#506947}

…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

…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 <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#507888}

Recent investigation determined more problems with the module script graph fetching algorithms with regard to error handling. Essentially, they were not robust to network races during fetching. Certain errors would be propagated nondeterministically; worse, Evaluate() could be called on modules with errored dependencies, due to another module script graph interleaving its instantiation between a graph's instantiation and evaluation. This revision avoids all these problems. The strategy is essentially: * Do not record instantiation errors of dependencies. This is mostly a feature of a corresponding revision to the JavaScript specification; we just no longer look for such errors. Instead, we call Instantiate(), which now does a from-scratch re-instantiation attempt, finding the appropriate error each time. * Do not record parse errors of dependencies. Instead, traverse the graph right before instantiation to deterministically find the first parse error. To implement this strategy, a new error-to-rethrow item was added to the script struct, whose purpose is to store an error that is to be re-thrown during instantiation. (An alternate implementation would have been to find another way of passing the error from "prepare a script" and its associated script-fetching algorithms, to "execute a script block" and its associated script-running algorithms. But the script struct was a convenient place to keep this.) Note that with this strategy we no longer inspect the [[Status]] or [[ErrorCompletion]] fields of JavaScript's Source Text Module Records; instead, we can simply call Instantiate() or Evaluate() and deal with any thrown errors. This is a much nicer encapsulation boundary. This follows upon tc39/ecma262@c0e07a8.

domenic · 2017-10-12T22:26:35Z

ES side change is merged. For tests, I want to confirm we have a plan for updating them via Chromium's work to match the spec, and then I will merge this.

domenic · 2017-10-13T00:53:06Z

Confirmed that we have a plan, and filed web-platform-tests/wpt#7747 to track that plan.

…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 <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#507888}

Spec update: whatwg/html#2991 V8-side change: https://chromium-review.googlesource.com/763369 Data members for errors are restructured: Removed module script's "is errored" #concept-module-script-is-errored => ModuleScript::IsErrored(). Removed module script's "error" #concept-module-script-error => Modulator::GetError(), ModuleScript::CreateError(), and ModuleScript::CreateErrorInternal(). Removed "set the pre-instantiation error" #concept-module-script-set-pre-instantiation-error => ModuleScript::SetErrorAndClearRecord(). Introduced script's "error to rethrow" #concept-script-error-to-rethrow => ModuleScript::error_to_rethrow_ and its accessors. Also renamed ModuleScript::preinstantiation_error_ to parse_error_, and added its accessors, HTML/Blink no longer checks modules' status: Removed references to [[Status]] and [[ErrorCompletion]] field. => Removed Modulator::GetRecordStatus(), ModuleScript::RecordStatus(), and calls to ScriptModule::ErrorCompletion(). Removed module script's "has instantiated" #concept-module-script-has-instantiated => Removed ModuleScript::HasInstantiated(). A subsequent CL will do further cleanup. Error handling algorithms were updated: - #creating-a-module-script - #run-a-module-script - [FFPE] #finding-the-first-parse-error - [IMSGF] #internal-module-script-graph-fetching-procedure - [FD] #fetch-the-descendants-of-a-module-script - [FDaI] #fetch-the-descendants-of-and-instantiate-a-module-script - #hostresolveimportedmodule(referencingscriptormodule,-specifier) This CL updates the following accordingly: - ModuleScript::Create() - ModulatorImplBase::ExecuteModule(). - ModuleTreeLinker.cpp. - ScriptModuleResolverImpl::Resolve() The behavior changes are: - User-facing: the error reported (to window.onerror etc.) is changed and made deterministic, as intended by the spec update. - V8-facing: this CL - invokes module instantiation of a module graph with existing instantiation/evaluation errors. - invokes evaluation of a module graph with existing evaluation errors. Bug: 763597 Change-Id: Ia02bb484290c5c07ab27e9c966db19a6e8d2596f

Spec update: whatwg/html#2991 V8-side change: https://chromium-review.googlesource.com/763369 Data members for errors are restructured: Removed module script's "is errored" #concept-module-script-is-errored => Removed ModuleScript::IsErrored(). Removed module script's "error" #concept-module-script-error => Removed Modulator::GetError(), ModuleScript::CreateError(), and ModuleScript::CreateErrorInternal(). Removed "set the pre-instantiation error" #concept-module-script-set-pre-instantiation-error => Removed ModuleScript::SetErrorAndClearRecord(). Introduced script's "error to rethrow" #concept-script-error-to-rethrow => Added ModuleScript::error_to_rethrow_ and its accessors. Also renamed ModuleScript::preinstantiation_error_ to parse_error_, and added its accessors, HTML/Blink no longer checks modules' status: Removed references to [[Status]] and [[ErrorCompletion]] field. => Removed Modulator::GetRecordStatus(), ModuleScript::RecordStatus(), and calls to ScriptModule::ErrorCompletion(). Removed module script's "has instantiated" #concept-module-script-has-instantiated => Removed ModuleScript::HasInstantiated(). A subsequent CL will do further cleanup: https://chromium-review.googlesource.com/802465 Error handling algorithms in the HTML spec were updated: - #creating-a-module-script - #run-a-module-script - [FFPE] #finding-the-first-parse-error - [IMSGF] #internal-module-script-graph-fetching-procedure - [FD] #fetch-the-descendants-of-a-module-script - [FDaI] #fetch-the-descendants-of-and-instantiate-a-module-script - #hostresolveimportedmodule(referencingscriptormodule,-specifier) And thus this CL updates the following accordingly: - ModuleScript::Create() - ModulatorImplBase::ExecuteModule() - ModuleTreeLinker.cpp - ScriptModuleResolverImpl::Resolve() The behavior changes are: - User-facing: the error reported (to window.onerror etc.) is changed and made deterministic, as intended by the spec update. - V8-facing: this CL - invokes module instantiation of a module graph with existing instantiation/evaluation errors. - invokes evaluation of a module graph with existing evaluation errors. These cases already occur, but this CL does these intentionally. Bug: 763597 Change-Id: Ia02bb484290c5c07ab27e9c966db19a6e8d2596f

Spec update: whatwg/html#2991 V8-side change: https://chromium-review.googlesource.com/763369 Data members for errors are restructured: Removed module script's "is errored" #concept-module-script-is-errored => Removed ModuleScript::IsErrored(). Removed module script's "error" #concept-module-script-error => Removed Modulator::GetError(), ModuleScript::CreateError(), and ModuleScript::CreateErrorInternal(). Removed "set the pre-instantiation error" #concept-module-script-set-pre-instantiation-error => Removed ModuleScript::SetErrorAndClearRecord(). Introduced script's "error to rethrow" #concept-script-error-to-rethrow => Added ModuleScript::error_to_rethrow_ and its accessors. Also renamed ModuleScript::preinstantiation_error_ to parse_error_, and added its accessors, HTML/Blink no longer checks modules' status: Removed references to [[Status]] and [[ErrorCompletion]] field. => Removed Modulator::GetRecordStatus(), ModuleScript::RecordStatus(), and calls to ScriptModule::ErrorCompletion(). Removed module script's "has instantiated" #concept-module-script-has-instantiated => Removed ModuleScript::HasInstantiated(). A subsequent CL will do further cleanup: https://chromium-review.googlesource.com/802465 Error handling algorithms in the HTML spec were updated: - #creating-a-module-script - #run-a-module-script - [FFPE] #finding-the-first-parse-error - [IMSGF] #internal-module-script-graph-fetching-procedure - [FD] #fetch-the-descendants-of-a-module-script - [FDaI] #fetch-the-descendants-of-and-instantiate-a-module-script - #hostresolveimportedmodule(referencingscriptormodule,-specifier) And thus this CL updates the following accordingly: - ModuleScript::Create() - ModulatorImplBase::ExecuteModule() - ModuleTreeLinker.cpp - ScriptModuleResolverImpl::Resolve() The behavior changes are: - User-facing: the error reported (to window.onerror etc.) is changed and made deterministic, as intended by the spec update. - V8-facing: this CL - invokes module instantiation of a module graph with existing instantiation/evaluation errors. - invokes evaluation of a module graph with existing evaluation errors. These cases already occur, but this CL does these intentionally. Bug: 763597 Change-Id: Ia02bb484290c5c07ab27e9c966db19a6e8d2596f Reviewed-on: https://chromium-review.googlesource.com/698467 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#525168}

They should be no longer used in Blink, as the spec update whatwg/html#2991 removes the references them from the HTML spec. Bug: 763597 Change-Id: I515b12a36a8e6f97b72ef6695a05da2f89ecfa8e Reviewed-on: https://chromium-review.googlesource.com/802465 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Hiroki Nakagawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#525235}

1. This CL applies the spec change by whatwg/html#2971. Since [1], the code has already been behaving like the spec PR 2971, and this CL makes the code align with PR 2971 more completely, by removing AncestorList and adding spec comments. 2. This CL also flattens the code structure of ModuleTreeLinker, which is enabled by the spec PR 2971, to make the code simpler to make further optimizations easy. That is, instead of creating a ModuleTreeLinker for each module script (i.e. for each "fetch the descendants" call) in a module graph, this CL creates a single ModuleTreeLinker that corresponds to a top-level module graph script. Most of fetching-related classes/methods, including - ModuleTreeLinker::DependencyModuleClient - ModuleTreeLinker::FetchDescendants() - ModuleTreeLinker::NotifyOneDescendantFinished() are merged into the single method NotifyModuleLoadFinished() that implements the main body of "fetch the descendants" and "internal module script graph fetching procedure". This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL> directly, as we no longer have to share it across multiple ModuleTreeLinkers. Modulator::FetchTreeInternal() is removed as we no longer create ModuleTreeLinkers for subtree fetching via Modulator. 3. This CL applies a part of whatwg/html#2991, particularly introduces FindFirstParseError() that corresponds to "find the first parse error" and use it to find the error to be reported, instead of propagating errors based on Step 6.1 and 6.2 of "fetch the descendants". (*) In some subtle cases, this might cause behavior changes in error reporting, but these changes shouldn't be significant, because anyway the spec before PR 2991 (and thus the previous implementation) behaves nondeterministically in some similarly subtle cases. This CL is an intermediate step to apply spec PRs 2971 and 2991. This CL refactors largely ModuleTreeLinker with keeping the existing behavior mostly (except for (*)), and subsequent CLs will apply the behavior changes and remaining code structure changes introduced by PR 2991. Bug: 763597 Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d Reviewed-on: https://chromium-review.googlesource.com/583552 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#503034} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 5a168dfca798f0088b3257f21781a66b4132ba7b

This reverts commit 5a168dfca798f0088b3257f21781a66b4132ba7b. Reason for revert: find-it says this bloke the linux build Original change's description: > Flatten ModuleTreeLinker > > 1. This CL applies the spec change by > whatwg/html#2971. > Since [1], the code has already been behaving like the spec PR 2971, and this CL > makes the code align with PR 2971 more completely, by removing AncestorList > and adding spec comments. > > 2. This CL also flattens the code structure of ModuleTreeLinker, which is > enabled by the spec PR 2971, to make the code simpler to make further > optimizations easy. > That is, instead of creating a ModuleTreeLinker for each module script > (i.e. for each "fetch the descendants" call) in a module graph, this CL > creates a single ModuleTreeLinker that corresponds to a top-level module graph > script. > > Most of fetching-related classes/methods, including > - ModuleTreeLinker::DependencyModuleClient > - ModuleTreeLinker::FetchDescendants() > - ModuleTreeLinker::NotifyOneDescendantFinished() > are merged into the single method NotifyModuleLoadFinished() that implements > the main body of "fetch the descendants" and > "internal module script graph fetching procedure". > > This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL> > directly, as we no longer have to share it across multiple ModuleTreeLinkers. > Modulator::FetchTreeInternal() is removed as we no longer create > ModuleTreeLinkers for subtree fetching via Modulator. > > 3. This CL applies a part of > whatwg/html#2991, particularly introduces > FindFirstParseError() that corresponds to "find the first parse error" and > use it to find the error to be reported, instead of propagating errors > based on Step 6.1 and 6.2 of "fetch the descendants". > > (*) In some subtle cases, this might cause behavior changes in error reporting, > but these changes shouldn't be significant, because anyway the spec before > PR 2991 (and thus the previous implementation) behaves nondeterministically > in some similarly subtle cases. > > This CL is an intermediate step to apply spec PRs 2971 and 2991. > This CL refactors largely ModuleTreeLinker with keeping the existing behavior > mostly (except for (*)), and subsequent CLs will apply the behavior changes > and remaining code structure changes introduced by PR 2991. > > Bug: 763597 > Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d > Reviewed-on: https://chromium-review.googlesource.com/583552 > Commit-Queue: Hiroshige Hayashizaki <[email protected]> > Reviewed-by: Kouhei Ueno <[email protected]> > Cr-Commit-Position: refs/heads/master@{#503034} [email protected],[email protected],[email protected],[email protected] Change-Id: I297bca8d91c91f49e4ef1e46a01a1af90a7e67ef No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 763597 Reviewed-on: https://chromium-review.googlesource.com/674784 Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#503059} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: e81cbfd9f8b19631241e926ebfe66aabedc0ef53

This CL relands https://chromium-review.googlesource.com/c/chromium/src/+/583552 with a fix for test failure in release builds: Move FindFirstParseError() call out of DCHECK(). Original CL description: 1. This CL applies the spec change by whatwg/html#2971. Since [1], the code has already been behaving like the spec PR 2971, and this CL makes the code align with PR 2971 more completely, by removing AncestorList and adding spec comments. 2. This CL also flattens the code structure of ModuleTreeLinker, which is enabled by the spec PR 2971, to make the code simpler to make further optimizations easy. That is, instead of creating a ModuleTreeLinker for each module script (i.e. for each "fetch the descendants" call) in a module graph, this CL creates a single ModuleTreeLinker that corresponds to a top-level module graph script. Most of fetching-related classes/methods, including - ModuleTreeLinker::DependencyModuleClient - ModuleTreeLinker::FetchDescendants() - ModuleTreeLinker::NotifyOneDescendantFinished() are merged into the single method NotifyModuleLoadFinished() that implements the main body of "fetch the descendants" and "internal module script graph fetching procedure". This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL> directly, as we no longer have to share it across multiple ModuleTreeLinkers. Modulator::FetchTreeInternal() is removed as we no longer create ModuleTreeLinkers for subtree fetching via Modulator. 3. This CL applies a part of whatwg/html#2991, particularly introduces FindFirstParseError() that corresponds to "find the first parse error" and use it to find the error to be reported, instead of propagating errors based on Step 6.1 and 6.2 of "fetch the descendants". (*) In some subtle cases, this might cause behavior changes in error reporting, but these changes shouldn't be significant, because anyway the spec before PR 2991 (and thus the previous implementation) behaves nondeterministically in some similarly subtle cases. This CL is an intermediate step to apply spec PRs 2971 and 2991. This CL refactors largely ModuleTreeLinker with keeping the existing behavior mostly (except for (*)), and subsequent CLs will apply the behavior changes and remaining code structure changes introduced by PR 2991. Bug: 763597 Change-Id: I4136db0db4bc8a0cdc7f5c23b18787b405d8c98f Reviewed-on: https://chromium-review.googlesource.com/674748 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#503126} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: b2ca65cb5a61eb6f69a573a336cbd1f9ea794fcd

The web platform tests testing module script errors are not deterministic until we have the new algorithm being proposed at HTML PRs below: - whatwg/html#2971 - whatwg/html#2991 This CL marks the test as flaky until we land the new algorithm. [email protected] Bug: 767093, 763597 Change-Id: I9ef17f2ae6dc51acbef4dbab1f7f7c7438d17eb7 Reviewed-on: https://chromium-review.googlesource.com/676403 Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#503304} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: e35c1229109f3eb213a4f62ebb27b1b864e5ba10

IsErrored() and HasInstantiated() are to be removed because they are removed from the HTML spec in whatwg/html#2991. This CL - Replaces IsErrored() calls in unit tests with checks for Instantiate() results or with HasEmptyRecord(), depending on whether instantiation errors or parse errors are tested, and - Removes EXPECT_FALSE(HasInstantiated()) from ModuleMapTest.cpp, because instantiation is already prohibited by NOTREACHED() in DummyModulator::InstantiateModule(). Bug: 763597 Change-Id: I272a03f3d5ce8ddf6ea645a3d0d1a1ae7543f2be Reviewed-on: https://chromium-review.googlesource.com/703483 Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#506947} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 56367281f9b94f9e151770e0a5da5f74623690e5

Previously, we could invoke the internal module script graph fetching procedure once for each combination of node and ancestor list, i.e. once for each node and for each path to the node. This could be up to O(2^n) for n nodes, as seen in https://gist.github.com/domenic/d877f8b2e6d6ae62a9c94f916739e4db. This change simplifies the model by using a single visited set per top-level fetch, which ensures that the internal module script graph fetching procedure is only invoked once for each node. This change is technically editorial, as it has no visible, testable effects. (It may subtly change the choice of error reported, but that was already nondeterministic, and being fixed in whatwg#2991.) It's intended to make it much easier for implementers to follow the standard as written without causing themselves massive slowdowns.

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

domenic force-pushed the module-fixes-6 branch from 6c20425 to 491afb1 Compare August 31, 2017 23:33

domenic added the needs tests Moving the issue forward requires someone to write tests label Aug 31, 2017

hiroshige-g reviewed Sep 5, 2017

View reviewed changes

domenic force-pushed the module-fixes-6 branch from 491afb1 to 751ec3e Compare September 6, 2017 20:42

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

domenic force-pushed the module-fixes-6 branch from 10c1760 to 5737282 Compare September 8, 2017 01:05

domenic added the do not merge yet Pull request must not be merged per rationale in comment label Sep 8, 2017

domenic mentioned this pull request Sep 8, 2017

Track the source of fetching errors in module graphs #3025

Open

hiroshige-g reviewed Sep 8, 2017

View reviewed changes

domenic mentioned this pull request Sep 22, 2017

Prevent execution of (some) scripts moving between documents #2673

Merged

3 tasks

GeorgNeis mentioned this pull request Sep 22, 2017

Do not remember module instantiation errors. tc39/ecma262#1006

Merged

chromium-wpt-export-bot mentioned this pull request Oct 11, 2017

[ES6 modules] HRIM may return errored module until V8 forgets instantiation errors web-platform-tests/wpt#7664

Merged

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

domenic force-pushed the module-fixes-6 branch from 7a7e768 to 9a25217 Compare October 12, 2017 22:25

domenic mentioned this pull request Oct 13, 2017

Some JS module error test cases are outdated web-platform-tests/wpt#7747

Open

domenic removed the needs tests Moving the issue forward requires someone to write tests label Oct 13, 2017

nyaxt approved these changes Oct 13, 2017

View reviewed changes

domenic merged commit 165101a into master Oct 13, 2017

domenic deleted the module-fixes-6 branch October 13, 2017 01:00

chromium-wpt-export-bot mentioned this pull request Dec 1, 2017

[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic web-platform-tests/wpt#8527

Merged

libin1991 mentioned this pull request Jun 27, 2018

在浏览器中使用原生 JavaScript 模块（译） libin1991/libin_Blog#582

Open

domenic added the topic: error reporting label Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix module script error handling, again #2991

Fix module script error handling, again #2991

domenic commented Aug 31, 2017 •

edited

Loading

hiroshige-g Sep 5, 2017 •

edited

Loading

domenic Sep 6, 2017

hiroshige-g Sep 5, 2017 •

edited

Loading

domenic Sep 6, 2017

domenic Sep 6, 2017

hiroshige-g Sep 7, 2017

domenic Sep 7, 2017

hiroshige-g Sep 7, 2017

domenic Sep 8, 2017

hiroshige-g commented Sep 5, 2017 •

edited

Loading

domenic commented Sep 6, 2017

domenic commented Sep 8, 2017

hiroshige-g Sep 8, 2017

domenic Sep 8, 2017

hiroshige-g Sep 8, 2017

domenic Sep 8, 2017

domenic commented Sep 14, 2017 •

edited

Loading

nyaxt commented Sep 25, 2017

domenic commented Oct 12, 2017

domenic commented Oct 13, 2017

Fix module script error handling, again #2991

Fix module script error handling, again #2991

Conversation

domenic commented Aug 31, 2017 • edited Loading

hiroshige-g Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroshige-g Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroshige-g commented Sep 5, 2017 • edited Loading

domenic commented Sep 6, 2017

domenic commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Sep 14, 2017 • edited Loading

nyaxt commented Sep 25, 2017

domenic commented Oct 12, 2017

domenic commented Oct 13, 2017

domenic commented Aug 31, 2017 •

edited

Loading

hiroshige-g Sep 5, 2017 •

edited

Loading

hiroshige-g Sep 5, 2017 •

edited

Loading

hiroshige-g commented Sep 5, 2017 •

edited

Loading

domenic commented Sep 14, 2017 •

edited

Loading