163260 - Need to catch Cocoa exceptions in all gecko callbacks

Status: RESOLVED INCOMPLETE

(Core :: Widget: Cocoa, defect, P3)

Description

[Simon Fraser [no longer active]]

If a cocoa exception is thrown in the middle of a gecko callback, really bad
things happen (like you can no longer load any pages, for example). Cocoa
exceptions are basically setjmp/longjmp, and, presumably, without a handler,
will jump back to the main event loop's default handler. This will unwind the
stack without any stack cleanup.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This bug has some serious security implications.  We rely on deallocation of stack-based C++ objects to manage XPConnect's principal stack.  That means that if  JS calls into cocoa widget code somehow and the widget code throws we'll be stuck with some random principal on the stack thereafter.  That is generally NOT good.

Not to mention that it breaks other things, of course....

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think this is a must-fix for 1.9....

Comment 3

[Colin Barrett [:cbarrett]]

There are three ways of catching and handling all exceptions in ObjC.

1. Old-style NS_DURING and friends and new-style @try/@catch. Here's an example of the old style, from when ObjC++ made it's debut: http://developer.apple.com/releasenotes/Cocoa/Objective-C++.html#exceptions

2. NSSetUncaughtExceptionHandler. Here's the documentation: http://developer.apple.com/documentation/Cocoa/Reference/Foundation/Miscellaneous/Foundation_Functions/Reference/reference.html#//apple_ref/c/func/NSSetUncaughtExceptionHandler

3. NSExceptionHandler's delegate and it's delegate methods. In particular,
http://developer.apple.com/documentation/Cocoa/Reference/NSExceptionHandle_Class/Reference/Reference.html#//apple_ref/occ/instm/NSExceptionHandler/exceptionHandler:shouldHandleException:mask: 

There is also Wolf Rentzsch's NSXException. http://rentzsch.com/nsxexception/doxygen/index.html

This is all particularly nasty because literally any call to an Objective-C method can generate an exception. Here's an example:

2007-03-01 20:53:01.869 frameworkless growl app[27343] *** Uncaught exception: <NSInvalidArgumentException> *** -[NSURL dockDescription]: selector not recognized [self = 0x3515a0]

It seems that at least on 10.2, it is highly likely that all of these are implemented setjmp and longjmp. According to timeless, you're not allowed to longjmp past XPCOM boundaries. Effectively, each NSIMETHODIMP method needs to handle all exceptions.

There is hope, though. The new 10.3 exception handling (@try/@catch/@finally) is a language feature, not just a macro. See: http://gcc.gnu.org/onlinedocs/gcc/Objective_002dC-and-Objective_002dC_002b_002b-Dialect-Options.html

It would be wonderful if we could use the native exception handling features (I would suggest option 3), rather than having to resort to black magic of some sort.

Comment 4

[Colin Barrett [:cbarrett]]

I just realized that if the 10.3+ exception handling were free of setjmp/longjmp, how could this even happen in the first place.

We are probably SOL in this case.

Comment 5

[Colin Barrett [:cbarrett]]

sayrer and I created the following wiki page: http://wiki.mozilla.org/Cocoa_In_XPCOM

I'll be updating it a bit more tomorrow as I do more research.

Comment 6

[Simon Fraser [no longer active]]

