Closed
Bug 486574
Opened 15 years ago
Closed 15 years ago
should all objc exceptions be fatal?
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: shaver, Assigned: jaas)
References
(Depends on 1 open bug)
Details
(Keywords: fixed1.9.0.12, fixed1.9.1, topcrash)
Attachments
(2 files)
4.94 KB,
patch
|
roc
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
5.50 KB,
patch
|
Details | Diff | Splinter Review |
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.
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
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.
Attachment #378451 -
Flags: review?(roc)
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+
I agree on trunk, we might want to leave it alone on the 1.9.1 branch though.
Attachment #378451 -
Flags: superreview?(bzbarsky)
Comment 6•15 years ago
|
||
Comment on attachment 378451 [details] [diff] [review] trunk fix v1.0 Please file a followup on the naming.
Attachment #378451 -
Flags: superreview?(bzbarsky) → superreview+
Reporter | ||
Comment 7•15 years ago
|
||
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•15 years ago
|
||
Any reason we can't backport this to 1.9.0 to improve stability?
Flags: wanted1.9.0.x?
Reporter | ||
Comment 9•15 years ago
|
||
I'd be in favour of that.
Assignee | ||
Comment 10•15 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
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #378451 -
Flags: approval1.9.1?
Updated•15 years ago
|
Attachment #378451 -
Flags: approval1.9.1? → approval1.9.1+
Comment 11•15 years ago
|
||
Comment on attachment 378451 [details] [diff] [review] trunk fix v1.0 a191=beltzner
Assignee | ||
Comment 12•15 years ago
|
||
Filed bug 494008 about renaming the macros.
Assignee | ||
Comment 13•15 years ago
|
||
pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c28d6d000ea3
Keywords: fixed1.9.1
Comment 14•15 years ago
|
||
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•15 years ago
|
||
Lets give this a couple of days before going to 1.9.0.
Comment 16•15 years ago
|
||
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•15 years ago
|
||
Shouldn't get lost with the right flags set, I'll post a patch soon.
Updated•15 years ago
|
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.
Comment 19•15 years ago
|
||
(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•15 years ago
|
||
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
Keywords: fixed1.9.0
Updated•15 years ago
|
Keywords: fixed1.9.0 → fixed1.9.0.12
You need to log in
before you can comment on or make changes to this bug.
Description
•