Closed Bug 514033 Opened 11 years ago Closed 10 years ago

Error recovery for imagelib

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(20 files, 4 obsolete files)

48.83 KB, image/png
Details
96.94 KB, image/png
Details
195.31 KB, image/jpeg
Details
479.33 KB, image/jpeg
Details
3.32 KB, patch
joe
: review+
Details | Diff | Splinter Review
35.16 KB, patch
joe
: review+
Details | Diff | Splinter Review
1.18 KB, patch
joe
: review+
Details | Diff | Splinter Review
50.70 KB, patch
joe
: review+
Details | Diff | Splinter Review
8.94 KB, patch
joe
: review+
Details | Diff | Splinter Review
2.25 KB, patch
joe
: review+
Details | Diff | Splinter Review
9.17 KB, patch
joe
: review+
Details | Diff | Splinter Review
795 bytes, patch
joe
: review+
Details | Diff | Splinter Review
5.62 KB, patch
joe
: review+
Details | Diff | Splinter Review
6.97 KB, patch
joe
: review+
Details | Diff | Splinter Review
9.56 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.08 KB, patch
bholley
: review+
Details | Diff | Splinter Review
11.69 KB, patch
joe
: review+
Details | Diff | Splinter Review
7.95 KB, patch
joe
: review+
Details | Diff | Splinter Review
20.79 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.02 KB, patch
bholley
: review+
Details | Diff | Splinter Review
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.
Attached image truncated png
Attached image original png
Attached image truncated jpg
Attached image original jpg
Blocks: 557018
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.
Blocks: 578684
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Assignee: nobody → bobbyholley+bmo
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 582333
Blocks: 565773
Blocks: 584092
We should also at some point implement the solution I talked about in bug 584092 comment 5.
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.
Status: NEW → ASSIGNED
Depends on: 513681
probably related: bug 543674
Depends on: 593489
This patch introduces machinery to track errors in the superclass.
Attachment #472447 - Flags: review?(joe)
This makes the decoders use the aforementioned framework.
Attachment #472450 - Flags: review?(joe)
We're soon going to stop returning errors entirely in the decoder implementations. This is a necessary first step.
Attachment #472451 - Flags: review?(joe)
This patch makes the decoder implementations return void, switching us over completely to the new framework.
Attachment #472452 - Flags: review?(joe)
We send the same notifications in every InitInternal implementation. Combine them!
Attachment #472453 - Flags: review?(joe)
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.
Attachment #472454 - Flags: review?(joe)
Helper patch to make some stuff easier later on.
Attachment #472455 - Flags: review?(joe)
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.
Attachment #472456 - Flags: review?(joe)
With those helper patches, we can now move the end-of-decode notifications into the superclass.
Attachment #472457 - Flags: review?(joe)
The fruit of the previous patches. No notifications are sent anymore from decoder implementations, so we can make mObserver superclass-private! Woohoo!
Attachment #472458 - Flags: review?(joe)
It was complicating things to have Finish() called conditionally. This makes us call Finish() unconditionally.
Attachment #472459 - Flags: review?(joe)
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.
Attachment #472460 - Flags: review?(joe)
Up until now, we had all these "No forgiveness" checks in decoder implementations. Replace those with asserts.
Attachment #472461 - Flags: review?(joe)
We want to tell web developers when something's wrong with their images.
Attachment #472462 - Flags: review?(joe)
Blocks: 594592
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?
Attachment #472447 - Flags: review?(joe) → review+
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.
Attachment #472450 - Flags: review?(joe) → review+
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) ?
Attachment #472451 - Flags: review?(joe) → review+
Attachment #472452 - Flags: review?(joe) → review+
Attachment #472453 - Flags: review?(joe) → review+
Attachment #472454 - Flags: review?(joe) → review+
Attachment #472455 - Flags: review?(joe) → review+
Attachment #472456 - Flags: review?(joe) → review+
Attachment #472457 - Flags: review?(joe) → review+
Attachment #472458 - Flags: review?(joe) → review+
Attachment #472459 - Flags: review?(joe) → review+
Attachment #472460 - Flags: review?(joe) → review+
Attachment #472461 - Flags: review?(joe) → review+
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. :)
Attachment #472462 - Flags: review?(joe) → review+
> 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.
> 
> 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.
(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.
Fixed some merge conflicts when rebasing to trunk. Carrying over review.
Attachment #472454 - Attachment is obsolete: true
Attachment #474435 - Flags: review+
Addressed joe's review comments. Carrying over review.
Attachment #472462 - Attachment is obsolete: true
Attachment #474436 - Flags: review+
Followup patch as promised. Flagging joe for review.
Attachment #474437 - Flags: review?(joe)
Other followup patch as promised. Flagging joe.
Attachment #474439 - Flags: review?(joe)
Attachment #474437 - Flags: review?(joe) → review+
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.
Attachment #474439 - Flags: review?(joe) → review+
PCShell bustage fix, part 1. Carrying over review.
Attachment #472457 - Attachment is obsolete: true
Attachment #474468 - Flags: review+
PCShell bustage fix, part 2. Carrying over review.
Attachment #472459 - Attachment is obsolete: true
Attachment #474469 - Flags: review+
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.
(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?
Depends on: 604495
Depends on: 619048
Duplicate of this bug: 527094
Duplicate of this bug: 580190
You need to log in before you can comment on or make changes to this bug.