857623 - ABORT: Decode already done!: '!mDecodeDone' loading animated gif

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: testcase.gif

1. http://www.absba.org/showthread.php?t=884984
2. ABORT: Decode already done!: '!mDecodeDone', file c:/work/mozilla/builds/nightly/mozilla/image/src/Decoder.cpp, line 366

Aurora/22, Nightly/23 Windows, Linux, OSX. Don't see it in Beta.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Taking a look at this.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Ensure we always call PostFrameStop before PostDecodeDone in nsGIFDecoder2.cpp.

Here's the fix. We could previous end up calling PostDecodeDone without calling PostFrameStop, but now we use FinishInternal rather than calling PostDecodeDone directly, which calls PostFrameStop if needed.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

This also fixes bug 856616.  The question (if you look at the patch for that bug) is if PostDecodeDone should be called even for the size decode or not.  The simpler (but wrong?) patch from bug 856616 also fixes both problems. PostDecodeDone is in the base class, and it wasn't clear if it should be called for size decodes as well.

Comment 4

[Seth Fowler [:seth] [:s2h]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> The question (if you look at the patch for that
> bug) is if PostDecodeDone should be called even for the size decode or not. 

I assume Joe doesn't think so, or he wouldn't have NS_ABORT_IF_FALSE(!IsSizeDecode()) in PostDecodeDone. (You'd have to ask Joe to find out the reason things are designed that way, though.)

> The simpler (but wrong?) patch from bug 856616 also fixes both problems.

I feel more comfortable with this patch than that one. I'd rather not duplicate this logic in multiple places, and I think calling FinishInternal nicely takes care of several potential problems at once.

Maybe we should dupe bug 856616 to this bug?

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll make it a dupe and comment in the other bug for Joe to check if he meant that assert to be there.  I'm a bit nervous about not setting mGIFOpen to false anymore in the size decode case, but I don't know if we can ever get there.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Yeah, I know what you mean regarding mGIFOpen. I don't like that the invariants are so unclear in this code. But it looks like it doesn't matter, because if you look at the "case gif_image_header" code, if we're doing a size decode we just bail and don't bother to jump to the "gif_done" case anyway.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Perhaps we want a !IsSizeDecode() assert in gif_done case as well?  It is the assumption right now, may as well protect from it changing in the future?

Comment 9

[Seth Fowler [:seth] [:s2h]]

Yeah, I think that sounds like a good idea. I'll update the patch shortly to add that assertion.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: Ensure we always call PostFrameStop before PostDecodeDone in nsGIFDecoder2.cpp.

Updated patch which includes the assertion suggested in comment #8.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=321a9b0c672e

Comment 12

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! If try looks good I'll push this in later today.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Looks good. Pushing to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e19dec4264f

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e19dec4264f