Bug 1664691 - Remove nsImageLoadingContent.imageBlockingStatus. r=edgar
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 15 Sep 2020 11:59:28 +0000
changeset 548709 645005f957c73ce88c448e07ee18392de08c653e
parent 548708 25389e8dcd237591543f6c69e030dfde90af60b5
child 548710 fdaabda9a455dc5cd896dce389a627795ecdc86c
push id126351
push userealvarez@mozilla.com
push dateTue, 15 Sep 2020 13:33:03 +0000
treeherderautoland@645005f957c7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedgar
bugs1664691
milestone82.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1664691 - Remove nsImageLoadingContent.imageBlockingStatus. r=edgar Differential Revision: https://phabricator.services.mozilla.com/D90037
dom/base/nsContentPolicy.cpp
dom/base/nsIImageLoadingContent.idl
dom/base/nsImageLoadingContent.cpp
dom/base/nsImageLoadingContent.h
dom/base/test/browser_blocking_image.js
dom/base/test/test_blocking_image.html
dom/webidl/HTMLImageElement.webidl
layout/generic/nsImageFrame.cpp
--- a/dom/base/nsContentPolicy.cpp
+++ b/dom/base/nsContentPolicy.cpp
@@ -61,17 +61,16 @@ nsContentPolicy::~nsContentPolicy() = de
 
 #endif  // defined(DEBUG)
 
 inline nsresult nsContentPolicy::CheckPolicy(CPMethod policyMethod,
                                              nsIURI* contentLocation,
                                              nsILoadInfo* loadInfo,
                                              const nsACString& mimeType,
                                              int16_t* decision) {
-  nsContentPolicyType contentType = loadInfo->InternalContentPolicyType();
   nsCOMPtr<nsISupports> requestingContext = loadInfo->GetLoadingContext();
   // sanity-check passed-through parameters
   MOZ_ASSERT(decision, "Null out pointer");
   WARN_IF_URI_UNINITIALIZED(contentLocation, "Request URI");
 
 #ifdef DEBUG
   {
     nsCOMPtr<nsINode> node(do_QueryInterface(requestingContext));
@@ -87,19 +86,16 @@ inline nsresult nsContentPolicy::CheckPo
   nsCOMPtr<nsIContent> node = do_QueryInterface(requestingContext);
   if (node) {
     doc = node->OwnerDoc();
   }
   if (!doc) {
     doc = do_QueryInterface(requestingContext);
   }
 
-  nsContentPolicyType externalType =
-      nsContentUtils::InternalContentPolicyTypeToExternal(contentType);
-
   /*
    * Enumerate mPolicies and ask each of them, taking the logical AND of
    * their permissions.
    */
   nsresult rv;
   const nsCOMArray<nsIContentPolicy>& entries = mPolicies.GetCachedEntries();
 
   nsCOMPtr<nsPIDOMWindowOuter> window;
@@ -119,26 +115,16 @@ inline nsresult nsContentPolicy::CheckPo
 
   int32_t count = entries.Count();
   for (int32_t i = 0; i < count; i++) {
     /* check the appropriate policy */
     rv = (entries[i]->*policyMethod)(contentLocation, loadInfo, mimeType,
                                      decision);
 
     if (NS_SUCCEEDED(rv) && NS_CP_REJECTED(*decision)) {
-      // If we are blocking an image, we have to let the
-      // ImageLoadingContent know that we blocked the load.
-      if (externalType == nsIContentPolicy::TYPE_IMAGE ||
-          externalType == nsIContentPolicy::TYPE_IMAGESET) {
-        nsCOMPtr<nsIImageLoadingContent> img =
-            do_QueryInterface(requestingContext);
-        if (img) {
-          img->SetBlockedRequest(*decision);
-        }
-      }
       /* policy says no, no point continuing to check */
       return NS_OK;
     }
   }
 
   // everyone returned failure, or no policies: sanitize result
   *decision = nsIContentPolicy::ACCEPT;
   return NS_OK;
--- a/dom/base/nsIImageLoadingContent.idl
+++ b/dom/base/nsIImageLoadingContent.idl
@@ -60,24 +60,16 @@ interface nsIImageLoadingContent : imgIN
   /**
    * setLoadingEnabled is used to enable and disable loading in
    * situations where loading images is unwanted.  Note that enabling
    * loading will *not* automatically trigger an image load.
    */
   [notxpcom, nostdcall] void setLoadingEnabled(in boolean aEnabled);
 
   /**
-   * Returns the image blocking status (@see nsIContentPolicy).  This
-   * will always be an nsIContentPolicy REJECT_* status for cases when
-   * the image was blocked.  This status always refers to the
-   * CURRENT_REQUEST load.
-   */
-  [notxpcom, nostdcall] readonly attribute short imageBlockingStatus;
-
-  /**
    * Used to register an image decoder observer.  Typically, this will
    * be a proxy for a frame that wants to paint the image.
    * Notifications from ongoing image loads will be passed to all
    * registered observers.  Notifications for all request types,
    * current and pending, will be passed through.
    *
    * @param aObserver the observer to register
    */
@@ -99,25 +91,16 @@ interface nsIImageLoadingContent : imgIN
    * is thrown)
    *
    * @throws NS_ERROR_UNEXPECTED if the request type requested is not
    * known
    */
   [noscript] imgIRequest getRequest(in long aRequestType);
 
   /**
-   * Call this function when the request was blocked by any of the
-   * security policies enforced.
-   *
-   * @param aContentDecision the decision returned from nsIContentPolicy
-   *                         (any of the types REJECT_*)
-   */
-  [notxpcom, nostdcall] void setBlockedRequest(in int16_t aContentDecision);
-
-  /**
    * Used to notify the image loading content node that a frame has been
    * created.
    */
   [notxpcom] void frameCreated(in nsIFrame aFrame);
 
   /**
    * Used to notify the image loading content node that a frame has been
    * destroyed.
@@ -137,17 +120,17 @@ interface nsIImageLoadingContent : imgIN
    */
   [noscript] long getRequestType(in imgIRequest aRequest);
 
   /**
    * Gets the URI of the current request, if available.
    * Otherwise, returns the last URI that this content tried to load, or
    * null if there haven't been any such attempts.
    */
-  [noscript] readonly attribute nsIURI currentURI;
+  [noscript, infallible] readonly attribute nsIURI currentURI;
 
   /**
    * loadImageWithChannel allows data from an existing channel to be
    * used as the image data for this content node.
    *
    * @param aChannel the channel that will deliver the data
    *
    * @return a stream listener to pump the image data into
--- a/dom/base/nsImageLoadingContent.cpp
+++ b/dom/base/nsImageLoadingContent.cpp
@@ -91,17 +91,16 @@ const nsAttrValue::EnumTable* nsImageLoa
     &nsImageLoadingContent::kDecodingTable[0];
 
 nsImageLoadingContent::nsImageLoadingContent()
     : mCurrentRequestFlags(0),
       mPendingRequestFlags(0),
       mObserverList(nullptr),
       mOutstandingDecodePromises(0),
       mRequestGeneration(0),
-      mImageBlockingStatus(nsIContentPolicy::ACCEPT),
       mLoadingEnabled(true),
       mIsImageStateForced(false),
       mLoading(false),
       // mBroken starts out true, since an image without a URI is broken....
       mBroken(true),
       mNewRequestsWillNeedAnimationReset(false),
       mUseUrgentStartForChannel(false),
       mLazyLoading(false),
@@ -542,20 +541,16 @@ void nsImageLoadingContent::MaybeForceSy
 
   if (imageFrame) {
     imageFrame->SetForceSyncDecoding(forceSync);
   } else {
     svgImageFrame->SetForceSyncDecoding(forceSync);
   }
 }
 
-int16_t nsImageLoadingContent::GetImageBlockingStatus() {
-  return ImageBlockingStatus();
-}
-
 static void ReplayImageStatus(imgIRequest* aRequest,
                               imgINotificationObserver* aObserver) {
   if (!aRequest) {
     return;
   }
 
   uint32_t status = 0;
   nsresult rv = aRequest->GetImageStatus(&status);
@@ -905,43 +900,39 @@ nsImageLoadingContent::GetRequestType(im
                                       int32_t* aRequestType) {
   MOZ_ASSERT(aRequestType, "Null out param");
 
   ErrorResult result;
   *aRequestType = GetRequestType(aRequest, result);
   return result.StealNSResult();
 }
 
-already_AddRefed<nsIURI> nsImageLoadingContent::GetCurrentURI(
-    ErrorResult& aError) {
+already_AddRefed<nsIURI> nsImageLoadingContent::GetCurrentURI() {
   nsCOMPtr<nsIURI> uri;
   if (mCurrentRequest) {
     mCurrentRequest->GetURI(getter_AddRefs(uri));
   } else {
     uri = mCurrentURI;
   }
 
   return uri.forget();
 }
 
 NS_IMETHODIMP
 nsImageLoadingContent::GetCurrentURI(nsIURI** aURI) {
   NS_ENSURE_ARG_POINTER(aURI);
-
-  ErrorResult result;
-  *aURI = GetCurrentURI(result).take();
-  return result.StealNSResult();
+  *aURI = GetCurrentURI().take();
+  return NS_OK;
 }
 
 already_AddRefed<nsIURI> nsImageLoadingContent::GetCurrentRequestFinalURI() {
   nsCOMPtr<nsIURI> uri;
   if (mCurrentRequest) {
     mCurrentRequest->GetFinalURI(getter_AddRefs(uri));
   }
-
   return uri.forget();
 }
 
 NS_IMETHODIMP
 nsImageLoadingContent::LoadImageWithChannel(nsIChannel* aChannel,
                                             nsIStreamListener** aListener) {
   imgLoader* loader =
       nsContentUtils::GetImgLoaderForChannel(aChannel, GetOurOwnerDoc());
@@ -1102,32 +1093,29 @@ nsresult nsImageLoadingContent::LoadImag
   // Data documents, or documents from DOMParser shouldn't perform image
   // loading.
   //
   // FIXME(emilio): Shouldn't this check be part of
   // Document::ShouldLoadImages()? Or alternatively check ShouldLoadImages here
   // instead? (It seems we only check ShouldLoadImages in HTMLImageElement,
   // which seems wrong...)
   if (aDocument->IsLoadedAsData() && !aDocument->IsStaticDocument()) {
-    // This is the only codepath on which we can reach SetBlockedRequest while
-    // our pending request exists.  Just clear it out here if we do have one.
+    // Clear our pending request if we do have one.
     ClearPendingRequest(NS_BINDING_ABORTED, Some(OnNonvisible::DiscardImages));
 
-    SetBlockedRequest(nsIContentPolicy::REJECT_REQUEST);
-
     FireEvent(u"error"_ns);
     FireEvent(u"loadend"_ns);
     return NS_OK;
   }
 
   // URI equality check.
   //
-  // We skip the equality check if our current image was blocked, since in that
+  // We skip the equality check if we don't have a current image, since in that
   // case we really do want to try loading again.
-  if (!aForce && NS_CP_ACCEPTED(mImageBlockingStatus)) {
+  if (!aForce && mCurrentRequest) {
     nsCOMPtr<nsIURI> currentURI;
     GetCurrentURI(getter_AddRefs(currentURI));
     bool equal;
     if (currentURI && NS_SUCCEEDED(currentURI->Equals(aNewURI, &equal)) &&
         equal) {
       // Nothing to do here.
       return NS_OK;
     }
@@ -1206,19 +1194,16 @@ nsresult nsImageLoadingContent::LoadImag
       }
     }
   } else {
     MOZ_ASSERT(!req, "Shouldn't have non-null request here");
     // If we don't have a current URI, we might as well store this URI so people
     // know what we tried (and failed) to load.
     if (!mCurrentRequest) {
       mCurrentURI = aNewURI;
-      if (mImageBlockingStatus == nsIContentPolicy::ACCEPT) {
-        mImageBlockingStatus = nsIContentPolicy::REJECT_REQUEST;
-      }
     }
 
     FireEvent(u"error"_ns);
     FireEvent(u"loadend"_ns);
   }
 
   return NS_OK;
 }
@@ -1327,19 +1312,17 @@ void nsImageLoadingContent::UpdateImageS
   }
 
   nsIContent* thisContent = AsContent();
 
   mLoading = mBroken = false;
 
   // If we were blocked, we're broken, so are we if we don't have an image
   // request at all or the image has errored.
-  if (mImageBlockingStatus != nsIContentPolicy::ACCEPT) {
-    mBroken = true;
-  } else if (!mCurrentRequest) {
+  if (!mCurrentRequest) {
     if (!mLazyLoading) {
       // In case of non-lazy loading, no current request means error, since we
       // weren't disabled or suppressed
       mBroken = true;
       RejectDecodePromises(NS_ERROR_DOM_IMAGE_BROKEN);
     }
   } else {
     uint32_t currentLoadStatus;
@@ -1447,44 +1430,18 @@ RefPtr<imgRequestProxy>& nsImageLoadingC
   // We only want to cancel the existing current request if size is not
   // available. bz says the web depends on this behavior.
   // Otherwise, we get rid of any half-baked request that might be sitting there
   // and make this one current.
   return HaveSize(mCurrentRequest) ? PreparePendingRequest(aImageLoadType)
                                    : PrepareCurrentRequest(aImageLoadType);
 }
 
-void nsImageLoadingContent::SetBlockedRequest(int16_t aContentDecision) {
-  // If this is not calling from LoadImage, for example, from ServiceWorker,
-  // bail out.
-  if (!mIsStartingImageLoad) {
-    return;
-  }
-
-  // Sanity
-  MOZ_ASSERT(!NS_CP_ACCEPTED(aContentDecision), "Blocked but not?");
-
-  // We should never have a pending request after we got blocked.
-  MOZ_ASSERT(!mPendingRequest, "mPendingRequest should be null.");
-
-  if (HaveSize(mCurrentRequest)) {
-    // PreparePendingRequest set mPendingRequestFlags, now since we've decided
-    // to block it, we reset it back to 0.
-    mPendingRequestFlags = 0;
-  } else {
-    mImageBlockingStatus = aContentDecision;
-  }
-}
-
 RefPtr<imgRequestProxy>& nsImageLoadingContent::PrepareCurrentRequest(
     ImageLoadType aImageLoadType) {
-  // Blocked images go through SetBlockedRequest, which is a separate path. For
-  // everything else, we're unblocked.
-  mImageBlockingStatus = nsIContentPolicy::ACCEPT;
-
   // Get rid of anything that was there previously.
   ClearCurrentRequest(NS_BINDING_ABORTED, Some(OnNonvisible::DiscardImages));
 
   if (mNewRequestsWillNeedAnimationReset) {
     mCurrentRequestFlags |= REQUEST_NEEDS_ANIMATION_RESET;
   }
 
   if (aImageLoadType == eImageLoadType_Imageset) {
--- a/dom/base/nsImageLoadingContent.h
+++ b/dom/base/nsImageLoadingContent.h
@@ -61,23 +61,22 @@ class nsImageLoadingContent : public nsI
   NS_DECL_NSIIMAGELOADINGCONTENT
 
   // Web IDL binding methods.
   // Note that the XPCOM SetLoadingEnabled, ForceImageState methods are OK for
   // Web IDL bindings to use as well, since none of them throw when called via
   // the Web IDL bindings.
 
   bool LoadingEnabled() const { return mLoadingEnabled; }
-  int16_t ImageBlockingStatus() const { return mImageBlockingStatus; }
   void AddObserver(imgINotificationObserver* aObserver);
   void RemoveObserver(imgINotificationObserver* aObserver);
   already_AddRefed<imgIRequest> GetRequest(int32_t aRequestType,
                                            mozilla::ErrorResult& aError);
   int32_t GetRequestType(imgIRequest* aRequest, mozilla::ErrorResult& aError);
-  already_AddRefed<nsIURI> GetCurrentURI(mozilla::ErrorResult& aError);
+  already_AddRefed<nsIURI> GetCurrentURI();
   already_AddRefed<nsIURI> GetCurrentRequestFinalURI();
   void ForceReload(bool aNotify, mozilla::ErrorResult& aError);
 
   mozilla::dom::Element* FindImageMap();
 
   /**
    * Toggle whether or not to synchronously decode an image on draw.
    */
@@ -561,17 +560,16 @@ class nsImageLoadingContent : public nsI
    * An incrementing counter representing the current request generation;
    * Each time mCurrentRequest is modified with a different URI, this will
    * be incremented. Each QueueDecodeAsync call will cache the generation
    * of the current request so that when it is processed, it knows if it
    * should have rejected because the request changed.
    */
   uint32_t mRequestGeneration;
 
-  int16_t mImageBlockingStatus;
   bool mLoadingEnabled : 1;
 
   /**
    * When true, we return mForcedImageState from ImageState().
    */
   bool mIsImageStateForced : 1;
 
   /**
--- a/dom/base/test/browser_blocking_image.js
+++ b/dom/base/test/browser_blocking_image.js
@@ -26,18 +26,16 @@ add_task(async function load_image_from_
     content.document.body.appendChild(img);
 
     try {
       await imgListener(img);
       Assert.ok(true);
     } catch (e) {
       Assert.ok(false);
     }
-
-    Assert.equal(img.imageBlockingStatus, Ci.nsIContentPolicy.ACCEPT);
   });
 
   gBrowser.removeTab(tab);
 });
 
 /**
  * Loading an image from http:// should be rejected.
  */
@@ -60,22 +58,16 @@ add_task(async function load_image_from_
     content.document.body.appendChild(img);
 
     try {
       await imgListener(img);
       Assert.ok(true);
     } catch (e) {
       Assert.ok(false);
     }
-
-    Assert.equal(
-      img.imageBlockingStatus,
-      Ci.nsIContentPolicy.REJECT_SERVER,
-      "images from http should be blocked"
-    );
   });
 
   gBrowser.removeTab(tab);
 });
 
 /**
  * Loading an image from http:// immediately after loading from https://
  * The load from https:// should be replaced.
@@ -107,33 +99,25 @@ add_task(async function load_https_and_h
     content.document.body.appendChild(img);
 
     try {
       await imgListener(img);
       Assert.ok(true);
     } catch (e) {
       Assert.ok(false);
     }
-
-    Assert.equal(
-      img.imageBlockingStatus,
-      Ci.nsIContentPolicy.REJECT_SERVER,
-      "image.src changed to http should be blocked"
-    );
   });
 
   gBrowser.removeTab(tab);
 });
 
 /**
  * Loading an image from https.
  * Then after we have size information of the image, we immediately change the
- * location to a http:// site (hence should be blocked by CSP). This will make
- * the 2nd request as a PENDING_REQUEST, also blocking 2nd load shouldn't change
- * the imageBlockingStatus value.
+ * location to a http:// site (hence should be blocked by CSP).
  */
 add_task(async function block_pending_request_test() {
   let tab = BrowserTestUtils.addTab(gBrowser, TEST_URI);
   await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
 
   gBrowser.selectedTab = tab;
 
   await SpecialPowers.spawn(tab.linkedBrowser, [], async function() {
@@ -188,13 +172,12 @@ add_task(async function block_pending_re
     // Now we change to load from http:// site, which will be blocked.
     img.src = "http://example.com/tests/image/test/mochitest/shaver.png";
 
     Assert.equal(
       img.getRequest(Ci.nsIImageLoadingContent.CURRENT_REQUEST),
       req,
       "CURRENT_REQUEST shouldn't be replaced."
     );
-    Assert.equal(img.imageBlockingStatus, Ci.nsIContentPolicy.ACCEPT);
   });
 
   gBrowser.removeTab(tab);
 });
--- a/dom/base/test/test_blocking_image.html
+++ b/dom/base/test/test_blocking_image.html
@@ -41,18 +41,16 @@ function onLoad() {
       // `iframe` is a content iframe, and thus not in the same docgroup with
       // us, which are a chrome-privileged test.
       //
       // Ensure the frame is laid out so that this cross-origin
       // getComputedStyle call is guaranteed to work after bug 1440537.
       iframe.getBoundingClientRect();
 
       ok(img2.matches(":-moz-broken"), "should match ':-moz-broken' selector");
-      is(img.imageBlockingStatus, Ci.nsIContentPolicy.REJECT_SERVER,
-         "imageBlockingStatues should be REJECT_SERVER.");
 
       img2.src = "https://test.invalid";
       doc.body.appendChild(img2);
       return imgListener(img2);
     }).then(() => {
       ok(true, "img2 shouldn't be loaded");
       ok(img2.matches(":-moz-broken"), "should match ':-moz-broken' selector");
 
@@ -65,18 +63,16 @@ function onLoad() {
       // which will be mapped to :-moz-broken too.
       img3.src = "https://example.com/tests/image/test/mochitest/shaver.png";
       doc.body.appendChild(img3);
       return imgListener(img3);
     }).then(() => {
       ok(true, "img3 shouldn't be loaded");
 
       ok(img3.matches(":-moz-broken"), "should match ':-moz-broken' selector");
-      is(img3.imageBlockingStatus, Ci.nsIContentPolicy.REJECT_TYPE,
-         "imageBlockingStatues should be REJECT_TYPE.");
 
       SimpleTest.finish();
     }).catch((e) => {
       // Expected early return
       if(e === true) {
         SimpleTest.finish();
         return;
       }
--- a/dom/webidl/HTMLImageElement.webidl
+++ b/dom/webidl/HTMLImageElement.webidl
@@ -92,35 +92,33 @@ interface mixin MozImageLoadingContent {
   const long UNKNOWN_REQUEST = -1;
   [ChromeOnly]
   const long CURRENT_REQUEST = 0;
   [ChromeOnly]
   const long PENDING_REQUEST = 1;
 
   [ChromeOnly]
   attribute boolean loadingEnabled;
-  [ChromeOnly]
-  readonly attribute short imageBlockingStatus;
   /**
    * Same as addNativeObserver but intended for scripted observers or observers
    * from another or without a document.
    */
   [ChromeOnly]
   void addObserver(imgINotificationObserver aObserver);
   /**
    * Same as removeNativeObserver but intended for scripted observers or
    * observers from another or without a document.
    */
   [ChromeOnly]
   void removeObserver(imgINotificationObserver aObserver);
   [ChromeOnly,Throws]
   imgIRequest? getRequest(long aRequestType);
   [ChromeOnly,Throws]
   long getRequestType(imgIRequest aRequest);
-  [ChromeOnly,Throws]
+  [ChromeOnly]
   readonly attribute URI? currentURI;
   // Gets the final URI of the current request, if available.
   // Otherwise, returns null.
   [ChromeOnly]
   readonly attribute URI? currentRequestFinalURI;
   /**
    * forceReload forces reloading of the image pointed to by currentURI
    *
--- a/layout/generic/nsImageFrame.cpp
+++ b/layout/generic/nsImageFrame.cpp
@@ -192,19 +192,22 @@ bool nsImageFrame::ShouldShowBrokenImage
   // check for broken images. valid null images (eg. img src="") are
   // not considered broken because they have no image requests
   if (nsCOMPtr<imgIRequest> currentRequest = GetCurrentRequest()) {
     uint32_t imageStatus;
     return NS_SUCCEEDED(currentRequest->GetImageStatus(&imageStatus)) &&
            (imageStatus & imgIRequest::STATUS_ERROR);
   }
 
-  nsCOMPtr<nsIImageLoadingContent> loader = do_QueryInterface(mContent);
-  MOZ_ASSERT(loader);
-  return loader->GetImageBlockingStatus() != nsIContentPolicy::ACCEPT;
+  nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mContent);
+  MOZ_ASSERT(imageLoader);
+  // Show the broken image icon only if we've tried to perform a load at all
+  // (that is, if we have a current uri).
+  nsCOMPtr<nsIURI> currentURI = imageLoader->GetCurrentURI();
+  return !!currentURI;
 }
 
 nsImageFrame* nsImageFrame::CreateContinuingFrame(
     mozilla::PresShell* aPresShell, ComputedStyle* aStyle) const {
   return new (aPresShell)
       nsImageFrame(aStyle, aPresShell->GetPresContext(), mKind);
 }