[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic [chromium/src : master]
Hiroshige Hayashizaki (Gerrit)
PTAL,
kouhei@ nhiroki@ ksakamoto@, in general, especially for Blink changes,
neis@ mainly for test updates.
I tested this CL with and without Georg's V8-side CL, and added/modified tests accordingly.
I observed no serious issues (e.g. crashing) in the module wpt tests and webkit_unit_tests, but I expect there're still rooms for improving the test coverage.
As we are going to remove GetStatus() method, and GetStatus() calls in non-test/non-DCHECK() code are removed (because in the new spec they are not needed), but I still have to replace/remove DCHECK()s and unit test expectations that uses GetStatus().
Could you take a look at them (I added inline comments below) how to handle them?
Most of Blink code changes are needed to be done in a single CL.
(I might be able to split out some of test changes into separate CLs)
11 comments:
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #20, Line 3262:
crbug.com/763597 external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html [ Failure Pass ]instantiation-error-*.html in the current patchset passes even just after this CL (might be failing flakily though), because I relaxed the test expectations.
Patch Set #20, Line 3268:
crbug.com/763597 external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html [ Failure Pass ]These three tests (Lines 3268-3270) fails just after this CL, and will pass once neis@'s v8-side CL is landed.
Patch Set #20, Line 27:
assert_equals(log[9], 5);neis@, should we also check that each exception object are distinct? i.e.
assert_not_equals(log[1], log[3]) etc.
Probably ditto for instantiation-error-{2,3,4}.html
Patch Set #20, Line 22:
assert_not_equals(log[0], log[2], "errors should be different");In this case we can/should assert the errors are different object, because they are different instantiation errors caused by different module scripts IIUC.
File third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp:
I removed this because we check Evaluate()'s result is empty (i.e. no evaluation error).
Replaces this Status() check with an explicit check of Evaluate() result in Line 312.
Patch Set #20, Line 230:
TEST(ScriptModuleTest, DISABLED_EvaluationErrrorIsRemembered) {I confirmed that this test passes if neis@'s v8-side CL is applied.
File third_party/WebKit/Source/core/dom/ModulePendingScript.cpp:
See comments in ModuleTreeLinker::AdvanceState().
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:
Patch Set #20, Line 131:
DCHECK(!result_ || result_->IsReadyForEvaluationForAssert())Previously, ModulePendingScriptTreeClient::NotifyModuleTreeLoadFinished() has an assertion to check that the returned module script is ready for evaluation, or has an error.
After this CL (and the spec update), the concept of "is ready for evaluation" (instantiated successfully, evaluated successfully, or has evaluation error) cannot be checked from Blink/HTML-spec side.
How can, or should we keep the assertion?
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:
Can we remove this test (and thus Lines 243--258 on the right), as we no longer assumes something about MTL with a module script graph that experienced previous instantiation error, because we no longer remember instantiation errors.
(Strictly speaking, we expect that instantiation errors are not remembered, but if we want to test it probably we should add a new test where the not-remembering instantiation error affects observable changes)Patch Set #20, Line 97:
script_state_->GetIsolate(), "Parse failure.");I changed this to a parse error (from instantiation error), because MTL is basically no longer affected by instantiation errors.
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/w3c/web-platform-tests/pull/8527.
If this CL lands and Travis CI upstream is green, we will auto-merge the PR.
Note: Please check the Travis CI status (at the bottom of the PR) before landing this CL and only land this CL if the status is green. Otherwise a human needs to step in and resolve it manually. (This may be automated in the future, see https://crbug.com/711447)
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md#Automatic-export-process
Georg Neis (Gerrit)
1 comment:
Patch Set #20, Line 27:
assert_equals(log[9], 5);
neis@, should we also check that each exception object are distinct? i.e. […]
Yes, we should. Do I understand correctly, that errors 1, 2 and 4 are still guaranteed to be the same by HTML? Equally for 3 and 5.
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Georg Neis (Gerrit)
As we are going to remove GetStatus() method, and GetStatus() calls in non-test/non-DCHECK() code are removed (because in the new spec they are not needed), but I still have to replace/remove DCHECK()s and unit test expectations that uses GetStatus().
Could you take a look at them (I added inline comments below) how to handle them?
I have decided to keep GetStatus and GetException in v8's api, so you could just keep them in your DCHECKs. But note that GetStatus will return "uninstantiated" for a module that failed to instantiate, and GetException must only be called when evaluation failed (i.e. when GetStatus returns "errored").
Hiroshige Hayashizaki (Gerrit)
1 comment:
Patch Set #20, Line 27:
assert_equals(log[9], 5);
Do I understand correctly, that errors 1, 2 and 4 are still guaranteed to be the same by HTML?
The situation was more complicated than I thought.
- According to the spec: Yes they will be the same in this case, however:
- It looks like somehow being the same accidentially, because Instantiate() is called three time and one error object returned by one Instantiate() invocation is reported to all three <script>s.
- Error object identity can be nondeterministic (e.g. affected by the network fetch order) in general, and I feel developers can't rely on error object identity.
So if we want to check the error object identity (1==2==4 and 3==5), I think we'll have to make the spec more deterministic as for error object identity.
- According to the Blink impl, No, they are all distinct.
This is because, in the spec, Step 8 of
https://html.spec.whatwg.org/#fetch-the-descendants-of-and-instantiate-a-module-script
is asynchronous, while in Blink it completes synchronously.Instantiation is executed once for each <script> element, and thus the error object created for each <script> element is distinct (if ECMAScript-side spec makes instantiation return distinct error objects for each invocation for the same module script).
But the error objects are put to the same place, i.e. the <script> element's error-to-rethrow, and thus in some cases, some of error objects reported to the <script> elements can be the same, because error-to-rethrow can overwritten before execution.
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
+domenic@ for spec/testing discussions.
(Currently there's discussion about error object identity in instantiation-error-1.html)
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki would like Domenic Denicola to review this change.
[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
Spec update: https://github.com/whatwg/html/pull/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
---
M third_party/WebKit/LayoutTests/TestExpectations
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-1.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-1a.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-1b.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-2.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-2a.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-2b.js
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-1.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-1.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-2.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-2.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-3.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-3.js
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-4.html
D third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-5-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-5.html
M third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp
M third_party/WebKit/Source/core/dom/DynamicModuleResolver.cpp
M third_party/WebKit/Source/core/dom/DynamicModuleResolverTest.cpp
M third_party/WebKit/Source/core/dom/Modulator.h
M third_party/WebKit/Source/core/dom/ModulatorImplBase.cpp
M third_party/WebKit/Source/core/dom/ModulatorImplBase.h
M third_party/WebKit/Source/core/dom/ModulePendingScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.h
M third_party/WebKit/Source/core/workers/WorkletModuleTreeClient.cpp
38 files changed, 508 insertions(+), 433 deletions(-)
Kouhei Ueno (Gerrit)
lgtm % foxmes
Patch set 20:Code-Review +1
7 comments:
File third_party/WebKit/Source/core/dom/ModulatorImplBase.cpp:
Patch Set #20, Line 175:
// The settings object is |this|.module_script->DCheckSettingsObject(this) if you want.
File third_party/WebKit/Source/core/dom/ModuleScript.cpp:
modulator->GetExecutionContext()->CanExecuteScripts(kAboutToExecuteScript)
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:
Patch Set #20, Line 126:
// should be either: - nullptr (fetch error), - module script with an errorreformat err
Patch Set #20, Line 131:
DCHECK(!result_ || result_->IsReadyForEvaluationForAssert())
Previously, ModulePendingScriptTreeClient::NotifyModuleTreeLoadFinished() has an assertion to check […]
I'm ok simply removing this DCHECK
Patch Set #20, Line 400:
// skip FindFirstParseError() call.DCHECK(FindFirstParseError().IsEmpty()) then?
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:
Can we remove this test (and thus Lines 243--258 on the right), as we no longer assumes something ab […]
remove sgtm
Patch Set #20, Line 97:
script_state_->GetIsolate(), "Parse failure.");
I changed this to a parse error (from instantiation error), because MTL is basically no longer affec […]
Should this be a generic Error or does this need to be a specific type of error?
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Domenic Denicola (Gerrit)
1 comment:
Patch Set #20, Line 27:
assert_equals(log[9], 5);
> Do I understand correctly, that errors 1, 2 and 4 are still guaranteed to be the same by HTML? […]
So the general principle in the current spec is that error identity is not important to preserve. But, the spec still preserves it some times, in determistic ways.
So we should write tests for what the spec says, since it's implementable unambiguously, even if we're not testing a property that's useful for developers.
In particular, we should add tests that 1 === 2 and 2 === 4, but that others are distinct. Just because that's what the spec says, and we want interoperable implementations there.
This is somewhat similar to how we write tests for silly things like `imgEl.decode.length === 0`, even though it's not a useful property for developers. It's still specified, and we want even these things to be interoperable and tested.
Does that make sense?
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Georg Neis (Gerrit)
1 comment:
Patch Set #20, Line 27:
assert_equals(log[9], 5);
So the general principle in the current spec is that error identity is not important to preserve. […]
Hiroshige's comment sounds like it's fundamentally hard to implement the specified behavior in blink. Is that right?
Independent of that, I'm trying to understand if the identity of some of these errors is something we explicitly desired or is rather a consequence of how things are set up. One of the key conclusions from our module summit was "don't record instantiation errors", and I didn't realize until recently that we are in fact recording instantiation errors in HTML.
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Domenic Denicola (Gerrit)
1 comment:
Patch Set #20, Line 27:
assert_equals(log[9], 5);
Hiroshige's comment sounds like it's fundamentally hard to implement the specified behavior in blink […]
My takeaway was more "don't preserve instantiation error identity", not "ensure every instantiation error has a unique identity".
That said, if it is easier to implement re-generating a unique instantiation error every time, we can try to update the spec to match. I'm a little worried though about this Blink/spec mismatch on sync/async though; there might be a deeper problem there. I guess I did not intend "async" to necessarily mean "event loop async", so a sync implementation should be fine. I've filed https://github.com/whatwg/infra/issues/181 to try to clarify this at the spec level.
Perhaps the right path is to move toward "ensure every instantiation error has a unique identity" since that allows more flexible implementation strategies?
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "rebase fix": https://github.com/w3c/web-platform-tests/pull/8527
Hiroshige Hayashizaki (Gerrit)
Addressed comments in the implementation code.
I removed DCHECK()s using GetStatus() etc.
I think that kind of conditions are checked in V8 side (in the impl for ECMAscript spec).
Double-checking the conditions in Blink side might be helpful for catching bugs, but I slightly perfer removing those DCHECK()s and removing the calls to GetStatus()/GetException() completely from Blink, in order to make it clear that Blink (i.e. HTML-spec-side implementation) is implemented without any dependencies to any state changes after instantiation.
8 comments:
Patch Set #20, Line 175:
// The settings object is |this|.module_script->DCheckSettingsObject(this) if you want.
If we want to do this, we should do it separately because there are many other locations we should check.
(I don't have immediate motivations to do that though)
File third_party/WebKit/Source/core/dom/ModulePendingScript.cpp:
See comments in ModuleTreeLinker::AdvanceState().
As discussed there, this DCHECK() was removed.
File third_party/WebKit/Source/core/dom/ModuleScript.cpp:
modulator->GetExecutionContext()->CanExecuteScripts(kAboutToExecuteScript)
I'll do in a separete CL.
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:
Patch Set #20, Line 126:
// result.reformat err
Removed.
Patch Set #20, Line 131:
void ModuleTreeLinker::FetchRoot(const ModuleScriptFetchRequest& request) {
I'm ok simply removing this DCHECK
Removed.
Patch Set #20, Line 400:
ScriptValue instantiation_error = modulator_->InstantiateModule(record);DCHECK(FindFirstParseError(). […]
Done
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:
remove sgtm
Removed.
Patch Set #20, Line 97:
script_state_->GetIsolate(), "Parse failure.");
Should this be a generic Error or does this need to be a specific type of error?
The unit tests don't check the specific type of errors.
According to the spec, the parse error can be SyntaxError or ReferenceError (errors caused in ECMAscript-side ParseModule()) or TypeError (when "resolving a module specifier" fails).
I feel it's OK to leave as a generic Error unless we want to test specific error cases, but it's also fine with me to change this into e.g. SyntaxError. Do you have preferences for using specific error types?
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
1 comment:
Patch Set #20, Line 27:
assert_equals(log[9], 5);
Sorry for delay.
In the context of this CL, I'd like to land the tests without checking the error object identity, and do the spec discussion separately.
Split out my responses and thoughts, and summarized the discussion in a doc:
https://docs.google.com/a/chromium.org/document/d/1MUnJ1Cp4l1yGNDxrRRV-35odkoqALrqhutBL_15Hlfc/edit?usp=sharingHiroshige's comment sounds like it's fundamentally hard to implement the specified behavior in blink. Is that right?
Yes. Because this is like a race condition.
We have to make the behaviors spec-conformant everywhere (not only in module-related code but also in loading/parsing), because the race condition can be broken by a single sync/async difference.
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Georg Neis (Gerrit)
1 comment:
Patch Set #20, Line 27:
assert_equals(log[9], 5);
Sorry for delay. […]
ACK to land now and discuss separately.
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki uploaded patch set #23 to this change.
[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
Spec update: https://github.com/whatwg/html/pull/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.
38 files changed, 488 insertions(+), 437 deletions(-)
To view, visit change 698467. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/w3c/web-platform-tests/pull/8527
Hiroshige Hayashizaki (Gerrit)
Slightly updated tests:
- Updated stale comments.
- Added choice-of-error-3.html to check that evaluation errors are remembered/cached.
- Replaced errorhandling-parseerror-dependent.js with syntaxerror.js.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Test refine": https://github.com/w3c/web-platform-tests/pull/8527
Kouhei Ueno (Gerrit)
still cr+1
Hiroshige Hayashizaki (Gerrit)
Patch set 24:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Test refine" https://chromium-review.googlesource.com/c/698467/24
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/698467/24
Bot data: {"action": "start", "triggered_at": "2017-12-19T21:56:27.0Z", "cq_cfg_revision": "449ceedce2207329148e0a132f0c79a46cde3381", "revision": "bf412764a2a0c9096846b49595ac2bd11a2d5378"}
Commit Bot (Gerrit)
Commit Bot merged this change.
[ES6 Module] Reflect Spec PR 2991 to make error handling deterministic
Spec update: https://github.com/whatwg/html/pull/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 <hiro...@chromium.org>
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525168}
---
M third_party/WebKit/LayoutTests/TestExpectations
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-1.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-1a.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-1b.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-2.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-2a.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-2b.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-3.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-3a.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/choice-of-error-3b.js
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-1.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-1.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-2.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-2.js
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-3.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/error-type-3.js
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-4.html
D third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-5-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-5.html
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/throw2.js
M third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp
M third_party/WebKit/Source/core/dom/DynamicModuleResolver.cpp
M third_party/WebKit/Source/core/dom/DynamicModuleResolverTest.cpp
M third_party/WebKit/Source/core/dom/Modulator.h
M third_party/WebKit/Source/core/dom/ModulatorImplBase.cpp
M third_party/WebKit/Source/core/dom/ModulatorImplBase.h
M third_party/WebKit/Source/core/dom/ModulePendingScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.h
M third_party/WebKit/Source/core/workers/WorkletModuleTreeClient.cpp
42 files changed, 533 insertions(+), 437 deletions(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/w3c/web-platform-tests/pull/8527