461381 - [10.4] Crash [@ nsObjCExceptionLogAbort][@ -[GeckoNSMenu performKeyEquivalent:]] if I attempt to execute the Window > Zoom command with a custom Mac OS X keyboard shortcut

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Johnny Arthur]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3

Mozilla crashes when you try to access the Window > Zoom function with a Global Mac OS X shortcut because the shortcut gets assigned to View > Zoom (Zoom in, Zoom Out, Reset, Zoom text only)

Reproducible: Always

Steps to Reproduce:
1.Create a Global shortcut in Mac OS X for the Zoom command in All Applications (Keyboard & Mouse > Keyboard shortcuts 
2. Restart the computer to allow the shortcut to take effect
3. Attempt to use the shortcut in Firefox
Actual Results:  
Firefox crashes

Expected Results:  
Zoomed to full screen

Comment 1

[Steven]

I have been hit with this bug as well.

I see the problem being that firefox has two zoom commands. one under view and one under window. 

mac maps to the wording of the menu command.

one hopes it would map to window > zoom but instead it maps to view > zoom.

will crash firefox everytime.

Comment 2

[Steven]

forgot to mention in my previous comment. Perhaps naming the window > zoom command "maximize" could fix the problem.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

auenfx2 heard about this bug in #firefox and ran through a bunch of tests with me on #camino, and it seems like this is ultimately an OS bug on 10.4 where the OS will assign a shortcut to a submenu (on 10.5, the OS steadfastly refuses to assign shortcuts to submenus, no matter how hard we try) and then understandably throw an exception when the shortcut is pressed.

On 10.5 bug 429824 prevents anyone from using a custom keyboard shortcut that's been assigned to Firefox (the menu flashes, but the command is not executed), but this OS bug doesn't exist on 10.5 (see the Camino/Safari tests below if you want to verify yourself).

Since Firefox has the misfortune to have a submenu that has the same name as a standard Mac OS X menu item (that has no shortcut), and since Edit:Zoom submenu appears in the menus before Window:Zoom command and the first instance of a menu "item" wins the shortcut, Firefox seems much more likely to trigger the OS bug->exception->nsObjCExceptionLogAbort crash. 

[6:40pm] auenfx2: <kithpom> auenfx2: here http://crash-stats.mozilla.com/report/index/fd83145e-1da3-4ca3-b122-b9f572081201

[8:58pm] auenfx2: "Mozilla has caught an Obj-C exception"
[8:58pm] auenfx2: 2008-12-02 10:29:21.020 firefox-bin[13222] *** -[GeckoNSMenu submenuAction:]: selector not recognized [self = 0x8ff7e90]
[8:58pm] auenfx2: 2008-12-02 10:29:21.116 firefox-bin[13222] Mozilla has caught an Obj-C exception [NSInvalidArgumentException: *** -[GeckoNSMenu submenuAction:]: selector not recognized [self = 0x8ff7e90]]
[8:59pm] auenfx2: heh, selector not recognised
[9:00pm] auenfx2: http://sc.nitroware.net/i/fxosxzoom.jpg

[9:56pm] auenfx2: http://sc.nitroware.net/i/cmosxmenu.jpg
[10:00pm] auenfx2: http://sc.nitroware.net/i/sfosxmenu.jpg

[10:00pm] auenfx2: doesnt crash safari
[10:00pm] auenfx2: havent tried to crash camino yet
[10:01pm] sauron: the crash is probably Gecko's fault
[10:01pm] auenfx2: doesnt crash camino
[10:01pm] sauron: do you see an exception in Safari?
[10:01pm] sauron: er, in the console with Safari
[10:01pm] auenfx2: in console.log?
[10:01pm] sauron: yeah
[10:01pm] auenfx2: 2008-12-02 14:00:38.521 Safari[13362] *** -[NSMenu submenuAction:]: selector not recognized [self = 0x4302c0]
[10:01pm] auenfx2: 2008-12-02 14:00:38.604 Safari[13362] *** -[NSMenu submenuAction:]: selector not recognized [self = 0x4302c0]
[10:01pm] auenfx2: 2008-12-02 14:01:02.374 Camino[13355] *** -[NSMenu submenuAction:]: selector not recognized [self = 0x1d306f0]
[10:01pm] auenfx2: 2008-12-02 14:01:02.458 Camino[13355] *** -[NSMenu submenuAction:]: selector not recognized [self = 0x1d306f0]

So Camino and Safari are eating the exception harmlessly, while the nsObjCException crash-trigger crashes Firefox.

Though I haven't experienced this crash myself, we have three different people who have, there's a breakpad report and the exception info from console.log, and auenfx2 can reliably be found on #camino and elsewhere on moznet and can crash at will using the STR in comment 0, so I'm confirming the bug.

Can we safely eat this exception?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #3)

> it seems like this is ultimately an OS bug on 10.4 where the OS will
> assign a shortcut to a submenu (on 10.5, the OS steadfastly refuses
> to assign shortcuts to submenus, no matter how hard we try)

I've confirmed this on 10.4 (and that it doesn't happen on 10.5).

