486574 - should all objc exceptions be fatal?

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Shaver (:shaver emeritus)]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090402 Shiretoko/3.5b4pre

http://crash-stats.mozilla.com/report/index/f18d1f5e-829d-4df4-949b-c35f72090402
http://crash-stats.mozilla.com/report/index/a4be67bd-4d55-4507-9022-4db722090402
http://crash-stats.mozilla.com/report/index/f85204d5-fc96-4981-9daf-7a3ee2090402

2 of these are in icon stream, one in widget, all with the exception

NSInternalInconsistencyException: Error (1000) creating CGSWindow

Are these all things that should be fatal?  Not getting the icon would probably be OK, given that it would let me open the download manager without crashing.  (I think the non-icon-stream ones is from crashing during restart after the first crash.)

Feels like we're a fair bit crashier on OS X these days, and often around the download manager and spawning helper apps (Preview.app is like the kiss of death for me some days) -- wonder if catching some of these exceptions and converting to an nsresult would be a better plan.

Comment 1

[Josh Aas]

I've been thinking about this for a while, I think we should make exceptions not be fatal by default. Patch coming up.

Comment 2

[Josh Aas]

Attachment: trunk fix v1.0

This makes our log and abort macros not actually abort. In optimized builds we output exception data to the console and append that data to crashreporter's application notes. In debug builds we also generate a stack trace.

You can still do an actual log and abort by calling "nsObjCExceptionLogAbort" from an exception handler.

I think for now we should not attempt to rename all of the macro usage in the tree. I made a note about this in the modified header, that macros claiming to abort do not actually abort. We can change those macros later on if we stick with this approach.

Comment 3

[Josh Aas]

The trunk patch applies and works just fine on the 1.9.1 branch.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 378451 [details] [diff] [review]
trunk fix v1.0

I think we need to change the macro names on trunk ASAP. It's really bad to have macros named ABORT that don't.

Comment 5

[Josh Aas]

I agree on trunk, we might want to leave it alone on the 1.9.1 branch though.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378451 [details] [diff] [review]
trunk fix v1.0

Please file a followup on the naming.

Comment 7

[Mike Shaver (:shaver emeritus)]

Can we at least fix the comments in the header?

 // For wrapping blocks of Obj-C calls. Terminates app after logging.

etc.  Seems like they should say

 // For wrapping blocks of Obj-C calls.  DOES NOT TERMINATE app after
 // logging, in spite of name (see bug XXXXXXX for renaming).

Comment 8

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

Any reason we can't backport this to 1.9.0 to improve stability?

Comment 9

[Mike Shaver (:shaver emeritus)]

I'd be in favour of that.

Comment 10

[Josh Aas]

pushed to mozilla-central with a comment fix, will file a bug on macro naming if it sticks

http://hg.mozilla.org/mozilla-central/rev/a071dfefae96

Comment 11

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

Comment on attachment 378451 [details] [diff] [review]
trunk fix v1.0

a191=beltzner

Comment 12

[Josh Aas]

Filed bug 494008 about renaming the macros.

Comment 13

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c28d6d000ea3

Comment 14

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

Josh, can you work up a 1.9.0 patch for this?

For my reference: Doing this will "fix" a bunch of the exception crashes we see across all branches. This has already landed on mozilla-central and 1.9.1 as of today.

Comment 15

[Josh Aas]

Lets give this a couple of days before going to 1.9.0.

Comment 16

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

Josh: The 1.9.0 tree isn't even open yet and won't open for a couple more days. We won't start approving patches until then and it won't need to land for a week or two. I'm simply asking for a patch for us to approve when the time is right, so this bug doesn't get forgotten or lost.

Comment 17

[Josh Aas]

Shouldn't get lost with the right flags set, I'll post a patch soon.

Comment 18

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

It would be really nice if all the swizzling that was added to not crash on certain exceptions could be removed everywhere as well, now that no exceptions will be fatal.

Comment 19

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

(In reply to comment #16)
> Josh: The 1.9.0 tree isn't even open yet and won't open for a couple more days.
> We won't start approving patches until then and it won't need to land for a
> week or two. I'm simply asking for a patch for us to approve when the time is
> right, so this bug doesn't get forgotten or lost.

(In reply to comment #17)
> Shouldn't get lost with the right flags set, I'll post a patch soon.

Josh: Today is code freeze. This bug has all the right flags and I've emailed requesting a patch. Can you please attach a 1.9.0 patch to this bug today?

Comment 20

[Josh Aas]

Attachment: 1.9.0 fix, v1.0

landed this on 1.9.0

Checking in xpcom/base/nsObjCExceptions.h;
/cvsroot/mozilla/xpcom/base/nsObjCExceptions.h,v  <--  nsObjCExceptions.h
new revision: 1.8; previous revision: 1.7