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

RESOLVED FIXED in mozilla1.9beta1

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: cbarrett, Assigned: cbarrett)

Tracking

Trunk
mozilla1.9beta1
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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.

Comment 1

11 years ago
Benjamin - ideas on where to put -fobjc-exceptions?

Comment 2

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

Comment 3

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

Comment 4

11 years ago
Created attachment 283797 [details] [diff] [review]
fix v1.0

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)

Comment 5

11 years ago
Why nsPrintfCString? What is wrong with printf or NSLog?
(Assignee)

Comment 6

11 years ago
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 7

11 years ago
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-
(Assignee)

Comment 8

11 years ago
Created attachment 284211 [details] [diff] [review]
fix v1.1

Use tabs, like the rest of the file.
Attachment #283797 - Attachment is obsolete: true
Attachment #284211 - Flags: review?(joshmoz)

Comment 9

11 years ago
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+
(Assignee)

Comment 10

11 years ago
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
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

11 years ago
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 → ---

Comment 13

11 years ago
Colin - please post a new patch and request review again before landing.
(Assignee)

Comment 14

11 years ago
So I'm still unable to reproduce those build errors on my machine.
(Assignee)

Comment 15

11 years ago
Created attachment 284835 [details] [diff] [review]
fix v2.0

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)

Updated

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

Comment 17

11 years ago
I'll just wait until after beta on this one.
Whiteboard: [checkin-needed][after beta1]
(Assignee)

Comment 18

11 years ago
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
Last Resolved: 11 years ago11 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.