Closed Bug 580669 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: ImageLib, defect)

x86
All
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Nomis101, Assigned: bholley)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files, 1 obsolete file)

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).
Attached patch patch v1 (obsolete) — Splinter Review
Give this patch a go?
Assignee: nobody → bobbyholley+bmo
Status: NEW → ASSIGNED
oh hey, what do you know - I just crashed on Firefox with this too.
Summary: Thunderbird crashes with enabled image discarding → Mac Menu icon crash with enabled image discarding
Blocks: 563088
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.
Attachment #459048 - Flags: review?(joe)
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.
Attachment #459048 - Flags: review?(joe) → review-
Keywords: crash
Summary: Mac Menu icon crash with enabled image discarding → Mac Menu icon crash with enabled image discarding [@ imgContainer::SyncDecode]
This blocks 563088, which is a blocker. Nominating.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Attached patch patch v2Splinter Review
(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.
Attachment #459048 - Attachment is obsolete: true
Attachment #460301 - Flags: review?(joe)
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!)
Attachment #460301 - Flags: review?(joe) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comparing the behaviour of a Thunderbird tinderbox build from today and yesterday, I can verify that this is now fixed.
Status: RESOLVED → VERIFIED
Depends on: 583205
Crash Signature: [@ imgContainer::SyncDecode]
You need to log in before you can comment on or make changes to this bug.