[ES6 modules] Return previous error when an instantiation is reattempt.

[ES6 modules] Return previous error when an instantiation is reattempt.

Before this CL, when instantiation was reattempt for previously errored module,
ModuleTreeLinker::Instantiate() crashed.

This CL updates the code to follow whatwg/html update:
https://github.com/whatwg/html/pull/2559
so that we rethrow previous error when instantiation is reattempt.
TEST=webkit_unit_test --gtest_filter=ModuleTreeLinkerTest.FetchTreePreviousInstantiationFailure
BUG=594639, 714521
Review-Url: https://codereview.chromium.org/2839563002
Cr-Commit-Position: refs/heads/master@{#467882}
Committed: https://chromium.googlesource.com/chromium/src/+/321b6ea1a1e2f392c629e2f074ae509a3aaefc42

[kouhei (in TOK), 2017-04-24 07:04:50]

kouhei@chromium.org changed reviewers:
+ hiroshige@chromium.org, japhet@chromium.org, neis@chromium.org,
yhirano@chromium.org

[kouhei (in TOK), 2017-04-24 07:04:56]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 07:05:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kouhei (in TOK), 2017-04-24 07:07:05]

Description was changed from

==========
[ES6 modules] Return previous error when an instantiation is reattempt.

Before this CL, when instantiation was reattempt for previously errored module,
ModuleTreeLinker::Instantiate() crashed.

This CL updates the code to follow whatwg/html update:
https://github.com/whatwg/html/pull/2559
so that we rethrow previous error when instantiation is reattempt.

BUG=594639,714521
==========

to

==========
[ES6 modules] Return previous error when an instantiation is reattempt.

Before this CL, when instantiation was reattempt for previously errored module,
ModuleTreeLinker::Instantiate() crashed.

This CL updates the code to follow whatwg/html update:
https://github.com/whatwg/html/pull/2559
so that we rethrow previous error when instantiation is reattempt.
TEST=webkit_unit_test
--gtest_filter=ModuleTreeLinkerTest.FetchTreePreviousInstantiationFailure
BUG=594639,714521
==========

[commit-bot: I haz the power, 2017-04-24 09:12:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 09:12:55]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-04-24 09:46:05]

kouhei@chromium.org changed reviewers:
+ domenic@chromium.org

[kouhei (in TOK), 2017-04-24 09:46:05]

cc: +module-dev

[kouhei (in TOK), 2017-04-24 09:46:07]

cc: +module-dev

[yhirano, 2017-04-24 10:06:47]

https://github.com/whatwg/html/pull/2559

It seems domenic expects you to write a WPT, right? Should it be included in
this CL?

[kouhei (in TOK), 2017-04-25 20:31:39]

On 2017/04/24 10:06:47, yhirano wrote:
> https://github.com/whatwg/html/pull/2559
> 
> It seems domenic expects you to write a WPT, right? Should it be included in
> this CL?

This is being added in https://codereview.chromium.org/2839913002/

[hiroshige, 2017-04-27 22:56:40]

