Last Comment Bug 641748 - GIF decoder doesn't support multipart/x-mixed-replace, leading to ABORT: Decoder frame count doesn't match image's!: 'mFrameCount == mImage->GetNumFrames()'
: GIF decoder doesn't support multipart/x-mixed-replace, leading to ABORT: Deco...
Status: RESOLVED FIXED
: assertion
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla8
Assigned To: Joe Drew (not getting mail)
:
Mentors:
http://solarimg.org/artis/
Depends on:
Blocks: 532972
  Show dependency treegraph
 
Reported: 2011-03-14 21:29 PDT by Bob Clary [:bc:]
Modified: 2011-07-15 06:53 PDT (History)
8 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
testcase (83 bytes, text/html)
2011-03-14 21:29 PDT, Bob Clary [:bc:]
no flags Details
Image Properites box mostly grey (18.86 KB, image/png)
2011-03-15 14:22 PDT, B.J. Herbison
no flags Details
Change from RasterImage::AppendFrame to RasterImage::EnsureFrame (17.14 KB, patch)
2011-06-30 15:12 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
gif webcam testcase (9.59 KB, patch)
2011-06-30 16:22 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
Change imgFrame::mPaletteDepth from signed to unsigned. (2.45 KB, patch)
2011-07-13 14:29 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review

Description Bob Clary [:bc:] 2011-03-14 21:29:53 PDT
Created attachment 519328 [details]
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 [:bz] (Out June 25-July 6) 2011-03-15 07:41:30 PDT
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 2011-03-15 10:23:30 PDT
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:] 2011-03-15 10:32:01 PDT
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 [:bz] (Out June 25-July 6) 2011-03-15 11:34:17 PDT
> 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) 2011-03-15 12:14:25 PDT
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 2011-03-15 14:22:02 PDT
Created attachment 519511 [details]
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) 2011-06-29 15:08:40 PDT
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) 2011-06-30 15:12:28 PDT
Created attachment 543276 [details] [diff] [review]
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) 2011-06-30 16:22:05 PDT
Created attachment 543293 [details] [diff] [review]
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] 2011-07-05 07:27:45 PDT
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] 2011-07-05 11:26:04 PDT
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) 2011-07-13 14:29:56 PDT
Created attachment 545751 [details] [diff] [review]
Change imgFrame::mPaletteDepth from signed to unsigned.
Comment 13 Joe Drew (not getting mail) 2011-07-13 14:39:33 PDT
(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!

Note You need to log in before you can comment on or make changes to this bug.