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

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Josh Aas]

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

Comment 1

[Stan Shebs]

Attachment: Wrapping for osxspell sources

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

Comment 2

[Josh Aas]

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?

Comment 3

[Stan Shebs]

    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.

Comment 4

[jhp (no longer active)]

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 5

[Josh Aas]

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

I'd give this r+, but please also wrap the MacJawt file too.

Comment 6

[Stan Shebs]

Attachment: Version that wraps MacJawt.mm too

This version wraps MacJawt.mm also, and was tested by building XULRunner with Java XPCOM enabled.

Comment 7

[Josh Aas]

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.

Comment 8

[Josh Aas]

Attachment: _NSNULL macro, v1.0

Comment 9

[Stan Shebs]

Attachment: Better wrapping

Fixed per review.

Comment 10

[Josh Aas]

Comment on attachment 304629 [details] [diff] [review]
Better wrapping

Semicolon goes after all the macros, missing in your patch. Otherwise looks good.

Comment 11

[Stan Shebs]

Attachment: Now with semicolons in every bite!

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ideally extensions, plugins etc should be catching ObjC exceptions themselves.

Why did you end up wrapping in MacJawt? It doesn't call anything.

Comment 13

[Josh Aas]

Checked in without MacJawt wrapping. I don't know why I was confused about that before, we don't need to wrap it.

Comment 14

[Josh Aas]

I just remembered that we need some build work for this first

Comment 15

[Josh Aas]

As far as I can tell the compiler flag "-fobjc-exceptions" enables the exception handling, things compile fine without it though, so I landed.