Basically looks good.

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModuleScript.h (right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModuleScript.h:59: v8::Local<v8::Value>
CreateInstantiationError(v8::Isolate* isolate) const {
ModulatorImpl::GetInstantiationError() and
ModuleScript::CreateInstantiationError() looks quite similar in its
interface and developers might be not sure which function to use.

I expect GetInstantiationError() should be preferred because it enforces
the correct |isolate| to be used, if we use the correct modulator.
So how about renaming CreateInstantiationError() to something like
CreateInstantiationErrorInternal() and adding a comment that this should
be used only from ModulatorImpl::GetInstantiationError()?

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:349:
error = modulator_->GetInstantiationError(module_script_);
DCHECK(!error.IsEmpty());

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
(right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:96:
EXPECT_EQ(url, pending_request_url_);
EXPECT_EQ(state, module_script->InstantiationState()) around here?

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:188:
ScriptState::Scope scope(script_state_.Get());
Can we make this just the same as ModulatorImpl's one?

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:294:
TEST_F(ModuleTreeLinkerTest, FetchTreePreviousInstantiationFailure) {
This looks almost the same as FetchTreeInstantiationFailure.
Probably we should do an additional Fetch() call at the end of this test.

[kouhei (in TOK), 2017-04-28 00:55:09]

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModuleScript.h (right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModuleScript.h:59: v8::Local<v8::Value>
CreateInstantiationError(v8::Isolate* isolate) const {
On 2017/04/27 22:56:40, hiroshige wrote:
> ModulatorImpl::GetInstantiationError() and
> ModuleScript::CreateInstantiationError() looks quite similar in its
> interface and developers might be not sure which function to use.
> 
> I expect GetInstantiationError() should be preferred because it enforces
> the correct |isolate| to be used, if we use the correct modulator.
> So how about renaming CreateInstantiationError() to something like
> CreateInstantiationErrorInternal() and adding a comment that this should
> be used only from ModulatorImpl::GetInstantiationError()?
Yes. That's the idea. Done.

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:349:
error = modulator_->GetInstantiationError(module_script_);
On 2017/04/27 22:56:40, hiroshige wrote:
> DCHECK(!error.IsEmpty());

Done.

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
(right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:96:
EXPECT_EQ(url, pending_request_url_);
On 2017/04/27 22:56:40, hiroshige wrote:
> EXPECT_EQ(state, module_script->InstantiationState()) around here?

Done.

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:188:
ScriptState::Scope scope(script_state_.Get());
On 2017/04/27 22:56:40, hiroshige wrote:
> Can we make this just the same as ModulatorImpl's one?

Done.

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:294:
TEST_F(ModuleTreeLinkerTest, FetchTreePreviousInstantiationFailure) {
On 2017/04/27 22:56:40, hiroshige wrote:
> This looks almost the same as FetchTreeInstantiationFailure.
> Probably we should do an additional Fetch() call at the end of this test.

It is similar, but I'd like to keep them separate.
- ModuleTreeLinkerModulator doesn't have a "real" module map to dedupe
previously resolved module fetch.
- This test is expected to fail without Modulator::InstantiateModule().

[hiroshige, 2017-04-28 01:28:35]

lgtm with comments.

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModuleScript.h (right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModuleScript.h:59: v8::Local<v8::Value>
CreateInstantiationError(v8::Isolate* isolate) const {
On 2017/04/28 00:55:09, kouhei wrote:
> On 2017/04/27 22:56:40, hiroshige wrote:
> > ModulatorImpl::GetInstantiationError() and
> > ModuleScript::CreateInstantiationError() looks quite similar in its
> > interface and developers might be not sure which function to use.
> > 
> > I expect GetInstantiationError() should be preferred because it enforces
> > the correct |isolate| to be used, if we use the correct modulator.
> > So how about renaming CreateInstantiationError() to something like
> > CreateInstantiationErrorInternal() and adding a comment that this should
> > be used only from ModulatorImpl::GetInstantiationError()?
> Yes. That's the idea. Done.

nit: Probably unifying the Create...() and Get...() prefixes might be better.

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
(right):

https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:294:
TEST_F(ModuleTreeLinkerTest, FetchTreePreviousInstantiationFailure) {
On 2017/04/28 00:55:09, kouhei wrote:
> On 2017/04/27 22:56:40, hiroshige wrote:
> > This looks almost the same as FetchTreeInstantiationFailure.
> > Probably we should do an additional Fetch() call at the end of this test.
> 
> It is similar, but I'd like to keep them separate.
> - ModuleTreeLinkerModulator doesn't have a "real" module map to dedupe
> previously resolved module fetch.
> - This test is expected to fail without Modulator::InstantiateModule().

Oh, understood.
So
> GetModulator()->ResolveSingleModuleScriptFetch(
>       url, {}, ModuleInstantiationState::kErrored);
Emulates "previous instantiation failure", and
this tests the case where Fetch() results in kErrored when *previous*
instantiation is failed.

And FetchTreeInstantiationFailure tests the case where
Fetch() results in kErrored when *current* instantiation is failed
emulated by SetInstantiateShouldFail(true);

Probably adding some comments might be helpful.

[kouhei (in TOK), 2017-04-28 01:48:21]

On 2017/04/28 01:28:35, hiroshige wrote:
> lgtm with comments.
> 
>
https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/ModuleScript.h (right):
> 
>
https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/ModuleScript.h:59: v8::Local<v8::Value>
> CreateInstantiationError(v8::Isolate* isolate) const {
> On 2017/04/28 00:55:09, kouhei wrote:
> > On 2017/04/27 22:56:40, hiroshige wrote:
> > > ModulatorImpl::GetInstantiationError() and
> > > ModuleScript::CreateInstantiationError() looks quite similar in its
> > > interface and developers might be not sure which function to use.
> > > 
> > > I expect GetInstantiationError() should be preferred because it enforces
> > > the correct |isolate| to be used, if we use the correct modulator.
> > > So how about renaming CreateInstantiationError() to something like
> > > CreateInstantiationErrorInternal() and adding a comment that this should
> > > be used only from ModulatorImpl::GetInstantiationError()?
> > Yes. That's the idea. Done.
> 
> nit: Probably unifying the Create...() and Get...() prefixes might be better.
I'd like to address this separately.

>
https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
> File
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2839563002/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:294:
> TEST_F(ModuleTreeLinkerTest, FetchTreePreviousInstantiationFailure) {
> On 2017/04/28 00:55:09, kouhei wrote:
> > On 2017/04/27 22:56:40, hiroshige wrote:
> > > This looks almost the same as FetchTreeInstantiationFailure.
> > > Probably we should do an additional Fetch() call at the end of this test.
> > 
> > It is similar, but I'd like to keep them separate.
> > - ModuleTreeLinkerModulator doesn't have a "real" module map to dedupe
> > previously resolved module fetch.
> > - This test is expected to fail without Modulator::InstantiateModule().
> 
> Oh, understood.
> So
> > GetModulator()->ResolveSingleModuleScriptFetch(
> >       url, {}, ModuleInstantiationState::kErrored);
> Emulates "previous instantiation failure", and
> this tests the case where Fetch() results in kErrored when *previous*
> instantiation is failed.
> 
> And FetchTreeInstantiationFailure tests the case where
> Fetch() results in kErrored when *current* instantiation is failed
> emulated by SetInstantiateShouldFail(true);
> 
> Probably adding some comments might be helpful.

Done.

[kouhei (in TOK), 2017-04-28 01:48:30]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-04-28 01:48:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/2839563002/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2017-04-28 01:49:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 03:57:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493344110402020,
"parent_rev": "fa0a7e66dd1583ab71436f26ee362278437e1972", "commit_rev":
"321b6ea1a1e2f392c629e2f074ae509a3aaefc42"}

[commit-bot: I haz the power, 2017-04-28 03:58:29]

Description was changed from

==========
[ES6 modules] Return previous error when an instantiation is reattempt.

Before this CL, when instantiation was reattempt for previously errored module,
ModuleTreeLinker::Instantiate() crashed.

This CL updates the code to follow whatwg/html update:
https://github.com/whatwg/html/pull/2559
so that we rethrow previous error when instantiation is reattempt.
TEST=webkit_unit_test
--gtest_filter=ModuleTreeLinkerTest.FetchTreePreviousInstantiationFailure
BUG=594639,714521
==========

to

==========
[ES6 modules] Return previous error when an instantiation is reattempt.

Before this CL, when instantiation was reattempt for previously errored module,
ModuleTreeLinker::Instantiate() crashed.

This CL updates the code to follow whatwg/html update:
https://github.com/whatwg/html/pull/2559
so that we rethrow previous error when instantiation is reattempt.
TEST=webkit_unit_test
--gtest_filter=ModuleTreeLinkerTest.FetchTreePreviousInstantiationFailure
BUG=594639,714521

Review-Url: https://codereview.chromium.org/2839563002
Cr-Commit-Position: refs/heads/master@{#467882}
Committed:
https://chromium.googlesource.com/chromium/src/+/321b6ea1a1e2f392c629e2f074ae...
==========

[commit-bot: I haz the power, 2017-04-28 03:58:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/321b6ea1a1e2f392c629e2f074ae...