Closed Bug 1249576 Opened 7 years ago Closed 7 years ago

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


(Core :: Graphics: ImageLib, defect)

Not set



Tracking Status
firefox44 --- wontfix
firefox45 + wontfix
firefox46 + fixed
firefox47 + fixed


(Reporter: cbook, Assigned: tnikkel)




(Keywords: assertion)


(1 file)

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

Steps to reproduce:
-> Load

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]
I can reproduce.
Assignee: nobody → milan
OS: Unspecified → All
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?
Depends on: 1191114
Flags: needinfo?(tnikkel)
Looks like according to spec ( ) 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.
Flags: needinfo?(tnikkel)
Attached patch patchSplinter Review
Assignee: milan → tnikkel
Attachment #8722822 - Flags: review?(edwin)
Comment on attachment 8722822 [details] [diff] [review]

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

LGTM. Tiny nit: "encouter" in checkin comment.
Attachment #8722822 - Flags: review?(edwin) → review+
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.
landed this on mozilla-inbound as and fixed also the commit message typo :) 

Sylvestre: n-i because of comment #6 for tracking
Flags: needinfo?(sledru)
If we want to uplift it, it is today or never!
Danke Tomcat for the ni!
Flags: needinfo?(tnikkel)
Flags: needinfo?(sledru)
Flags: needinfo?(milan)
Comment on attachment 8722822 [details] [diff] [review]

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.
Flags: needinfo?(tnikkel)
Flags: needinfo?(milan)
Attachment #8722822 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8722822 [details] [diff] [review]

Pokemon gifs are important, please uplift to aurora
Attachment #8722822 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.