Tidy up ScriptLoader (MIME) type matching.

third_party/WebKit/Source/core/dom/ScriptLoader.cpp

Index: third_party/WebKit/Source/core/dom/ScriptLoader.cpp
diff --git a/third_party/WebKit/Source/core/dom/ScriptLoader.cpp b/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
index 41d6c04c4745d5614d07952d1ecfdfddce57bbd0..d8ed4acac2f0ea2e3b1f81b6a841a475586e0587 100644
--- a/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
+++ b/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
@@ -129,7 +129,6 @@ void ScriptLoader::detach() {
   m_pendingScript = nullptr;
 }
 
-// Helper function. Must take a lowercase language as input.
 static bool isLegacySupportedJavaScriptLanguage(const String& language) {
   // Mozilla 1.8 accepts javascript1.0 - javascript1.7, but WinIE 7 accepts only
   // javascript1.1 - javascript1.3.
@@ -141,13 +140,18 @@ static bool isLegacySupportedJavaScriptLanguage(const String& language) {
 
   // FIXME: This function is not HTML5 compliant. These belong in the MIME
   // registry as "text/javascript<version>" entries.
-  DCHECK_EQ(language, language.lower());
-  return language == "javascript" || language == "javascript1.0" ||
-         language == "javascript1.1" || language == "javascript1.2" ||
-         language == "javascript1.3" || language == "javascript1.4" ||
-         language == "javascript1.5" || language == "javascript1.6" ||
-         language == "javascript1.7" || language == "livescript" ||
-         language == "ecmascript" || language == "jscript";
+  return equalIgnoringCase(language, "javascript") ||

[tkent, 2016/11/14 23:44:32]

Maybe equalIgnoringASCIICase() should be used.
AFAIK, equalIgnoringCase(input, literal) and equalIgnoringASCIICase(input,
literal) has a different behavior only if the literal contains 'k'.

[sof, 2016/11/15 08:03:43]

Now that sounds too interesting :-) Which equality predicate is wrongly folding
some codepoints onto 'k'?

[tkent, 2016/11/15 08:16:49]

equalIgnoringCase() assumes U+212A KELVIN SIGN and U+006B 'k' are equal :)
http://unicode.org/Public/UCD/latest/ucd/CaseFolding.txt

I think there are no other cases which map a non-ASCII character to a lower
ASCII character.

[sof, 2016/11/15 08:48:53]

Excellent, thanks for the pointer - so not behaving wrongly at all (if you know
your case folding tables :) )

I've switched to equalIgnoringASCIICase().

