Closed Bug 417563 Opened 13 years ago Closed 13 years ago

protect against Obj-C exceptions in "extensions" top-level directory

Categories

(Core :: General, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: stanshebs)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 3 obsolete files)

For implementing the strategy we decided on in bug 163260 we are filing a bug for each top-level directory that needs work.

mozilla/extensions
Status: NEW → ASSIGNED
Assignee: joshmoz → stanshebs
Status: ASSIGNED → NEW
Attached patch Wrapping for osxspell sources (obsolete) — Splinter Review
There's only the one file in extensions/ that needs wrapping. (MacJawt.mm contains no actual ObjC, nor any framework calls that I could see, so nothing to do to it.)
Attachment #303578 - Flags: review?(joshmoz)
From MacJawt.mm:
#import <JavaVM/jawt_md.h>

Presumably the Mac OS X Java impl uses Obj-C behind the scenes, are you sure we shouldn't wrap that?
    Presumably the Mac OS X Java impl uses Obj-C behind the scenes, are you sure we
shouldn't wrap that?

Probably we need some kind of generic extensions-wrapping strategy? The Java impl is not part of our tree, nor is there much Mac-specific code involved, so I'm guessing we eventually want to add some kind of isolator for extensions in general. In any case, it's out of scope for our first pass, seems likely that any such exception catchers are going to be installed in all-platform code.

If I recall correctly, the reason that code is in an Obj-C file is that 'cocoaViewRef' is an NSView and I was getting errors if I tried to compile it in a CPP file.

> The Java impl is not part of our tree...

This code is built by default for XULRunner.

Comment on attachment 303578 [details] [diff] [review]
Wrapping for osxspell sources

I'd give this r+, but please also wrap the MacJawt file too.
Attachment #303578 - Flags: review?(joshmoz)
This version wraps MacJawt.mm also, and was tested by building XULRunner with Java XPCOM enabled.
Attachment #303578 - Attachment is obsolete: true
Attachment #304325 - Flags: review?(joshmoz)
Comment on attachment 304325 [details] [diff] [review]
Version that wraps MacJawt.mm too

> NS_IMETHODIMP mozOSXSpell::GetDictionary(PRUnichar **aDictionary)
> {
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL

The return type for this and almost everything else in the file is nsresult, not an Obj-C pointer. Please use the _NSRESULT macros.

> - (PRUnichar*)createNewUnicodeBuffer
> {
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL

This works, but to keep it clean lets add a macro for nsnull in nsObjCExceptions.h. Put it under the _NIL macro, make it like so:

#define NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSNULL @try {
#define NS_OBJC_END_TRY_ABORT_BLOCK_NSNULL   } @catch(NSException *_exn) {        \
                                            nsObjCExceptionLogAbort(_exn);     \
                                          }                                    \
                                          return nsnull;

That is exactly like the _NIL macro but with nsnull instead of nil.
Attachment #304325 - Flags: review?(joshmoz) → review-
Flags: blocking1.9+
Attachment #304371 - Flags: superreview?(roc)
Attachment #304371 - Flags: superreview?(roc) → superreview+
Attached patch Better wrapping (obsolete) — Splinter Review
Fixed per review.
Attachment #304325 - Attachment is obsolete: true
Attachment #304629 - Flags: review?(joshmoz)
Comment on attachment 304629 [details] [diff] [review]
Better wrapping

Semicolon goes after all the macros, missing in your patch. Otherwise looks good.
Attachment #304629 - Flags: review?(joshmoz) → review-
Attachment #304629 - Attachment is obsolete: true
Attachment #304636 - Flags: superreview?(roc)
Attachment #304636 - Flags: review+
Priority: -- → P1
Attachment #304636 - Flags: superreview?(roc) → superreview+
Ideally extensions, plugins etc should be catching ObjC exceptions themselves.

Why did you end up wrapping in MacJawt? It doesn't call anything.
Checked in without MacJawt wrapping. I don't know why I was confused about that before, we don't need to wrap it.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
I just remembered that we need some build work for this first
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As far as I can tell the compiler flag "-fobjc-exceptions" enables the exception handling, things compile fine without it though, so I landed.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.