The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 13

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla13
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 715308
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Fix bmp/ico orange (bug 721917) by fixing bmp decoder → Fix bmp/ico orange (bug 721917) by listening for image data errors
(Assignee)

Comment 2

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=63022c335913
(Assignee)

Comment 3

5 years ago
Damn, that did not fix it.  :(
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5b75286fd31f
(Assignee)

Comment 7

5 years ago
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);
(Assignee)

Comment 8

5 years ago
Created attachment 596822 [details] [diff] [review]
Patch v1
Attachment #596822 - Flags: review?(joe)
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Tryserver runs for patch v1: https://tbpl.mozilla.org/?tree=Try&rev=044938ec3603
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+
(Assignee)

Comment 12

5 years ago
> 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
status-firefox13: --- → fixed
(Assignee)

Updated

5 years ago
Depends on: 721917
https://hg.mozilla.org/mozilla-central/rev/e2efe112d306
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.