442245 - FF3 crashes on exotic Cmd+[non english alphabet] key combination [@ nsObjCExceptionLogAbort]

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P5)

Description

[miceuz]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008061004 Firefox/3.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008061004 Firefox/3.0

i have installed a custom keyboard layout, which places the letter 'ž' on the key 'w'. Firefox crashes every time i hit Cmd+ž to close the tab. This happens when i press the Cmd+key combination for other national letters too.

Reproducible: Always

Steps to Reproduce:
1.download http://megalogika.stp.lt/mic/Lithuanian-ISOIEC9995.keylayout
2.save it to your /Library/Keyboard layouts
3.add it to your keyboar layout switcher and switch to this keyboard
4.start firefox 3
5.hit Cmd+w
Actual Results:  
firefox crashed

Expected Results:  
tab should be closed

Comment 1

[miceuz]

Add-ons: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}:0.7.5.5,[e-mail]:1.2.0b3,{3d7eb24f-2740-49df-8937-200b1cc08f8a}:1.5.6,{a6fd85ed-e919-4a43-a5af-8da18bda539f}:1.0b2,{AE93811A-5C9A-4d34-8462-F7B864FC4696}:3.23,{c45c406e-ab73-11d8-be73-000a95be3b12}:1.1.6,{972ce4c6-7e08-4474-a285-3208198ce6fd}:3.0
BuildID: 2008061004
CrashTime: 1214573019
InstallTime: 1213773376
ProductName: Firefox
SecondsSinceLastCrash: 934
StartupTime: 1214572089
Theme: classic/1.0
URL: https://bugzilla.mozilla.org/post_bug.cgi
UserID: 935dd9e5-34cf-2f42-94ee-267f66dca57b
Vendor: Mozilla
Version: 3.0

This report also contains technical information about the state of the application when it crashed.

Comment 2

[miceuz]

Attachment: the problematic keyboard layout

this is the keyboard layout that causes the bug

Comment 3

[Matthias Versen [:Matti]]

Please type about:crashes as URL and post the Crash ID of this crash.

Comment 4

[Marcia Knous [:marcia]]

The link noted in your initial comment gives me an XML parsing error.

Comment 5

[miceuz]

the crash id is 5dfa5219-444c-11dd-9b18-001cc45a2c28

the file is an XML so your firefox tries to interpret it. i've attached the same keymap file to this ticket, no need to download it

Comment 6

[Mats Palmgren (inactive)]

nsObjCExceptionLogAbort	 mozilla/widget/src/cocoa/nsCocoaWindow.mm:138
-[NSWindow nsCocoaWindow_NSWindow_sendEvent:]	mozilla/widget/src/cocoa/nsCocoaWindow.mm:2201
-[ToolbarWindow sendEvent:]	mozilla/widget/src/cocoa/nsCocoaWindow.mm:1839
AppKit@0xdb430	
AppKit@0x38e26	
nsAppShell::Run	mozilla/widget/src/cocoa/nsAppShell.mm:588
nsAppStartup::Run	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:181
XRE_main	mozilla/toolkit/xre/nsAppRunner.cpp:3170
main	mozilla/browser/app/nsBrowserApp.cpp:158 

Comment 7

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

I can confirm this using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1pre) Gecko/2008062704 GranParadiso/3.0.1pre.

Console messages:

6/28/08 12:29:38 PM firefox-bin[7192] Mozilla has caught an Obj-C exception [NSInternalInconsistencyException: Failed to get CharCodes from EventRef (-9870)] 
6/28/08 12:29:38 PM [0x0-0x345345].org.mozilla.firefox 2008-06-28 12:29:38.003 firefox-bin[7192:10b] Mozilla has caught an Obj-C exception [NSInternalInconsistencyException: Failed to get CharCodes from EventRef (-9870)] 

Note: This doesn't happen in Firefox 2.

nsObjCExceptionLogAbort(NSException*) is one of our topcrashers and this appears to be a case that causes it. Requesting blocking.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stack of exception that triggers crash

Here's a stack of where the exception takes place that triggers this
crash.  As you can see, the exception happens in system code (not in
browser code).

Following the STR from comment #0 in Safari causes exactly the same
error message to appear in the Console ... but there's no crash.

Safari[18312:10b] Failed to get CharCodes from EventRef (-9870)

This is	a clear-cut case of our exception-handling code crashing on a
non-fatal error.  It's the first one that I'm aware of.  But this kind
of thing was anticipated from the beginning.

The question now is, what do we do about it? 

