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.

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.

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.

probably related: bug 543674

This patch introduces machinery to track errors in the superclass.

This makes the decoders use the aforementioned framework.

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

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

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

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.

Helper patch to make some stuff easier later on.

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.

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

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

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

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.

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

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

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 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 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 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. :)

> 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.

Addressed joe's review comments. Carrying over review.

Followup patch as promised. Flagging joe for review.

Other followup patch as promised. Flagging joe.

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.

PCShell bustage fix, part 1. Carrying over review.

PCShell bustage fix, part 2. Carrying over review.

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.

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?