856616 - GIF Assertion failure: !IsSizeDecode() (Size decodes shouldn't reach gif_done) [@mozilla::image::nsGIFDecoder2::WriteInternal]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

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

Attachment: We're not covering the scenario where there are no images in the container.

Is there a point of doing PostDecode... if there was no image in the first place?

Comment 3

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

Before reviewing, see conversation in bug 857623

Comment 4

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

Comment on attachment 731919 [details] [diff] [review]
We're not covering the scenario where there are no images in the container.

Joe, can you confirm that PostDecodeDone should never be called for the size decode case, thus the assert?

Comment 5

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

The solution to this and bug 857623 is the same, the test cases are different and trigger different reasons for hitting it.

Comment 6

[Christoph Diehl [:posidron]]

The testcase still works after patch in bug 857623 landed.

Assertion failure: !IsSizeDecode() (Size decodes shouldn't reach gif_done), at /Users/cdiehl/dev/repos/mozilla/mozilla-inbound/image/decoders/nsGIFDecoder2.cpp:1038

Comment 7

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 8

[Christoph Diehl [:posidron]]

This is still reproducible with Nightly.

Comment 9

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

Right - we're still not covering the case where there are no images in the container.  Seth, the assertion we kept from bug 857623 is correct when there is an image to decode; if we go straight from global header/global colormap to GIF_TRAILER/gif_done, we hit this assertion. Now, that assertion is useful in the other scenarios, so I'd rather not just remove it.  Seth, Joe, opinions?

Comment 10

[Joe Drew (not getting mail)]

The easiest way is to just remove the assert. The more correct way is to PostSize() when we get the gif_global_header, and just return then.

Comment 11

[Joe Drew (not getting mail)]

My preference is to fix the code correctly, but I'll accept removing the assert (if we really want that fixed) and filing a follow-up bug to fix the code and restore the assert.

Comment 12

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

Attachment: PostSize in the global header, no need to continue when doing size decode

Is this the direction you had in mind?

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 774800 [details] [diff] [review]
PostSize in the global header, no need to continue when doing size decode

Yep!

Comment 14

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

Attachment: PostSize in the global header, no need to continue when doing size decode.

The code is the same as in the (obsolete) patch f+ by Joe; this one just adds the test.

Comment 15

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

Comment on attachment 807415 [details] [diff] [review]
PostSize in the global header, no need to continue when doing size decode.

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

Looks good.

Comment 16

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

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=424345dcad4e.  Will do a trivial rebase and attach the new patch.

Comment 17

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

Attachment: PostSize in the global header, no need to continue when doing size decode. r=seth

Carry Seth's r+, trivial rebase.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0cb62be1f04

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0cb62be1f04