580669 - Mac Menu icon crash with enabled image discarding [@ imgContainer::SyncDecode]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nomis101]

Attachment: crash stack from a debug build

Bug 563088 will re-enable image discarding in Core. Currently this will crash Thunderbird if you go to "File --> Attachments".

STR:
1. Start Thunderbird and set image.mem.discardable to true
2. Open a mail with one or more attachments
3. Go to "File --> Attachments"
--> Thunderbird will crash immediately

Normally you will see the file icon in front of "Attachments". With image.mem.discardable to true there is no file icon. It's the same (and same crash report) if you set image.mem.decodeondraw to true. 

I've debugged this with gdb:
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
mozalloc_abort (msg=0xbfffcb80 "###!!! ABORT: Yikes, forcing sync in reentrant call!: '!mInDecoder', file /Volumes/Developer/temp/src/mozilla/modules/libpr0n/src/imgContainer.cpp, line 2340") at /Volumes/Developer/temp/src/mozilla/memory/mozalloc/mozalloc_abort.cpp:85
85	    TouchBadMemory();


Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2pre) Gecko/20100721 Thunderbird/3.2a1pre

Comment 1

[Nomis101]

Attachment: crash stack from a non-debug build

crash stack from a non-debug build is different

Comment 2

[Bobby Holley (:bholley)]

This is simple enough. We probably shouldn't be requesting a sync decode here:

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsMenuItemIconX.mm#445

We're in OnStopFrame, so we're guaranteed to have the decoded frame in question, which makes the whole sync decode thing moot. All this does is try to flush the entire image, when maybe it's not all there yet (but this frame is).

Comment 3

[Bobby Holley (:bholley)]

Attachment: patch v1

Give this patch a go?

Comment 4

[Bobby Holley (:bholley)]

oh hey, what do you know - I just crashed on Firefox with this too.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 459048 [details] [diff] [review]
patch v1

flagging joe for review. I think something in the last nightly or two activated this code, because it doesn't crash on my 20100718 nightly.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 459048 [details] [diff] [review]
patch v1

Can you make it so we don't have that footgun any more too? If we ask for a sync decode and we already have the data, it should just be a no-op. The assertion isn't helping anybody, I think.

Comment 7

[Bobby Holley (:bholley)]

This blocks 563088, which is a blocker. Nominating.

Comment 8

[Bobby Holley (:bholley)]

Attachment: patch v2

(In reply to comment #6)
> Comment on attachment 459048 [details] [diff] [review]
> patch v1
> 
> Can you make it so we don't have that footgun any more too? If we ask for a
> sync decode and we already have the data, it should just be a no-op. The
> assertion isn't helping anybody, I think.

So I really don't think we should be making it easier for anyone to ask for a sync decode from within a decoder event. Even if we happen to have the frame we want sometimes, we won't always. Checking if we have the frame would also duplicate some code.

My proposed alternative solution is to make this requirement more explicit. I documented it in the IDL, and made each API method that takes the sync decode flag reject re-entrant calls of this form.

Reflagging for review.

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 460301 [details] [diff] [review]
patch v2

I'd like to see a test added here, even if it's as simple as making sure all the methods that can accept FLAG_SYNC_DECODE correctly error out if called from within any of the imgIDecoderObserver functions. (Steal some of my framework from bug 572520!)

Comment 10

[Bobby Holley (:bholley)]

It turns out that the interfaces we want to test were noscript, so joe decided on a followup bug. Filed bug 582054.

Pushed this as 09bd4d9ffd66. Resolving fixed.

Comment 11

[Nomis101]

Comparing the behaviour of a Thunderbird tinderbox build from today and yesterday, I can verify that this is now fixed.