One way to resolve this particular problem is to stop wrapping the
call to super in nsCocoaWindow_NSWindow_sendEvent: in
NS_OBJC_..._TRY_ABORT_BLOCK macros.  Can anyone else think of another
way?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

This isn't a topcrasher.  It's just gotten mixed in the with grab-bag
of (otherwise unrelated) nsObjCExceptionLogAbort(NSException*)
crashes.

Comment 10

[timeless]

our Cocoa access is thread limited, right? so we could register with it a list of expected exceptions using another stack object and have the handler check against those before it kills the process....

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Provisional fix

> One way to resolve this particular problem is to stop wrapping the
> call to super in nsCocoaWindow_NSWindow_sendEvent: in
> NS_OBJC_..._TRY_ABORT_BLOCK macros.

This doesn't work as described (you just crash elsewhere, at a higher
level).  Instead you need to suspend aborting around the call to super
(i.e. log the exception but don't abort).  Here's a patch that does
this.

> our Cocoa access is thread limited, right?

Yes, Cocoa calls must only happen on the main thread.

> so we could register with it a list of expected exceptions using
> another stack object and have the handler check against those before
> it kills the process....

Or we could just hard-code a list of non-fatal exceptions.

But the possible exceptions aren't well documented, and it's
conceivable that a given exception should be fatal in one place but
not in another.

I think it's better to take the approach used in my patch.

The original purpose of aborting on Objective-C exceptions was to
prevent XPCOM code from getting into an unstable (and potentially
exploitable) state if Objective-C exception handlers cause an XPCOM
method to be exited without its cleanup code having been called.
Because it was too difficult to limit our use of these macros to XPCOM
code, the mission got expanded to include (in principle) any browser
code that might make system calls, and which therefore might trigger
an Objective-C exception.

But there shouldn't be any need for our ABORT macros in code that's
all system code, from top to bottom.  The call to super that my patch
wraps in LOGONLY macros is an example of this.  Yes,
nsCocoaWindow_NSWindow_sendEvent: exists in the browser.  But (by the
magic of method swizzling) it hooks a system call ([NSWindow
sendEvent:]).  And we need only worry about (and catch exceptions in)
code in nsCocoaWindow_NSWindow_sendEvent: that calls elsewhere in the
browser.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #11)

Note that my "provisional fix" patch (attachment 327986 [details] [diff] [review]) swallows all
Objective-C exceptions (after logging them) that take place "below"
the call to super in nsCocoaWindow_NSWindow_sendEvent:.  This happens
even with relatively serious exceptions like the "unrecognized
selector" exception.

But Safari does the same kind of thing (in fact it appears to swallow
_all_ Objective-C exceptions -- see bug 441880 comment #1).  And the
call to super in nsCocoaWindow_NSWindow_sendEvent: will still crash on
serious non-Objective-C exceptions -- like dereferencing a NULL or
invalid pointer, or one that points to a deleted object.  (You can
test this for yourself with my patch from bug 441880 comment #1, or by
adding exception-triggering code just before the call to super in
nsCocoaWindow_NSWindow_sendEvent:, but still inside the LOGONLY
macros.)

So my patch should be quite safe.

Nonethess, my patch isn't trivial, so I think it should bake on the
trunk for a while before it gets landed on the 1.9.0 branch.

Waiting a while on the 1.9.0 branch for this patch shouldn't be a
problem -- this bug's crash isn't a topcrasher, and in fact I suspect
it's quite rare.

Comment 13

[Josh Aas]

I think we should WONTFIX this, it isn't a problem with Gecko. Most likely it is a problem with that keyboard file the user added to his or her system, it could be an Apple bug too.

If we were going to fix this I'd probably go with Steven's approach, but I'd rather not eat all all exceptions coming out of sendEvent: like that. That could put us in a weird state for lots of problems that might cause exceptions. If we're going to start eating exceptions here we should do so as part of a general exception handling policy change.

Comment 14

[Stuart Morgan]

I strongly disagree with this approach, for the same reasons I gave in bug 453222. Given that it swizzles sendEvent: for every single window in the application, whether it even involves Gecko or not, it should do as little as humanly possible.

Comment 15

[Josh Aas]

Comment on attachment 327986 [details] [diff] [review]
Provisional fix

Given bug 453222, we should not crash anywhere in the swizzled method on an exception. Lets log and eat (and not crash) for the entire swizzled method. It is a little inconsistent but we mitigate the impact on embedding without unreasonably jeopardizing the protection that eating the exceptions gives us.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (eat exceptions everywhere) (hg)

As per Josh's request, here's a version of my patch that eats
Objective-C exceptions (avoids crashing) everywhere in
nsCocoaWindow_NSWindow_sendEvent: (our replacement for the "hooked"
[NSWindow sendEvent:]).

I still think my previous patch is better (more precisely targeted).
And I'm (basically) submitting this under protest.

But (as far as I know) this patch isn't particularly risky:  The only
places it calls browser code are in GetFocusedElement() and [ChildView
sendFocusEvent:].  1) It's unlikely an Objective-C exception will
occur there, and 2) it's unlikely that (if one does occur) taking an
early out from either method (or ones called from there) will have bad
consequences.

