Use ScriptLoader::errorOccurred() instead of Resource in ScriptLoader

Use ScriptLoader::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ made
ScriptLoader::errorOccurred() and
PendingScript::errorOccurred() inconsistent and thus causes
the following issue:
  If the integrity check fails for a script that falls into
  3rd Clause of Step 23 of "prepare a script",
  then notifyScriptReady() is called because
  Resource::errorOccurred() is false,
  but execution of the script and the subsequent scripts are blocked
  forever by ScriptRunner::scheduleReadyInOrderScripts()
  because ScriptLoader::errorOccurred() is true.

This CL fixes it and add a layout test for this case.

BUG=686281
Review-Url: https://codereview.chromium.org/2715533007
Cr-Commit-Position: refs/heads/master@{#453056}
Committed: https://chromium.googlesource.com/chromium/src/+/bb1b87fb08ec4407a2cf1cdcf6d9e400e253e578

[hiroshige, 2017-02-23 19:58:03]

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

[commit-bot: I haz the power, 2017-02-23 19:58:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-02-23 20:00:42]

Description was changed from

==========
Use PendingScript::errorOccurred() instead of Resource in ScriptLoader

BUG=
==========

to

==========
Use PendingScript::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ introduced a regression
by making ScriptLoader and PendingScript::errorOccurred() inconsistent,
and this CL fixes it.

BUG=686281
==========

[hiroshige, 2017-02-23 20:00:42]

hiroshige@chromium.org changed reviewers:
+ kouhei@chromium.org, sigbjornf@opera.com

[hiroshige, 2017-02-23 20:33:27]

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

[commit-bot: I haz the power, 2017-02-23 20:34:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-02-23 20:36:53]

Description was changed from

==========
Use PendingScript::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ introduced a regression
by making ScriptLoader and PendingScript::errorOccurred() inconsistent,
and this CL fixes it.

BUG=686281
==========

to

==========
Use PendingScript::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ introduced a regression
by making ScriptLoader and PendingScript::errorOccurred() inconsistent,
because if the integrity check fails for a script that falls into
3rd Clause of Step 23 of "prepare a script", then notifyScriptReady()
is called because |m_resource->errorOccurred()| is false, but its
execution is blocked forever by
ScriptRunner::scheduleReadyInOrderScripts() because
ScriptLoader::errorOccurred() is true.

This CL fixes it and add a layout test for this case.

BUG=686281
==========

[hiroshige, 2017-02-23 20:41:03]

hiroshige@chromium.org changed reviewers:
+ mkwst@chromium.org

[hiroshige, 2017-02-23 20:41:04]

PTAL.

I think we should modify other tests under
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/ to
comprehensively tests the script cases, but didn't do that in this CL because
manual rewriting looks time-consuming and error-prone. (generator is needed?)

[hiroshige, 2017-02-23 22:26:37]

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

[commit-bot: I haz the power, 2017-02-23 22:26:42]

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-02-24 00:21:28]

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

[commit-bot: I haz the power, 2017-02-24 00:21:28]

Dry run: This issue passed the CQ dry run.

[sof, 2017-02-24 07:29:22]

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html
(right):

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:10:
var test1 = async_test('1st Clause, Step 23 of prepare a script');
Could you make clear that these spec references are relative to
https://html.spec.whatwg.org/#prepare-a-script ? Also, it would be helpful to
say what these alternatives represent (script element attribute variations), as
extensions to the upstream spec algorithm would leave these as dangling
pointers.

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:11:
var test2 = async_test('2st Clause, Step 23 of prepare a script');
2nd

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:12:
var test3 = async_test('3st Clause, Step 23 of prepare a script');
3rd

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:13:
var test4 = async_test('4st Clause, Step 23 of prepare a script');
fourth/4th

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:855:
DCHECK_EQ(pendingScript->resource(), m_resource);
Given the DCHECK_EQ() at the start of this method, we don't need this one also.

[hiroshige, 2017-02-24 18:16:53]

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

[hiroshige, 2017-02-24 18:16:57]

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html
(right):

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:10:
var test1 = async_test('1st Clause, Step 23 of prepare a script');
On 2017/02/24 07:29:22, sof wrote:
> Could you make clear that these spec references are relative to
> https://html.spec.whatwg.org/#prepare-a-script ?
Done.

> Also, it would be helpful to
> say what these alternatives represent (script element attribute variations),
as
> extensions to the upstream spec algorithm would leave these as dangling
> pointers.
Added a short comment.
(Longer comment would be straightforward description of the code below or the
spec)

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:11:
var test2 = async_test('2st Clause, Step 23 of prepare a script');
On 2017/02/24 07:29:22, sof wrote:
> 2nd

Done.

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:12:
var test3 = async_test('3st Clause, Step 23 of prepare a script');
On 2017/02/24 07:29:22, sof wrote:
> 3rd

Done.

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-script-cors-bad-integrity.html:13:
var test4 = async_test('4st Clause, Step 23 of prepare a script');
On 2017/02/24 07:29:22, sof wrote:
> fourth/4th

Done.

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:855:
DCHECK_EQ(pendingScript->resource(), m_resource);
On 2017/02/24 07:29:22, sof wrote:
> Given the DCHECK_EQ() at the start of this method, we don't need this one
also.

The DCHECK_EQ() at the start of this method checks that
|m_pendingScript == pendingScript|, and
this DCHECK_EQ() virtually checks that
|m_pendingScript->resource() == m_resource|, and
I think these are two orthogonal assumptions
and are useful to catch current bugs or future regressions.

