1290747 - Assertion failure: mAnimationState, at RasterImage.cpp:1643

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Carsten Book [:Tomcat]]

Attachment: complete stack

Found by bughunter during topsite-testruns on 
http://www.zing.vn
http://persianblog.ir/

while loading this sites with a current windows debug tindebox build. Maybe a regression from bug 1289957

Assertion failure: mAnimationState, at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/image/RasterImage.cpp:1643
#01: mozilla_dump_image[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0xd4ed87]
#02: imgLoader::SupportImageWithMimeType[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0xd6dbf9]
#03: XRE_AddStaticComponent[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x20a694]
C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll + 0x232046
#05: mozilla::net::LoadInfo::TriggeringPrincipal[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x4ff7b9]
#06: mozilla::net::LoadInfo::TriggeringPrincipal[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x489e8c]
#07: UnregisterWeakMemoryReporter[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x1d07c1]
#08: UnregisterWeakMemoryReporter[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x1d08ea]
#09: XRE_TermEmbedding[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x25e2ac9]
#10: XRE_GetFileFromPath[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x25d7dbc]
#11: XRE_GetFileFromPath[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x25d8190]
#12: XRE_GetFileFromPath[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x25dd432]
#13: XRE_main[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\xul.dll +0x25dff3b]
#14: ???[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\firefox.exe +0x1c3b]
#15: ???[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\firefox.exe +0x15f4]
#16: ???[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\firefox.exe +0x201e]
#17: TargetNtUnmapViewOfSection[C:\cygwin\mozilla\builds\nightly\mozilla\firefox-debug\dist\bin\firefox.exe +0x319e5]
#18: BaseThreadInitThunk[C:\Windows\system32\kernel32.dll +0x4ef1c]
C:\Windows\SYSTEM32\ntdll.dll + 0x54608
#20: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x545db]

Comment 1

[Carsten Book [:Tomcat]]

edwin, since you reviewed bug 1289957 could you take a look if this is related to this change ?

Comment 2

[Carsten Book [:Tomcat]]

the tested debug build was based btw on https://hg.mozilla.org/mozilla-central/rev/e5859dfe0bcbd40f4e33f4a633f73ea3473a7849 (just for reference)

Comment 3

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

Looks like you actually want thread 0's stack:

Thread 0 (crashed)
 0  xul.dll!mozilla::image::RasterImage::NotifyProgress(unsigned int,mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const &,mozilla::Maybe<unsigned int> const &,mozilla::image::SurfaceFlags) [RasterImage.cpp:e5859dfe0bcb : 1630 + 0x2b]
    eip = 0x5aea028d   esp = 0x0021f3f8   ebp = 0x0021f428   ebx = 0x00000001
    esi = 0x0000066b   edi = 0x0021f4a4   eax = 0x5d889aa8   ecx = 0x6bb2705d
    edx = 0x77976074   efl = 0x00200202
    Found by: given as instruction pointer in context
 1  xul.dll!mozilla::image::RasterImage::FinalizeDecoder(mozilla::image::Decoder *) [RasterImage.cpp:e5859dfe0bcb : 1696 + 0x4b]
    eip = 0x5ae9ed87   esp = 0x0021f430   ebp = 0x0021f4b8
    Found by: call frame info
 2  xul.dll!mozilla::detail::RunnableFunction<<lambda_1760969938a3f65bda425f7b8ef1273d> >::Run() [nsThreadUtils.h:e5859dfe0bcb : 278 + 0x1b]
    eip = 0x5aebdbf9   esp = 0x0021f4c0   ebp = 0x0021f4c4
    Found by: call frame info
 3  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:e5859dfe0bcb : 1068 + 0xe]
    eip = 0x5a35a694   esp = 0x0021f4cc   ebp = 0x0021f538
    Found by: call frame info
 4  xul.dll!NS_ProcessNextEvent(nsIThread *,bool) [nsThreadUtils.cpp:e5859dfe0bcb : 290 + 0xd]
    eip = 0x5a382046   esp = 0x0021f540   ebp = 0x0021f54c
    Found by: call frame info
 5  xul.dll!mozilla::net::nsHttpConnectionMgr::Shutdown() [nsHttpConnectionMgr.cpp:e5859dfe0bcb : 191 + 0xd]
    eip = 0x5a64f7b9   esp = 0x0021f554   ebp = 0x0021f578
    Found by: call frame info
 6  xul.dll!mozilla::net::nsHttpHandler::Observe(nsISupports *,char const *,char16_t const *) [nsHttpHandler.cpp:e5859dfe0bcb : 2135 + 0x8]
    eip = 0x5a5d9e8c   esp = 0x0021f580   ebp = 0x0021f69c
    Found by: call frame info
 7  xul.dll!nsObserverList::NotifyObservers(nsISupports *,char const *,char16_t const *) [nsObserverList.cpp:e5859dfe0bcb : 112 + 0x1a]
    eip = 0x5a3207c1   esp = 0x0021f6a4   ebp = 0x0021f6bc
    Found by: call frame info
 8  xul.dll!nsObserverService::NotifyObservers(nsISupports *,char const *,char16_t const *) [nsObserverService.cpp:e5859dfe0bcb : 305 + 0xe]
    eip = 0x5a3208ea   esp = 0x0021f6c4   ebp = 0x0021f6d8
    Found by: call frame info
 9  xul.dll!nsXREDirProvider::DoShutdown() [nsXREDirProvider.cpp:e5859dfe0bcb : 1216 + 0x1a]
    eip = 0x5c732ac9   esp = 0x0021f6e0   ebp = 0x0021f70c
    Found by: call frame info
