[ES6 modules] accessing document.currentScript on module scripts should not crash

[ES6 modules] accessing document.currentScript on module scripts should not crash

Before this CL, accessing document.currentScript caused crash. It dereferenced nullptr
pushed to Document::current_script_stack_ by ScriptLoader::DoExecuteScript() for
script == ScriptType::kModule.

This CL adds null check in Document::currentScriptForBinding so that it is aware that
the item on the stack may be nullptr.

TEST=wpt/html/semantics/scripting-1/the-script-element/module/currentScript-null.html
BUG=716951
Review-Url: https://codereview.chromium.org/2860913002
Cr-Commit-Position: refs/heads/master@{#469435}
Committed: https://chromium.googlesource.com/chromium/src/+/3d379f511131f8692bc7f46e617794d685935a0b

[kouhei (in TOK), 2017-05-04 01:34:46]

kouhei@chromium.org changed reviewers:
+ adamk@chromium.org, domenic@chromium.org, hiroshige@chromium.org,
japhet@chromium.org, module-dev@chromium.org

[kouhei (in TOK), 2017-05-04 01:34:51]

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

[commit-bot: I haz the power, 2017-05-04 01:35:40]

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-05-04 02:10:47]

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

[kouhei (in TOK), 2017-05-04 02:10:47]

+haraken as I saw him online

[domenic, 2017-05-04 02:32:54]

It would be good to add a test that it's also null when no import statements are
involved and the script is not inline. So something like

<script type="module" src="set-current-script-on-window.js"></script>
<script>
// test boilerplate...
assert_equals(window.currentScriptRecorded, null);
</script>

// set-current-script-on-window.js
window.currentScriptRecorded = document.currentScript;

[commit-bot: I haz the power, 2017-05-04 03:16:55]

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

[commit-bot: I haz the power, 2017-05-04 03:16:57]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-05-04 14:43:00]

LGTM with domenic's comment addressed.

[kouhei (in TOK), 2017-05-04 15:09:53]

On 2017/05/04 02:32:54, domenic wrote:
> It would be good to add a test that it's also null when no import statements
are
> involved and the script is not inline. So something like
> 
> <script type="module" src="set-current-script-on-window.js"></script>
> <script>
> // test boilerplate...
> assert_equals(window.currentScriptRecorded, null);
> </script>
> 
> // set-current-script-on-window.js
> window.currentScriptRecorded = document.currentScript;

Done.

[domenic, 2017-05-04 15:13:00]

lgtm

https://codereview.chromium.org/2860913002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/currentScript-null.html
(right):

https://codereview.chromium.org/2860913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/currentScript-null.html:2:
<title>html-script-module-crossOrigin</title>
Nit: title is not correct. Can just be removed; those Microsoft tests are bad
form in duplicating the filename into the <title>

[kouhei (in TOK), 2017-05-04 16:32:05]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-05-04 16:32:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, domenic@chromium.org
Link to the patchset: https://codereview.chromium.org/2860913002/#ps40001
(title: "remove bad title")

[commit-bot: I haz the power, 2017-05-04 16:32:48]

CQ is trying da patch.

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

[hiroshige, 2017-05-04 16:41:56]

Filename format looks inconsistent:
currentscript.js (all lowercase letters)
currentScript-null.html (with a uppercase letter)

[hiroshige, 2017-05-04 16:48:22]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-05-04 18:58:52]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-05-04 18:59:16]

On 2017/05/04 16:41:56, hiroshige wrote:
> Filename format looks inconsistent:
> currentscript.js (all lowercase letters)
> currentScript-null.html (with a uppercase letter)

Committing anyway to fix the crash. Let's fix the tests later soon.

[commit-bot: I haz the power, 2017-05-04 18:59: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-05-04 19:53:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493924332236320,
"parent_rev": "cb0550026886b2cdcd7bb9c975910dcb85056b4c", "commit_rev":
"3d379f511131f8692bc7f46e617794d685935a0b"}

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

Description was changed from

==========
[ES6 modules] accessing document.currentScript on module scripts should not
crash

Before this CL, accessing document.currentScript caused crash. It dereferenced
nullptr
pushed to Document::current_script_stack_ by ScriptLoader::DoExecuteScript() for
script == ScriptType::kModule.

This CL adds null check in Document::currentScriptForBinding so that it is aware
that
the item on the stack may be nullptr.

TEST=wpt/html/semantics/scripting-1/the-script-element/module/currentScript-null.html
BUG=716951
==========

to

==========
[ES6 modules] accessing document.currentScript on module scripts should not
crash

Before this CL, accessing document.currentScript caused crash. It dereferenced
nullptr
pushed to Document::current_script_stack_ by ScriptLoader::DoExecuteScript() for
script == ScriptType::kModule.

This CL adds null check in Document::currentScriptForBinding so that it is aware
that
the item on the stack may be nullptr.

TEST=wpt/html/semantics/scripting-1/the-script-element/module/currentScript-null.html
BUG=716951

Review-Url: https://codereview.chromium.org/2860913002
Cr-Commit-Position: refs/heads/master@{#469435}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d379f511131f8692bc7f46e6177...
==========

[commit-bot: I haz the power, 2017-05-04 19:54:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3d379f511131f8692bc7f46e6177...