(In reply to comment #3)
> There are three ways of catching and handling all exceptions in ObjC.
> 
> 1. Old-style NS_DURING and friends and new-style @try/@catch. Here's an example
> of the old style, from when ObjC++ made it's debut:
> http://developer.apple.com/releasenotes/Cocoa/Objective-C++.html#exceptions

Right, the is the one to use

> 2. NSSetUncaughtExceptionHandler. Here's the documentation:
> http://developer.apple.com/documentation/Cocoa/Reference/Foundation/Miscellaneous/Foundation_Functions/Reference/reference.html#//apple_ref/c/func/NSSetUncaughtExceptionHandler

This is no use for the problem at hand. Once this gets called, the stack has already been unwound back up the top.

> 3. NSExceptionHandler's delegate and it's delegate methods. In particular,
> http://developer.apple.com/documentation/Cocoa/Reference/NSExceptionHandle_Class/Reference/Reference.html#//apple_ref/occ/instm/NSExceptionHandler/exceptionHandler:shouldHandleException:mask:

This uses NSSetUncaughtExceptionHandler, so is also not useful.
  
> There is also Wolf Rentzsch's NSXException.
> http://rentzsch.com/nsxexception/doxygen/index.html

Also not useful. XPCOM is not (yet) C++ exception aware or friendly.

The problem described in this bug has a very well defined scope: Obj-C exception handling must avoid unwinding any stack frame that contains XPCOM code. In other words, all Obj-C exceptions must be caught  within down-calls from XPCOM into Obj-C code, and additional care must be taken in mixed Obj-C/XPCOM code to avoid throwing exceptions that fail to destroy local XPCOM objects.

Comment 7

[Simon Fraser [no longer active]]

I should also add that stack unwinding (though there is no unwinding as such) as the result of an Obj-C exception does NOT result in C++ destructors being called, at least with gcc 3.3. Exceptions are still essentially setjmp/longjmp.

Comment 8

[Colin Barrett [:cbarrett]]

So, *all* XPCOM methods that touch ObjC code must have a handler around them, since any method call can generate an exception.

This isn't so bad on branch, there are not that many files. But pretty much every function in widget cocoa will have to have guards placed around it. That doesn't sound incredibly pleasant.

Comment 9

[chris hofmann]

josh, any progress on this one?

Comment 10

[Colin Barrett [:cbarrett]]

Attachment: small test extension

I've created a small extension that demonstrates the problem in a reduced form. I'm still optimistic a solution can be found other than blindly writing guard macros around *everything*.

I'm going to be doing additional testing on this to see if we can handle these and produce useful stack traces, at least.

Comment 11

[Colin Barrett [:cbarrett]]

bug 340453 would at least give us a safe place to die, and give us useful stacktraces.

We should be catching ObjC exceptions (or avoid causing them to be thrown), however. 

Comment 12

[Steven Michaud [:smichaud] (Retired)]

The current plan for this bug (and also bug 340453) is to crash the
browser whenever an exception is (or would have been) raised by
[NSException raise], after having NSLogged whatever information is in
the NSException object (i.e. it's name and description, and possibly
also its userInfo (if present)).

I hope to have a patch to accomplish this by the end of next week (by
2007-06-08).

Comment 13

[(not currently active) Ted Mielczarek]

Crash the browser in what way?  Will we be able to catch it with Breakpad?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

> Crash the browser in what way?

I haven't decided yet.

> Will we be able to catch it with Breakpad?

Yes, I'll make sure of this.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #10)

Colin, your extension no longer causes any messages to be displayed in
the system console or any crashes (as of a trunk nightly dated
2007-05-22).  (I installed the extension and followed the instructions
in its README.)

So this bug may no longer have any security implications.

But I'll continue working on my patch to do what I described in
comment #12.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Quick-fix patch

Here's a quick-fix patch that does what I outlined in comment #12 --
it hooks -[NSException raise] and crashes the browser whenever there's
a Cocoa exception.

Not every Cocoa exception is fatal, so doing this will cause the
browser to crash "too often".  But the problem is that we can't (or
don't yet know that we can) rely on Apple's code to distinguish
between exceptions that should be fatal and ones that shouldn't.  And
we don't yet know enough about the possible exceptions to determine
this reliably on our own.

Over the next month or two I hope to find out more about this subject.
With luck I'll be able to revise this patch or roll it back
altogether.  In the meantime I think my patch is the best solution
that can be achieved quickly (with very little reliable information).

I haven't yet done much testing, so I'm not yet requesting a review
for this patch.  But what testing I've done indicates that it gets
along fine with Breakpad and with Apple's crashreporterd.

(By the way, in the most recent Minefield nightlies Breakpad seems to
be on by default.  In ones more than a few days old you need to set
the MOZ_AIRBAG environment variable for Breakpad to work (e.g. 'export
MOZ_AIRBAG=1' at the command line).)

(By default Breakpad pre-empts Apple's crashreporterd -- if Breakpad
is enabled you only see the Breakpad window on a crash.  You can do
the following at a Terminal prompt to use them both at once:

  defaults write org.mozilla.firefox OSCrashReporter 1

For more information see bug 382541.)

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Attachment: Corrected small test extension

(Following up comment #15)

Colin, your test extension (the one you actually posted) never worked
properly -- you swapped the "CID" and the "contract ID", so
Components.classes["@cbarrett/objcxx-exceptions"] never located it.

I've fixed it up and reposted it.  (I also made it a universal
binary.)

My fixed copy of your extension now works as you described in comment
#10 -- even with the latest Minefield nightly.

Interestingly, with my appshell patch (the one that stops [NSApp run]
from being called on a delayed perform, which I emailed to you and
Josh about a month ago), your extension's exception is still ignored,
but no crash takes place.  This _could_ mean that Apple's code is
right to ignore the exception in that case ... but I think it's best
not to take any chances (at least until we know a lot more).  So I
don't think my appshell patch will obsolete my quick-fix patch for
this bug.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Something I forgot to mention:  Colin's small test extension
(attachment 267776 [details], the one I fixed up) is a very handy way to test my
patch.

Comment 19

[Simon Fraser [no longer active]]

I'd very strongly suggest to not use poseAsClass: and to make the much larger code change to wrap all calls into Obj-C code from XPCOM code in exception handlers.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> I'd very strongly suggest to not use poseAsClass:

Why?

Comment 21

[Simon Fraser [no longer active]]

(In reply to comment #20)
> > I'd very strongly suggest to not use poseAsClass:
> 
> Why?

<http://adiumx.com/pipermail/adium-devl_adiumx.com/2007-May/002924.html>

Also, this crashy exception handler will terminate the app if ANY code throws exceptions. How do you know that no frameworks used by Camino don't use exception handling internally for that own needs? Can you say that for all plug-ins? Can you say that for all future OS versions?

The fix for this bug really needs to be a focused on the core problem; Objective-C exceptions must not propagate through XPCOM code. The only way to fix that is to catch them at all the boundary points.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> The fix for this bug really needs to be a focused on the core problem;
> Objective-C exceptions must not propagate through XPCOM code. The only way to
> fix that is to catch them at all the boundary points.

Hear hear. I cc'ed Taras in case he can help with an analysis we could tinderbox on the code. The question is, how would an Oink-based (C++ full, unambiguous AST) analysis recognize the boundary points?

/be

Comment 23

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #17)

> Interestingly, with my appshell patch (the one that stops [NSApp
> run] from being called on a delayed perform, which I emailed to you
> and Josh about a month ago), your extension's exception is still
> ignored, but no crash takes place.

Unfortunately I was wrong (I was making too may changes at once) -- my
appshell patch _doesn't_ fix the crash you get with Colin's test
extension (attachment 267776 [details]).

Comment 24

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #21, part 1)

> <http://adiumx.com/pipermail/adium-devl_adiumx.com/2007-May/002924.html>

I suppose you're referring to this part of this message:

> Just a note on poseAsClass:. It might be to your benefit to use
> swizzling instead -- AIUI, Leopard introduces changes that break
> poseAsClass, but include a mechanism for a blessed type of
> swizzling.

I know from my work with the Java Embedding Plugin (which relies
heavily on poseAsClass:) that poseAsClass: works just fine in all ADC
developer seeds of OS X 10.5 (Leopard) that have been released to
date.  I think it's highly unlikely that Apple would make such a major
change to their Objective C architecture at such a late date.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #21, part 2)

> How do you know that no frameworks used by Camino don't use
> exception handling internally for that own needs? Can you say that
> for all plug-ins? Can you say that for all future OS versions?

These are all valid objections ... though my quick-fix patch will most
likely only degrade the performance of external frameworks and plugins
that use Objective C exception handling, and though I'm currently
unaware of any examples.  But (as you'll have noticed) my quick-fix is
just that -- stop-gap measure until we can come up with something
better.

> The fix for this bug really needs to be a focused on the core
> problem; Objective-C exceptions must not propagate through XPCOM
> code. The only way to fix that is to catch them at all the boundary
> points.

To do this (and particularly to test it) will be very difficult and
time-consuming, even with the help of Oink.  Does it really make sense
to try to do this before the Firefox 3.0 release?  Would it be
acceptable to delay the Firefox 3.0 release in order to finish this
work?

What if I alter my quick-fix so that it only crashes the browser when
the browser is "inside XPCOM"?  (At other times it would just pass the
call to its superclass.)  This would require an additional XPCOM API
(perhaps a private extension to nsISupports) ... but implementing that
would surely be much less difficult than wrapping all XPCOM calls.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

I originally hoped to use NSSetUncaughtExceptionHandler() to install a
handler that would only crash the browser on "uncaught" exceptions.
But even the exception raised by Colin's test extention somehow
doesn't count as "uncaught" -- it never gets passed to my handler.
(Neither does the exception from bug 173122.)

I also tried playing with the NSExceptionHandler class's delegate
methods (exceptionHandler:shouldHandleException:mask: and –
exceptionHandler:shouldLogException:mask:), with similar results.

Comment 27

[Simon Fraser [no longer active]]

Has anyone tried to make a rough quantification of which code needs to catch Obj-C exceptions?

It's mostly widget code (except in Camino, where XPCOM callbacks are implemented in Obj-C in the app). More specifically, it's the code in widget/src/cocoa/ that implements XPCOM interfaces by calling into Obj-C code. Someone should just take a stab at throwing in exception handlers there, hidden behind some macros. That's really only a few hours of boring, repetitive work.

Is there code outside of widget/src/cocoa that also needs treatment?

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
toolkit/components/alerts/src/mac/

Comment 29

[Steven Michaud [:smichaud] (Retired)]

As best I can tell, Objective C exception handling is implemented (in
/usr/lib/libobjc.A.dylib) using a bunch of undocumented functions
whose names start with objc_exception_ (e.g. objc_exception_try_enter
and objc_exception_try_exit).

Grepping on "objc_exception_" in /usr/lib turns up only
libobjc.A.dylib itself.

Grepping on "objc_exception_" in /System/Library/Frameworks turns up
the following list of frameworks (on OS X 10.4.9) (I don't include
apps, dylibs and "plugins" inside the frameworks):

AddressBook.framework/AddressBook
AppKit.framework/AppKit
CoreData.framework/CoreData
DiscRecording.framework/Frameworks/DiscRecordingContent.framework/DiscRecordingContent
Foundation.framework/Foundation
InstantMessage.framework/InstantMessage
Message.framework/Message
QuartzCore.framework/QuartzCore
SenTestingKit.framework/SenTestingKit

Comment 30

[(dormant account)]

(In reply to comment #25)
> (In reply to comment #21, part 2)
> 
> > How do you know that no frameworks used by Camino don't use
> > exception handling internally for that own needs? Can you say that
> > for all plug-ins? Can you say that for all future OS versions?
> 
> These are all valid objections ... though my quick-fix patch will most
> likely only degrade the performance of external frameworks and plugins
> that use Objective C exception handling, and though I'm currently
> unaware of any examples.  But (as you'll have noticed) my quick-fix is
> just that -- stop-gap measure until we can come up with something
> better.
> 
> > The fix for this bug really needs to be a focused on the core
> > problem; Objective-C exceptions must not propagate through XPCOM
> > code. The only way to fix that is to catch them at all the boundary
> > points.
> 
> To do this (and particularly to test it) will be very difficult and
> time-consuming, even with the help of Oink.  Does it really make sense
> to try to do this before the Firefox 3.0 release?  Would it be
> acceptable to delay the Firefox 3.0 release in order to finish this
> work?

One could use a complete Firefox callgraph (which I haven't gotten working yet.
It's hard.) to flag XPCOM boundary points featuring Cocoa exception-unsafe
code. I would have to have to look for sort of pattern for identifying
exception-causing code.
The benefit would be peace of mind that comes with knowing that all causes of
exceptions are plugged.
Downside is that this may be a lot of work. Oink's C++ parser barfs on certain
platform-specific parts of Mozilla. Would need to correct misc Apple C/C++
compatibility issues in oink.
I would also have to finally solve the scalability issues to do with callgraph
building and convince myself that the graph is complete. This implies that the
graph is aware of all of the callbacks in the code. The graph is needed for a
lot of other analyses, so it would be beneficial to finally have a way of
building it. 
This would take a while to get right.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #25)

> What if I alter my quick-fix so that it only crashes the browser
> when the browser is "inside XPCOM"?  (At other times it would just
> pass the call to its superclass.)  This would require an additional
> XPCOM API (perhaps a private extension to nsISupports) ... but
> implementing that would surely be much less difficult than wrapping
> all XPCOM calls.

The additional XPCOM API needn't be an extension to nsISupports -- it
could be simply a new "service".  The browser would be "inside XPCOM"
whenever any XPCOM object's reference count was greater than 0.  Some
kind of hack would be needed for those objects (like nsFrame objects)
that don't use reference counting.

The new "service" objects would (presumably) themselves never be
AddRefed or Released.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #30)

> I would have to have to look for sort of pattern for identifying
> exception-causing code.

You should probably assume that any call that uses Objective C syntax
might cause an Objective C exception (some won't, but tracking them
down probably isn't worth the trouble).

Any C/C++ method defined in one of the frameworks listed in comment
#29 should also be included (plus those defined in frameworks that
import symbols from the ones I listed, directly or indirectly).  Once
again, it's probably best to make the simplifying assumption that any
C/C++ method in any of Apple's frameworks (those in
/System/Library/Frameworks) might cause an Objective C exception.

Most of these methods' names start with "NS..."
(e.g. NSSetUncaughtExceptionHandler() or NSLog()), but not all of them
do.

It'd take some time, but in principle I could assemble a list of all
the externally defined symbols in all the frameworks that Cocoa
Firefox links to and pass this along to you (this list would contain
thousands of symbols, and possibly tens of thousands of them).

Comment 33

[(dormant account)]

(In reply to comment #32)
> (In reply to comment #30)
> 
> > I would have to have to look for sort of pattern for identifying
> > exception-causing code.
> 
> You should probably assume that any call that uses Objective C syntax
> might cause an Objective C exception (some won't, but tracking them
> down probably isn't worth the trouble).

Files that are not pure C or C++ are going to be a problem as oink does not support Objective C & C++ languages.

> 
> Any C/C++ method defined in one of the frameworks listed in comment
> #29 should also be included (plus those defined in frameworks that
> import symbols from the ones I listed, directly or indirectly).  Once
> again, it's probably best to make the simplifying assumption that any
> C/C++ method in any of Apple's frameworks (those in
> /System/Library/Frameworks) might cause an Objective C exception.


Matching on things defined within /System/Library/Frameworks should be pretty easy.

> 
> Most of these methods' names start with "NS..."
> (e.g. NSSetUncaughtExceptionHandler() or NSLog()), but not all of them
> do.

I'm guessing that by matching on the definition path, I'd get all of the methods.

> 
> It'd take some time, but in principle I could assemble a list of all
> the externally defined symbols in all the frameworks that Cocoa
> Firefox links to and pass this along to you (this list would contain
> thousands of symbols, and possibly tens of thousands of them).
> 

Handling big lists isn't a problem. A bigger problem is building the said callgraph. If I drop everything, I *should* be able to the get a basic callgraph within a week or two. I'm not sure how long it would take to get a more or less correct and complete graph.
Another big issue is parsing Objective C++ files. It would take a quite bit of work to add a 3rd language to oink correctly.
Overall I don't see this taking less than a month of work for me. Since I need callgraphs and plan on working on them anyways, this isn't a problem for me.


What about a more hacky solution? Is it possible to have Firefox continuously output backtraces(either the supposed OSX 10.5 Dtrace or by modifying the code)? Those could be assembled and analyzed much sooner.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

>> Any C/C++ method defined in one of the frameworks listed in comment
>> #29 should also be included (plus those defined in frameworks that
>> import symbols from the ones I listed, directly or indirectly).
>> Once again, it's probably best to make the simplifying assumption
>> that any C/C++ method in any of Apple's frameworks (those in
>> /System/Library/Frameworks) might cause an Objective C exception.
>
> Matching on things defined within /System/Library/Frameworks should
> be pretty easy.

Does this mean you have your own ways to extract lists of externally
defined symbols from these frameworks ... or do you need me to make
those lists? :-)

Comment 35

[Steven Michaud [:smichaud] (Retired)]

> What about a more hacky solution? Is it possible to have Firefox
> continuously output backtraces(either the supposed OSX 10.5 Dtrace
> or by modifying the code)? Those could be assembled and analyzed
> much sooner.

For what it's worth, the two most recent Leopard ADC seeds contain an
app called Xray that bundles a DTracePlugin.  There's a minimal
description of Xray at http://developer.apple.com/leopard/overview/
(you have to scroll down a bit).  No, I haven't yet used it.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

There's also a "dtrace" in /usr/sbin.  It's man page makes it look
like a clone of the Solaris utility.

Comment 37

[(dormant account)]

> Does this mean you have your own ways to extract lists of externally
> defined symbols from these frameworks ... or do you need me to make
> those lists? :-)
> 

As long as the headers are C/C++, my tool will easily match on them. How much Objective C/C++ is involved in this? Is it possible to somehow manually work-around the Objective C++ bits?
Or is the problem pretty much ObjC++ specific(if there is ObjC++, it needs to be fixed?)

Comment 38

[Steven Michaud [:smichaud] (Retired)]

The header files mix C/C++ and Objective C syntax.  Take a look at a
few to see for yourself, but I think you might have an easier time
with the results of using nm -g on the framework binaries (which is
what I was planning to do, though mostly by hand).

Most (all?) the framework headers are in standard locations.  For
example the Foundation framework headers are in
/System/Library/Frameworks/Foundation.framework/Headers.  But there
can be frameworks inside of frameworks, so (for example) the HIToolbox
framework headers are in
/System/Library/Frameworks/Carbon.framework/Frameworks/HIToolbox.framework/Headers.

The framework binaries are also in standard locations, for example:

/System/Library/Frameworks/Foundation.framework/Foundation
/System/Library/Frameworks/Carbon.framework/Frameworks/HIToolbox.framework/HIToolbox

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Here's a list of all the frameworks that are linked to anywhere in the
Mozilla source tree (I made the list by grepping on '-framework',
which is the command to link in a framework).  You'd need to work
through each of these and all of their subframeworks.  But my list is
a lot smaller than the list of everything under
/System/Library/Frameworks.

/System/Library/Frameworks/AppKit.framework
/System/Library/Frameworks/Foundation.framework
/System/Library/Frameworks/Carbon.framework
/System/Library/Frameworks/Cocoa.framework
/System/Library/Frameworks/OpenGL.framework
/System/Library/Frameworks/AGL.framework
/System/Library/Frameworks/QuickTime.framework
/System/Library/Frameworks/IOKit.framework
/System/Library/Frameworks/Python.framework
/System/Library/Frameworks/CoreServices.framework
/System/Library/Frameworks/CoreFoundation.framework

The following is used only by airbag, so probably isn't needed:

/System/Library/Frameworks/System.framework

Likewise with the following, which is used only when MOZ_JAVAXPCOM is
defined:

/System/Library/Frameworks/JavaVM.framework

And the following is used only by asencode, which is obsolete:

/System/Library/Frameworks/ApplicationServices.framework

Comment 40

[(dormant account)]

That means that we'd need Objective C++ support in Elsa for this to work. I'll look into what that entails tomorrow. 

Comment 41

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #39)
> Here's a list of all the frameworks that are linked to anywhere in the
> Mozilla source tree (I made the list by grepping on '-framework',

That list looks like it's missing anything that is linked only in project files (e.g., Camino links against at least Security.framework not mentioned above; I did a project patch for that change)--but Camino may be the only project file linking additional frameworks.

Comment 42

[Steven Michaud [:smichaud] (Retired)]

My first search did go into project files ... but it turns out that
the '-framework' syntax isn't the only way to link in frameworks.

This time I grepped on '.framework' (the last part of a framework's
directory name) and came up with the following additions (including
the Security framework):

/System/Library/Frameworks/AddressBook.framework
/System/Library/Frameworks/Security.framework
/System/Library/Frameworks/SystemConfiguration.framework
/System/Library/Frameworks/PreferencePanes.framework
/System/Library/Frameworks/InterfaceBuilder.framework
/System/Library/Frameworks/CoreData.framework

Also, it seems that the following is used by more than just asencode,
so it should also be included in the main list.

/System/Library/Frameworks/ApplicationServices.framework

Thanks, Smokey, for catching my mistake!

Comment 43

[Simon Fraser [no longer active]]

Here's another thought. The real concern here is that throwing Obj-C exceptions will break the XPConnect stack stuff that tells the browser whether it's in a trusted or untrusted context.

Why not put the exception-handling there? That should guarantee that an unwind never goes past a XPConnect stack boundary. You could then crash the browser in that handler if you really want to.

Comment 44

[Steven Michaud [:smichaud] (Retired)]

Simon, I'll test your idea as soon as I get a chance.  Right now I'm
following another hunch (which I hope to report on later today).

Comment 45

[(dormant account)]

(In reply to comment #40)
> That means that we'd need Objective C++ support in Elsa for this to work. I'll
> look into what that entails tomorrow. 
> 

Parsing/analysis of ObjC++ would take too long for me to do. Sorry, I can't help with oink.

Comment 46

[timeless]

shouldn't you be able to change the loader so that it recognizes cocoa libraries and wraps all objects that are created with wrappers that include the catch calls around all objc method invocations? If necessary, you can provide a standard cocoa NSGetModule impl that everyone links in instead of them writing their own, and have that glue be responsible for it instead of hacking the loader (there's not much of a difference).

trying to do this on the xpconnect side means having to do it for pyconnect and javaconnect and the others. the rule is that exceptions should not be passed out, i.e. it should be the obligation of the module to squish the exception, not other modules to recognize that there's a misbehaving module.

Comment 47

[Simon Fraser [no longer active]]

(In reply to comment #46)
> shouldn't you be able to change the loader so that it recognizes cocoa
> libraries and wraps all objects that are created with wrappers that include the
> catch calls around all objc method invocations?

This is crazy talk.

Comment 48

[Steven Michaud [:smichaud] (Retired)]

Attachment: Quick-fix patch rev1 (plays nice with other exception handlers)

Here's a revision to my "quick fix" patch that gets along with 3rd
party Objective C exception handling (elsewhere in the browser, in
extensions, in plugins or in linked frameworks).

I found an undocumented Foundation framework function
(_NSExceptionHandlerCount()) that allows us to know how deeply nested
we are in Apple's @try-@catch block infrastructure.  Knowing this
allows me to distinguish between calls to [NSException raise] that
come from 3rd party exception handlers (which I pass along to Apple's
original NSException class) and those that happen at the "top level"
(on which I crash the browser).

I also found and fixed another flaw in my first quick fix --
exceptions thrown with the @throw statement never pass through
[NSException raise], so my first quick fix allowed them to be
mis-handled by Apple's exception handlers (on these exceptions
stack-based XPCOM objects were still left hanging, resulting in the
same oddball crashes).

I still "hook" [NSException raise] (which is needed to provide
accurate browser-crash stack traces).  But now I also wrap a couple of
top-level calls into the browser (in a way that I hope covers all
possible sources of Objective C exceptions).

I could conceivably put my top-level exception "wrappers" in different
locations (along the lines of what Simon discusses in comment #43).
But unless these wrappers are put exactly where the exceptions occur
(are raised), I'll still need to hook [NSException raise] to provide
accurate crash log stack traces.

Note:  Though the XPCOM stack-unwinding problem may be the most
pressing problem we have with the current state of Objective C
exception handling, it's not the only one.  The larger problem is (as
I say in my comments above [PosedNSException raise]) that the current
Objective C exception handling ignores many exceptions that should be
considered fatal (e.g. the bad selector exception).

Another Note:  My patch will still probably crash the browser on "too
many" exceptions (ones that should really be ignored).  My (rather
limited) tests on OS X 10.4.X and 10.5 indicate that this won't be a
problem.  But we won't really know the scale of the problem until my
patch (or something like it) gets into the hands of more testers.

Yet Another Note:  You need to compile with -fobjc-exceptions in order
to use @try, @catch and friends.  But gcc won't allow this unless
MACOSX_DEPLOYMENT_TARGET is set to at least 10.3.  This is why I
changed configure and configure.in.

And Yet Another Note:  I tested _NSExceptionHandlerCount() pretty
thoroughly on OS X 10.4.9 PPC, OS X 10.4.9 Intel and OS X 10.5 build
9A410 (the latest ADC developer seed, from a couple of months ago).
I'll test on the WWDC 10.5 beta as soon as that gets released to ADC
members.  And I hope to be able to test on OS X 10.3.9 (though my
current builds crash on startup on that OS).

Comment 49

[Steven Michaud [:smichaud] (Retired)]

Attachment: Expanded small test extension

I've added a whole bunch of new tests to Colin's extension.  This is
what I've used to do most of my testing.

Comment 50

[Simon Fraser [no longer active]]

Comment on attachment 268502 [details] [diff] [review]
Quick-fix patch rev1 (plays nice with other exception handlers)

>+// But at least in Mozilla.org browsers,
>+// no exception ever makes it to these handlers.

If you could fix this, would all this hackery be necessary?

Comment 51

[Steven Michaud [:smichaud] (Retired)]

>>+// But at least in Mozilla.org browsers,
>>+// no exception ever makes it to these handlers.
>
>If you could fix this, would all this hackery be necessary?

Fixing this will be a lot more difficult than my "hackery", because
its causes are buried even deeper in Apple's OS X internals.  If we
want a solution anytime soon, my "hackery" is the way to go.

The ultimate solution (for Mozilla.org browsers) is (probably) to
integrate workarounds for the problems with Apple's Objective C
exception handling into the support for C++ exception handling that
Mozilla.org browsers will eventually get.

Along the way, I'm sure we'll discover more about exactly in what ways
Apple's Objective C exception handling is disfunctional.  But we'll
probably never be able to "fix" it -- we may be able to come up with
better hacks, but they'll always be hacks.

Comment 52

[Steven Michaud [:smichaud] (Retired)]

>>+// But at least in Mozilla.org browsers,
>>+// no exception ever makes it to these handlers.
>
>If you could fix this, would all this hackery be necessary?

One more thing:  Even if I could get Apple's uncaught exception
handlers to work properly (a _very_ big if), I suspect the stack
traces you'd get from crashing there would be less accurate than those
you get from crashing in [NSException raise].

Comment 53

[Simon Fraser [no longer active]]

(In reply to comment #51)
> >>+// But at least in Mozilla.org browsers,
> >>+// no exception ever makes it to these handlers.
> >
> >If you could fix this, would all this hackery be necessary?
> 
> Fixing this will be a lot more difficult than my "hackery", because
> its causes are buried even deeper in Apple's OS X internals

Is it also true in Camino? I'm guessing that Camino will get the uncaught exception handler stuff because it's a real Cocoa app using NSApplication, but FF isn't, so won't. If you really want Camino to use XULRunner, this problem will have to be solved.

I still highly discourage the approach you are taking, especially now that you're using private API. And I haven't yet heard good answers to my questions in comment 27 and comment 43.

Comment 54

[Steven Michaud [:smichaud] (Retired)]

> Is it also true in Camino?

I don't know, but I suspect so.

> I'm guessing that Camino will get the uncaught exception handler
> stuff because it's a real Cocoa app using NSApplication, but FF
> isn't, so won't.

If you're right, I'll have learned something.

I'll probably test in Camino next week.  Though in general I'll be
putting this on the back burner until I can test in the WWDC beta of
OS X 10.5 -- I've got other, equally pressing things I need to do :-)

> And I haven't yet heard good answers to my questions in comment 27
> and comment 43.

As for comment #27, I don't think there's much point in pursing it if
we can't use Oink.

As for comment #43, please spell out in more detail what you're
getting at (including references to relevant parts of the source
code).

Comment 55

[(dormant account)]

> 
> As for comment #27, I don't think there's much point in pursing it if
> we can't use Oink.

I just want to clarify the application of oink here. It would take a few weeks or a month to get basic ObjC++ support into oink. It might be possible to add simple "This function has some objC++ in it" support sooner. Adding another language to C/C++ is a big task. 
As far as I know ObjC++ is more "dynamic" than C/C++ which would make things like call-graphs difficult to figure out.

What exactly do you envision oink doing? I can see it being a checker to make sure that every call into the Obj-C++ land is guarded with an try/catch. If you think you need this and can't achieve this effect any easier way and you can wait a while for ObjC++ to be added to oink, I could try to implement it.

Comment 56

[Steven Michaud [:smichaud] (Retired)]

> What exactly do you envision oink doing? I can see it being a
> checker to make sure that every call into the Obj-C++ land is
> guarded with an try/catch.

This is (more or less) exactly what I was hoping for from Oink -- I
wanted it to find every place ObjC code or (non-ObjC)
/System/Library/Frameworks methods are called from.

But I also think this work should wait until support for C++ exception
handling is added to the Mozilla tree (if I understand correctly, this
is targeted for Mozilla 4).  As long as we have a simple, workable
solution to the XPCOM stack-unwinding problem (as provided by my
"quick fix" patch (attachment 268503 [details])), I don't think we should spend
a lot of time now (before Mozilla 3), doing half-baked work that will
only have to be redone correctly a year or two down the line.

I'd think this even if Oink supported Objective-C right now.  But I
think it's _especially_ true if we can't use Oink, or if asking you to
add Objective-C support to Oink (and Elsa) would take too much of your
time away from other projects.

I _do_ think Oink and Elsa will eventually need to support
Objective-C/C++, but only before C++ exception handling is added to
the Mozilla tree.

(I hope you don't think I wasted your time by asking questions about
Oink even though I don't think it should be used to fix this bug.  I
wanted to explore alternatives to my position.)

Comment 57

[(dormant account)]

(In reply to comment #56)
> > What exactly do you envision oink doing? I can see it being a
> > checker to make sure that every call into the Obj-C++ land is
> > guarded with an try/catch.
> 
> This is (more or less) exactly what I was hoping for from Oink -- I
> wanted it to find every place ObjC code or (non-ObjC)
> /System/Library/Frameworks methods are called from.

Ok. This sounds like a good spec.

> 
> But I also think this work should wait until support for C++ exception
> handling is added to the Mozilla tree (if I understand correctly, this
> is targeted for Mozilla 4).  As long as we have a simple, workable
> solution to the XPCOM stack-unwinding problem (as provided by my
> "quick fix" patch (attachment 268503 [details])), I don't think we should spend
> a lot of time now (before Mozilla 3), doing half-baked work that will
> only have to be redone correctly a year or two down the line.

Once a tool is written, it'll do the same check a year or two from now so it's never too early to start specing out the tools.

> 
> I'd think this even if Oink supported Objective-C right now.  But I
> think it's _especially_ true if we can't use Oink, or if asking you to
> add Objective-C support to Oink (and Elsa) would take too much of your
> time away from other projects.
> 
> I _do_ think Oink and Elsa will eventually need to support
> Objective-C/C++, but only before C++ exception handling is added to
> the Mozilla tree.

Great. So I'll start planning for ObjC++ support. 

> 
> (I hope you don't think I wasted your time by asking questions about
> Oink even though I don't think it should be used to fix this bug.  I
> wanted to explore alternatives to my position.)

This isn't a waste of time. These tools take time to write, it's really nice to have a heads-up ahead of time. I just wanted to clarify whether you need oink in the longterm or not. Looks like you do.

Comment 58

[Brendan Eich [:brendan]]

Steven: Mozilla 2, not 4 -- Firefox 4 : Mozilla 2 :: Firefox 3 : Mozilla 1.9 in SAT problem form (the branding for Firefox is subject to change, but Mozilla 2 is getting under way -- see http://hg.mozilla.org/).

/be

Comment 59

[Damon Sicore (:damons)]

Any news on this?

Comment 60

[Steven Michaud [:smichaud] (Retired)]

I haven't had time to work on this in a while.

With luck I should be able to get back to it after the rush to get M9
out.  It should help that I now know more about appshell event
processing (from my experience writing the bug 395397 appshell patch).

Comment 61

[Colin Barrett [:cbarrett]]

I'll reiterate that I think the best strategy would be to use a top level exception handler to grab uncaught exceptions and then bring down the app.

That said, people need to read the documentation and know what exception types a method can throw (-[NSImage lockFocus] for example). It would be awesome to have a script to find all methods that are documented to throw an exception.

Comment 62

[Daniel Veditz [:dveditz]]

Raising priority to P2 on implementing Colin's suggestion in comment 61. We don't care if it crashes safely (those are bugs that can be fixed later if found) but we don't want to leave potential unsafe crashes.

Comment 63

[chris hofmann]

moving to P1.  best to find these crashes as part of beta3 and not try to recover in an RC or after final is out the door.

Comment 64

[Steven Michaud [:smichaud] (Retired)]

It's very unlikely that I'll be able to get this done by the beta3
freeze (even now that the date's apparently been moved back).

This by itself is an argument for having a beta4, I think.

Comment 65

[Damon Sicore (:damons)]

Steven, based on the discussion in the Mac Gecko meeting today, can you please clarify your estimate?  I thought I heard you say that you fear this even making Fx3.  Can we implement a quicker fix that addresses the unsafe crashes (comment 62) without risk and with the understanding we'll address the issue as soon as the appropriate solution is completed?  What are the negatives if we do so?

Comment 66

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #65)

This bug is both difficult and contentious (for example, I think I've
already established that the relatively simple approach discussed in
comment #61 doesn't work).  So, including time spent on the inevitable
discussions and debates, I think it will take a while to resolve
satisfactorily (to make reasonably sure that, when a Cocoa exception
occurs in XPCOM, we always crash safely).

It's hard to say exactly how long.  But I'm pretty sure it won't be
possible to do this by the new freeze date for beta3 (January 20th).
I think (and I suspect everyone agrees) that this change shouldn't be
landed in an RC -- so it needs to be landed in a beta.  Presuming that
there's a beta4 and that its freeze date is, say, around February
20th, I think there's a reasonable hope of including it in beta4 (if
this is more-or-less the only bug I work on during the lead up to
beta4).  But if there isn't going to be a beta4, then (yes) I'm afraid
a satisfactory fix for this bug won't make it into Firefox 3.

So (as you say) the question arises of writing a
less-than-satisfactory but still useful fix by the code freeze date
for beta3 (Jan 29th).  But I'm afraid this isn't feasible.  I've
already come up with what I think is a reasonable approach to tacking
this bug.  And I'm quite sure that the alternatives already mentioned
simply won't work (at least in the time we have available).  So there
really isn't any middle ground.

We need to examine whether we can get away with letting this problem
slide until after the Firefox 3 release.  I don't exactly favor this
solution.  But if there isn't going to be a beta4, I think it's the
most reasonable thing to do.  This bug has been open for a long time,
and the problem has existed on the trunk since Cocoa widgets were
landed, and in Camino for even longer.  But (as far as I know) there
haven't been any problems reported that anyone has tried to blame on
this bug.  It's true that, since this bug's problem is precisely the
_absence_ of crashes, it's very difficult to make the conection
between some kind of wierdness and this bug.  But I'm unaware of any
reproducible problems (or problems reported in any detail) that remain
completely unexplained, and which therefore might possibly be blamed
on this bug.

Comment 67

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

Steven, can you explain in this bug why Colin's suggestion in comment #61 won't work?

And what is your plan exactly? If it's just adding Cocoa exception handlers around all Cocoa calls (or code that calls Cocoa), that would would parallelize quite well, wouldn't it? We might be able to find resources to do that (I have people in mind).

Comment 68

[Steven Michaud [:smichaud] (Retired)]

> Steven, can you explain in this bug why Colin's suggestion in
> comment #61 won't work?

See comment #26.

> And what is your plan exactly?

I already have code (attachment 268502 [details] [diff] [review]) that (in effect) hacks
out a replacement for Colin's strategy (one that works, in the
tests I've done so far).  The appshell code has changed since I
wrote the patch, so I'll have to accomodate those changes.

I've also since realized that there are some events that come in
via "handlers" like [ChildView mouseDown:] that my hacked
top-level exception handler doesn't cover.  I'll need to make it
cover those, too.

My general strategy is to wrap my top-level exception handler
around _all_ code that might possibly send events to Gecko (and
thereby invoke XPCOM code).  To make this feasible, I need to
install the "wrappers" at as low a level (in the call stack) as
possible.  But I think the plan I outlined above should cover it.

Let me know if you think I've missed anything.

Comment 69

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

Seems like it would be a lot less hacky to just wrap Cocoa exception catchers around all our Cocoa calls.

Comment 70

[Steven Michaud [:smichaud] (Retired)]

> Seems like it would be a lot less hacky to just wrap Cocoa
> exception catchers around all our Cocoa calls.

This probably won't work ... for the same reasons a top-level
uncaught exception handler didn't work (comment #26).

But maybe I've misunderstood you.  Please be more specific.

Comment 71

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

I mean a regular try/catch handler, not a generic exception-raise hook.

Comment 72

[Steven Michaud [:smichaud] (Retired)]

> I mean a regular try/catch handler, not a generic
> exception-raise hook.

Both have the same problem -- if they're set too low in the stack
frame, exceptions don't reach them (without my hacks, that is).

This is definitely true of the @try/@catch blocks that are in my
current patch (attachment 268502 [details] [diff] [review]), and will probably be true of
any @try/@catch blocks installed around "event handlers" like
[ChildView mouseDown:].

Comment 73

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

What do you mean by "low"?

If every call from C++ into Cocoa is wrapped in a @try/@catch, then I don't see how exceptions can propagate from Cocoa into C++.

Comment 74

[Steven Michaud [:smichaud] (Retired)]

> What do you mean by "low"?

I'm not entirely sure ... aside from the examples I've already given.

> If every call from C++ into Cocoa is wrapped in a @try/@catch,
> then I don't see how exceptions can propagate from Cocoa into
> C++.

Actually, the problem is that the exceptions often _won't_
propagate to the @try/@catch blocks we install -- instead
they'll "jump around" our @try/@catch blocks (without my hacks).
I've already seen this in my tests.

This is surely an Apple bug ... but just as surely it's not going
to get fixed anytime soon, at least on OS X 10.4.X.  (Note that
all my tests were done in OS X 10.4.X -- the situation may be
somewhat different in OS X 10.5.X.)

And in any case I don't think it's reasonable to try to do this
without Oink.

Comment 75

[(dormant account)]

> And in any case I don't think it's reasonable to try to do this
> without Oink.
> 

We hook into GCC for C++ analysis now. Should be straightforward to extend that to ObjC++. Can you write a spec of what you need?

Comment 76

[David Mandelin [:dmandelin]]

Is this an accurate summary:

We would like to have one piece of code that catches all ObjC++ exceptions, logs a stack trace, and crashes--either via NSUncaughtExceptionHandler, or a @try block in the equivalent of 'main'. This won't work because something in the system Framework that is called between the top level and the place where the exceptions are generated is catching the exceptions first. For example,

  main calls ... calls FrameworkFoo ... calls XPCOMBar ... calls CocoaBaz

  Then, CocoaBaz throws. So XPCOMBar is unwound without dtors, which is Bad. Then FrameworkFoo catches the exception, so we never get to see it in either main or in NSUncaughtExceptionHandler.


If I've got it right, then in order to Not Do Bad Things, we must have try blocks of some sort in the XPCOMBar part along every call graph path that starts at a framework function that catches exceptions, passes through XPCOM code, and then into a framework function that can throw an exception.

Solution 1 (Steven's) is to find all the entry points into XPCOM code from Framework/ObjC++ and wrap them in a @try block (which for now will just terminate the program). 

Solution 2, also discussed, is to find all the entry points into Framework/ObjC++ from XPCOM and wrap those in @try blocks. (Presumably this means Framework methods can only be called directly from Mozilla wrapper methods written with @try blocks in ObjC++.) 

Short-term, I guess all that matters is which set of entry points is smaller and easier to identify, which someone must know but I would guess is Steven's. By the way, is there any chance of using dtrace to identify the first Moz function called after a @try is entered? 

Medium-term, it sounds like at least some exceptions can be caught and handled without terminating. Are there many? It sounds like there aren't that many, in which case they can be identified through testing and then have wrappers with exception handlers written manually.

Long-term, if we get Moz onto C++ exceptions, then we might arrange it so that when an ObjC++ exception is thrown, a C++ exception is thrown once it gets to the C++ boundary. (This confuses me: can an ObjC++ @catch do a C++ throw? How would this be done?) In that case, the program would crash, or the exception could be caught and handled if the intervening XPCOM code is made exception safe.

Comment 77

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #75)

Taras, we went through this all before :-)

Starting around comment #30 and continuing through comment #57.
At comment #45 you said the work wouldn't be done by the Firefox
3 release.  I gave you a spec in comment #56 and you responded in
comment #57.

I still think my spec is reasonable ... but (like I've already
said), you could do everything in my spec and we might still have
problems (we still might have Cocoa exceptions "jumping around"
our @try/@catch blocks).

Comment 78

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #76)

David, I think your summary is basically accurate and touches on
all the important points.

> then in order to Not Do Bad Things, we must have try blocks of
> some sort in the XPCOMBar part along every call graph path that
> starts at a framework function that catches exceptions, passes
> through XPCOM code, and then into a framework function that can
> throw an exception.

Yes, basically.  But I want to emphasize that my patch is a way
around this (a way to do it "virtually").  With my strategy (as
best I can tell), all you need to do is wrap Gecko event
loop(s) (on the main thread) and Cocoa "event handlers" like
[ChildView mouseDown:].

Comment 79

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

(In reply to comment #74)
> Actually, the problem is that the exceptions often _won't_
> propagate to the @try/@catch blocks we install -- instead
> they'll "jump around" our @try/@catch blocks (without my hacks).
> I've already seen this in my tests.

I'm confused. That seems like a fundamental bug that must kill every application that mixes C++ with Cocoa.

Comment 80

[Simon Fraser [no longer active]]

(In reply to comment #79)
> (In reply to comment #74)
> > Actually, the problem is that the exceptions often _won't_
> > propagate to the @try/@catch blocks we install -- instead
> > they'll "jump around" our @try/@catch blocks (without my hacks).
> > I've already seen this in my tests.
> 
> I'm confused. That seems like a fundamental bug that must kill every
> application that mixes C++ with Cocoa.

This is the approach I've advocated before, and I think the correct one. I don't understand the ""jump around" our @try/@catch blocks" comment; the leaf-most handler on the stack will catch an exception, and at that point the method should just return NS_FAILURE.

WebKit has this exception handling strategy, so it has to work reasonably well. It's tedious, but easy to find all the callers into Obj-C (you only have to look at .mm files).

Comment 81

[Josh Aas]

Attachment: fix v0.1, wrapping macros

This is the header file for the fix I plan on submitting. Posting it now to get feedback while I'm working on implementing this plan in all of the files that need attention.

For Obj-C use *outside* of Cocoa widgets, I plan to wrap all Cocoa calls that might throw exceptions using:

BEGIN_BLOCK_OBJC_EXCEPTIONS
END_BLOCK_OBJC_EXCEPTIONS

Because these macros target particular Obj-C calls that could be in any method/function, we don't have a consistent way to signal an error condition in the exception handler. Because of this we crash the app when these macros handle an exception.

Since there are huge numbers of Obj-C calls that might throw exceptions *within* Cocoa widgets, targeting them with the above macros would be a huge amount of work and would add too much clutter. I plan to use a different strategy - meet:

BEGIN_IMETHOD_BLOCK_OBJC_EXCEPTIONS
END_IMETHOD_BLOCK_OBJC_EXCEPTIONS

These macros are made to wrap entire NS_IMETHODIMP methods. These will wrap every XPCOM interface method in Cocoa widgets to stop Obj-C exceptions from propagating past XPCOM boundaries. We must wrap every XPCOM interface method because Obj-C calls that throw could be indirect, via some other function/method that is not Obj-C. Note that the exception handler for these macros does not crash the app - since XPCOM interface methods have a consistent mechanism for signaling an error condition we use that instead (return NS_ERROR_FAILURE).

I'd like feedback on this approach as quickly as possible as it is a lot of work to implement and I'd rather not find out we have a problem after doing too much of it. In particular, I'd like feedback on the following issues to make sure I'm on the right track:

- Can we really get away with returning NS_ERROR_FAILURE for the NS_IMETHODIMP wrapping macros instead of crashing? Is this somehow problematic enough that we should just crash instead?
- If an Obj-C exception handler wants to crash the app, is raise("SIGINT") the best way to do so?
- Do I need to wrap constructors/destructors for objects that implement XPCOM interfaces? How about copy constructors?
- Does this approach cover all cases where we might have problematic stack unwinding?

Comment 82

[(not currently active) Ted Mielczarek]

Josh: over in bug 381049 aaronlev is adding a method to nsICrashReporter to pass a Win32 exception (from Win32 SEH) to Breakpad.  Some variation on that might be useful here.

Perhaps mento can comment?

Comment 83

[Simon Fraser [no longer active]]

(In reply to comment #81)

> Since there are huge numbers of Obj-C calls that might throw exceptions
> *within* Cocoa widgets, targeting them with the above macros would be a huge
> amount of work and would add too much clutter. I plan to use a different
> strategy - meet:
> 
> BEGIN_IMETHOD_BLOCK_OBJC_EXCEPTIONS
> END_IMETHOD_BLOCK_OBJC_EXCEPTIONS
> 
> These macros are made to wrap entire NS_IMETHODIMP methods.

What do you mean? Will you make a NS_IMETHOD_COCOA_IMP that puts one of these
at the start, and one at the end of the method body? Or still manually paste
these in to every widget method that calls Cocoa?

> These will wrap
> every XPCOM interface method in Cocoa widgets to stop Obj-C exceptions from
> propagating past XPCOM boundaries. We must wrap every XPCOM interface method
> because Obj-C calls that throw could be indirect, via some other
> function/method that is not Obj-C.

Do you mean in Mozilla code, or framework code? If in mozilla code, you need to
insert these macros into the leaf-most function that calls Cocoa (there is no
"indirect" in this case, since you control the code).

> Note that the exception handler for these
> macros does not crash the app - since XPCOM interface methods have a consistent
> mechanism for signaling an error condition we use that instead (return
> NS_ERROR_FAILURE).

I'm slightly concerned about the early return being buried in the macro. I
think you need to make this very explicit in the macro name (facetious
suggestion: END_IMETHOD_BLOCK_OBJC_EXCEPTIONS_AND_RETURN_FAILURE).

Comment 84

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

> Because these macros target particular Obj-C calls that could be in any
> method/function, we don't have a consistent way to signal an error condition in
> the exception handler. Because of this we crash the app when these macros
> handle an exception.

We don't have to crash, we could handle the exception any way we want. Just like if we called a C or C++ platform API and got an error result.

I don't think it's safe to just put END_IMETHOD_BLOCK_OBJC_EXCEPTIONS at the end of XPCOM methods. The methods could have C++ local objects in the scope of the @try whose destructors won't be called, right?

So how about wrapping 

#define NS_OBJC_TRY(_e, _fail)                    \
  @try { _e; }                                    \
  @catch(NSException *_exn) {                     \
    NSLog(@"%@: %@", [_exn name], [_exn reason]); \
    _fail;                                        \
  }

#define NS_OBJC_TRY_EXPR(_e, _fail)                  \
  ({ typeof(_e) _tmp;                                \
     @try { _tmp = (_e); }                           \
     @catch(NSException *_exn) {                     \
       NSLog(@"%@: %@", [_exn name], [_exn reason]); \
       _fail;                                        \
     }                                               \
     _tmp; })

#define NS_OBJC_TRY_ERROR_FAILURE(_e) \
                NS_OBJC_TRY(_e, return NS_ERROR_FAILURE)
#define NS_OBJC_TRY_EXPR_ERROR_FAILURE(_e) \
                NS_OBJC_TRY_EXPR(_e, return NS_ERROR_FAILURE)
#define NS_OBJC_TRY_EXPR_NULL(_e) \
                NS_OBJC_TRY_EXPR(_e, _tmp = 0)
#define NS_OBJC_TRY_IGNORE(_e) \
                NS_OBJC_TRY(_e, )
#define NS_OBJC_TRY_ABORT(_e) \
                NS_OBJC_TRY(_e, abort())
#define NS_OBJC_TRY_EXPR_ABORT(_e) \
                NS_OBJC_TRY_EXPR(_e, abort())

Then you can write

  NSButtonCell* cell = NS_OBJC_TRY_EXPR_NULL([NSButtonCell alloc]);

or

  NSButtonCell* cell = NS_OBJC_TRY_EXPR_ABORT([NSButtonCell alloc]);

or

  NSButtonCell* cell = NS_OBJC_TRY_EXPR_ERROR_FAILURE([NSButtonCell alloc]);

or

  NS_OBJC_TRY_IGNORE([cell setButtonType:NSMomentaryPushInButton]);

or

  NS_OBJC_TRY_ERROR_FAILURE([cell setButtonType:NSMomentaryPushInButton]);

You'd still have to be careful not to construct C++ temporaries in method call parameters, at least for classes with nontrivial destructors.

Comment 85

[Josh Aas]

Attachment: fix v0.2

I am abandoning the idea of the NS_IMETHODIMP wrapping. This patch has a new header file based on the macros that roc proposed, and an example its use in the Çocoa appshell.

I wonder if we should still have a block abort macro that could wrap blocks of Obj-C calls to cut down on the number of wrappings, which could only be used for blocks that do not contain local C++ objects that would need their destructors run.

Here is what WebKit does:

http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/platform/mac/BlockExceptions.h

And an example of its use:

http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/platform/network/mac/ResourceHandleMac.mm

Feedback appreciated.

Comment 86

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

It would probably be good if you just called a helper function to do the logging, passing in the exception. That will save some code size.

I see that Webkit is just swallowing the exceptions. We should feel free to do that too. A block swallowing or abort macro could be useful, but we'll have to be vigilant to make sure C++ temporaries or variables don't creep in.

Comment 87

[Stan Shebs]

Just to add a little FUD, it's not necessarily the case that the only uses of Cocoa are via obvious ObjC method calls; in theory any ordinary system/library calls (such as open() ) could wander down into Cocoa code at some point. Although in general Apple tries to segregate by levels, so Cocoa calls on Unix-level functionality but not vice versa, it's not an official rule. Exception machinery is defined in the Foundation framework, so any frameworks using that could be involved. I'm still thinking about ways to identify any such frameworks.

Comment 88

[Stan Shebs]

OK, not just FUD, check this out:

http://lists.apple.com/archives/Carbon-dev/2007/Dec/msg00053.html

A statement from one of the big bosses of Cocoa, explicitly saying that CoreFoundation (which is a plain C API) is allowed to throw Cocoa exceptions. So we can get closer to safety by wrapping method calls, but we can't be complete without doing everything that comes from frameworks. Posix calls and Mach message stuff should be safe though, they are designed to not need ObjC at all, and you can verify this by being able to link an executable without needing to mention any frameworks.

Comment 89

[Stan Shebs]

Being more systematic about this, I went through all of the dozen-odd frameworks found in mxr: http://mxr.mozilla.org/firefox/search?string=-framework&find=&findi=&filter=&tree=firefox

All of them ultimately load Foundation, which is where NSException lives. Even unlikely-seeming frameworks such as IOKit pull in CoreFoundation, which as I noted in the previous comment, can throw exceptions even on simple CFString operations. On the plus side, the System framework (aka C and Unix library) does not make any reference to other frameworks, and so cannot cause a Cocoa exception to be thrown.  Every other OS X call will need to be wrapped, no matter what source language is in use.

Comment 90

[Stan Shebs]

Attachment: list of framework functions called from XUL

Here is a list of the 600-odd functions that the XUL dylib wants to get from frameworks that can potentially throw Cocoa exceptions. This is not a complete list, but is by far the largest of the dylibs of interest. For complete reliability, all of these will need to be wrapped somehow. Not clear how to wrap an ObjC try block around a call in C/C++ code, but without it, the throw will pass up through our call stack until it gets to the NSApplication run.

Comment 91

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

This is ridiculous --- basically no-one should use frameworks from C or C++ code?

Comment 92

[Stan Shebs]

   basically no-one should use frameworks from C or C++ code?

Yes, at least if you care about the possibility of a Cocoa exception opening a security hole. As you can see from the mail exchange I cite, the Adobe guys aren't too happy about it either, and Ali Ozer's defense is basically that in those situations the app is going down in flames anyway - which is only true if Apple's jillion lines of framework code is bug-free, but I think it's irresponsible to count on that.

Comment 93

[Josh Aas]

Attachment: nspr header, probjc.h, v1.0

I am proposing that we add this header to nspr to provide macros for handling Obj-C exceptions.

Comment 94

[Josh Aas]

Attachment: fix v1.0

In conjunction with the nspr header I just attached (which would reside in nsprpub/pr/include), this is how I propose we tackle the problem.

The macros are set up in such a way that we can easily wrap entire functions, which speeds up the process of applying the macros by quite a bit. The new file in xpcom/base is an extension of probjc.h which includes a macro tailored to methods that return the xpcom nsresult type.

Comment 95

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Need to remember to see if breakpad catches these abort()'s.

Comment 96

[(not currently active) Ted Mielczarek]

It doesn't.

Comment 97

[Stan Shebs]

Comment on attachment 302750 [details]
nspr header, probjc.h, v1.0

This is OK, with the addition of a comment mentioning two caveats: 1) this cannot be included in a plain C or C++ source file (there's a macro to test, I'm looking it up), and 2) the compiler must be passed -fobjc-exceptions, which currently only happens in widget/src/cocoa. (We could pass -fobjc-exceptions globally I think, need to check on that.)

Comment 98

[Stan Shebs]

Comment on attachment 302751 [details] [diff] [review]
fix v1.0

Same caveats for nsObjCExceptions.h as for probjc.h.

Comment 99

[Stan Shebs]

The test for ObjC code is #ifdef __OBJC__ .

Comment 100

[Brendan Eich [:brendan]]

Echoing comment 91 and agog at stuff cited in comment 92. Cc'ing wtc since NSPR is involved (I suppose Mac can do what it needs there, but module ownership must mean something here).

/be

Comment 101

[Josh Aas]

Comment on attachment 302751 [details] [diff] [review]
fix v1.0

Thanks Stan.

The only change we might need on checkin is a comment in the nspr file about only including in obj-c files.

As for -fobjc-exceptions we can fix that as needed. I know most things we care about already use it, we can make that global if necessary.

Comment 102

[Wan-Teh Chang]

Josh, why don't you just have xpcom/base/nsObjCExceptions.h?  Why do you need
a separate probj.h header, which is not used by any of the components below xpcom,
including NSPR itself?  It seems that given the current needs, xpcom is the right layer
in which to define these macros.

Comment 103

[Stan Shebs]

NSPR currently uses these Mac functions from frameworks that can throw Cocoa exceptions:

_CFBundleCreate
_CFBundleGetFunctionPointerForName
_CFRelease
_CFStringCreateWithCString
_CFURLCreateWithFileSystemPath
_CloseConnection
_FindSymbol
_c2pstrcpy

So we're going to have to do some wrapping there too eventually.

Comment 104

[Josh Aas]

And outside of nspr, I am pretty sure we have some components that don't use xpcom but do use nspr. I forget what they are though.

Comment 105

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

So how exactly are we going to handle C/C++ code that calls unsafe functions? Convert it all to .mm files? That sounds really painful, especially if there are files that are used on other platforms as well (maybe #ifdefed Mac code). I suppose we could go with the wrapper approach where we define one .mm file with exception-catching wrappers around the C functions we need.

Comment 106

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

Maybe instead of abort() we should do something that actually triggers breakpad? Something hacky like "*(int*)0 = 0;"...

Comment 107

[Stan Shebs]

We can always tweak rules.mk to pass -x objective-c++ to .cpp files, they'll be handled as if they were .mm . This is kind of risky, because ObjC does add to the namespace ("id" for instance), but I'll give it a try, see what complains. A dedicated .mm wrapping C/C++ functions is safer, albeit slower.

Comment 108

[Josh Aas]

Attachment: nspr header, probjc.h, v1.1

This updated header has 3 improvements:
1. Crashes with a mach-o signal, makes Mozilla's crash reporter on Mac OS X work
2. Modified macros to minimize code size.
3. Improved comments

Comment 109

[Josh Aas]

Attachment: fix v1.1

Incorporates nspr header fix v1.1.

Comment 110

[Wan-Teh Chang]

Comment on attachment 303062 [details] [diff] [review]
nspr header, probjc.h, v1.1

Josh, please rename these macros with the NS_ prefix and move them to
xpcom/base/nsObjCExceptions.h.  I don't want to block your work while
I study this issue.  Thanks.

Comment 111

[Josh Aas]

Attachment: fix v2.0

No additions to nspr for now, this should get us started. Aside from nspr->xpcom, more comment updates.

This will allow us to get the bulk of the work done, we can produce strategies for the areas that don't use xpcom later.

Comment 112

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

Why do we need #ifdef XP_MACOSX?

+#define NS_OBJC_TRY_EXPR_NULL(_e)                  \
+NS_OBJC_TRY_EXPR(_e, _tmp = 0)

"0", not "_tmp = 0"

Other than that, looks good. I mean, it looks crap, but that's Apple's fault.

Comment 113

[Josh Aas]

Attachment: fix v2.1

Comment 114

[Josh Aas]

fix v2.1 landed on trunk

Comment 115

[Stan Shebs]

As a random data point, on my Tiger system, when you ask to print a page, some low-level part of the printing framework throws an NSException that is caught within the framework, and eventually the usual print dialog comes up. The code that does this is under PMSessionPrintDialog, which we call from embedding/components/printingui/src/mac/nsPrintingPromptServiceX.cpp . In practice, exceptions don't seem to be thrown often, took me a while to find this one example of a "normal" intra-framework throw.

Comment 116

[Stan Shebs]

Here's a charming little bit from /usr/include/c++/4.0.0/exception_defines.h :

// Iff -fno-exceptions, transform error handling code to work without it.
# define try      if (true)
# define catch(X) if (false)

You can imagine the puzzling effect it has on the @try/@catch sequences! (We had to make '@' its own separate token in GCC for reasons that I've forgotten due to protective amnesia, ha ha.) This came up in gfx/thebes/src/gfxQuartzFontCache.mm, which is compiled with -fno-exceptions. #undef at the top of the source file is a workaround for the moment.

Comment 117

[Stan Shebs]

Attachment: rules.mk tweak to pass -fobjc-exceptions everywhere

Here is a preliminary version of the patch to rules.mk that will pass -fobjc-exceptions to all ObjC/ObjC++ files. We'll need something like this before doing large-scale checkins outside of widget/src/cocoa.

Comment 118

[Stan Shebs]

Attachment: Better -fobjc-exceptions patch

OK, here is the review-worthy version. It should probably get review from a build person like bsmedberg, although he's gone until next Tuesday or so.

Comment 119

[Stan Shebs]

Forgot to mention, I tested this patch by building on both Linux and Mac, so I'm confident it's properly platform-agnostic. (If other platforms start using ObjC, we will have to add the more elaborate form of flags support, using configure etc.)

Comment 120

[Shawn Wilsher :sdwilsh]

If you are rushed to get this in, I think that ted can do build reviews as well.

Comment 121

[Stan Shebs]

OK, the -x objective-c++ idea doesn't work, there are just too many places where "id" is used as a local variable. An unfortunate implication is that files renamed to .mm may need some editing to become ObjC++ compatible, although renaming locals is not that big of a problem.

Comment 122

[Stan Shebs]

Attachment: Patch to use for testing

Here's a little aid for testing; a function vomit() that does nothing but throw, callable from GDB. Install this patch, then run under GDB and stop at some point you think is properly wrapped. Then call vomit() directly and see what happens. Here's what I get for instance from stopping inside a wrapped routine in the OS X-specific spell checker bits:

Breakpoint 5, mozOSXSpell::Check (this=0x220d70e0, aWord=0xbfffe078, aResult=0xbfffdf5c) at /Users/shebs/s/mozilla/extensions/spellcheck/osxspell/src/mozOSXSpell.mm:218
218	  *aResult = PR_FALSE;
(gdb) call vomit()
2008-02-15 18:23:30.563 firefox-bin[52448:813] deliberate throw: for testing

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
nsObjCExceptionLogAbort (e=0x186df040) at /Users/shebs/s/mozilla/extensions/spellcheck/osxspell/src/mozOSXSpell.mm:69
69	  nsObjCExceptionAbort();
The program being debugged was signaled while in a function called from GDB.
GDB remains in the frame where the signal was received.
To change this behavior use "set unwindonsignal on"
Evaluation of the expression containing the function (vomit()) will be abandoned.
(gdb) whe
#0  nsObjCExceptionLogAbort (e=0x186df040) at /Users/shebs/s/mozilla/extensions/spellcheck/osxspell/src/mozOSXSpell.mm:69
#1  0x221f8ba8 in mozOSXSpell::Check (this=0x220d70e0, aWord=0xbfffe078, aResult=0xbfffdf5c) at /Users/shebs/s/mozilla/extensions/spellcheck/osxspell/src/mozOSXSpell.mm:69
#2  0x221e05c3 in mozSpellChecker::CheckWord (this=0x186dd1d0, aWord=@0xbfffdfb4, aIsMisspelled=0xbfffe110, aSuggestions=0x0) at /Users/shebs/s/mozilla/extensions/spellcheck/osxspell/src/mozOSXSpell.mm:69
[...]

The result of the call is going to get the abort-causing exception from the try macros, then you can look at the backtrace and see if you've stopped in my wrapped function. If the backtrace just showed nsAppShell::Run above nsObjCExceptionLogAbort, then most likely it's thrown through some xpcom layers, which is bad (nsAppShell::Run is wrapped now, so it will tend to catch exceptions not otherwise trapped).

Comment 123

[(not currently active) Ted Mielczarek]

Comment on attachment 303437 [details] [diff] [review]
Better -fobjc-exceptions patch

>Index: config/rules.mk
>===================================================================

>+# Darwin doesn't cross-compile, so just set both types of flags here.
>+HOST_OBJCFLAGS += -fobjc-exceptions
>+HOST_OBJCXXFLAGS += -fobjc-exceptions
>+COMPILE_OBJCFLAGS += -fobjc-exceptions
>+COMPILE_OBJCXXFLAGS += -fobjc-exceptions
> endif

These should go in config.mk instead.  See the declaration of COMPILE_CFLAGS/CXXFLAGS:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/config/config.mk&rev=3.382#587

I don't think you need to bother with the HOST_ versions, the only things that get built with that are build system tools.  Also, I think you should probably name these COMPILE_CMFLAGS/CMMFLAGS for consistency with CMSRCS/CMMSRCS.

r=me with those changes.

Comment 124

[Stan Shebs]

Attachment: Still better -fobjc-exceptions patch

I made the requested changes and retested. I kept the HOST_ flags, because if by some chance somebody builds a Darwin-only host tool (like maybe for breakpad), and uses any one of what is likely to be hundreds of wrapped files, the tree will burn. No reason to have an open snakepit in the middle of the living room floor. :-)

Comment 125

[(not currently active) Ted Mielczarek]

Comment on attachment 304621 [details] [diff] [review]
Still better -fobjc-exceptions patch

>+	@echo "COMPILE_CMFLAGS    = $(COMPILE_CMFLAGS)"
>+	@echo "COMPILE_CMMFLAGS    = $(COMPILE_CMMFLAGS)"

nit: make those line up.

Comment 126

[Josh Aas]

landed "Still better -fobjc-exceptions patch" on trunk

Comment 127

[Window Snyder]

What is this bug waiting for?  Please update status.

Comment 128

[Josh Aas]

Once we fix bug 417560 we can close this out as far as I'm concerned. I'm working on that bug, but it is a tricky one and I'm waiting for answers on something. I'll probably close it out this week.

Comment 129

[Josh Aas]

I don't think this needs to block any longer. We have pretty good coverage on the issue, followup can be wanted1.9.0.x.

Comment 130

[Brandon Sterne (:bsterne)]

I've pinged the owners of the bugs blocking this for status updates.

Comment 131

[Jesse Ruderman]

Is what's left here still potentially sg:critical?  Does a missing macro cause the stack to unwind unsafely with the exception never being noticed?

Comment 132

[Josh Aas]

I don't think there is anything left that should be sg:critical. If we don't have a macro guard somewhere and obj-c exception gets thrown, the results are undefined. We might crash, we might be fine, we might have stack corruption but still be up and running, all depends on exactly what the context is.

Comment 133

[Jesse Ruderman]

And bug 417560 means gfx/ is missing those guards, right?

Comment 134

[Josh Aas]

Yes, however at least last time I checked (April?), gfx doesn't actually use Obj-C. While it is technically still possible for a normal C call into Apple's frameworks to throw an Obj-C exception, it is really really unlikely. Especially for graphics-related things (CG).

Comment 135

[John Daggett (:jtd)]

(In reply to comment #134)
> Yes, however at least last time I checked (April?), gfx doesn't actually use
> Obj-C. 

That's not quite true, the code in gfxQuartzFontCache.mm definitely uses Obj-C, we use Cocoa font API's to set up our list of available fonts.  Most of this occurs at startup.

Comment 136

[Markus Stange [:mstange]]

A patch landed for this bug in February 2008. Let's close it. We can do further work in other bugs.
https://hg.mozilla.org/mozilla-central/rev/d5fa1f6b2bc925f6e1ba7644a04cce24cf63ab17