641748 - GIF decoder doesn't support multipart/x-mixed-replace, leading to ABORT: Decoder frame count doesn't match image's!: 'mFrameCount == mImage->GetNumFrames()'

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: testcase

1. http://solarimg.org/artis/
2. ABORT: Decoder frame count doesn't match image's!: 'mFrameCount == mImage->GetNumFrames()', file /work/mozilla/builds/2.0.0/mozilla/modules/libpr0n/src/Decoder.cpp, line 227

reproduced 2.0.0 mac/linux

reduced testcase 
<img src="http://tycho.usno.navy.mil/cgi-bin/nph-usnoclock.gif?zone=UTC&ticks=60">

does not crash opt nor 1.9.2

Comment 1

[Boris Zbarsky [:bzbarsky]]

Presumably this is really bad, if it's bad enough to do a fatal abort in a debug build...  In particular, it looks like we might be doing some out-of-bounds access when this assert fails.

Bobby, can you look into this?  Or are you still busy with school stuff?

Comment 2

[B.J. Herbison]

I don't get an error on Windows XP (nothing in web console, error console, or the command line), but the image show up once and then vanishes. This is a regression from 3.6.15 where the image changes a few times and then sticks around.

Windows XP
Mozilla/5.0 (Windows NT 5.2; WOW64; rv:2.0b13pre) Gecko/20110315 Firefox/4.0b13pre

Comment 3

[Bob Clary [:bc] (inactive)]

I can reproduce on Windows XP in a debug build loading the testcase. Note that just loading the image doesn't abort.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I don't get an error on Windows XP

In a debug build?  The abort message above is debug-only.

Comment 5

[Joe Drew (not getting mail)]

I believe that this is a safe assertion; it represents a logic error, but not a security problem. In particular, RasterImage::GetImgFrame, which actually does the frame array indexing, is safe to call with invalid indices.

Comment 6

[B.J. Herbison]

Attachment: Image Properites box mostly grey

(In reply to comment #4)
> In a debug build?  The abort message above is debug-only.

I tested with the standard nightly, so no abort. As you noted, the image itself works, but the image didn't reload correctly in either the original URL or the reduced test case (a regression from 3.6.15).

If I display the image and then right-click/View Image Info I get one of three situations:

1) If done after the image stops updating, the Media Preview box shows the final version of the image.

2) Sometimes I saw the initial version of the image and then the image went blank (the same symptoms as the test case).

3) Sometimes the image properties box was mostly grey (see the attached image).

Comment 7

[Joe Drew (not getting mail)]

This is a multipart/x-mixed-replace image, which the GIF decoder doesn't support. It doesn't fill me with joy that decoders themselves apprently need to support this.

Comment 8

[Joe Drew (not getting mail)]

Attachment: Change from RasterImage::AppendFrame to RasterImage::EnsureFrame

Fundamentally, this problem occurred because decoders don't really know (and shouldn't really *need* to know) that multipart/x-mixed-replace exists. They just happily go about their business appending frames to the RasterImage, leaving the decoder (which gets reset) and the RasterImage (which doesn't) out of sync.

So let's be more explicit about what we we're doing when we decode images. I'm removing RasterImage::AppendFrame in favour of RasterImage::EnsureFrame, which takes as a parameter the frame number we're ensuring exists (and returns the same pointers, etc as AppendFrame did). This should transparently make all image formats support multipart/x-mixed-replace, and fixes this bug.

Comment 9

[Joe Drew (not getting mail)]

Attachment: gif webcam testcase

This is a gif "webcam" that emulates the same failures as the testcase in this bug. It aborts without my patch, and passes with it.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 543276 [details] [diff] [review]
Change from RasterImage::AppendFrame to RasterImage::EnsureFrame

Review of attachment 543276 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/src/imgFrame.h
@@ +138,5 @@
>    // returns an estimate of the memory used by this imgFrame
>    PRUint32 EstimateMemoryUsed() const;
>  
> +  PRUint8 GetPaletteDepth() const { return PRUint8(mPaletteDepth); }
> +

Please fix the type of mPaletteDepth in a precursor to this patch.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 543293 [details] [diff] [review]
gif webcam testcase

As suggested, please strip the color profiles from the gifs.

Comment 12

[Joe Drew (not getting mail)]

Attachment: Change imgFrame::mPaletteDepth from signed to unsigned.

Comment 13

[Joe Drew (not getting mail)]

(In reply to comment #12)
> Created attachment 545751 [details] [diff] [review] [review]
> Change imgFrame::mPaletteDepth from signed to unsigned.

This was done from the command-line with hg bzexport. I am very impressed!

Comment 14

[Joe Drew (not getting mail)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d50618fed642
http://hg.mozilla.org/integration/mozilla-inbound/rev/15af6861b21b
http://hg.mozilla.org/integration/mozilla-inbound/rev/451154c37cb3

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d50618fed642
http://hg.mozilla.org/mozilla-central/rev/15af6861b21b
http://hg.mozilla.org/mozilla-central/rev/451154c37cb3