User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051109 Camino/1.0+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051109 Camino/1.0+ Load GIF, watch Camino go boom. Trunk-only recent regression (last 48 hours or so). Reproducible: Always Steps to Reproduce:

Relevant portion of crash log. I initially thought this might be a Core bug (looks like a crash in the GIF decoder) but I can't find any recent check-ins that look relevant. cl

In the interest of narrowing down the regression window, the Firefox trunk build from 10 November doesn't crash. The 10 November build of Camino does. The two were built about six hours apart, with Firefox being the earlier build. (Waiting for tomorrow's Fx nightly to check that, hoping that the crash shows up there.) cl

This crashes today's Firefox trunk.

Talkback reports are most likely [@ CoreGraphics.256.27.0], I'll upload the lib for better symbol tracking.

Looking at the regression window, bug 301594 seems like an early leader. Alfred? http://bonsai.mozilla.org/cvsquery.cgi?branch=HEAD&branchtype=match&date=explicit&mindate=2005-11-10+00%3A00&maxdate=2005-11-10+09%3A00

Confirming bug 301594 as the cause.

Diagnosed. The call from imgContainerGIF::BlackenFrame to gfxImageFrame::SetImageData is not doing what's expected. http://lxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/gif/imgContainerGIF.cpp#961 http://lxr.mozilla.org/mozilla/source/gfx/src/shared/gfxImageFrame.cpp#284 This image is 80x80, !mTopToBottom. On the Mac, we have 4bpp, and aLength is properly coming in as 25600, and aOffset is 0 corresponding to the top left. The newOffset computation for !mTopToBottom gives 25280, which is the top-left pixel for a bottom-to-top image, but there's no way that memset(data+25280, 0, 25600) would be a good idea or do the right thing. SetImageData correctly notices this and bails out. This then crashes on the Mac because ImageUpdated is never called. I haven't tested trunk builds on other platforms. SetImageData is broken for setting image data in a bottom-to-top image that crosses row boundaries. It should implement a different model for setting the data in a bottom-to-top image. I wouldn't mind taking a swing at this.

*** Bug 316010 has been marked as a duplicate of this bug. ***

From a post on MZ Forums: It doesn't crash Camino if you disable image animation. If that helps at all. cl

The whole !mTopToBottom thing is not right for functions which work with an 'offset' and a 'length' (and not with a 'x,y,w,h rect'). The 'offset' and 'length' try to set the area of the image to be set (or cleared), but as the logic is reversed for Mac (Camino), the adjustement of offset into the real top-left should be thus replaced with code to get to the offset of the first affected row. So, if offset is zero (meaning top-left), and length is 100 rows than the ajusted offset should be 100 rows from the bottom for !mTopToBottom (end-100*bpr), so that memset(data+end-100*bpr, 100*bpr) will work...

Boris, with this patch, SetImageData will at least do the right thing when clearing data. Setting real image data with multiple rows will probably reverse the image. But setting real image data is currently always per row.

I think that SetImageData and SetAlphaData should be fixed for all cases, not just the clear-only case. I was planning on handling this by looping over the rows. The proposed patch is still wrong when clearing data that doesn't begin and end on line boundaries. It's a good optimization over looping for most cases, but I don't see why it should be left broken.

*** Bug 316169 has been marked as a duplicate of this bug. ***

Re #12, Mark, I agree, but let's first fix the crashing part, and then do the Right Thing.

Comment on attachment 202797 [details] [diff] [review] Patch for correctly clear multiple rows I'm not a module owner or peer for this module, so I can't review this patch (I also happen to not really know this code).

*** Bug 316237 has been marked as a duplicate of this bug. ***

*** Bug 316357 has been marked as a duplicate of this bug. ***

*** Bug 316485 has been marked as a duplicate of this bug. ***

This is crashing all over the place on the mac ff branch. Talkback reports are showing it as the #1 crash on the 1.5 branch: topcrash keyword.

Zach, this bug is a regression from a patch that was checked in on trunk but NOT on branch. So whatever is going on on branch is a different issue.

I don't see this on the branch Talkback pages at all. I *do* see it as the #1 topcrasher for trunk builds of Camino and Mac FF.

This bug also crashes Seamonkey badly. First appeared in built: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; de-AT; rv:1.9a1) Gecko/20051110 SeaMonkey/1.5a. The nightly from 20051109 is perfectly o.k.!

Please note that the above attached patch will prevent the crash! Even if not correctly doing all the 'across multiple rows SetImageData', at least the clearing will be correct.

*** Bug 316721 has been marked as a duplicate of this bug. ***

Comment on attachment 202797 [details] [diff] [review] Patch for correctly clear multiple rows lets get this in

