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

RESOLVED FIXED in mozilla8

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bc, Assigned: Joe Drew (not getting mail))

Tracking

(Blocks: 1 bug, {assertion})

Trunk
mozilla8
x86
All
assertion
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

(URL)

Attachments

(5 attachments)

(Reporter)

Description

6 years ago
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
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?
blocking2.0: --- → ?

Comment 2

6 years ago
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
(Reporter)

Comment 3

6 years ago
I can reproduce on Windows XP in a debug build loading the testcase. Note that just loading the image doesn't abort.
Group: core-security
> I don't get an error on Windows XP

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

Updated

6 years ago
Assignee: nobody → joe
(Assignee)

Comment 5

6 years ago
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.
Group: core-security

Comment 6

6 years ago
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).
blocking2.0: ? → -
(Assignee)

Comment 7

6 years ago
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.
Summary: ABORT: Decoder frame count doesn't match image's!: 'mFrameCount == mImage->GetNumFrames()' → GIF decoder doesn't support multipart/x-mixed-replace, leading to ABORT: Decoder frame count doesn't match image's!: 'mFrameCount == mImage->GetNumFrames()'
(Assignee)

Comment 8

6 years ago
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.
Attachment #543276 - Flags: review?(jmuizelaar)
(Assignee)

Comment 9

6 years ago
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.
Attachment #543293 - Flags: review?(jmuizelaar)
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.
Attachment #543276 - Flags: review?(jmuizelaar) → review+
Comment on attachment 543293 [details] [diff] [review]
gif webcam testcase

As suggested, please strip the color profiles from the gifs.
Attachment #543293 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 545751 [details] [diff] [review]
Change imgFrame::mPaletteDepth from signed to unsigned.
Attachment #545751 - Flags: review?(jmuizelaar)
(Assignee)

Comment 13

6 years ago
(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!
Attachment #545751 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 14

6 years ago
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
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/d50618fed642
http://hg.mozilla.org/mozilla-central/rev/15af6861b21b
http://hg.mozilla.org/mozilla-central/rev/451154c37cb3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.