Bug 1685816 - Don't fire rel=preload error events for invalid type/me…

…dia/as attributes. r=smaug This matches other browsers, w3c/preload#155 is the spec issue. There was a test for invalid as values. Differential Revision: https://phabricator.services.mozilla.com/D101243
dothq · Jan 10, 2021 · 72d0992 · 72d0992
1 parent 3ea697e
commit 72d0992
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 53 deletions.
diff --git a/dom/base/nsContentSink.cpp b/dom/base/nsContentSink.cpp
@@ -746,29 +746,27 @@ void nsContentSink::PreloadHref(const nsAString& aHref, const nsAString& aAs,
                                 const nsAString& aSrcset,
                                 const nsAString& aSizes, const nsAString& aCORS,
                                 const nsAString& aReferrerPolicy) {
-  nsAttrValue asAttr;
-  HTMLLinkElement::ParseAsValue(aAs, asAttr);
-  auto policyType = HTMLLinkElement::AsValueToContentPolicy(asAttr);
-
-  if (policyType == nsIContentPolicy::TYPE_INVALID) {
-    // Ignore preload with a wrong or empty as attribute.
+  auto encoding = mDocument->GetDocumentCharacterSet();
+  nsCOMPtr<nsIURI> uri;
+  NS_NewURI(getter_AddRefs(uri), aHref, encoding, mDocument->GetDocBaseURI());
+  if (!uri) {
+    // URL parsing failed.
     return;
   }
 
+  nsAttrValue asAttr;
+  HTMLLinkElement::ParseAsValue(aAs, asAttr);
+
   nsAutoString mimeType;
   nsAutoString notUsed;
   nsContentUtils::SplitMimeType(aType, mimeType, notUsed);
-  if (!HTMLLinkElement::CheckPreloadAttrs(asAttr, mimeType, aMedia,
-                                          mDocument)) {
-    policyType = nsIContentPolicy::TYPE_INVALID;
-  }
 
-  auto encoding = mDocument->GetDocumentCharacterSet();
-  nsCOMPtr<nsIURI> uri;
-  NS_NewURI(getter_AddRefs(uri), aHref, encoding, mDocument->GetDocBaseURI());
-
-  if (!uri) {
-    // URL parsing failed.
+  auto policyType = HTMLLinkElement::AsValueToContentPolicy(asAttr);
+  if (policyType == nsIContentPolicy::TYPE_INVALID ||
+      !HTMLLinkElement::CheckPreloadAttrs(asAttr, mimeType, aMedia,
+                                          mDocument)) {
+    // Ignore preload wrong or empty attributes.
+    HTMLLinkElement::WarnIgnoredPreload(*mDocument, *uri);
     return;
   }
 

diff --git a/dom/html/HTMLLinkElement.cpp b/dom/html/HTMLLinkElement.cpp
@@ -587,24 +587,21 @@ void HTMLLinkElement::
   }
 
   if (linkTypes & ePRELOAD) {
-    nsCOMPtr<nsIURI> uri(GetURI());
-    if (uri) {
+    if (nsCOMPtr<nsIURI> uri = GetURI()) {
       nsContentPolicyType policyType;
 
       nsAttrValue asAttr;
       nsAutoString mimeType;
       nsAutoString media;
       GetContentPolicyMimeTypeMedia(asAttr, policyType, mimeType, media);
 
-      if (policyType == nsIContentPolicy::TYPE_INVALID) {
+      if (policyType == nsIContentPolicy::TYPE_INVALID ||
+          !CheckPreloadAttrs(asAttr, mimeType, media, OwnerDoc())) {
         // Ignore preload with a wrong or empty as attribute.
+        WarnIgnoredPreload(*OwnerDoc(), *uri);
         return;
       }
 
-      if (!CheckPreloadAttrs(asAttr, mimeType, media, OwnerDoc())) {
-        policyType = nsIContentPolicy::TYPE_INVALID;
-      }
-
       StartPreload(policyType);
       return;
     }
@@ -659,24 +656,21 @@ void HTMLLinkElement::UpdatePreload(nsAtom* aName, const nsAttrValue* aValue,
   nsAutoString media;
   GetContentPolicyMimeTypeMedia(asAttr, asPolicyType, mimeType, media);
 
-  if (asPolicyType == nsIContentPolicy::TYPE_INVALID) {
+  if (asPolicyType == nsIContentPolicy::TYPE_INVALID ||
+      !CheckPreloadAttrs(asAttr, mimeType, media, OwnerDoc())) {
     // Ignore preload with a wrong or empty as attribute, but be sure to cancel
     // the old one.
     CancelPrefetchOrPreload();
+    WarnIgnoredPreload(*OwnerDoc(), *uri);
     return;
   }
 
-  nsContentPolicyType policyType = asPolicyType;
-  if (!CheckPreloadAttrs(asAttr, mimeType, media, OwnerDoc())) {
-    policyType = nsIContentPolicy::TYPE_INVALID;
-  }
-
   if (aName == nsGkAtoms::crossorigin) {
     CORSMode corsMode = AttrValueToCORSMode(aValue);
     CORSMode oldCorsMode = AttrValueToCORSMode(aOldValue);
     if (corsMode != oldCorsMode) {
       CancelPrefetchOrPreload();
-      StartPreload(policyType);
+      StartPreload(asPolicyType);
     }
     return;
   }
@@ -718,17 +712,14 @@ void HTMLLinkElement::UpdatePreload(nsAtom* aName, const nsAttrValue* aValue,
     }
   }
 
-  if ((policyType != oldPolicyType) &&
-      (oldPolicyType != nsIContentPolicy::TYPE_INVALID)) {
+  if (asPolicyType != oldPolicyType &&
+      oldPolicyType != nsIContentPolicy::TYPE_INVALID) {
     CancelPrefetchOrPreload();
   }
 
   // Trigger a new preload if the policy type has changed.
-  // Also trigger load if the new policy type is invalid, this will only
-  // trigger an error event.
-  if ((policyType != oldPolicyType) ||
-      (policyType == nsIContentPolicy::TYPE_INVALID)) {
-    StartPreload(policyType);
+  if (asPolicyType != oldPolicyType) {
+    StartPreload(asPolicyType);
   }
 }
 
@@ -743,12 +734,12 @@ void HTMLLinkElement::CancelPrefetchOrPreload() {
   }
 }
 
-void HTMLLinkElement::StartPreload(nsContentPolicyType policyType) {
+void HTMLLinkElement::StartPreload(nsContentPolicyType aPolicyType) {
   MOZ_ASSERT(!mPreload, "Forgot to cancel the running preload");
 
   auto referrerInfo = MakeRefPtr<ReferrerInfo>(*this);
-  RefPtr<PreloaderBase> preload =
-      OwnerDoc()->Preloads().PreloadLinkElement(this, policyType, referrerInfo);
+  RefPtr<PreloaderBase> preload = OwnerDoc()->Preloads().PreloadLinkElement(
+      this, aPolicyType, referrerInfo);
   mPreload = preload.get();
 }
 
@@ -804,12 +795,12 @@ bool HTMLLinkElement::CheckPreloadAttrs(const nsAttrValue& aAs,
     return true;
   }
 
-  nsString type = nsString(aType);
-  ToLowerCase(type);
-
   if (policyType == nsIContentPolicy::TYPE_INTERNAL_FETCH_PRELOAD) {
     return true;
   }
+
+  nsAutoString type(aType);
+  ToLowerCase(type);
   if (policyType == nsIContentPolicy::TYPE_MEDIA) {
     if (aAs.GetEnumValue() == DESTINATION_TRACK) {
       return type.EqualsASCII("text/vtt");
@@ -841,6 +832,17 @@ bool HTMLLinkElement::CheckPreloadAttrs(const nsAttrValue& aAs,
   return false;
 }
 
+void HTMLLinkElement::WarnIgnoredPreload(const Document& aDoc, nsIURI& aURI) {
+  AutoTArray<nsString, 1> params;
+  {
+    nsCString uri = nsContentUtils::TruncatedURLForDisplay(&aURI);
+    AppendUTF8toUTF16(uri, *params.AppendElement());
+  }
+  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, "DOM"_ns, &aDoc,
+                                  nsContentUtils::eDOM_PROPERTIES,
+                                  "PreloadIgnoredInvalidAttr", params);
+}
+
 bool HTMLLinkElement::IsCSSMimeTypeAttributeForLinkElement(
     const Element& aSelf) {
   // Processing the type attribute per

diff --git a/dom/html/HTMLLinkElement.h b/dom/html/HTMLLinkElement.h
@@ -188,6 +188,7 @@ class HTMLLinkElement final : public nsGenericHTMLElement,
 
   static bool CheckPreloadAttrs(const nsAttrValue& aAs, const nsAString& aType,
                                 const nsAString& aMedia, Document* aDocument);
+  static void WarnIgnoredPreload(const Document&, nsIURI&);
 
  protected:
   virtual ~HTMLLinkElement();

diff --git a/dom/locales/en-US/chrome/dom/dom.properties b/dom/locales/en-US/chrome/dom/dom.properties
@@ -408,3 +408,5 @@ FolderUploadPrompt.acceptButtonLabel = Upload
 InputPickerBlockedNoUserActivation=<input> picker was blocked due to lack of user activation.
 ExternalProtocolFrameBlockedNoUserActivation=Iframe with external protocol was blocked due to lack of user activation, or because not enough time has passed since the last such iframe was loaded.
 MultiplePopupsBlockedNoUserActivation=Opening multiple popups was blocked due to lack of user activation.
+# LOCALIZATION NOTE: %S is the URL of the preload that was ignored.
+PreloadIgnoredInvalidAttr=Preload of %S was ignored due to unknown “as” or “type” values, or non-matching “media” attribute.
diff --git a/testing/web-platform/tests/preload/onerror-event.html b/testing/web-platform/tests/preload/onerror-event.html
@@ -15,7 +15,6 @@
     var trackFailed = false;
     var gibberishFailed = false;
     var fetchFailed = false;
-    var emptyFailed = false;
 </script>
 <link rel=preload href="non-existent/dummy.js" as=script onerror="scriptFailed = true;">
 <link rel=preload href="non-existent/dummy.css" as=style onerror="styleFailed = true;">
@@ -24,9 +23,11 @@
 <link rel=preload href="non-existent/test.mp4" as=video onerror="videoFailed = true;">
 <link rel=preload href="non-existent/test.oga" as=audio onerror="audioFailed = true;">
 <link rel=preload href="non-existent/security/captions.vtt" as=track onerror="trackFailed = true;">
-<link rel=preload href="non-existent/dummy.xml?foo" as=foobarxmlthing onerror="gibberishFailed = true;">
 <link rel=preload href="non-existent/dummy.xml?fetch" as=fetch onerror="fetchFailed = true;">
-<link rel=preload href="non-existent/dummy.xml?empty" onerror="emptyFailed = true;">
+<link rel=preload href="non-existent/dummy.xml?foo" as=foobarxmlthing onerror="assert_unreached('invalid as value should not fire error event')">
+<link rel=preload href="non-existent/dummy.xml?empty" onerror="assert_unreached('empty as value should not fire error event')">
+<link rel=preload href="non-existent/dummy.xml?media" as=style media=print onerror="assert_unreached('non-matching media should not fire error event')">
+<link rel=preload href="non-existent/dummy.xml?media" as=style type='text/html' onerror="assert_unreached('invalid mime type should not fire error event')">
 <body>
 <script>
     setup({single_test: true});
@@ -35,7 +36,7 @@
 
     function check_finished() {
         if (styleFailed && scriptFailed && imageFailed && fontFailed && videoFailed && audioFailed &&
-            trackFailed && !gibberishFailed && fetchFailed && !emptyFailed) {
+            trackFailed && fetchFailed) {
             done();
         }
         iterations++;
@@ -48,9 +49,7 @@
             assert_true(videoFailed, "video triggered error event");
             assert_true(audioFailed, "audio triggered error event");
             assert_true(trackFailed, "track triggered error event");
-            assert_false(gibberishFailed, "gibberish as value did not trigger error event");
             assert_true(fetchFailed, "fetch as triggered error event");
-            assert_false(emptyFailed, "empty as triggered error event");
             done();
         } else {
             step_timeout(check_finished, 500);

diff --git a/uriloader/preload/PreloadService.cpp b/uriloader/preload/PreloadService.cpp
@@ -63,15 +63,12 @@ already_AddRefed<PreloaderBase> PreloadService::PreloadLinkElement(
   // Even if the pref is disabled, we still want to collect telemetry about
   // attempted preloads.
   const bool preloadEnabled = StaticPrefs::network_preload();
-
   if (!CheckReferrerURIScheme(aReferrerInfo)) {
     return nullptr;
   }
 
   if (aPolicyType == nsIContentPolicy::TYPE_INVALID) {
-    if (preloadEnabled) {
-      NotifyNodeEvent(aLinkElement, false);
-    }
+    MOZ_ASSERT_UNREACHABLE("Caller should check");
     return nullptr;
   }
 
@@ -124,6 +121,7 @@ void PreloadService::PreloadLinkHeader(
   }
 
   if (aPolicyType == nsIContentPolicy::TYPE_INVALID) {
+    MOZ_ASSERT_UNREACHABLE("Caller should check");
     return;
   }
 
@@ -286,6 +284,8 @@ dom::ReferrerPolicy PreloadService::PreloadReferrerPolicy(
   return referrerPolicy;
 }
 
+// FIXME(emilio): Other browsers don't seem to have this check (preload loads
+// just fine from a file:// URI). Why is this?
 bool PreloadService::CheckReferrerURIScheme(nsIReferrerInfo* aReferrerInfo) {
   if (!aReferrerInfo) {
     return false;