To make the problem happen in Safari, you need to set the global
shortcut's "Menu Title" to something else than "Zoom" (since Safari
doesn't have a "Zoom" submenu).  "Services" will do ... in fact for
any app on 10.4 (since they all have a "Services" submenu).

> So Camino and Safari are eating the exception harmlessly, while the
> nsObjCException crash-trigger crashes Firefox.

> Can we safely eat this exception?

I think so.  In my next comment I'll post a patch that does this.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a patch that stops Firefox from crashing when this bug is
triggered (on OS X 10.4).  It does this by hooking the [NSMenu
performKeyEquivalent:] "system call", and eating (and logging) all
Objective-C exceptions that happen there.  This is perfectly safe,
since we never call XPCOM code from our "hooked" method
(nsMenuX_NSMenu_performKeyEquivalent:).

A tryserver build will follow in a while (perhaps a long while).

Comment 6

[Steven Michaud [:smichaud] (Retired)]

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

This patch uses the same strategy as my patch for bug 442245.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

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

Here's a tryserver build made with my patch:

https://build.mozilla.org/tryserver-builds/2008-12-03_09:[e-mail]-bugzilla461381/[e-mail]

Comment 8

[Steven]

Any possibility of not only fixing the crash but changing the naming of the menu commands so that it would be possible to assign a keyboard shortcut to maximizing the window?

I know that this was working for me previously. I don't know if there was a change in one of the firefox updates that then made it start crashing.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #8)

> Any possibility of not only fixing the crash but changing the naming
> of the menu commands so that it would be possible to assign a
> keyboard shortcut to maximizing the window?

I'd be against it.

This is (ultimately) an Apple bug, and very seldom encountered.
Furthermore it's one that Apple's fixed in their latest OS (10.5.5).

Finally, we'd also need to fix bug 429824 ... which *isn't* going to
be easy, and therefore (probably) won't happen soon.  (As far as I
know, bug 429824 effects both 10.5 and 10.4.)

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central (changeset 1ea7f41ebd97).

This should (I think) eventually be landed on the 1.9.1 and 1.9.0 branches.  But it probably isn't a blocker.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

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

This problem isn't frequently encountered.  But the fix is trivial,
and almost no risk.  Furthermore it's now been baking on the trunk for
about a week.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Landed on the 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f8d8322e2150

Comment 14

[Samuel Sidler (old account; do not CC)]

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

This needs to bake a bit longer on 1.9.1 before we'll consider it for 1.9.0. Pushing out the nomination.

Comment 15

[Daniel Veditz [:dveditz]]

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

Low risk is not no risk, let's leave this as a 1.9.1 improvement unless we know for sure this is a common complaint.