518704 - "Invalid frame index!" fatal assertion from nsICODecoder::Close

Status: RESOLVED DUPLICATE of bug 522174

(Core :: Graphics: ImageLib, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Since at least Saturday morning (Sept. 19) I've been hitting this fatal assertion about once a day; it's usually related to me intentionally or accidentally opening the bookmarks menu or bookmarks sidebar:

#7  0x00007f9bca5357b7 in PR_Abort ()
    at /home/dbaron/builds/mozilla-central/mozilla/nsprpub/pr/src/io/prlog.c:548
#8  0x00007f9bcac2c949 in Abort (aMsg=0x344f <Address 0x344f out of bounds>)
    at /home/dbaron/builds/mozilla-central/mozilla/xpcom/base/nsDebugImpl.cpp:370
#9  0x00007f9bcac2cc00 in NS_DebugBreak_P (aSeverity=3,
    aStr=0x7f9bb601d621 "Invalid frame index!",
    aExpr=0x7f9bb601d604 "aFrameNum < mFrames.Length()",
    aFile=0x7f9bb601d740
"/home/dbaron/builds/mozilla-central/mozilla/modules/libpr0n/src/imgContainer.cpp"
, aLine=835)
    at /home/dbaron/builds/mozilla-central/mozilla/xpcom/base/nsDebugImpl.cpp:321
#10 0x00007f9bb5fcf665 in imgContainer::FrameUpdated (this=0x1b6947a0,
    aFrameNum=0, aUpdatedRect=@0x7fffd4161a90)
    at
/home/dbaron/builds/mozilla-central/mozilla/modules/libpr0n/src/imgContainer.cpp:8
35
#11 0x00007f9bb5ff8def in nsICODecoder::Close (this=0x175a8a80,
    aFlags=<value optimized out>)
    at
/home/dbaron/builds/mozilla-central/mozilla/modules/libpr0n/decoders/bmp/nsICODeco
der.cpp:117
#12 0x00007f9bb5fccab7 in imgContainer::ShutdownDecoder (this=0x1b6947a0,
    aIntent=imgContainer::eShutdownIntent_Done)
    at
/home/dbaron/builds/mozilla-central/mozilla/modules/libpr0n/src/imgContainer.cpp:2
115
#13 0x00007f9bb5fd04e9 in imgContainer::SourceDataComplete (this=0x1b6947a0)
    at
/home/dbaron/builds/mozilla-central/mozilla/modules/libpr0n/src/imgContainer.cpp:1
246
#14 0x00007f9bb5fe8c47 in imgRequest::OnStopRequest (this=0xdfdd2b0,
    aRequest=0x21230db0, ctxt=0x0, status=0)
    at 
/home/dbaron/builds/mozilla-central/mozilla/modules/libpr0n/src/imgRequest.cpp:905



aFrameNum is 0, but mFrames.Length() is also 0.


The image it was loading one of the times was favicon:http://ourairports.com/favicon.ico but I couldn't get the assertion again by reloading that image (the image looked fine).

This is making using a debug build as my primary browser pretty difficult.

Comment 1

[Bobby Holley (:bholley)]

I added the assert, but the check-and-return-failure was always there.

I don't understand how this is non-deterministic. In theory decoders shouldn't be sensitive to grouping of data blocks they get. We're shutting down with completion intent, meaning that we should have written all the data the decoder. Either that data ends up calling AppendFrame or it doesn't.

At the same time though, it does look like we have a bug in this decoder (or in the design in general) in that it's possible to put some bogus data in (only a few bytes, for example) that doesn't get far enough to append the frame. Since decoding complete gets called in the close function (something that's always bugged me about bmp/ico decoders), we don't do any actual checking of whether we got something useful before calling close with "success" intent.

Maybe something that could be fixed with bug 513681.

In the mean time, I'm a bit busy, so maybe joe could take a look at the non-determinism issue?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I changed this to a non-fatal assertion:
http://hg.mozilla.org/mozilla-central/rev/7a4a65c915b1
because leaving fatal assertions that fire regularly in the tree isn't acceptable.  Debug builds need to be usable or people won't use them.

Comment 3

[Bobby Holley (:bholley)]

(In reply to comment #2)
> I changed this to a non-fatal assertion:
> http://hg.mozilla.org/mozilla-central/rev/7a4a65c915b1
> because leaving fatal assertions that fire regularly in the tree isn't
> acceptable.  Debug builds need to be usable or people won't use them.

sounds good - sorry I didn't get to it earlier. I've been pretty swamped.

Comment 4

[Bobby Holley (:bholley)]

Marking this as a dupe of bug 522174 since that has a testcase.