397039 - Crash downloading file

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Dave Townsend [:mossop]]

From a bugzilla bug, right click on an attachment, do save link as..., file picker appears, enter a filename and hit ok, Minefield crashes almost every time.

http://crash-stats.mozilla.com/report/index/617939ee-6822-11dc-9962-001a4bd43ed6

I believe that this is a regression from bug 395397

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007092004 Minefield/3.0a8pre ID:2007092004

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I'm able to reproduce this.

It may be related to bug 396860 -- both happen when a window is closed.

Both are probably old bugs that have been uncovered by my bug 395397
patch.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a fix for this bug (397039).  Turns out it's not related to bug
396860 ... and it's _really_ wierd.

The crash is caused by [NSApp sendEvent:] trying to process one of the
fake events used by ProcessGeckoEvents() (in nsAppShell.mm) to wake up
the event loop in ProcessNextNativeEvent() (also in nsAppShell.mm).
But the '-1' that the event's 'windowNumber' parameter was originally
set to has been changed into the number of an actual NSWindow object,
and (by chance) that NSWindow object has just been destroyed!

Setting the windowNumber parameter of a fake event to '-1' is a bit
... eccentric.  In the Java Embedding Plugin I've had good luck
setting it to '0'.  I tried that here ... and the crashes went away!

Comment 3

[Josh Aas]

Comment on attachment 281891 [details] [diff] [review]
Fix

Looks good, how did you figure this out? Just curious about how you tracked this down.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> how did you figure this out?

I have a debugging build that (among other things) logs all calls to
[NSApp sendEvent:], and I'm used to seeing ProcessGeckoEvent()'s fake
events in those logs.  So the first part was pretty easy (recognizing
that [NSApp sendEvent:] was choking on a fake event sent to an
NSWindow object that had just been destroyed).

The second part took a little head-scratching (realizing that it might
be wrong to set the fake event's windowNumber to '-1').  But here my
Java Embedding Plugin experience helped.  (In the JEP I use fake
events for exactly the same reason -- to wake up an event loop.  And
I've always set their windowNumber to '0' ... which has always worked
fine.)

Comment 5

[Steven Michaud [:smichaud] (Retired)]

I should add that (as best I can tell) setting a fake event's
windowNumber to '0' prevents the OS from ever trying to associate it
with a particular NSWindow object (or trying to send it there).

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk.

Comment 7

[Dave Townsend [:mossop]]

Looks good

Comment 8

[Simon Fraser [no longer active]]

Does CFRunLoopWakeUp() not work to wake up the run loop, rather than posting bogus events?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> Does CFRunLoopWakeUp() not work to wake up the run loop, rather than
> posting bogus events?

Not in this case.  [NSApplication nextEventMatchingMask:...] has been
told not to return until it receives an event.