10  xul.dll!ScopedXPCOMStartup::~ScopedXPCOMStartup() [nsAppRunner.cpp:e5859dfe0bcb : 1335 + 0xb]
    eip = 0x5c727dbc   esp = 0x0021f714   ebp = 0x0021f720
    Found by: call frame info
11  xul.dll!mozilla::DefaultDelete<ScopedXPCOMStartup>::operator()(ScopedXPCOMStartup *) [UniquePtr.h:e5859dfe0bcb : 528 + 0xe]
    eip = 0x5c728190   esp = 0x0021f728   ebp = 0x0021f728
    Found by: call frame info
12  xul.dll!XREMain::XRE_main(int,char * * const,nsXREAppData const *) [nsAppRunner.cpp:e5859dfe0bcb : 4411 + 0x12]
    eip = 0x5c72d432   esp = 0x0021f730   ebp = 0x0021f75c
    Found by: call frame info
13  xul.dll!XRE_main [nsAppRunner.cpp:e5859dfe0bcb : 4474 + 0x10]
    eip = 0x5c72ff3b   esp = 0x0021f764   ebp = 0x0021f87c
    Found by: call frame info
14  firefox.exe!do_main [nsBrowserApp.cpp:e5859dfe0bcb : 247 + 0x16]
    eip = 0x01271c3b   esp = 0x0021f884   ebp = 0x0021fa18
    Found by: call frame info
15  firefox.exe!NS_internal_main(int,char * *,char * *) [nsBrowserApp.cpp:e5859dfe0bcb : 380 + 0xf]
    eip = 0x012715f4   esp = 0x0021fa20   ebp = 0x0021fa74
    Found by: call frame info
16  firefox.exe!wmain [nsWindowsWMain.cpp:e5859dfe0bcb : 115 + 0xf]
    eip = 0x0127201e   esp = 0x0021fa7c   ebp = 0x0021fab4
    Found by: call frame info
17  firefox.exe!__scrt_common_main_seh [exe_common.inl : 255 + 0x1d]
    eip = 0x012a19e5   esp = 0x0021fabc   ebp = 0x0021fb00
    Found by: call frame info
18  kernel32.dll + 0x4ef1c
    eip = 0x770def1c   esp = 0x0021fb08   ebp = 0x0021fb0c
    Found by: call frame info
19  ntdll.dll + 0x54608
    eip = 0x77994608   esp = 0x0021fb14   ebp = 0x0021fb4c
    Found by: previous frame's frame pointer
20  ntdll.dll + 0x545db
    eip = 0x779945db   esp = 0x0021fb54   ebp = 0x0021fb64
    Found by: previous frame's frame pointer

Comment 4

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

This is the assertion:

https://dxr.mozilla.org/mozilla-central/source/image/RasterImage.cpp?q=RasterImage%3A%3ANotifyProgress&redirect_type=single#1643

The cause is that a decoder allocated a second frame without calling PostIsAnimated(). This is a little surprising given this other assertion here, which didn't fire:

https://dxr.mozilla.org/mozilla-central/source/image/Decoder.cpp#276

That *should've* detected the problem as soon as the frame was allocated. The issue, then, is almost certainly that that code *wasn't executed*, because this check failed:

https://dxr.mozilla.org/mozilla-central/source/image/Decoder.cpp#274

I have no idea what the point of that check even is. I walked back through the blame to the version of the file at git revision d8cd01802 (hg 7cef16ef5ac49b176d122632bb0d0d09dbdeaf21) and it appears to me that this is an artifact from an ancient version of this code where the only way to tell if we successfully allocated a new frame was to check the frame out. Obviously we can now directly check whether |mCurrentFrame| contains a frame, and the outer |if| in this block of code *does* check that.

The first thing to do is rip out this pointless check, which should then give us a stack at the location where the error is actually introduced.

Comment 5

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

s/check the frame out/check the frame count/, above.

Comment 6

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

Attachment: Fix bad assert in RasterImage::NotifyProgress().

Heh, actually once I was able to reproduce the problem (couldn't get it to
happen with e10s on), the actual issue turned out to be much simpler. If we hit
an error, in RasterImage::DoError() we reset |mAnimationState| to |Nothing()|.
If this happened before the decoder for an animated image finished running, we
could wrongly trip this assert. (Or rather, the assert was incorrect for this
case.)

This patch fixes the assert.

That code I pointed out in comment 4 is still super sketchy, though. I'm going
to post a followup to fix that.

Comment 7

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

(In reply to Seth Fowler [:seth] [:s2h] from comment #6)
> That code I pointed out in comment 4 is still super sketchy, though. I'm
> going
> to post a followup to fix that.

That's bug 1290759; patch up.

Comment 8

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

Bughunter topsiterun found

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d45b49f61d5
Fix bad assert in RasterImage::NotifyProgress(). r=edwin

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d45b49f61d5

Comment 11

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

Comment on attachment 8776413 [details] [diff] [review]
Fix bad assert in RasterImage::NotifyProgress().

Approval Request Comment
[Feature/regressing bug #]: Bug 1289957
[User impact if declined]: None, but uplifting this will probably help our developers and testers.
[Describe test coverage new/current, TreeHerder]: None alas; it's kinda hard to write a test for this right now. That's something I'm looking into.
[Risks and why]: Zero. All this does is change a debug-only assert that was asserting the wrong thing.
[String/UUID change made/needed]: None.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Regression, tracked for 50.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776413 [details] [diff] [review]
Fix bad assert in RasterImage::NotifyProgress().

Low risk, taking it. Aurora50+

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/587904a10d9a

Comment 15

[TOny Duy]

anyone check my website errors via san go cong nghiep : https://sangocongnghiepcaocap.com/
Thanh you