should all objc exceptions be fatal?

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: shaver, Assigned: Josh Aas)

Tracking

(Depends on: 2 bugs, {fixed1.9.0.12, fixed1.9.1, topcrash})

1.9.1 Branch
x86
Mac OS X
fixed1.9.0.12, fixed1.9.1, topcrash
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.12 +
wanted1.9.0.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
(Assignee)

Comment 1

9 years ago
I've been thinking about this for a while, I think we should make exceptions not be fatal by default. Patch coming up.
Assignee: nobody → joshmoz
(Assignee)

Comment 2

9 years ago
Created attachment 378451 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Attachment #378451 - Flags: review?(roc)
(Assignee)

Comment 3

9 years ago
The trunk patch applies and works just fine on the 1.9.1 branch.
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.
Attachment #378451 - Flags: review?(roc) → review+
(Assignee)

Comment 5

9 years ago
I agree on trunk, we might want to leave it alone on the 1.9.1 branch though.
(Assignee)

Updated

9 years ago
Attachment #378451 - Flags: superreview?(bzbarsky)
Comment on attachment 378451 [details] [diff] [review]
trunk fix v1.0

Please file a followup on the naming.
Attachment #378451 - Flags: superreview?(bzbarsky) → superreview+
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).
Any reason we can't backport this to 1.9.0 to improve stability?
Flags: wanted1.9.0.x?
I'd be in favour of that.
(Assignee)

Comment 10

9 years ago
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Attachment #378451 - Flags: approval1.9.1?
Attachment #378451 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 378451 [details] [diff] [review]
trunk fix v1.0

a191=beltzner
(Assignee)

Comment 12

9 years ago
Filed bug 494008 about renaming the macros.
(Assignee)

Comment 13

9 years ago
pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c28d6d000ea3
Keywords: fixed1.9.1
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.
Flags: blocking1.9.0.12?
(Assignee)

Comment 15

9 years ago
Lets give this a couple of days before going to 1.9.0.
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.
(Assignee)

Comment 17

9 years ago
Shouldn't get lost with the right flags set, I'll post a patch soon.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12+
Keywords: topcrash
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.
(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?
(Assignee)

Comment 20

9 years ago
Created attachment 383536 [details] [diff] [review]
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
(Assignee)

Updated

9 years ago
Keywords: fixed1.9.0
Keywords: fixed1.9.0 → fixed1.9.0.12

Updated

8 years ago
Depends on: 511700
Depends on: 564325

Updated

2 years ago
Depends on: 1258945
You need to log in before you can comment on or make changes to this bug.