Closed Bug 397381 Opened 17 years ago Closed 17 years ago

Enable @try/@catch/@finally for Cocoa widgets

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: cbarrett, Assigned: cbarrett)

Details

Attachments

(1 file, 2 obsolete files)

Now that we do not support 10.2, we can use the (much nicer) @try/@catch/@finally blocks to do exception handling, instead of the very delicate NS_DURING/NS_HANDLER/NS_ENDHANDLER.

We just need to add -fobjc-exceptions (or some such) to our compiler flags. I'm not sure what the optimal place to add that is, though.
Benjamin - ideas on where to put -fobjc-exceptions?
What code needs to have this flag? I'm guessing only the widget code? If so, just add CPPFLAGS += -fobjc-exceptions to the relevant makefiles, ifdefed if necessary.
Yeah, if it's needed in other modules for whatever reason (it could potentially be useful anywhere there is a .mm file), we can always just turn it on there as well.
Attached patch fix v1.0 (obsolete) — Splinter Review
Alright. I enabled this for widget/src/cocoa, and replaced all our uses of NS_HANDLER and friends with @try/@catch.

While I was there I took the opportunity to use warnings instead of assertions (a mistake on my part initally).

In nsClipboard.mm, I had to move the exception handling code below the calls to EqualsLiteral. Basically, it's necessary because of the way ObjC exceptions (setjmp/longmp based) interact with local variables (which need to be declared volatile in that case by the C ANSI standard). Thanks to dcamp for help with it.

Trying to clear off some of the lower hanging fruit from my bug list.
Attachment #283797 - Flags: review?(joshmoz)
Why nsPrintfCString? What is wrong with printf or NSLog?
I wanted it to be an NS_WARNING, because people (like Jesse) look for the syntax that NS_WARNING produces.

A random NSLog or printf doesn't give line/function/file information either, and putting that information in the NSLog is just as ugly looking as what I did already, IMO.
Comment on attachment 283797 [details] [diff] [review]
fix v1.0

> CXXFLAGS += \
>   -DUSE_COCOA \
>+	-fobjc-exceptions \

You're mixing two spaces vs. a tab. Whichever one is right use it consistently.
Attachment #283797 - Flags: review?(joshmoz) → review-
Attached patch fix v1.1 (obsolete) — Splinter Review
Use tabs, like the rest of the file.
Attachment #283797 - Attachment is obsolete: true
Attachment #284211 - Flags: review?(joshmoz)
Comment on attachment 284211 [details] [diff] [review]
fix v1.1

", make sure to catch any exceptions (timeouts)"

This part of the two comments is not necessary. I think it is pretty obvious that we're catching exceptions here. Also the handler will print out the cause so no need to copy the documentation on the method call to the comment. Fix on checkin.
Attachment #284211 - Flags: superreview?(roc)
Attachment #284211 - Flags: review?(joshmoz)
Attachment #284211 - Flags: review+
Attachment #284211 - Flags: superreview?(roc)
Attachment #284211 - Flags: superreview+
Attachment #284211 - Flags: approval1.9+
Checking in widget/src/cocoa/Makefile.in;
/cvsroot/mozilla/widget/src/cocoa/Makefile.in,v  <--  Makefile.in
new revision: 1.75; previous revision: 1.74
done
Checking in widget/src/cocoa/nsClipboard.mm;
/cvsroot/mozilla/widget/src/cocoa/nsClipboard.mm,v  <--  nsClipboard.mm
new revision: 1.19; previous revision: 1.18
done
Checking in widget/src/cocoa/nsNativeThemeCocoa.mm;
/cvsroot/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm,v  <--  nsNativeThemeCocoa.mm
new revision: 1.64; previous revision: 1.63
done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Checking in widget/src/cocoa/nsClipboard.mm;
/cvsroot/mozilla/widget/src/cocoa/nsClipboard.mm,v  <--  nsClipboard.mm
new revision: 1.20; previous revision: 1.19
done

^
this fixed bustage.
I backed this out since it was still busted and Colin wasn't responding to hails.  There was further bustage after the first bustage fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Colin - please post a new patch and request review again before landing.
So I'm still unable to reproduce those build errors on my machine.
Attached patch fix v2.0Splinter Review
I've separated the @try calls into separate functions, since AFAICT only variables local to that function get tagged with volatile after @try/@catch (setjmp/longjmp).

Can't run on try server, it seems to be down :(
Attachment #284211 - Attachment is obsolete: true
Attachment #284835 - Flags: review?(joshmoz)
Attachment #284835 - Flags: superreview?(roc)
Attachment #284835 - Flags: review?(joshmoz)
Attachment #284835 - Flags: review+
Attachment #284835 - Flags: superreview?(roc)
Attachment #284835 - Flags: superreview+
Attachment #284835 - Flags: approval1.9+
Just a reminder to please commit this by Monday if you want to get it in before beta. Otherwise, approval1.9+ will be revoked, and you will need to re-request it after M9 if you still want to land the patch. If you would like somebody else to commit this for you, please add the "checkin-needed" keyword.
I'll just wait until after beta on this one.
Whiteboard: [checkin-needed][after beta1]
Reed, go ahead and land this since you're all ready to.
Whiteboard: [checkin-needed][after beta1] → [checkin-needed]
Checking in widget/src/cocoa/Makefile.in;
/cvsroot/mozilla/widget/src/cocoa/Makefile.in,v  <--  Makefile.in
new revision: 1.78; previous revision: 1.77
done
Checking in widget/src/cocoa/nsClipboard.mm;
/cvsroot/mozilla/widget/src/cocoa/nsClipboard.mm,v  <--  nsClipboard.mm
new revision: 1.25; previous revision: 1.24
done
Checking in widget/src/cocoa/nsNativeThemeCocoa.mm;
/cvsroot/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm,v  <--  nsNativeThemeCocoa.mm
new revision: 1.66; previous revision: 1.65
done
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → mozilla1.9 M9
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: