Closed Bug 726004 Opened 8 years ago Closed 8 years ago

Fix bmp/ico orange (bug 721917) by listening for image data errors

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox13 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file)

It appears that the bmp decoder notices invalid bits, writes them to the output buffer, and then does not immediately return an error code.  This seems pretty bogus to me, and I think it may be what's causing bug 721917.

Why my patch to bug 715308 causes this, I do not know.
Depends on: 715308
Ah, I see.  The BMP decoder sends a data error, but not a decoder error.  And we listen only for decoder errors.  So this isn't a bug in the BMP decoder, but maybe I can fix it in RasterImage.
Summary: Fix bmp/ico orange (bug 721917) by fixing bmp decoder → Fix bmp/ico orange (bug 721917) by listening for image data errors
Damn, that did not fix it.  :(
This push [1], which adds a sync-until-size decode to AddSourceData (because originally we'd do a sync decode there, but I changed it so we don't), also did not fix things 

However, this push [2] has had many green reftest runs.  The relevant change is

    1.65 @@ -3001,22 +3012,22 @@ RasterImage::DecodeWorker::DecodeSomeOfImage
    1.66      }
    1.67  
    1.68      // We keep decoding chunks until either:
    1.69      //  * we're an UNTIL_SIZE decode and we get the size,
    1.70      //  * we don't have any data left to decode,
    1.71      //  * the decode completes, or
    1.72      //  * we run out of time.
    1.73  
    1.74 -    if (aDecodeType == DECODE_TYPE_UNTIL_SIZE && aImg->mHasSize)
    1.75 -      break;
    1.80 +    /*if (aDecodeType == DECODE_TYPE_UNTIL_SIZE && aImg->mHasSize)
    1.81 +      break;*/

I'm not sure why this is the magic change...

[1] https://tbpl.mozilla.org/?tree=Try&rev=23e6d49c4a3e
[2] https://tbpl.mozilla.org/?tree=Try&rev=ab57bf32c9fa
What's particularly strange is that all of the images in image/test/reftest/bmp/bmp-corrupted have size less than 4096, which is the value of image.mem.decode_bytes_at_a_time.  Therefore, DecodeSomeOfImage should always decode all of the available data.  The loop should not run twice, and therefore the early break shouldn't make any difference.

Perhaps in some error case, we bail early and don't recognize the error until a second pass through the loop.
The patch below seems to fix the problem, or at least make it significantly more rare (I have 234 greens on try).

But I think returning NS_ERROR_FAILURE here may cause us to throw onerror for corrupt image data, which is not what we want to do (onerror is for network errors, according to bholley and jst).  I'll see if I can return NS_OK here.

diff --git a/image/src/RasterImage.cpp b/image/src/RasterImage.cpp
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -3001,16 +3001,22 @@ RasterImage::DecodeWorker::DecodeSomeOfI
     }
 
     // We keep decoding chunks until either:
     //  * we're an UNTIL_SIZE decode and we get the size,
     //  * we don't have any data left to decode,
     //  * the decode completes, or
     //  * we run out of time.
 
+    if (aImg->mDecoder->HasError()) {
+      printf("Hit aImg->mDecoder->HasError().  Bailing.\n");
+      aImg->DoError();
+      return NS_ERROR_FAILURE;
+    }
+
     if (aDecodeType == DECODE_TYPE_UNTIL_SIZE && aImg->mHasSize)
       break;
 
   } while (aImg->mSourceData.Length() > aImg->mBytesDecoded &&
            !aImg->IsDecodeFinished() &&
            TimeStamp::Now() < deadline);
 
   aImg->mDecodeRequest.mDecodeTime += (TimeStamp::Now() - start);
Attached patch Patch v1Splinter Review
Attachment #596822 - Flags: review?(joe)
AFAICT, this just happened to work before bug 715308.

What I think must have happened is, on these synchronous decodes (e.g. triggered by SourceDataComplete), we'd to through the decode loop *twice*.  The first time, we'd notice invalid data and bail.  Then the second time, DecodeSomeData() must have returned an error code, since we'd already pushed all our data into the decoder.  (It returns an error code when mDecoder is null, and I'm not sure exactly how that happens.)  So we wouldn't end up flushing invalidations, because we'd return early.

Now we break out of the loop after one iteration on synchronous until-size decodes, so we don't get the benefit of this behavior.

The simple thing to do is not paint if the decoder flags an error.  This means that some bogus images may be cut off even earlier than they would be before, but I think that's OK.  We make no guarantees about being able to paint part of corrupted images.
Comment on attachment 596822 [details] [diff] [review]
Patch v1

Review of attachment 596822 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +3018,5 @@
>    if (chunkCount && !aImg->mDecoder->IsSizeDecode()) {
>      Telemetry::Accumulate(Telemetry::IMAGE_DECODE_CHUNKS, chunkCount);
>    }
>  
> +  // Flush invalidations after we've decoded all the chunks we're going to.

Add a "(and hence paint)" after "Flush invalidations". And maybe add a blank line.
Attachment #596822 - Flags: review?(joe) → review+
> The first time, we'd notice invalid data and bail.

To be clear, we'd notice invalid data and bail out of the decoder, having written some possibly invalid data to the output buffer, but we wouldn't bail out of the decode worker's loop.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2efe112d306
Assignee: nobody → justin.lebar+bug
Depends on: 721917
https://hg.mozilla.org/mozilla-central/rev/e2efe112d306
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.