Fire error event when script integrity check fails.

Source/core/dom/ScriptLoader.cpp

Index: Source/core/dom/ScriptLoader.cpp
diff --git a/Source/core/dom/ScriptLoader.cpp b/Source/core/dom/ScriptLoader.cpp
index e5f117d0674de95638fcd0353c74521e05d32d9b..46c1d17c972c4cfdc070eac692cd9188a2f49f1b 100644
--- a/Source/core/dom/ScriptLoader.cpp
+++ b/Source/core/dom/ScriptLoader.cpp
@@ -266,7 +266,8 @@ bool ScriptLoader::prepareScript(const TextPosition& scriptStartPosition, Legacy
         // Reset line numbering for nested writes.
         TextPosition position = elementDocument.isInDocumentWrite() ? TextPosition() : scriptStartPosition;
         KURL scriptURL = (!elementDocument.isInDocumentWrite() && m_parserInserted) ? elementDocument.url() : KURL();
-        executeScript(ScriptSourceCode(scriptContent(), scriptURL, position));
+        if (!executeScript(ScriptSourceCode(scriptContent(), scriptURL, position)))
+            return false;
     }
 
     return true;
@@ -317,17 +318,17 @@ bool isSVGScriptLoader(Element* element)
     return isSVGScriptElement(*element);
 }
 
-void ScriptLoader::executeScript(const ScriptSourceCode& sourceCode, double* compilationFinishTime)
+bool ScriptLoader::executeScript(const ScriptSourceCode& sourceCode, double* compilationFinishTime)
 {
     ASSERT(m_alreadyStarted);
 
     if (sourceCode.isEmpty())
-        return;
+        return true;
 
     RefPtrWillBeRawPtr<Document> elementDocument(m_element->document());
     RefPtrWillBeRawPtr<Document> contextDocument = elementDocument->contextDocument().get();
     if (!contextDocument)
-        return;
+        return true;
 
     LocalFrame* frame = contextDocument->frame();
 
@@ -337,26 +338,26 @@ void ScriptLoader::executeScript(const ScriptSourceCode& sourceCode, double* com
         || csp->allowScriptWithHash(sourceCode.source());
 
     if (!m_isExternalScript && (!shouldBypassMainWorldCSP && !csp->allowInlineScript(elementDocument->url(), m_startLineNumber, sourceCode.source())))
-        return;
+        return true;

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

Ah, this is apparently what we do. Why wouldn't we return `false` here?

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

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.

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

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

 
     if (m_isExternalScript) {
         ScriptResource* resource = m_resource ? m_resource.get() : sourceCode.resource();
         if (resource && !resource->mimeTypeAllowedByNosniff()) {
             contextDocument->addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, ErrorMessageLevel, "Refused to execute script from '" + resource->url().elidedString() + "' because its MIME type ('" + resource->mimeType() + "') is not executable, and strict MIME type checking is enabled."));
-            return;
+            return true;

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

Or here?

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

See above.

         }
 
         if (resource && resource->mimeType().lower().startsWith("image/")) {
             contextDocument->addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, ErrorMessageLevel, "Refused to execute script from '" + resource->url().elidedString() + "' because its MIME type ('" + resource->mimeType() + "') is not executable."));
             UseCounter::count(frame, UseCounter::BlockedSniffingImageToScript);
-            return;
+            return true;

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

Or here?

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

See above.

         }
     }
 
     // FIXME: Can this be moved earlier in the function?
     // Why are we ever attempting to execute scripts without a frame?
     if (!frame)
-        return;
+        return true;

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

Or here? (We should probably try making this an ASSERT at some point)

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

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).

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

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.

 
     AccessControlStatus corsCheck = NotSharableCrossOrigin;
     if (!m_isExternalScript || (sourceCode.resource() && sourceCode.resource()->passesAccessControlCheck(&m_element->document(), m_element->document().securityOrigin())))
@@ -365,7 +366,7 @@ void ScriptLoader::executeScript(const ScriptSourceCode& sourceCode, double* com
     if (m_isExternalScript) {
         const KURL resourceUrl = sourceCode.resource()->resourceRequest().url();
         if (!SubresourceIntegrity::CheckSubresourceIntegrity(*m_element, sourceCode.source(), sourceCode.resource()->url(), sourceCode.resource()->mimeType(), *sourceCode.resource())) {
-            return;
+            return false;
         }
     }
 
@@ -386,6 +387,8 @@ void ScriptLoader::executeScript(const ScriptSourceCode& sourceCode, double* com
         ASSERT(contextDocument->currentScript() == m_element);
         contextDocument->popCurrentScript();
     }
+
+    return true;
 }
 
 void ScriptLoader::execute()
@@ -399,8 +402,10 @@ void ScriptLoader::execute()
     if (errorOccurred) {
         dispatchErrorEvent();
     } else if (!m_resource->wasCanceled()) {
-        executeScript(source);
-        dispatchLoadEvent();
+        if (executeScript(source))
+            dispatchLoadEvent();
+        else
+            dispatchErrorEvent();
     }
     m_resource = 0;
 }