Bug 1709577 - Fix invalid src events for images. r=edgar

My previous patch still causes one WPT regression (invalid-src.html),
because we stopped firing error event for src="". However that test
times out because it doesn't correctly handle the invalid URI case. This
patch fixes it and cleans up the code a bit.

This fixes bug 1466138 too, and matches Chrome.

Differential Revision: https://phabricator.services.mozilla.com/D114495

dom/base/nsImageLoadingContent.cpp

@@ -999,9 +999,8 @@ void nsImageLoadingContent::ForceReload(bool aNotify, ErrorResult& aError) {
   ImageLoadType loadType = (mCurrentRequestFlags & REQUEST_IS_IMAGESET)
                                ? eImageLoadType_Imageset
                                : eImageLoadType_Normal;
-  nsresult rv =
-      LoadImage(currentURI, true, aNotify, loadType,
-                nsIRequest::VALIDATE_ALWAYS | LoadFlags(), true, nullptr);
+  nsresult rv = LoadImage(currentURI, true, aNotify, loadType,
+                          nsIRequest::VALIDATE_ALWAYS | LoadFlags());
   if (NS_FAILED(rv)) {
     aError.Throw(rv);
   }
@@ -1022,38 +1021,21 @@ nsresult nsImageLoadingContent::LoadImage(const nsAString& aNewURI, bool aForce,
     return NS_OK;
   }
 
-  // Pending load/error events need to be canceled in some situations. This
-  // is not documented in the spec, but can cause site compat problems if not
-  // done. See bug 1309461 and https://github.com/whatwg/html/issues/1872.
-  CancelPendingEvent();
-
-  if (aNewURI.IsEmpty()) {
-    // Cancel image requests and then fire only error event per spec.
-    CancelImageRequests(aNotify);
-    // Mark error event as cancelable only for src="" case, since only this
-    // error causes site compat problem (bug 1308069) for now.
-    FireEvent(u"error"_ns, true);
-    return NS_OK;
-  }
-
-  // Fire loadstart event
-  FireEvent(u"loadstart"_ns);
-
   // Parse the URI string to get image URI
   nsCOMPtr<nsIURI> imageURI;
-  nsresult rv = StringToURI(aNewURI, doc, getter_AddRefs(imageURI));
-  NS_ENSURE_SUCCESS(rv, rv);
-  // XXXbiesi fire onerror if that failed?
+  if (!aNewURI.IsEmpty()) {
+    Unused << StringToURI(aNewURI, doc, getter_AddRefs(imageURI));
+  }
 
-  return LoadImage(imageURI, aForce, aNotify, aImageLoadType, LoadFlags(),
-                   false, doc, aTriggeringPrincipal);
+  return LoadImage(imageURI, aForce, aNotify, aImageLoadType, LoadFlags(), doc,
+                   aTriggeringPrincipal);
 }
 
 nsresult nsImageLoadingContent::LoadImage(nsIURI* aNewURI, bool aForce,
                                           bool aNotify,
                                           ImageLoadType aImageLoadType,
                                           nsLoadFlags aLoadFlags,
-                                          bool aLoadStart, Document* aDocument,
+                                          Document* aDocument,
                                           nsIPrincipal* aTriggeringPrincipal) {
   MOZ_ASSERT(!mIsStartingImageLoad, "some evil code is reentering LoadImage.");
   if (mIsStartingImageLoad) {
@@ -1065,11 +1047,20 @@ nsresult nsImageLoadingContent::LoadImage(nsIURI* aNewURI, bool aForce,
   // done. See bug 1309461 and https://github.com/whatwg/html/issues/1872.
   CancelPendingEvent();
 
-  // Fire loadstart event if required
-  if (aLoadStart) {
-    FireEvent(u"loadstart"_ns);
+  if (!aNewURI) {
+    // Cancel image requests and then fire only error event per spec.
+    CancelImageRequests(aNotify);
+    if (aImageLoadType == eImageLoadType_Normal) {
+      // Mark error event as cancelable only for src="" case, since only this
+      // error causes site compat problem (bug 1308069) for now.
+      FireEvent(u"error"_ns, true);
+    }
+    return NS_OK;
   }
 
+  // Fire loadstart event if required
+  FireEvent(u"loadstart"_ns);
+
   if (!mLoadingEnabled) {
     // XXX Why fire an error here? seems like the callers to SetLoadingEnabled
     // don't want/need it.

dom/base/nsImageLoadingContent.h

@@ -151,7 +151,6 @@ class nsImageLoadingContent : public nsIImageLoadingContent {
    * @param aNotify If true, nsIDocumentObserver state change notifications
    *                will be sent as needed.
    * @param aImageLoadType The ImageLoadType for this request
-   * @param aLoadStart If true, dispatch "loadstart" event.
    * @param aDocument Optional parameter giving the document this node is in.
    *        This is purely a performance optimization.
    * @param aLoadFlags Optional parameter specifying load flags to use for
@@ -161,7 +160,6 @@ class nsImageLoadingContent : public nsIImageLoadingContent {
    */
   nsresult LoadImage(nsIURI* aNewURI, bool aForce, bool aNotify,
                      ImageLoadType aImageLoadType, nsLoadFlags aLoadFlags,
-                     bool aLoadStart = true,
                      mozilla::dom::Document* aDocument = nullptr,
                      nsIPrincipal* aTriggeringPrincipal = nullptr);
 
@@ -169,7 +167,7 @@ class nsImageLoadingContent : public nsIImageLoadingContent {
                      ImageLoadType aImageLoadType,
                      nsIPrincipal* aTriggeringPrincipal) {
     return LoadImage(aNewURI, aForce, aNotify, aImageLoadType, LoadFlags(),
-                     true, nullptr, aTriggeringPrincipal);
+                     nullptr, aTriggeringPrincipal);
   }
 
   /**

dom/html/HTMLImageElement.cpp

@@ -888,10 +888,10 @@ nsresult HTMLImageElement::LoadSelectedImage(bool aForce, bool aNotify,
     currentDensity = mResponsiveSelector->GetSelectedImageDensity();
   }
 
-  nsresult rv = NS_ERROR_FAILURE;
   nsCOMPtr<nsIURI> selectedSource;
   nsCOMPtr<nsIPrincipal> triggeringPrincipal;
   ImageLoadType type = eImageLoadType_Normal;
+  bool hasSrc = false;
   if (mResponsiveSelector) {
     selectedSource = mResponsiveSelector->GetSelectedImageURL();
     triggeringPrincipal =
@@ -899,10 +899,8 @@ nsresult HTMLImageElement::LoadSelectedImage(bool aForce, bool aNotify,
     type = eImageLoadType_Imageset;
   } else {
     nsAutoString src;
-    if (!GetAttr(nsGkAtoms::src, src) || src.IsEmpty()) {
-      CancelImageRequests(aNotify);
-      rv = NS_OK;
-    } else {
+    hasSrc = GetAttr(nsGkAtoms::src, src);
+    if (hasSrc && !src.IsEmpty()) {
       Document* doc = OwnerDoc();
       StringToURI(src, doc, getter_AddRefs(selectedSource));
       if (HaveSrcsetOrInPicture()) {
@@ -920,18 +918,23 @@ nsresult HTMLImageElement::LoadSelectedImage(bool aForce, bool aNotify,
     return NS_OK;
   }
 
-  if (selectedSource) {
-    // Before we actually defer the lazy-loading
-    if (mLazyLoading) {
-      if (!nsContentUtils::IsImageAvailable(
-              this, selectedSource, triggeringPrincipal, GetCORSMode())) {
-        return NS_OK;
-      }
-      StopLazyLoading(FromIntersectionObserver::No, StartLoading::No);
+  // Before we actually defer the lazy-loading
+  if (mLazyLoading) {
+    if (!selectedSource ||
+        !nsContentUtils::IsImageAvailable(this, selectedSource,
+                                          triggeringPrincipal, GetCORSMode())) {
+      return NS_OK;
     }
+    StopLazyLoading(FromIntersectionObserver::No, StartLoading::No);
+  }
 
+  nsresult rv = NS_ERROR_FAILURE;
+
+  // src triggers an error event on invalid URI, unlike other loads.
+  if (selectedSource || hasSrc) {
     rv = LoadImage(selectedSource, aForce, aNotify, type, triggeringPrincipal);
   }
+
   mLastSelectedSource = selectedSource;
   mCurrentDensity = currentDensity;
 

testing/web-platform/meta/html/semantics/embedded-content/the-img-element/invalid-src.html.ini

@@ -1,5 +0,0 @@
-[invalid-src.html]
-  expected: TIMEOUT
-  [src="http://["]
-    expected: TIMEOUT
-