[commit-bot: I haz the power, 2017-02-24 18:17:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-02-24 18:21:05]

Description was changed from

==========
Use PendingScript::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ introduced a regression
by making ScriptLoader and PendingScript::errorOccurred() inconsistent,
because if the integrity check fails for a script that falls into
3rd Clause of Step 23 of "prepare a script", then notifyScriptReady()
is called because |m_resource->errorOccurred()| is false, but its
execution is blocked forever by
ScriptRunner::scheduleReadyInOrderScripts() because
ScriptLoader::errorOccurred() is true.

This CL fixes it and add a layout test for this case.

BUG=686281
==========

to

==========
Use ScriptLoader::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ made
ScriptLoader::errorOccurred() and
PendingScript::errorOccurred() inconsistent and thus causes
the following issue:
  If the integrity check fails for a script that falls into
  3rd Clause of Step 23 of "prepare a script",
  then notifyScriptReady() is called because
  Resource::errorOccurred() is false,
  but execution of the script and the subsequent scripts are blocked
  forever by ScriptRunner::scheduleReadyInOrderScripts()
  because ScriptLoader::errorOccurred() is true.

This CL fixes it and add a layout test for this case.

BUG=686281
==========

[sof, 2017-02-24 18:26:44]

lgtm

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:855:
DCHECK_EQ(pendingScript->resource(), m_resource);
On 2017/02/24 18:16:57, hiroshige wrote:
> On 2017/02/24 07:29:22, sof wrote:
> > Given the DCHECK_EQ() at the start of this method, we don't need this one
> also.
> 
> The DCHECK_EQ() at the start of this method checks that
> |m_pendingScript == pendingScript|, and
> this DCHECK_EQ() virtually checks that
> |m_pendingScript->resource() == m_resource|, and
> I think these are two orthogonal assumptions
> and are useful to catch current bugs or future regressions.

If you really think that's valuable, then move it up next to that other
DCHECK_EQ() ?

[commit-bot: I haz the power, 2017-02-24 20:00:01]

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

[commit-bot: I haz the power, 2017-02-24 20:00:01]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-02-25 00:31:23]

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

[hiroshige, 2017-02-25 00:31:50]

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2715533007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:855:
DCHECK_EQ(pendingScript->resource(), m_resource);
On 2017/02/24 18:26:43, sof wrote:
> On 2017/02/24 18:16:57, hiroshige wrote:
> > On 2017/02/24 07:29:22, sof wrote:
> > > Given the DCHECK_EQ() at the start of this method, we don't need this one
> > also.
> > 
> > The DCHECK_EQ() at the start of this method checks that
> > |m_pendingScript == pendingScript|, and
> > this DCHECK_EQ() virtually checks that
> > |m_pendingScript->resource() == m_resource|, and
> > I think these are two orthogonal assumptions
> > and are useful to catch current bugs or future regressions.
> 
> If you really think that's valuable, then move it up next to that other
> DCHECK_EQ() ?

Done.

[commit-bot: I haz the power, 2017-02-25 00:31: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-02-25 00:34:11]

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

[commit-bot: I haz the power, 2017-02-25 00:34:12]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[hiroshige, 2017-02-25 01:45:40]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-02-25 01:45:41]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2715533007/#ps100001
(title: "Move DCHECK_EQ")

[commit-bot: I haz the power, 2017-02-25 01:45:53]

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-02-25 03:12:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487987140685110,
"parent_rev": "54dadab67189e7d6900570ea5699627deeb3ecce", "commit_rev":
"bb1b87fb08ec4407a2cf1cdcf6d9e400e253e578"}

[commit-bot: I haz the power, 2017-02-25 03:12:37]

Description was changed from

==========
Use ScriptLoader::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ made
ScriptLoader::errorOccurred() and
PendingScript::errorOccurred() inconsistent and thus causes
the following issue:
  If the integrity check fails for a script that falls into
  3rd Clause of Step 23 of "prepare a script",
  then notifyScriptReady() is called because
  Resource::errorOccurred() is false,
  but execution of the script and the subsequent scripts are blocked
  forever by ScriptRunner::scheduleReadyInOrderScripts()
  because ScriptLoader::errorOccurred() is true.

This CL fixes it and add a layout test for this case.

BUG=686281
==========

to

==========
Use ScriptLoader::errorOccurred() instead of Resource in ScriptLoader

https://codereview.chromium.org/2716543002/ made
ScriptLoader::errorOccurred() and
PendingScript::errorOccurred() inconsistent and thus causes
the following issue:
  If the integrity check fails for a script that falls into
  3rd Clause of Step 23 of "prepare a script",
  then notifyScriptReady() is called because
  Resource::errorOccurred() is false,
  but execution of the script and the subsequent scripts are blocked
  forever by ScriptRunner::scheduleReadyInOrderScripts()
  because ScriptLoader::errorOccurred() is true.

This CL fixes it and add a layout test for this case.

BUG=686281

Review-Url: https://codereview.chromium.org/2715533007
Cr-Commit-Position: refs/heads/master@{#453056}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb1b87fb08ec4407a2cf1cdcf6d9...
==========

[commit-bot: I haz the power, 2017-02-25 03:12:39]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/bb1b87fb08ec4407a2cf1cdcf6d9...

[kouhei (in TOK), 2017-02-27 00:03:39]

lgtm