Comment on attachment 202797 [details] [diff] [review] Patch for correctly clear multiple rows Checked this in on trunk to fix the crashes

Still crashing in Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051118 SeaMonkey/1.5a. Talkback incidents TB12038621M and TB12069400Q.

the first talkback incident you listed is a totally unrelated crash (the second isn't yet available).

As of the latest version 11/20/05 (maybe even earlier, but I skipped a few days) I no longer see this crash opening the prior web page that caused it. Somebody did something good it seems!

(In reply to comment #29) > As of the latest version 11/20/05 (maybe even earlier, but I skipped a few > days) I no longer see this crash opening the prior web page that caused it. > Somebody did something good it seems! Seconded! Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051120 SeaMonkey/1.5a

Proposed fix to also correctly set(/clear) multiple rows of data. This works as follows, if (!mTopToBottom): For each row to set, calculate the right offset, set (or clear) the row, and then do the next row. We have to do this in this way because the rows are in reversed order when !mTopToBottom (this happens on Windows, Mac, OS/2 (and others?)).

Can some people test the V2 patch on Mac, to see if it is crashproof?

Alfred - patch fails to apply to trunk Camino source from 11/22.

Alfred's patch v2 (attachment 203773 [details] [diff] [review]) was prepared against the original gfxImageFrame.cpp without his previous patch (attachment 202797 [details] [diff] [review]) applied. I regenerated the patch against current cvs head. With that in mind, the code here looks correct, but I think the patch could be better. There's a lot of duplication in SetImageData and SetAlphaData that could be factored out into either a private method or a static function. I'm picturing something either with a template like gfxImageFrame::SetData(const PRUint8* aData, PRUint32 aLength, PRInt32 aOffset, PRUint8* aBase, PRInt32 aRowStride), or possibly even gfxImageFrame::SetData(const PRUint8* aData, PRUint32 aLength, PRInt32 aOffset, PRBool aSetAlpha) (where aSetAlpha is the deciding factor between GetBits()/GetLineStride() and GetAlphaBits()/GetAlphaLineStride().) It would also be nice to do a bulk memset to clear consecutive rows in the bottom-to-top case as an optimization over looping, since that's the common case in either method.

Reassigning to Alfred - Alfred, if you don't want this any more, feel free to kick it back to me.

(In reply to comment #32) > Can some people test the V2 patch on Mac, to see if it is crashproof? worksforme on Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051121 SeaMonkey/1.5a (assuming the patch made the build...)

Refactored into gfxImageFrame::SetData(const PRUint8* aData, PRUint32 aLength, PRInt32 aOffset, PRBool aSetAlpha) and doing 'memset' in minimal calls. Note than when aData is null, and xOffset is not null, and aLength is not an equal number of rows the first row and last row need to be cleared partly, and thus need to do memset separately for these rows when going BottomToTop... In normal cases however aLength will be a complete row or complete image, and xOffset zero. This results than in one memset (as before).

Comment on attachment 204123 [details] [diff] [review] V3: refactored into SetData and do memset in less calls The code is good, I just have a few comments mostly about consistency and variable names. Re comment 32, this remains crash-free on the Mac. >Index: gfxImageFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/gfx/src/shared/gfxImageFrame.cpp,v >retrieving revision 1.34 >diff -u -8 -p -r1.34 gfxImageFrame.cpp >--- gfxImageFrame.cpp 17 Nov 2005 15:23:22 -0000 1.34 >+++ gfxImageFrame.cpp 24 Nov 2005 16:09:05 -0000 >+nsresult gfxImageFrame::SetData(const PRUint8 *aData, PRUint32 aLength, >+ PRInt32 aOffset, PRBool aSetAlpha) >+{ > if (!mInitalized) > return NS_ERROR_NOT_INITIALIZED; > > NS_ASSERTION(mMutable, "trying to set data on an immutable frame"); >+ NS_ASSERTION(aOffset, "can't have a negative offset"); The assertion condition doesn't match the text or subsequent failure condition, |aOffset < 0|. >+ if (!mMutable || aOffset < 0) >+ return NS_ERROR_FAILURE; >+ >+ if (aSetAlpha && !mImage->GetHasAlphaMask()) > return NS_ERROR_FAILURE; Should be NS_ERROR_NOT_INITIALIZED, right? >+ mImage->LockImagePixels(aSetAlpha); >+ PRUint8 *imgData = aSetAlpha ? mImage->GetAlphaBits() : mImage->GetBits(); >+ const PRUint32 row_stride = aSetAlpha ? mImage->GetAlphaLineStride() : mImage->GetLineStride(); Among imgLen, numRows, and startRow, why not rowStride? >+ const PRUint32 imgLen = row_stride * mSize.height; >+ const PRUint32 numRows = 1 + ((aLength-1) / row_stride); >+ const PRUint32 startRow = (aOffset / row_stride); I'm finding some of these variables hard to distinguish: for example, I'm confusing numRows with mSize.height. Suggest setNumRows or numRowsToSet? setStartRow or firstRowToSet? >+ if (mTopToBottom) { >+ // Easy situation >+ if (aData) >+ memcpy(imgData + aOffset, aData, aLength); >+ else >+ memset(imgData + aOffset, 0, aLength); >+ } else { >+ // Rows are stored in reverse order (BottomToTop) from those supplied (TopToBottom) >+ // yOffset is the offset into the reversed image data for startRow >+ PRUint32 xOffset = aOffset % row_stride; >+ PRUint32 yOffset = (mSize.height - startRow - 1) * row_stride; >+ if (aData) { >+ // Set the image data in reverse order >+ for (PRUint32 i=0; i<numRows; i++) { >+ const PRUint32 len = PR_MIN(row_stride - xOffset, aLength); There are so many different lengths at work in this method, can this |len| and other similar |len|s be renamed to better reflect its purpose as the length of data to set in a row? >+ memcpy(imgData + yOffset + xOffset, aData, len); >+ aData += len; >+ aLength -= len; >+ yOffset -= row_stride; >+ xOffset = 0; >+ } >+ } else { >+ // Clear the image data in reverse order >+ if (xOffset) { >+ // First row, if not starting at first column >+ const PRUint32 len = PR_MIN(row_stride - xOffset, aLength); >+ memset(imgData + yOffset + xOffset, 0, len); >+ aLength -= len; >+ yOffset -= row_stride; >+ } >+ // Zero all the whole rows nit: the comment for "first row" is inside its enclosing conditional, and the ones for "whole rows" and "last row" are outside. Be consistent. >+ if (aLength > row_stride) { >+ const PRUint32 wholeRows = row_stride * (PRUint32)(aLength / row_stride); >+ memset(imgData + yOffset - (wholeRows - row_stride), 0, wholeRows); >+ aLength -= wholeRows; >+ yOffset -= wholeRows; >+ } >+ // Last uncomplete row nit: INcomplete. >+ if (aLength) { >+ memset(imgData + yOffset, 0, aLength); >+ } >+ } >+ } >+ mImage->UnlockImagePixels(aSetAlpha); > >+ if (!aSetAlpha) { >+ // adjust for aLength < row_stride >+ nsIntRect r(0, startRow, mSize.width, numRows); >+ mImage->ImageUpdated(nsnull, nsImageUpdateFlags_kBitsChanged, &r); >+ } Since this code is for image data only and it's outside of the main "setting" chunk, I think it would be better if it remained in SetImageData, gated by an NS_SUCCEEDED on the SetData call. >Index: gfxImageFrame.h >=================================================================== >RCS file: /cvsroot/mozilla/gfx/src/shared/gfxImageFrame.h,v >retrieving revision 1.9 >diff -u -8 -p -r1.9 gfxImageFrame.h >--- gfxImageFrame.h 20 Aug 2005 06:22:52 -0000 1.9 >+++ gfxImageFrame.h 24 Nov 2005 16:09:05 -0000 >@@ -81,9 +81,12 @@ private: >+ >+ nsresult SetData(const PRUint8 *aData, PRUint32 aLength, >+ PRInt32 aOffset, PRBool aSetAlpha); Move the method above the member variable declarations.

All comments incorporated, except for the relocation of the ImageUpdated call, as that one also needs the numRowsToSet and firstRowToSet, so moving that one to SetImageData would result in code duplication. (Also, IMHO, the current location shows better that ImageUpdated will only be called for image data, and not for alpha data.)

Comment on attachment 204463 [details] [diff] [review] V4: With comments from mark incorporated >+ const PRUint32 lengthOfRowToSet = PR_MIN(rowStride - xOffset, aLength); Excellent. My only concern now is with this line, which you have twice in your patch. When the PR_MIN macro is expanded, the difference is computed twice. Let's do this instead: + PRUint32 lengthOfRowToSet = rowStride - xOffset; + lengthOfRowToSet = PR_MIN(lengthOfRowToSet, aLength);

The change to Alfred's patch was so trivial that I applied it myself in the interest of saving time. Pav, does this look good to you?

Actually, there is no real difference in code from MS VC 6.0... ;-)

(In reply to comment #42) > Actually, there is no real difference in code from MS VC 6.0... ;-) Sure, but there's no guarantee of that optimization! Checked in on the trunk. Thanks, Alfred.

*** Bug 361189 has been marked as a duplicate of this bug. ***

Note, bug 361189 is no longer a DUP of this one (it is confirmed being a separate issue...). Marking this one as verified.