This is an hg patch (for current mozilla-central code).  A cvs patch
(for current 1.9.0-branch code) will follow shortly.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (eat exceptions everywhere) (cvs)

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central.

Should it bake there a while, or should we seek 1.9.0-branch approval right away?

Comment 19

[Josh Aas]

Comment on attachment 336720 [details] [diff] [review]
Fix rev1 (eat exceptions everywhere) (cvs)

I don't think there is any need for this to bake.

Comment 20

[Marcia Knous [:marcia]]

I tested this using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080905020415 Minefield/3.1b1pre following the STR in Comment 0 and I did not crash. It would be great if the reporter could verify using the nightly if possible just as a second sanity check.

Comment 21

[miceuz]

i've tried Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080908020603 Minefield/3.1b1pre with problematic keyboard layout and it's not crashing.

Comment 22

[Marcia Knous [:marcia]]

Based on Comment 21 I am marking this verified fixed on the trunk (it seems the bug never was resolved fixed). I also set the nom flag for 1.9.0.3 since it did not seem to be nominated for that release, although the patch is.

Comment 23

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

We really, really need an automated testsuite before taking patches related to swizzling (or, really, a lot of Cocoa widget patches) -- i.e., bug 453222 scares me. Is there any work being done to create a testsuite for Cocoa widgets for Firefox 3.1?

I'm very disinclined to take this on the stable branch. Fixing one crash might create others in code that's completely untested.

Comment 24

[Stuart Morgan]

(In reply to comment #23)
> I'm very disinclined to take this on the stable branch. Fixing one crash might
> create others in code that's completely untested.

While this isn't the approach I'd prefer (see bug 453222 comment 13), it does fix the crash portion of 453222, and is essentially zero risk as compared to the current code.

If you don't allow this patch or my alternative (of which mine has, unarguably, higher risk to Firefox), don't plan on ever being able to ship Camino 2.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

In reply to comment #23)

> Is there any work being done to create a testsuite for Cocoa widgets
> for Firefox 3.1?

Some work has been done -- for example it's now possible to use
nsIDOMWindowUtils.sendNativeKeyEvent() to synthesize Cocoa key events
(something I've used in my chrome test for bug 428405).  But other
work remains to be done (for example there's still no way to
synthesize native Cocoa mouse events).

But none of that work is relevant to this bug.

It's very easy to test this bug "by hand" -- just (temporarily) add
some code to nsCocoaWindow_NSWindow_sendEvent: that triggers an
Objective-C exception, and observe this no longer causes any crashes.

But (as far as I know) it's not currently possible to automate this
kind of test on any platform.  For that to be possible, you'd need a
way to automatically insert code at various points into the browser.
You might be able to use an extension or plugin for this purpose.  But
that's something for the medium- or long-term future -- it shouldn't
block this bug.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 336720 [details] [diff] [review]
Fix rev1 (eat exceptions everywhere) (cvs)

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Landed on the 1.9.0 branch:

Checking in widget/src/cocoa/nsCocoaWindow.mm;
/cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.mm,v  <--  nsCocoaWindow.mm
new revision: 1.149; previous revision: 1.148
done
Checking in xpcom/base/nsObjCExceptions.h;
/cvsroot/mozilla/xpcom/base/nsObjCExceptions.h,v  <--  nsObjCExceptions.h
new revision: 1.6; previous revision: 1.5
done

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Verified this for 1.9.0.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102304 GranParadiso/3.0.4pre. I've verified the crash in 1.9.0.3 as well.

Comment 29

[miceuz]

i have to reopen this bug as širešoū started crashing again after i upgraded it to 3.6

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2

i don't see any crash report generated in about:crashes

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Please don't reopen this without any further data a year and a half later.

If there is a problem, it would be a 1.9.2 issue based on your Build ID and reopening this opens it as a trunk issue.

Please give repro steps for the problem in detail.

Comment 31

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

(In reply to comment #30)
> Please give repro steps for the problem in detail.

In a new bug.