396860 - Crash [@ -[NSApplication _handleKeyEquivalent:]] closing View Source window with Cmd+W

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. export MallocScribble=1
2. Run Firefox (Mac trunk debug)
3. Cmd+U
4. Cmd+W

Result: the View Source window closes, but the browser window never regains focus.  Instead, Firefox crashes while using a pointer from deleted memory:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x55555555

Thread 0 Crashed:
0   libobjc.A.dylib       	0x90a594b8 objc_msgSend + 8
1   com.apple.AppKit      	0x9342dfde -[NSApplication _handleKeyEquivalent:] + 58
2   com.apple.AppKit      	0x93361d87 -[NSApplication sendEvent:] + 3542
3   com.apple.AppKit      	0x9328cdfe -[NSApplication run] + 547
4   libwidget_mac.dylib   	0x05052fa0 nsAppShell::Run() + 130 (nsAppShell.mm:479)
5   libtoolkitcomps.dylib 	0x166f92d1 nsAppStartup::Run() + 147 (nsAppStartup.cpp:170)
6   XUL                   	0x0020f384 XRE_main + 10182 (nsAppRunner.cpp:3069)
7   org.mozilla.firefox   	0x00002798 main + 708 (nsBrowserApp.cpp:153)
8   org.mozilla.firefox   	0x00001dca _start + 216
9   org.mozilla.firefox   	0x00001cf1 start + 41

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I've reproduced this with an ordinary trunk nightly
(2007-09-20-04-trunk).  It seems that the key is turning on
MallocScribble (an OS-supported debugging facility that fills
deallocated memory with 0x5555...).

I can't reproduce it with the previous trunk nightly
(2007-09-19-04-trunk).  So yes, my appshell patch (bug 395397) does
seem to be involved.

But I suspect all my appshell patch has done is to uncover one or more
previously existing bugs.  I'll try to find it (or them).

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Bug 397039 may be related to this one -- both happen when a window is
closed.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Apparently my new appshell patch (bug 395397) made it possible for
[NSMenu performKeyEquivalent:] to prematurely close (and destroy) an
NSWindow object while processing a key-equivalent like Command+w or
Command+q.

This patch works around the problem by retaining the "current"
NSWindow before every call to [NSMenu performKeyEquivalent:] (in
[ChildView performKeyEquivalent:]) and releasing it afterwards.

On the face of it this seems like an Apple bug.

Camino makes a bunch of calls to [NSMenu performKeyEquivalent:] (in
addition to the one in [ChildView performKeyEquivalent:] that it also
uses).  Next week I'll check them to see if they work properly, and if
need be also repair them.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Something I forgot to mention:

This patch (attachment 281878 [details] [diff] [review]) doesn't fix bug 397039.  I'm working on
a separate patch for that bug.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk.

Comment 6

[Simon Fraser [no longer active]]

I see this in the latest nightly minefield, on Leopard.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

I've confirmed that this crash happens on OS X 10.5 (on the latest
build, 9A559) -- not just in the case described in comment #0, but
anytime a top-level window is closed with a key combination
(e.g. Command-W or Command-Q) (though not if a modal dialog is open,
like the "confirm close tabs" dialog).

Furthermore you no longer need to set MallocScribble -- so these
crashes will presumably happen a _lot_ on the Leopard build that Apple
is about to release (on October 26th).

So I think this bug should block the M9 beta.

I have a patch, which needs a bit more testing, but which I should be
able to release later today.

(By the way, these crashes don't happen in Camino.)

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix that works on both Tiger and Leopard

Here's a fix that works on both Tiger and Leopard.  It basically just
pushes the old fix one level higher in the call stack.

On both Tiger and Leopard, [NSView performKeyEquivalent:] is called
from [NSWindow performKeyEquivalent:].  But for some reason, [NSWindow
performKeyEquivalent:] on Leopard calls some _other_ NSView's
performKeyEquivalent: method (not [ChildView performKeyEquivalent:]).
So my previous fix, though it worked fine on Tiger, was never invoked
on Leopard.

By the way, I now realize that the underlying problem isn't an Apple
bug (contrary to what I said in comment #3).  Instead the problem is
that sending a command-w or command-q to Gecko causes the currently
focused widget (and its native window) to be destroyed before
[NSWindow performKeyEquivalent:] is finished with the native window.
(This is what my patch works around.)

Comment 9

[Damon Sicore (:damons)]

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

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.

Comment 10

[Reed Loden [:reed]]

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

Landed by smichaud on 2007-09-25 08:37.

Comment 11

[Damon Sicore (:damons)]

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

Reset approval flag to + as it was already checked in.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 285778 [details] [diff] [review]
Fix that works on both Tiger and Leopard

I'm seeking approval for the second, new patch (which changes the fix
to work on both Tiger and Leopard).

This problem is very bad on Leopard (which Apple is about to release)
-- much worse than it was on Tiger (before my first fix).  And the
risk is low (the new fix is just my previous fix, implemented slightly
differently).

I don't think there's any doubt this should block the M9 beta.  But
since I'm the one who set the target milestone to "mozilla1.9 M9", I'm
seeking 1.9 approval.

Comment 13

[Damon Sicore (:damons)]

Comment on attachment 285778 [details] [diff] [review]
Fix that works on both Tiger and Leopard

Please land ASAP for beta.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk.

Comment 16

[Marcia Knous [:marcia]]

verified fixed on Leopard Mac OS X 10.5 (9A559) using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre. I verified by trying various combinations of open and closing dialogs with Command-W, as well as trying the exact combination that caused me to crash consistently in Bug 400900. Will verify on Tiger as well.

Comment 17

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre