Block execution of failed 'crossorigin' <script>s.

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 69e0c6555209801044ea05c7cad837ee80c5a82f..8d7342386f967a7f1719686c318008570c1f4d37 100644
--- a/Source/core/dom/ScriptLoader.cpp
+++ b/Source/core/dom/ScriptLoader.cpp
@@ -66,6 +66,7 @@ ScriptLoader::ScriptLoader(Element* element, bool parserInserted, bool alreadySt
     , m_willExecuteWhenDocumentFinishedParsing(false)
     , m_forceAsync(!parserInserted)
     , m_willExecuteInOrder(false)
+    , m_asPotentiallyCORSEnabledLoad(false)
 {
     ASSERT(m_element);
     if (parserInserted && element->document().scriptableDocumentParser() && !element->document().isInDocumentWrite())
@@ -241,7 +242,7 @@ 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));
+        executePotentiallyCrossOriginScript(ScriptSourceCode(scriptContent(), scriptURL, position));

[abarth-chromium, 2013/11/14 16:34:48]

Does it matter that we're losing the return value here?  In particular, we do
need to return false from this function when we don't execute the script?

[sof, 2013/11/14 17:12:03]

It does matter , i.e., the prepareScript() return value indicates if the script
executes as far as I've been able to determine (which makes sense wrt
the underlying spec text.)

XMLDocumentParser is currently the only caller that checks; will test & fix
this.

[sof, 2013/11/15 08:05:23]

prepareScript() now returns 'false' if executePotentiallyCrossOriginScript()
fails.

     }
 
     return true;
@@ -265,6 +266,7 @@ bool ScriptLoader::fetchScript(const String& sourceUrl)
         if (!crossOriginMode.isNull()) {
             StoredCredentials allowCredentials = equalIgnoringCase(crossOriginMode, "use-credentials") ? AllowStoredCredentials : DoNotAllowStoredCredentials;
             request.setPotentiallyCrossOriginEnabled(elementDocument->securityOrigin(), allowCredentials);
+            m_asPotentiallyCORSEnabledLoad = true;
         }
         request.setCharset(scriptCharset());
 
@@ -276,9 +278,8 @@ bool ScriptLoader::fetchScript(const String& sourceUrl)
         m_isExternalScript = true;
     }
 
-    if (m_resource) {
+    if (m_resource)
         return true;
-    }
 
     dispatchErrorEvent();
     return false;
@@ -362,6 +363,18 @@ void ScriptLoader::execute(ScriptResource* resource)
     resource->removeClient(this);
 }
 
+bool ScriptLoader::executePotentiallyCrossOriginScript(const ScriptSourceCode& sourceCode)

[abarth-chromium, 2013/11/14 16:34:48]

It looks like the one caller ignores the return value...

+{
+    if (sourceCode.resource()
+        && asPotentiallyCORSEnabledLoad()
+        && !m_element->document().fetcher()->canAccess(sourceCode.resource(), asPotentiallyCORSEnabledLoad())) {

[abarth-chromium, 2013/11/14 16:34:48]

It seems strange that we need to check asPotentiallyCORSEnabledLoad given that
canAccess checks "resource->options().requestOriginPolicy ==
PotentiallyCrossOriginEnabled".  Do we need this duplicated state?

[sof, 2013/11/14 17:12:03]

We don't use that resource state any longer in canAccess(), as the resource may
have been cached & wouldn't have that policy set.

+        dispatchErrorEvent();
+        return false;
+    }
+    executeScript(sourceCode);
+    return true;
+}
+
 void ScriptLoader::notifyFinished(Resource* resource)
 {
     ASSERT(!m_willBeParserExecuted);
@@ -378,7 +391,7 @@ void ScriptLoader::notifyFinished(Resource* resource)
     ASSERT_UNUSED(resource, resource == m_resource);
     if (!m_resource)
         return;
-    if (!elementDocument->fetcher()->canAccess(m_resource.get())) {
+    if (!elementDocument->fetcher()->canAccess(m_resource.get(), asPotentiallyCORSEnabledLoad())) {
         dispatchErrorEvent();
         return;
     }