+         equalIgnoringCase(language, "javascript1.0") ||
+         equalIgnoringCase(language, "javascript1.1") ||
+         equalIgnoringCase(language, "javascript1.2") ||
+         equalIgnoringCase(language, "javascript1.3") ||
+         equalIgnoringCase(language, "javascript1.4") ||
+         equalIgnoringCase(language, "javascript1.5") ||
+         equalIgnoringCase(language, "javascript1.6") ||
+         equalIgnoringCase(language, "javascript1.7") ||
+         equalIgnoringCase(language, "livescript") ||
+         equalIgnoringCase(language, "ecmascript") ||
+         equalIgnoringCase(language, "jscript");
 }
 
 void ScriptLoader::dispatchErrorEvent() {
@@ -172,18 +176,17 @@ bool ScriptLoader::isValidScriptTypeAndLanguage(
   // - Allowing a different set of languages for language= and type=. language=
   //   supports Javascript 1.1 and 1.4-1.6, but type= does not.
   if (type.isEmpty()) {
-    String lowerLanguage = language.lower();
-    return language.isEmpty()  // assume text/javascript.
-           || MIMETypeRegistry::isSupportedJavaScriptMIMEType("text/" +
-                                                              lowerLanguage) ||
-           isLegacySupportedJavaScriptLanguage(lowerLanguage);
+    return language.isEmpty() ||  // assume text/javascript.
+           MIMETypeRegistry::isSupportedJavaScriptMIMEType("text/" +
+                                                           language) ||
+           isLegacySupportedJavaScriptLanguage(language);
   } else if (RuntimeEnabledFeatures::moduleScriptsEnabled() &&
              type == "module") {
     return true;
   } else if (MIMETypeRegistry::isSupportedJavaScriptMIMEType(
                  type.stripWhiteSpace()) ||
              (supportLegacyTypes == AllowLegacyTypeInTypeAttribute &&
-              isLegacySupportedJavaScriptLanguage(type.lower()))) {
+              isLegacySupportedJavaScriptLanguage(type))) {
     return true;
   }
 
@@ -401,28 +404,28 @@ bool isSVGScriptLoader(Element* element) {
   return isSVGScriptElement(*element);
 }
 
-void ScriptLoader::logScriptMimetype(ScriptResource* resource,
-                                     LocalFrame* frame,
-                                     String mimetype) {
-  String lowerMimetype = mimetype.lower();
-  bool text = lowerMimetype.startsWith("text/");
-  bool application = lowerMimetype.startsWith("application/");
-  bool expectedJs =
-      MIMETypeRegistry::isSupportedJavaScriptMIMEType(lowerMimetype) ||
-      (text && isLegacySupportedJavaScriptLanguage(lowerMimetype.substring(5)));
-  bool sameOrigin =
-      m_element->document().getSecurityOrigin()->canRequest(m_resource->url());
-  if (expectedJs) {
+void ScriptLoader::logScriptMIMEType(LocalFrame* frame,
+                                     ScriptResource* resource,
+                                     const String& mimeType) {
+  if (MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType))
     return;
-  }
+  bool isText = mimeType.startsWith("text/", TextCaseInsensitive);

[tkent, 2016/11/14 23:44:32]

Maybe TextCaseASCIIInsensitive instead of TextCaseInsensitive.

[sof, 2016/11/15 08:48:53]

Yes, a newer addition - switched to it.

+  if (isText && isLegacySupportedJavaScriptLanguage(mimeType.substring(5)))
+    return;
+  bool isSameOrigin =
+      m_element->document().getSecurityOrigin()->canRequest(resource->url());
+  bool isApplication =
+      !isText && mimeType.startsWith("application/", TextCaseInsensitive);

[tkent, 2016/11/14 23:44:32]

Ditto.

[sof, 2016/11/15 08:48:53]

Same, switched.

+
   UseCounter::Feature feature =
-      sameOrigin
-          ? (text ? UseCounter::SameOriginTextScript
-                  : application ? UseCounter::SameOriginApplicationScript
-                                : UseCounter::SameOriginOtherScript)
-          : (text ? UseCounter::CrossOriginTextScript
-                  : application ? UseCounter::CrossOriginApplicationScript
-                                : UseCounter::CrossOriginOtherScript);
+      isSameOrigin
+          ? (isText ? UseCounter::SameOriginTextScript
+                    : isApplication ? UseCounter::SameOriginApplicationScript
+                                    : UseCounter::SameOriginOtherScript)
+          : (isText ? UseCounter::CrossOriginTextScript
+                    : isApplication ? UseCounter::CrossOriginApplicationScript
+                                    : UseCounter::CrossOriginOtherScript);
+
   UseCounter::count(frame, feature);
 }
 
@@ -487,26 +490,26 @@ bool ScriptLoader::doExecuteScript(const ScriptSourceCode& sourceCode) {
         return false;
       }
 
-      String mimetype = resource->httpContentType();
-      if (mimetype.startsWith("image/") || mimetype == "text/csv" ||
-          mimetype.startsWith("audio/") || mimetype.startsWith("video/")) {
+      String mimeType = resource->httpContentType();

[tkent, 2016/11/14 23:44:32]

We should remove .lower() in Resource::httpContentType() for consistency.

[sof, 2016/11/15 08:48:53]

Maybe. I'm not convinced it represents an improvement unless you also change the
return type to be something that enforces correct matching/equality checking
over MIME types.

+      if (mimeType.startsWith("image/") || mimeType == "text/csv" ||
+          mimeType.startsWith("audio/") || mimeType.startsWith("video/")) {
         contextDocument->addConsoleMessage(ConsoleMessage::create(
             SecurityMessageSource, ErrorMessageLevel,
             "Refused to execute script from '" +
                 resource->url().elidedString() + "' because its MIME type ('" +
-                mimetype + "') is not executable."));
-        if (mimetype.startsWith("image/"))
+                mimeType + "') is not executable."));
+        if (mimeType.startsWith("image/"))
           UseCounter::count(frame, UseCounter::BlockedSniffingImageToScript);
-        else if (mimetype.startsWith("audio/"))
+        else if (mimeType.startsWith("audio/"))
           UseCounter::count(frame, UseCounter::BlockedSniffingAudioToScript);
-        else if (mimetype.startsWith("video/"))
+        else if (mimeType.startsWith("video/"))
           UseCounter::count(frame, UseCounter::BlockedSniffingVideoToScript);
-        else if (mimetype == "text/csv")
+        else if (mimeType == "text/csv")
           UseCounter::count(frame, UseCounter::BlockedSniffingCSVToScript);
         return false;
       }
 
-      logScriptMimetype(resource, frame, mimetype);
+      logScriptMIMEType(frame, resource, mimeType);
     }
   }