Fire error event when script integrity check fails.

Fire error event when script integrity check fails.

Currently, Blink has been firing load events in the case of subresource
integrity failure, while giving a console message. As per
https://github.com/w3c/webappsec/issues/149, the spec has been modified
to explicitly state that error events should be fired. This makes that
change for script tag integrity checks.

This modifies the behavior of ScriptLoader::executeScript to return a
bool that is false if and if only there is a fatal error, currently only
integrity check failures.

BUG=464648
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192482

[jww, 2015-03-23 18:39:06]

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

[jww, 2015-03-23 18:39:26]

Mike, can you take a look? I'll be uploading a separate CL to do the same for
stylesheets in a bit.

[Mike West, 2015-03-23 19:49:43]

I don't understand why we'd special-case integrity failures. If we can't run a
script for some other blocking reason (MIME, CSP, etc), why wouldn't we fire an
error event?

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:341: return true;
Ah, this is apparently what we do. Why wouldn't we return `false` here?

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:347: return true;
Or here?

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:353: return true;
Or here?

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:360: return true;
Or here? (We should probably try making this an ASSERT at some point)

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoader.h
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.h:56: // Returns false if and only if there was a
fatal execution error, namely
Hrm. Perhaps "if execution was blocked"? What do we do for CSP-blocked
resources?

[jww, 2015-03-23 21:56:52]

See my comments, but basically, yeah, we have a bug for CSP and MIME types that
I've filed a bug for (https://crbug.com/469877). However, I'd rather address
that in a separate CL than tacking it onto this one.

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:341: return true;
On 2015/03/23 19:49:43, Mike West wrote:
> Ah, this is apparently what we do. Why wouldn't we return `false` here?

So you've hit on a CSP bug that I came across writing this. I filed
https://crbug.com/469877 to track it, and we certainly should fix it, but
basically, we've been incorrectly *not* firing error events in several cases
where we should be for quite some time now. I'd like to make another CL after
this one to address it, however, as it's a whole set of tests that are unrelated
to the core SRI issue here.

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:347: return true;
On 2015/03/23 19:49:43, Mike West wrote:
> Or here?

See above.

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:353: return true;
On 2015/03/23 19:49:43, Mike West wrote:
> Or here?

See above.

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:360: return true;
On 2015/03/23 19:49:43, Mike West wrote:
> Or here? (We should probably try making this an ASSERT at some point)

This is the only one I feel slightly different about and would rather not touch.
I, frankly, don't understand the implications of this, and I think you're
probably right that it would be an ASSERT, so I'd rather just leave it untouched
for now (at the very least, I don't want to tack the ASSERT change onto another
CL as I suspect there's a good chance of breakage).

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoader.h
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.h:56: // Returns false if and only if there was a
fatal execution error, namely
On 2015/03/23 19:49:43, Mike West wrote:
> Hrm. Perhaps "if execution was blocked"? What do we do for CSP-blocked
> resources?

Done. See responses in ScriptLoader.cpp for more details.

[Mike West, 2015-03-24 04:51:31]

Alright, as long as you'll be taking care of the other cases in followup CLs,
LGTM.

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:341: return true;
On 2015/03/23 21:56:52, jww wrote:
> On 2015/03/23 19:49:43, Mike West wrote:
> > Ah, this is apparently what we do. Why wouldn't we return `false` here?
> 
> So you've hit on a CSP bug that I came across writing this. I filed
> https://crbug.com/469877 to track it, and we certainly should fix it, but
> basically, we've been incorrectly *not* firing error events in several cases
> where we should be for quite some time now. I'd like to make another CL after
> this one to address it, however, as it's a whole set of tests that are
unrelated
> to the core SRI issue here.

Ok. Can you add that bug to the CL description as followup work?

https://codereview.chromium.org/1010433007/diff/1/Source/core/dom/ScriptLoade...
Source/core/dom/ScriptLoader.cpp:360: return true;
On 2015/03/23 21:56:52, jww wrote:
> On 2015/03/23 19:49:43, Mike West wrote:
> > Or here? (We should probably try making this an ASSERT at some point)
> 
> This is the only one I feel slightly different about and would rather not
touch.
> I, frankly, don't understand the implications of this, and I think you're
> probably right that it would be an ASSERT, so I'd rather just leave it
untouched
> for now (at the very least, I don't want to tack the ASSERT change onto
another
> CL as I suspect there's a good chance of breakage).

I meant making it an ASSERT in a subsequent CL, certainly. I think it'd be a
good idea for you to try to do that, but I agree that it doesn't block this CL.

[jww, 2015-03-24 17:26:01]

Yup, I definitely will. I have a follow up CL almost ready.

[jww, 2015-03-24 17:26:07]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2015-03-24 17:26:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1010433007/20001

[commit-bot: I haz the power, 2015-03-24 20:28:07]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192482