Bug 1685796 - Return BAD_IMAGE when sync-decoding an incomplete, errored image. r=tnikkel

The only way that this can happen is if we get through the
ShouldTreatAsCompleteDueToSyncDecode check returning true in the case of
the image being errored.

https://hg.mozilla.org/integration/autoland/rev/645a4d6461ca was
supposed to deal with this, but my guess is that there is a slight race
condition in which the error status isn't there at the beginning, but is
there after the StartDecoding call.

It seems returning BAD_IMAGE rather than painting transparent if we hit
a broken image is a better thing to do than what we're doing now, and
should fix the intermittent issue.

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

image/ImgDrawResult.h

@@ -25,8 +25,8 @@ namespace image {
  *
  * SUCCESS_NOT_COMPLETE: The image was drawn successfully and completely, but
  * it hasn't notified about the sync-decode yet. This can only happen when
- * layout pokes at the internal image state beforehand via
- * StyleImage::StartDecoding. This should probably go away eventually, somehow,
+ * layout pokes at the internal image state beforehand via the result of
+ * StartDecodingWithResult. This should probably go away eventually, somehow,
  * see bug 1471583.
  *
  * INCOMPLETE: We successfully drew a frame that was partially decoded. (Note

layout/generic/nsBulletFrame.cpp

@@ -574,21 +574,7 @@ Maybe<BulletRenderer> nsBulletFrame::CreateBulletRenderer(
   CounterStyle* listStyleType = ResolveCounterStyle();
   nsMargin padding = mPadding.GetPhysicalMargin(GetWritingMode());
   const auto& image = StyleList()->mListStyleImage;
-  const bool isValidImage = [&] {
-    if (image.IsNone()) {
-      return false;
-    }
-    if (auto* request = image.GetImageRequest()) {
-      uint32_t status = imgIRequest::STATUS_ERROR;
-      request->GetImageStatus(&status);
-      if (status & imgIRequest::STATUS_ERROR) {
-        return false;
-      }
-    }
-    return true;
-  }();
-
-  if (isValidImage) {
+  if (!image.IsNone()) {
     nsRect dest(padding.left, padding.top,
                 mRect.width - (padding.left + padding.right),
                 mRect.height - (padding.top + padding.bottom));

layout/painting/nsImageRenderer.cpp

@@ -61,77 +61,63 @@ nsImageRenderer::nsImageRenderer(nsIFrame* aForFrame, const StyleImage* aImage,
       mExtendMode(ExtendMode::CLAMP),
       mMaskOp(StyleMaskMode::MatchSource) {}
 
-static bool ShouldTreatAsCompleteDueToSyncDecode(const StyleImage* aImage,
-                                                 uint32_t aFlags) {
-  if (!(aFlags & nsImageRenderer::FLAG_SYNC_DECODE_IMAGES)) {
-    return false;
-  }
-
-  imgRequestProxy* req = aImage->GetImageRequest();
-  if (!req) {
-    return false;
-  }
-
-  uint32_t status = 0;
-  if (NS_FAILED(req->GetImageStatus(&status))) {
-    return false;
-  }
-
-  if (status & imgIRequest::STATUS_ERROR) {
-    // The image is "complete" since it's a corrupt image. If we created an
-    // imgIContainer at all, return true.
-    nsCOMPtr<imgIContainer> image;
-    req->GetImage(getter_AddRefs(image));
-    return bool(image);
-  }
-
-  if (!(status & imgIRequest::STATUS_LOAD_COMPLETE)) {
-    // We must have loaded all of the image's data and the size must be
-    // available, or else sync decoding won't be able to decode the image.
-    return false;
-  }
-
-  return true;
-}
-
 bool nsImageRenderer::PrepareImage() {
-  if (mImage->IsNone() ||
-      (mImage->IsImageRequestType() && !mImage->GetImageRequest())) {
-    // mImage->GetImageRequest() could be null here if the StyleImage refused
-    // to load a same-document URL, or the url was invalid, for example.
+  if (mImage->IsNone()) {
     mPrepareResult = ImgDrawResult::BAD_IMAGE;
     return false;
   }
 
-  if (!mImage->IsComplete()) {
-    // Make sure the image is actually decoding.
-    bool frameComplete = mImage->StartDecoding();
-
-    // Boost the loading priority since we know we want to draw the image.
-    if ((mFlags & nsImageRenderer::FLAG_PAINTING_TO_WINDOW) &&
-        mImage->IsImageRequestType()) {
-      MOZ_ASSERT(mImage->GetImageRequest(),
-                 "must have image data, since we checked above");
-      mImage->GetImageRequest()->BoostPriority(imgIRequest::CATEGORY_DISPLAY);
-    }
-
-    // Check again to see if we finished.
-    // We cannot prepare the image for rendering if it is not fully loaded.
-    // Special case: If we requested a sync decode and the image has loaded,
-    // push on through because the Draw() will do a sync decode then.
-    if (!(frameComplete || mImage->IsComplete()) &&
-        !ShouldTreatAsCompleteDueToSyncDecode(mImage, mFlags)) {
-      mPrepareResult = ImgDrawResult::NOT_READY;
+  const bool isImageRequest = mImage->IsImageRequestType();
+  MOZ_ASSERT_IF(!isImageRequest, !mImage->GetImageRequest());
+  imgRequestProxy* request = nullptr;
+  if (isImageRequest) {
+    request = mImage->GetImageRequest();
+    if (!request) {
+      // request could be null here if the StyleImage refused
+      // to load a same-document URL, or the url was invalid, for example.
+      mPrepareResult = ImgDrawResult::BAD_IMAGE;
       return false;
     }
   }
 
-  if (mImage->IsImageRequestType()) {
-    MOZ_ASSERT(mImage->GetImageRequest(),
-               "must have image data, since we checked above");
+  if (!mImage->IsComplete()) {
+    MOZ_DIAGNOSTIC_ASSERT(isImageRequest);
+
+    // Make sure the image is actually decoding.
+    bool frameComplete =
+        request->StartDecodingWithResult(imgIContainer::FLAG_ASYNC_NOTIFY);
+
+    // Boost the loading priority since we know we want to draw the image.
+    if (mFlags & nsImageRenderer::FLAG_PAINTING_TO_WINDOW) {
+      request->BoostPriority(imgIRequest::CATEGORY_DISPLAY);
+    }
+
+    // Check again to see if we finished.
+    // We cannot prepare the image for rendering if it is not fully loaded.
+    if (!frameComplete && !mImage->IsComplete()) {
+      uint32_t imageStatus = 0;
+      request->GetImageStatus(&imageStatus);
+      if (imageStatus & imgIRequest::STATUS_ERROR) {
+        mPrepareResult = ImgDrawResult::BAD_IMAGE;
+        return false;
+      }
+
+      // Special case: If not errored, and we requested a sync decode, and the
+      // image has loaded, push on through because the Draw() will do a sync
+      // decode then.
+      const bool syncDecodeWillComplete =
+          (mFlags & FLAG_SYNC_DECODE_IMAGES) &&
+          (imageStatus & imgIRequest::STATUS_LOAD_COMPLETE);
+      if (!syncDecodeWillComplete) {
+        mPrepareResult = ImgDrawResult::NOT_READY;
+        return false;
+      }
+    }
+  }
+
+  if (isImageRequest) {
     nsCOMPtr<imgIContainer> srcImage;
-    DebugOnly<nsresult> rv =
-        mImage->GetImageRequest()->GetImage(getter_AddRefs(srcImage));
+    DebugOnly<nsresult> rv = request->GetImage(getter_AddRefs(srcImage));
     MOZ_ASSERT(NS_SUCCEEDED(rv) && srcImage,
                "If GetImage() is failing, mImage->IsComplete() "
                "should have returned false");

layout/style/ServoStyleConstsInlines.h

@@ -979,8 +979,6 @@ bool StyleImage::IsSizeAvailable() const;
 template <>
 bool StyleImage::IsComplete() const;
 template <>
-bool StyleImage::StartDecoding() const;
-template <>
 Maybe<StyleImage::ActualCropRect> StyleImage::ComputeActualCropRect() const;
 template <>
 void StyleImage::ResolveImage(dom::Document&, const StyleImage*);

layout/style/nsStyleStruct.cpp

@@ -1517,17 +1517,6 @@ Maybe<StyleImage::ActualCropRect> StyleImage::ComputeActualCropRect() const {
   return Some(ActualCropRect{finalRect, isEntireImage});
 }
 
-template <>
-bool StyleImage::StartDecoding() const {
-  if (IsImageRequestType()) {
-    imgRequestProxy* req = GetImageRequest();
-    return req &&
-           req->StartDecodingWithResult(imgIContainer::FLAG_ASYNC_NOTIFY);
-  }
-  // None always returns false from IsComplete, so we do the same here.
-  return !IsNone();
-}
-
 template <>
 bool StyleImage::IsOpaque() const {
   if (IsImageSet()) {

servo/ports/geckolib/cbindgen.toml

@@ -807,12 +807,6 @@ renaming_overrides_prefixing = true
   // is not `none`.
   bool IsSizeAvailable() const;
 
-  // Starts the decoding of a image. Returns true if the current frame of the
-  // image is complete. The return value is intended to be used instead of
-  // IsComplete because IsComplete may not be up to date if notifications
-  // from decoding are pending because they are being sent async.
-  bool StartDecoding() const;
-
   // Returns true if the item is definitely opaque --- i.e., paints every
   // pixel within its bounds opaquely, and the bounds contains at least a pixel.
   bool IsOpaque() const;