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 module script error handling, again #2991

Merged
merged 1 commit into from Oct 13, 2017

Conversation

3 participants
@domenic
Copy link
Member

domenic commented Aug 31, 2017

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

source Outdated
<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,

This comment has been minimized.

@hiroshige-g

hiroshige-g Sep 5, 2017

Contributor

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

This comment has been minimized.

@domenic

domenic Sep 6, 2017

Member

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

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>

This comment has been minimized.

@hiroshige-g

hiroshige-g Sep 5, 2017

Contributor

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

This comment has been minimized.

@domenic

domenic Sep 6, 2017

Member

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.

This comment has been minimized.

@domenic

domenic Sep 6, 2017

Member

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?

This comment has been minimized.

@hiroshige-g

hiroshige-g Sep 7, 2017

Contributor

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.

This comment has been minimized.

@domenic

domenic Sep 7, 2017

Member

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.

This comment has been minimized.

@hiroshige-g

hiroshige-g Sep 7, 2017

Contributor

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.

This comment has been minimized.

@domenic

domenic Sep 8, 2017

Member

Filed #3025 to track.

@hiroshige-g

This comment has been minimized.

Copy link
Contributor

hiroshige-g commented Sep 5, 2017

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

  1. 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".
  2. 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)

domenic added a commit that referenced this pull request Sep 6, 2017

Editorial: decrease traversals for module graph fetching
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 domenic force-pushed the module-fixes-6 branch from 491afb1 to 751ec3e Sep 6, 2017

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Sep 6, 2017

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

This comment has been minimized.

Copy link
Member

domenic commented Sep 8, 2017

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

source Outdated
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

This comment has been minimized.

@hiroshige-g

hiroshige-g Sep 8, 2017

Contributor

[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.

This comment has been minimized.

@domenic

domenic Sep 8, 2017

Member

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

source Outdated

<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

This comment has been minimized.

@hiroshige-g

hiroshige-g Sep 8, 2017

Contributor

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.

This comment has been minimized.

@domenic

domenic Sep 8, 2017

Member

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

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Sep 14, 2017

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.

MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017

Hiroshige Hayashizaki Commit Bot
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 <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503034}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017

Revert "Flatten ModuleTreeLinker"
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 <hiroshige@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503034}

TBR=hiroshige@chromium.org,ksakamoto@chromium.org,kouhei@chromium.org,nhiroki@chromium.org

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 <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503059}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017

Revert "Flatten ModuleTreeLinker"
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 <hiroshige@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503034}

TBR=hiroshige@chromium.org,ksakamoto@chromium.org,kouhei@chromium.org,nhiroki@chromium.org

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 <calamity@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503063}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017

Hiroshige Hayashizaki Commit Bot
Reland "Flatten ModuleTreeLinker"
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 <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503126}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017

[ES6 modules] WPT: mark error test cases as flaky
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.

TBR=hiroshige@chromium.org

Bug: 767093, 763597
Change-Id: I9ef17f2ae6dc51acbef4dbab1f7f7c7438d17eb7
Reviewed-on: https://chromium-review.googlesource.com/676403
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503304}
@nyaxt

This comment has been minimized.

Copy link
Member

nyaxt commented Sep 25, 2017

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.

MXEBot pushed a commit to mirror/chromium that referenced this pull request Oct 6, 2017

Hiroshige Hayashizaki Commit Bot
Reduce usage of ModuleScript::IsErrored() and HasInstantiated() in tests
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 <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506947}

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

[ES6 modules] HRIM may return errored module until V8 forgets instant…
…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 added a commit to web-platform-tests/wpt that referenced this pull request Oct 11, 2017

[ES6 modules] HRIM may return errored module until V8 forgets instant…
…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

[ES6 modules] HRIM may return errored module until V8 forgets instant…
…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}
Fix module script error handling, again
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 domenic force-pushed the module-fixes-6 branch from 7a7e768 to 9a25217 Oct 12, 2017

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Oct 12, 2017

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

This comment has been minimized.

Copy link
Member

domenic commented Oct 13, 2017

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

@domenic domenic removed the needs tests label Oct 13, 2017

@nyaxt

nyaxt approved these changes Oct 13, 2017

@domenic domenic merged commit 165101a into master Oct 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@domenic domenic deleted the module-fixes-6 branch Oct 13, 2017

jakearchibald added a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017

[ES6 modules] HRIM may return errored module until V8 forgets instant…
…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}

inexorabletash added a commit to inexorabletash/web-platform-tests that referenced this pull request Nov 27, 2017

[ES6 modules] HRIM may return errored module until V8 forgets instant…
…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 added a commit to web-platform-tests/wpt that referenced this pull request Dec 1, 2017

[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
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

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2017

[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
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

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 18, 2017

[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
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

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 18, 2017

[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
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

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 19, 2017

[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
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 <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525168}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 19, 2017

[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
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 <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525168}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Dec 20, 2017

Hiroshige Hayashizaki Commit Bot
[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
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 <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525168}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Dec 20, 2017

Hiroshige Hayashizaki Commit Bot
Remove ScriptModule::Status()/ErrorCompletion()
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 <hiroshige@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525235}

nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018

Hiroshige Hayashizaki Commit Bot
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 <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503034}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 5a168dfca798f0088b3257f21781a66b4132ba7b

nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018

Revert "Flatten ModuleTreeLinker"
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 <hiroshige@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503034}

TBR=hiroshige@chromium.org,ksakamoto@chromium.org,kouhei@chromium.org,nhiroki@chromium.org

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 <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503059}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: e81cbfd9f8b19631241e926ebfe66aabedc0ef53

nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018

Hiroshige Hayashizaki Commit Bot
Reland "Flatten ModuleTreeLinker"
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 <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503126}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: b2ca65cb5a61eb6f69a573a336cbd1f9ea794fcd

nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018

[ES6 modules] WPT: mark error test cases as flaky
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.

TBR=hiroshige@chromium.org

Bug: 767093, 763597
Change-Id: I9ef17f2ae6dc51acbef4dbab1f7f7c7438d17eb7
Reviewed-on: https://chromium-review.googlesource.com/676403
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503304}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: e35c1229109f3eb213a4f62ebb27b1b864e5ba10

nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018

Hiroshige Hayashizaki Commit Bot
Reduce usage of ModuleScript::IsErrored() and HasInstantiated() in tests
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 <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#506947}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 56367281f9b94f9e151770e0a5da5f74623690e5

alice added a commit to alice/html that referenced this pull request Jan 8, 2019

Editorial: decrease traversals for module graph fetching
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment