514033 - Error recovery for imagelib

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

bug 435296 implements proper error handling and robustness for imagelib, but this comes at the expense of leniency for busted images. Pre bug 435296, it looks like we'd display whatever the decoder had managed to stick in the container before it broke, whereas post bug 435296 the image container refuses to draw/getframe/etc when it's detected an error.

The simplest case of a busted image is an abruptly truncated image. For pngs, this means that we have some complete rows and some empty ones. For jpgs, it seems to mean that the image just gets fuzzier (don't know a lot about jpg internals).

I think bz is right that we should continue displaying what we've got when we encounter an error. It's possible that this would be less work after bug 513681 is fixed.

Comment 1

[Bobby Holley (:bholley)]

Attachment: truncated png

Comment 2

[Bobby Holley (:bholley)]

Attachment: original png

Comment 3

[Bobby Holley (:bholley)]

Attachment: truncated jpg

Comment 4

[Bobby Holley (:bholley)]

Attachment: original jpg

Comment 5

[Bobby Holley (:bholley)]

So here's the strategy I'm thinking of implementing:

We start by classifying errors based on whether they're the fault of corrupt image data or of mozilla. When we encounter an image data error, we determine if we've gotten at least one frame. If we haven't, we throw an error as we do now. If we have, we log a warning to the console, lock the image (preventing future discarding/redecode/error cycles), and mark the image as ok.

Comment 6

[Bobby Holley (:bholley)]

We should also at some point implement the solution I talked about in bug 584092 comment 5.

Comment 7

[Bobby Holley (:bholley)]

I've been looking into this more, and I've decided that we really need to have uniform decoder behavior first to make this workable. So I'm adding a dependency on bug 513681, and nominating that bug for blocking.

Comment 8

[Max Stepin]

probably related: bug 543674

Comment 9

[Bobby Holley (:bholley)]

Attachment: part 1 - v1 - Introduce error tracking to Decoder

This patch introduces machinery to track errors in the superclass.

Comment 10

[Bobby Holley (:bholley)]

Attachment: part 2 - v1 - Notify the superclass error reporting framework when an error occurs

This makes the decoders use the aforementioned framework.

Comment 11

[Bobby Holley (:bholley)]

Attachment: part 3 - v1 - Make errors returned by superclass methods contingent only on IsError()

We're soon going to stop returning errors entirely in the decoder implementations. This is a necessary first step.

Comment 12

[Bobby Holley (:bholley)]

Attachment: part 4 - v1 - Stop returning errors from image decoders.

This patch makes the decoder implementations return void, switching us over completely to the new framework.

Comment 13

[Bobby Holley (:bholley)]

Attachment: part 5 - v1 - Coalesce OnStartDecode into superclass

We send the same notifications in every InitInternal implementation. Combine them!

Comment 14

[Bobby Holley (:bholley)]

Attachment: part 6 - v1 - Get rid of EndFrameDecode() and friends.

EndFrameDecode was a wild-card that was difficult to place when coalescing teardown notifications. This gets rid of it.

This code is so incredibly difficult to reason about.

Comment 15

[Bobby Holley (:bholley)]

Attachment: part 7 - v1 - Move mIsHidden check inside nsPNGDecoder::EndImageFrame

Helper patch to make some stuff easier later on.

Comment 16

[Bobby Holley (:bholley)]

Attachment: part 8 - v1 - Remove GIFWrite

This patch removes GIFWrite, which was still returning errors and complicating logic.

This will have the effect of temporarily breaking some of our current GIF error recovery, but I don't think it's a big enough deal to spend time twiddling the patch stack.

Comment 17

[Bobby Holley (:bholley)]

Attachment: part 9 - v1 - Coalesce end-of-decode notifications into superclass

With those helper patches, we can now move the end-of-decode notifications into the superclass.

Comment 18

[Bobby Holley (:bholley)]

Attachment: part 10 - v1 - Make mObserver and friends Decoder-private

The fruit of the previous patches. No notifications are sent anymore from decoder implementations, so we can make mObserver superclass-private! Woohoo!

Comment 19

[Bobby Holley (:bholley)]

Attachment: part 11 - v1 - Move error-case teardown notifications into Decoder::Finish(), and call it unconditionally

It was complicating things to have Finish() called conditionally. This makes us call Finish() unconditionally.

Comment 20

[Bobby Holley (:bholley)]

Attachment: part 12 - v1 - Make Decoder API methods return void

This makes us rely on the Is*Error() methods when we're doing things in RasterImage, which frees us up to make the Decoder methods return void.

Comment 21

[Bobby Holley (:bholley)]

Attachment: part 13 - v1 - Stop checking IsError() in decoder implementations

Up until now, we had all these "No forgiveness" checks in decoder implementations. Replace those with asserts.

Comment 22

[Bobby Holley (:bholley)]

Attachment: part 14 - v1 - Log data errors to the console

We want to tell web developers when something's wrong with their images.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 472447 [details] [diff] [review]
part 1 - v1 - Introduce error tracking to Decoder


>diff --git a/modules/libpr0n/src/Decoder.cpp b/modules/libpr0n/src/Decoder.cpp

>+void
>+Decoder::PostDataError()
>+{
>+  mDataError = true;
>+
>+  //XXXbholley - we should probably log to the web console here
>+}

File a bug to do this?


>diff --git a/modules/libpr0n/src/Decoder.h b/modules/libpr0n/src/Decoder.h

>+  // Error tracking
>+  bool IsError() { return IsDataError() || IsDecoderError(); };
>+  bool IsDataError() { return mDataError; };
>+  bool IsDecoderError() { return NS_FAILED(mFailCode); };

I know this is going to be a little bikeshed-ish, but Is* doesn't really work for me here. The decoder isn't really an error, after all. HasError?

Comment 24

[Joe Drew (not getting mail)]

Comment on attachment 472450 [details] [diff] [review]
part 2 - v1 - Notify the superclass error reporting framework when an error occurs


>diff --git a/modules/libpr0n/decoders/nsBMPDecoder.cpp b/modules/libpr0n/decoders/nsBMPDecoder.cpp

>@@ -225,17 +224,17 @@ nsBMPDecoder::WriteInternal(const char* aBuffer, PRUint32 aCount)
>         if (mBIH.bpp <= 8) {
>             mNumColors = 1 << mBIH.bpp;
>             if (mBIH.colors && mBIH.colors < mNumColors)
>                 mNumColors = mBIH.colors;
> 
>             // Always allocate 256 even though mNumColors might be smaller
>             mColors = new colorTable[256];
>             if (!mColors) {
>-                mError = PR_TRUE;
>+                PostDecoderError(NS_ERROR_OUT_OF_MEMORY);
>                 return NS_ERROR_OUT_OF_MEMORY;
>             }

This should probably use fallible new if we're actually going to test for allocation errors.

> 
>             memset(mColors, 0, 256 * sizeof(colorTable));
>         }
>         else if (mBIH.compression != BI_BITFIELDS && mBIH.bpp == 16) {
>             // Use default 5-5-5 format
>             mBitFields.red   = 0x7C00;
>@@ -250,34 +249,34 @@ nsBMPDecoder::WriteInternal(const char* aBuffer, PRUint32 aCount)
>                                      (PRUint8**)&mImageData, &imageLength);
>         } else {
>             // mRow is not used for RLE encoded images
>             mRow = (PRUint8*)malloc((mBIH.width * mBIH.bpp)/8 + 4);
>             // +4 because the line is padded to a 4 bit boundary, but I don't want
>             // to make exact calculations here, that's unnecessary.
>             // Also, it compensates rounding error.
>             if (!mRow) {
>-                mError = PR_TRUE;
>+                PostDecoderError(NS_ERROR_OUT_OF_MEMORY);
>                 return NS_ERROR_OUT_OF_MEMORY;
>             }

Same here, but for moz_malloc. (And for mImageData.)

>diff --git a/modules/libpr0n/decoders/nsGIFDecoder2.cpp b/modules/libpr0n/decoders/nsGIFDecoder2.cpp

>     // Determine if we want to salvage the situation.
>     // If we're salvaging, send off notifications.
>     // Note that we need to make sure that we have 2 frames, since that tells us
>     // that the first frame is complete (the second could be in any state).
>     if (mImage && mImage->GetNumFrames() > 1) {
>       EndGIF(/* aSuccess = */ PR_TRUE);
>     }
> 
>-    // Otherwise, set mError
>+    // Otherwise, set an error
>     else
>-      mError = PR_TRUE;
>+      PostDataError();

While you're here, will you { } brace it up?

>diff --git a/modules/libpr0n/decoders/nsICODecoder.cpp b/modules/libpr0n/decoders/nsICODecoder.cpp
>index ff1bfc1..97d4e7c 100644
>--- a/modules/libpr0n/decoders/nsICODecoder.cpp
>+++ b/modules/libpr0n/decoders/nsICODecoder.cpp
>@@ -74,17 +74,16 @@ PRUint32 nsICODecoder::CalcAlphaRowSize()
> 
> nsICODecoder::nsICODecoder()
> {
>   mPos = mNumColors = mRowBytes = mImageOffset = mCurrIcon = mNumIcons = 0;
>   mCurLine = 1; // Otherwise decoder will never start
>   mColors = nsnull;
>   mRow = nsnull;
>   mHaveAlphaData = mDecodingAndMask = PR_FALSE;
>-  mError = PR_FALSE;
> }
> 
> nsICODecoder::~nsICODecoder()
> {
>   mPos = 0;
> 
>   delete[] mColors;
> 
>@@ -115,17 +114,17 @@ nsresult
> nsICODecoder::FinishInternal()
> {
>   nsresult rv = NS_OK;
> 
>   // We should never make multiple frames
>   NS_ABORT_IF_FALSE(GetFrameCount() <= 1, "Multiple ICO frames?");
> 
>   // Send notifications if appropriate
>-  if (!IsSizeDecode() && !mError && (GetFrameCount() == 1)) {
>+  if (!IsSizeDecode() && !IsError() && (GetFrameCount() == 1)) {
> 
>     // Invalidate
>     nsIntRect r(0, 0, mDirEntry.mWidth, mDirEntry.mHeight);
>     PostInvalidation(r);
> 
>     PostFrameStop();
>     mImage->DecodingComplete();
>     if (mObserver) {
>@@ -136,26 +135,26 @@ nsICODecoder::FinishInternal()
> 
>   return rv;
> }
> 
> nsresult
> nsICODecoder::WriteInternal(const char* aBuffer, PRUint32 aCount)
> {
>   // No forgiveness
>-  if (mError)
>+  if (IsError())
>     return NS_ERROR_FAILURE;
> 
>   if (!aCount) // aCount=0 means EOF
>     return NS_OK;
> 
>   while (aCount && (mPos < ICONCOUNTOFFSET)) { // Skip to the # of icons.
>     if (mPos == 2) { // if the third byte is 1: This is an icon, 2: a cursor
>       if ((*aBuffer != 1) && (*aBuffer != 2)) {
>-        mError = PR_TRUE;
>+        PostDataError();
>         return NS_ERROR_FAILURE;
>       }
>       mIsCursor = (*aBuffer == 2);
>     }
>     mPos++; aBuffer++; aCount--;
>   }
> 
>   if (mPos == ICONCOUNTOFFSET && aCount >= 2) {
>@@ -189,17 +188,17 @@ nsICODecoder::WriteInternal(const char* aBuffer, PRUint32 aCount)
>       ProcessDirEntry(e);
>       if ((e.mWidth == PREFICONSIZE && e.mHeight == PREFICONSIZE && e.mBitCount >= colorDepth)
>            || (mCurrIcon == mNumIcons && mImageOffset == 0)) {
>         mImageOffset = e.mImageOffset;
> 
>         // ensure mImageOffset is >= the size of the direntry headers (bug #245631)
>         PRUint32 minImageOffset = DIRENTRYOFFSET + mNumIcons*sizeof(mDirEntryArray);
>         if (mImageOffset < minImageOffset) {
>-          mError = PR_TRUE;
>+          PostDataError();
>           return NS_ERROR_FAILURE;
>         }
> 
>         colorDepth = e.mBitCount;
>         memcpy(&mDirEntry, &e, sizeof(IconDirEntry));
>       }
>     }
>   }
>@@ -243,23 +242,23 @@ nsICODecoder::WriteInternal(const char* aBuffer, PRUint32 aCount)
>           break;
>         case 4:
>           mNumColors = 16;
>           break;
>         case 8:
>           mNumColors = 256;
>           break;
>         default:
>-          mError = PR_TRUE;
>+          PostDataError();
>           return NS_ERROR_FAILURE;
>       }
> 
>       mColors = new colorTable[mNumColors];
>       if (!mColors) {
>-        mError = PR_TRUE;
>+        PostDecoderError(NS_ERROR_OUT_OF_MEMORY);

Fallible new.

>@@ -273,17 +272,17 @@ nsICODecoder::WriteInternal(const char* aBuffer, PRUint32 aCount)
>     }
> 
>     mCurLine = mDirEntry.mHeight;
>     mRow = (PRUint8*)malloc((mDirEntry.mWidth * mBIH.bpp)/8 + 4);
>     // +4 because the line is padded to a 4 bit boundary, but I don't want
>     // to make exact calculations here, that's unnecessary.
>     // Also, it compensates rounding error.
>     if (!mRow) {
>-      mError = PR_TRUE;
>+      PostDecoderError(NS_ERROR_OUT_OF_MEMORY);

moz_malloc.

Comment 25

[Joe Drew (not getting mail)]

Comment on attachment 472451 [details] [diff] [review]
part 3 - v1 - Make errors returned by superclass methods contingent only on IsError()

Can you switch from ternary operator to if (condition) ?

Comment 26

[Joe Drew (not getting mail)]

Comment on attachment 472462 [details] [diff] [review]
part 14 - v1 - Log data errors to the console


> void
> Decoder::PostDataError()
> {
>   mDataError = true;
> 
>-  //XXXbholley - we should probably log to the web console here
> }

Extra blank line here!

Also, ignore my previous comment about filing a bug. :)

Comment 27

[Bobby Holley (:bholley)]

> I know this is going to be a little bikeshed-ish, but Is* doesn't really work
> for me here. The decoder isn't really an error, after all. HasError?

This'll mess up the patch stack, so I'll post another part to handle this ex-post-facto.

Comment 28

[Bobby Holley (:bholley)]

> 
> While you're here, will you { } brace it up?
> 

This goes away in a later patch, so I'll skip it.


> 
> This should probably use fallible new if we're actually going to test for
> allocation errors.
> 

> 
> Same here, but for moz_malloc. (And for mImageData.)


> 
> Fallible new.

> 
> moz_malloc.

I'll do these on top.

Comment 29

[Bobby Holley (:bholley)]

(In reply to comment #25)
> Comment on attachment 472451 [details] [diff] [review]
> part 3 - v1 - Make errors returned by superclass methods contingent only on
> IsError()
> 
> Can you switch from ternary operator to if (condition) ?

This stuff is all temporary and goes away in a later patch, so I'll skip it.

Comment 30

[Bobby Holley (:bholley)]

Attachment: part 6 - v2 - Get rid of EndFrameDecode() and friends.

Fixed some merge conflicts when rebasing to trunk. Carrying over review.

Comment 31

[Bobby Holley (:bholley)]

Attachment: part 14 - v2 - Log data errors to the console

Addressed joe's review comments. Carrying over review.

Comment 32

[Bobby Holley (:bholley)]

Attachment: part 15 - v1 - Rename Is*Error to Has*Error

Followup patch as promised. Flagging joe for review.

Comment 33

[Bobby Holley (:bholley)]

Attachment: part 16 - Eliminate checks for infallible malloc, use infallible malloc when we need it.

Other followup patch as promised. Flagging joe.

Comment 34

[Joe Drew (not getting mail)]

Comment on attachment 474439 [details] [diff] [review]
part 16 - Eliminate checks for infallible malloc, use infallible malloc when we need it.

woooo

Only wish would be to say "Explicitly use fallible malloc" where you do, but that is a faint wish, one which you do not necessarily need to grant.

Comment 35

[Bobby Holley (:bholley)]

Attachment: part 9 - v2 - Coalesce end-of-decode notifications into superclass

PCShell bustage fix, part 1. Carrying over review.

Comment 36

[Bobby Holley (:bholley)]

Attachment: part 11 - v2 - Move error-case teardown notifications into Decoder::Finish(), and call it unconditionally

PCShell bustage fix, part 2. Carrying over review.

Comment 37

[Bobby Holley (:bholley)]

Pushed to mozilla-central:

part 1: http://hg.mozilla.org/mozilla-central/rev/bdad0efef300
part 2: http://hg.mozilla.org/mozilla-central/rev/03486e7a7b32
part 3: http://hg.mozilla.org/mozilla-central/rev/fc39a0377a13
part 4: http://hg.mozilla.org/mozilla-central/rev/bba73a834d33
part 5: http://hg.mozilla.org/mozilla-central/rev/817a480e3836
part 6: http://hg.mozilla.org/mozilla-central/rev/c2686243b32e
part 7: http://hg.mozilla.org/mozilla-central/rev/fb21557442d3
part 8: http://hg.mozilla.org/mozilla-central/rev/f5b73f397fed
part 9: http://hg.mozilla.org/mozilla-central/rev/0d39ca861f0a
part 10: http://hg.mozilla.org/mozilla-central/rev/af4885147126
part 11: http://hg.mozilla.org/mozilla-central/rev/342e9263a73d
part 12: http://hg.mozilla.org/mozilla-central/rev/b6f6dab80eef
part 13: http://hg.mozilla.org/mozilla-central/rev/f17865986035
part 14: http://hg.mozilla.org/mozilla-central/rev/0acc8dd00971
part 15: http://hg.mozilla.org/mozilla-central/rev/06346509f9fc
part 16: http://hg.mozilla.org/mozilla-central/rev/389e836517bc


Resolving fixed.

Comment 38

[[e-mail]]

Should this be switching from PR_FREEIF to moz_free? I'm not 100% sure we can guarantee that PR_FREEIF works in some edge cases.

Comment 39

[Bobby Holley (:bholley)]

(In reply to comment #38)
> Should this be switching from PR_FREEIF to moz_free? I'm not 100% sure we can
> guarantee that PR_FREEIF works in some edge cases.

I don't know the difference to be honest. What's the issue? Maybe file a separate bug?