1249576 - Assertion failure: IntRect(IntPoint(), GetSize()).IsEqualEdges(aFrameRect), at nsPNGDecoder.cpp:138

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Carsten Book [:Tomcat]]

Found via bughunter and reproduced on a win 7 trunk debug build based on todays m-c tip

Steps to reproduce:
-> Load http://www.pokepedia.fr/N
--> 

Assertion failure: IntRect(IntPoint(), GetSize()).IsEqualEdges(aFrameRect), at c:/Users/mozilla/debug-builds/mozilla-central/image/decoders/nsPNGDecoder.cpp:138

#01: mozilla::image::nsPNGDecoder::CreateFrame[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d756e3]
#02: mozilla::image::nsPNGDecoder::frame_info_callback[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d83ad3]
#03: MOZ_PNG_push_read_chunk[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x5580f0b]
#04: MOZ_PNG_proc_some_data[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x557f7eb]
#05: MOZ_PNG_process_data[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x557f86e]
#06: mozilla::image::nsPNGDecoder::WriteInternal[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d8275f]
#07: mozilla::image::Decoder::Write[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d495f9]
#08: mozilla::image::Decoder::Decode[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d3c0b8]
#09: mozilla::image::DecodePool::Decode[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d3bdf4]
#10: mozilla::image::DecodePoolWorker::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d4776c]
#11: nsThread::ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x5c32f7]
#12: NS_ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x62af52]
#13: mozilla::ipc::MessagePumpForNonMainThreads::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xc02e20]
#14: MessageLoop::RunInternal[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xb8939d]
#15: MessageLoop::RunHandler[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xb89312]
#16: MessageLoop::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xb88eed]
#17: nsThread::ThreadFunc[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x5cdedf]
#18: _PR_NativeRunThread[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2fe4cb]
#19: _PR_MD_YIELD[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2e3889]
#20: _get_flsindex[C:\Windows\system32\MSVCR120.dll +0x2c01d]
#21: _get_flsindex[C:\Windows\system32\MSVCR120.dll +0x2c001]
#22: BaseThreadInitThunk[C:\Windows\system32\kernel32.dll +0x4ef1c]
#23: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x63b53]
#24: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x63b26]

Comment 1

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

I can reproduce.

Comment 2

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

Part 1 of bug 1191114 changed the way we handle padding on the first frame of PNGs from tagging the image as transparent, to asserting (and not tagging it as transparent.)  Timothy, what's your call on this?

Comment 3

[Timothy Nikkel (:tnikkel)]

Looks like according to spec ( https://wiki.mozilla.org/APNG_Specification#.60fcTL.60:_The_Frame_Control_Chunk ) the old code was right. If the default image (the one displayed by non-APNG aware PNG decoders) is part of the animation then the fcTL chunk that precedes the IDAT chunk must have matching offset/size as the IHDR. However if the default image is not part of the animation then the first animation frame does not have this restriction. And this is the case we have here in this bug.

However, fixing this isn't as easy as just reverting to the old code. We also need to make sure we mark the image transparent when the first animation frame doesn't cover the whole image area.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8722822 [details] [diff] [review]
patch

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

LGTM. Tiny nit: "encouter" in checkin comment.

Comment 6

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

Nice!  Let's ask for uplifts here, at least to Aurora, but even beta - this is a safe patch.  The regression is from 43, so the sooner we can fix it, the better.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02a820dcf5ee

Comment 8

[Carsten Book [:Tomcat]]

landed this on mozilla-inbound as  https://hg.mozilla.org/integration/mozilla-inbound/rev/02a820dcf5ee and fixed also the commit message typo :) 

Sylvestre: n-i because of comment #6 for tracking

Comment 9

[Sylvestre Ledru [:Sylvestre]]

If we want to uplift it, it is today or never!
Danke Tomcat for the ni!

Comment 10

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

Comment on attachment 8722822 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1191114
[User impact if declined]: Some animated PNGs don't work properly.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: This is basically backing out part of the change that broke it.
[String/UUID change made/needed]:

Going to Aurora should be good enough.  It is a bit of a special case scenario, so it isn't all the animated PNGs that are affected.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02a820dcf5ee

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking affected, regression is from 43

Comment 13

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8722822 [details] [diff] [review]
patch

Pokemon gifs are important, please uplift to aurora

Comment 14

[Pulsebot]

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

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Pulsebot from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a03ba8b50b93

landed crashtest

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a41f3b09e121

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Just like Milan said in comment #10