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

crash stack from a non-debug build is different

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).

Give this patch a go?

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

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 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.

This blocks 563088, which is a blocker. Nominating.

(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 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!)

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.

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