[ES6 modules] ModuleTreeLinker::Instantiate shouldn't proceed on invalid context

[ES6 modules] ModuleTreeLinker::Instantiate shouldn't proceed on invalid context

Before this CL, ModuleTreeLinker::Instantiate assumed that it is only called for
modulator with a valid context. However, asynchronous module graph node load
completion may be triggered after the context was destroyed.

This CL fixes the issue by making ModuleTreeLinker::Instantiate fail without crashing
if the context is invalid.

BUG=594639, 716935
Review-Url: https://codereview.chromium.org/2886593002
Cr-Commit-Position: refs/heads/master@{#472246}
Committed: https://chromium.googlesource.com/chromium/src/+/87dc11bdb4e1cf4ba674f3907fa1127f8839ad40

[kouhei (in TOK), 2017-05-15 22:55:18]

Description was changed from

==========
[ES6 modules] ModuleTreeLinker::Instantiate shouldn't proceed on invalid context

Before this CL, ModuleTreeLinker::Instantiate assumed that it is only called for
modulator with a valid context. However, asynchronous module graph node load
completion may be triggered after the context was destroyed.

This CL fixes the issue by making ModuleTreeLinker::Instantiate fail without
crashing
if the context is invalid.

BUG=594639,716935
==========

to

==========
[ES6 modules] ModuleTreeLinker::Instantiate shouldn't proceed on invalid context

Before this CL, ModuleTreeLinker::Instantiate assumed that it is only called for
modulator with a valid context. However, asynchronous module graph node load
completion may be triggered after the context was destroyed.

This CL fixes the issue by making ModuleTreeLinker::Instantiate fail without
crashing
if the context is invalid.

BUG=594639,716935
==========

[kouhei (in TOK), 2017-05-15 22:55:18]

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

[neis, 2017-05-16 09:10:55]

lgtm with nit, but I'm not very familiar with the modulator code.

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:114: bool
ModulatorImpl::IsContextValid() {
Could you rename this to ContextIsValid (for consistency), or maybe
HasValidContext?

[hiroshige, 2017-05-16 16:54:58]

lgtm with a comment.

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/dom/script-module-load-incomplete-no-crash.html
(right):

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/dom/script-module-load-incomplete-no-crash.html:2:
<title>Module script graph fetch in flight when document is destroyed shouldn't
crash.</title>
Could you add a comment that describes how this test causes document to be
destroyed while script is being fetched?

IIUC: this test expects the testrunner terminates the test and thus kills
document somewhere before the document load event, i.e. while the module script
is being fetched. And thus testharness.js is not used.

[hiroshige, 2017-05-16 17:09:19]

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

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:379: //
[nospec] Abort the steps if the browsing context is discarded.
(Not blocking this CL) if we need to check the context here, then do we also
need to check the context in other locations?

[kouhei (in TOK), 2017-05-16 17:52:35]

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:114: bool
ModulatorImpl::IsContextValid() {
On 2017/05/16 09:10:55, neis wrote:
> Could you rename this to ContextIsValid (for consistency), or maybe
> HasValidContext?

Done.

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

https://codereview.chromium.org/2886593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:379: //
[nospec] Abort the steps if the browsing context is discarded.
On 2017/05/16 17:09:19, hiroshige wrote:
> (Not blocking this CL) if we need to check the context here, then do we also
> need to check the context in other locations?

I checked other places when writing this CL, but this seems to be the only
location as of now.

[kouhei (in TOK), 2017-05-16 17:52:42]

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

[commit-bot: I haz the power, 2017-05-16 17:53:55]

Dry run: 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-05-16 19:51:42]

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

[commit-bot: I haz the power, 2017-05-16 19:51:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kouhei (in TOK), 2017-05-16 19:54:24]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-05-16 19:54:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, neis@chromium.org
Link to the patchset: https://codereview.chromium.org/2886593002/#ps20001
(title: "hasvalidcontxt")

[commit-bot: I haz the power, 2017-05-16 19:55:42]

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-05-16 23:28:25]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494964464067880,
"parent_rev": "5d609a1b0650829208d50d74c26d4dd8dd8e9dd7", "commit_rev":
"87dc11bdb4e1cf4ba674f3907fa1127f8839ad40"}

[commit-bot: I haz the power, 2017-05-16 23:28:37]

Description was changed from

==========
[ES6 modules] ModuleTreeLinker::Instantiate shouldn't proceed on invalid context

Before this CL, ModuleTreeLinker::Instantiate assumed that it is only called for
modulator with a valid context. However, asynchronous module graph node load
completion may be triggered after the context was destroyed.

This CL fixes the issue by making ModuleTreeLinker::Instantiate fail without
crashing
if the context is invalid.

BUG=594639,716935
==========

to

==========
[ES6 modules] ModuleTreeLinker::Instantiate shouldn't proceed on invalid context

Before this CL, ModuleTreeLinker::Instantiate assumed that it is only called for
modulator with a valid context. However, asynchronous module graph node load
completion may be triggered after the context was destroyed.

This CL fixes the issue by making ModuleTreeLinker::Instantiate fail without
crashing
if the context is invalid.

BUG=594639,716935

Review-Url: https://codereview.chromium.org/2886593002
Cr-Commit-Position: refs/heads/master@{#472246}
Committed:
https://chromium.googlesource.com/chromium/src/+/87dc11bdb4e1cf4ba674f3907fa1...
==========

[commit-bot: I haz the power, 2017-05-16 23:28:41]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/87dc11bdb4e1cf4ba674f3907fa1...