Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7038010:
[Mac] Allow NSExceptions in certain cases. (Closed)
|
Created:
9 years, 7 months ago by Scott Hess - ex-Googler Modified:
9 years, 6 months ago CC:
chromium-reviews, pam+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] Allow NSExceptions in certain cases.
Thirdy-party print drivers seem to be a source of NSExceptions which
Chromium will never be able to fix. ScopedNSExceptionEnabler causes
the code which makes throwing an NSException fatal to allow throws.
The flag will be reset in -reportException: in most cases.
For now, allow exceptions to be thrown for -selectPDE: (bug 80686) and
PrintingContextMac::AskUserForSettings() (bug 82589).
BUG=80686, 82589
TEST=Monitor crash server.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86503
Patch Set 1 #
Total comments: 11
Patch Set 2 : Oops, sense of test was inverted. #Patch Set 3 : Shift scoped allow closer to super call. #Patch Set 4 : Local autorelease pool to force exceptions into the right handling context. #
Total comments: 4
Patch Set 5 : Comment and order cleanup for Mark. #Patch Set 6 : Move crash-key scoper back by crash-key setup code. #
Total comments: 13
Patch Set 7 : Mark is a harsh (but fair) taskmaster. #
Total comments: 2
Patch Set 8 : Use scoped_ptr for optional exception enabling. #Patch Set 9 : Oh, clang, you crazy fox. #
Messages
Total messages: 19 (0 generated)
I'm not super enthusiastic about this, but it seems likely that we'll just have to gradually whitelist specific areas, and this makes that feasible, so...
http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:374: base::mac::SetNSExceptionsAllowed(false); Given that you've designed these to nest arbitrarily, how do you know this will do the right thing? Shouldn't we instead catch (and rethrow if we want to) when we use a C++ scoped object to manage ObjC exceptions? http://codereview.chromium.org/7038010/diff/1/printing/printing_context_mac.mm File printing/printing_context_mac.mm (right): http://codereview.chromium.org/7038010/diff/1/printing/printing_context_mac.m... printing/printing_context_mac.mm:41: // This will allow exceptions to be raised, but does not handle them. I don't understand. Why would we *deliberately* allow exceptions to blow through C++ stacks, when we know that will cause weird crashes we won't understand? This seems like the worst possible behavior.
I have a couple extra things to say, but Stuart seems to be asking the right kinds of questions. http://codereview.chromium.org/7038010/diff/1/base/mac/scoped_nsexception_ena... File base/mac/scoped_nsexception_enabler.h (right): http://codereview.chromium.org/7038010/diff/1/base/mac/scoped_nsexception_ena... base/mac/scoped_nsexception_enabler.h:32: ScopedNSExceptionEnabler(bool enable); Mark this “explicit.” http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:98: if (fatal && allow) { If the exception is “fatal” because it matched something above, but “allow” is true because we’re inside a scoper on this thread, then…LOG(FATAL)? Is that right? http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:321: base::mac::ScopedNSExceptionEnabler enabler(enableNSExceptions); I’d prefer to get this as close to [super sendAction:to:from:] as possible. http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:374: base::mac::SetNSExceptionsAllowed(false); false instead of the original “was” value? (which would be hard to access at this point?)
http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:98: if (fatal && allow) { On 2011/05/17 19:07:34, Mark Mentovai wrote: > If the exception is “fatal” because it matched something above, but “allow” is > true because we’re inside a scoper on this thread, then…LOG(FATAL)? > > Is that right? Crap, did I get that backwards? I had another CL with added code to force exceptions at various points, and it all seemed right, but it's possible in clean-up I tweaked something so that I wasn't testing the cases I needed to test. http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:374: base::mac::SetNSExceptionsAllowed(false); On 2011/05/17 18:52:14, stuartmorgan wrote: > Given that you've designed these to nest arbitrarily, how do you know this will > do the right thing? Shouldn't we instead catch (and rethrow if we want to) when > we use a C++ scoped object to manage ObjC exceptions? In the 32-bit runtime, you cannot use C++ scoped objects to manage ObjC exceptions. This specific case is when the exception was caught by the exception handler AppKit wraps around the outermost event loop. If the flag is currently set, it's because the exception blew past all the scopers. Basically it's the same as the -clearIsHandlingSendEvent call just above. http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:374: base::mac::SetNSExceptionsAllowed(false); On 2011/05/17 19:07:34, Mark Mentovai wrote: > false instead of the original “was” value? > > (which would be hard to access at this point?) This is effectively the outermost scope. http://codereview.chromium.org/7038010/diff/1/printing/printing_context_mac.mm File printing/printing_context_mac.mm (right): http://codereview.chromium.org/7038010/diff/1/printing/printing_context_mac.m... printing/printing_context_mac.mm:41: // This will allow exceptions to be raised, but does not handle them. On 2011/05/17 18:52:14, stuartmorgan wrote: > I don't understand. Why would we *deliberately* allow exceptions to blow through > C++ stacks, when we know that will cause weird crashes we won't understand? This > seems like the worst possible behavior. The primary benefit of making NSExceptions fatal is that it gives us fast turnaround when _we_ write bad code. My short-term goal is to get as many exceptions fatal as possible for as much of the code as possible. As my TODO indicates, it's not entirely clear to me how we can generically deal with the issue in this case, but it is pretty clear that the printing subsystem is one area where we see exceptions that we aren't (apparently) at fault for. If the sub-system is handling them correctly, then this change is a reasonable change. If not, we'll still need to track down why they are being raised and what a reasonable way to address it is (I'm sure that will be FUN!).
http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/1/chrome/browser/chrome_browser_a... chrome/browser/chrome_browser_application_mac.mm:98: if (fatal && allow) { On 2011/05/17 19:34:40, shess wrote: > On 2011/05/17 19:07:34, Mark Mentovai wrote: > > If the exception is “fatal” because it matched something above, but “allow” is > > true because we’re inside a scoper on this thread, then…LOG(FATAL)? > > > > Is that right? > > Crap, did I get that backwards? Indeed, my forced exceptions were all NSGenericException, which wasn't fatal. Sigh.
Per my updates on http://crbug.com/82589 , I figured out a way to replicate the problem and have verified that the print subsystem is already fielding the exceptions. With the autorelease pool change, things run cleanly, at least WRT Chrome.
http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:345: ScopedCrashKey key(kActionKey, value); Can’t this precede ScopedNSExceptionEnabler? You can still put it after the logic that sets enableNSExceptions. (And you can still put the logic that sets enableNSExceptions above the thing that figures out actionString and value, so that those can stay right next to the ScopedCrashKey for kActionKey.) http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:374: // future exceptions are not masked. Can you at least say here that stuff will have unwound past the outermost ScopedNSExceptionEnabler?
http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:345: ScopedCrashKey key(kActionKey, value); On 2011/05/17 22:23:27, Mark Mentovai wrote: > Can’t this precede ScopedNSExceptionEnabler? You can still put it after the > logic that sets enableNSExceptions. (And you can still put the logic that sets > enableNSExceptions above the thing that figures out actionString and value, so > that those can stay right next to the ScopedCrashKey for kActionKey.) I'm not entirely sure I understand what you'd like to see. One has to come first, one has to come second, but they can generally be in any old order and the additional exposure of one order versus the other is pretty minimal. The reason I pulled setting |enableNSExceptions| down by using it was just because it seemed like keeping the new code all together was somewhat valuable. I'll grant a slight win to not allowing exceptions while setting a ScopedCrashKey. http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:374: // future exceptions are not masked. On 2011/05/17 22:23:27, Mark Mentovai wrote: > Can you at least say here that stuff will have unwound past the outermost > ScopedNSExceptionEnabler? OK.
I'm saying to order it this way: 1. Logic to decide what to do with ScopedCrashKey 2. ScopedCrashKey 3. Logic to decide what to do with ScopedAllowNSExceptions 4. ScopedAllowNSExceptions 5. super but I'm also saying that if there's a good reason to do so, you can put 3 before 1. The important points are: A. ScopedAllowNSExceptions should be as close to super as possible to avoid allowing exceptions for any longer than they should be allowed. B. The logic to decide what to do with a scoper should be as close to its scoper as possible as a general matter of not doing work until you need to. This helps readability and flow, among other things. C. You can violate B for 3 and 4, moving 3 before 1, if there's a compelling reason to do it, such as not wanting the ScopedCrashKey set in case that logic proves crashy. Apologies for getting the names wrong, I'm writing this from my phone. On May 17, 2011 6:48 PM, <shess@chromium.org> wrote: > > http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... > File chrome/browser/chrome_browser_application_mac.mm (right): > > http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... > chrome/browser/chrome_browser_application_mac.mm:345: ScopedCrashKey > key(kActionKey, value); > On 2011/05/17 22:23:27, Mark Mentovai wrote: >> Can’t this precede ScopedNSExceptionEnabler? You can still put it > after the >> logic that sets enableNSExceptions. (And you can still put the logic > that sets >> enableNSExceptions above the thing that figures out actionString and > value, so >> that those can stay right next to the ScopedCrashKey for kActionKey.) > > I'm not entirely sure I understand what you'd like to see. One has to > come first, one has to come second, but they can generally be in any old > order and the additional exposure of one order versus the other is > pretty minimal. The reason I pulled setting |enableNSExceptions| down > by using it was just because it seemed like keeping the new code all > together was somewhat valuable. > > I'll grant a slight win to not allowing exceptions while setting a > ScopedCrashKey. > > http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... > chrome/browser/chrome_browser_application_mac.mm:374: // future > exceptions are not masked. > On 2011/05/17 22:23:27, Mark Mentovai wrote: >> Can you at least say here that stuff will have unwound past the > outermost >> ScopedNSExceptionEnabler? > > OK. > > http://codereview.chromium.org/7038010/
Sorry, got distracted by SQLite 3.7.6.2. Super fun. No compelling reason not to put the crash-key scoper higher - the stack backtrace would resolve any confusion. Done. -scott On 2011/05/18 00:07:26, Mark Mentovai wrote: > I'm saying to order it this way: > > 1. Logic to decide what to do with ScopedCrashKey > 2. ScopedCrashKey > 3. Logic to decide what to do with ScopedAllowNSExceptions > 4. ScopedAllowNSExceptions > 5. super > > but I'm also saying that if there's a good reason to do so, you can put 3 > before 1. The important points are: > > A. ScopedAllowNSExceptions should be as close to super as possible to avoid > allowing exceptions for any longer than they should be allowed. > B. The logic to decide what to do with a scoper should be as close to its > scoper as possible as a general matter of not doing work until you need to. > This helps readability and flow, among other things. > C. You can violate B for 3 and 4, moving 3 before 1, if there's a compelling > reason to do it, such as not wanting the ScopedCrashKey set in case that > logic proves crashy. > > Apologies for getting the names wrong, I'm writing this from my phone. > On May 17, 2011 6:48 PM, <mailto:shess@chromium.org> wrote: > > > > > http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... > > File chrome/browser/chrome_browser_application_mac.mm (right): > > > > > http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... > > chrome/browser/chrome_browser_application_mac.mm:345: ScopedCrashKey > > key(kActionKey, value); > > On 2011/05/17 22:23:27, Mark Mentovai wrote: > >> Can’t this precede ScopedNSExceptionEnabler? You can still put it > > after the > >> logic that sets enableNSExceptions. (And you can still put the logic > > that sets > >> enableNSExceptions above the thing that figures out actionString and > > value, so > >> that those can stay right next to the ScopedCrashKey for kActionKey.) > > > > I'm not entirely sure I understand what you'd like to see. One has to > > come first, one has to come second, but they can generally be in any old > > order and the additional exposure of one order versus the other is > > pretty minimal. The reason I pulled setting |enableNSExceptions| down > > by using it was just because it seemed like keeping the new code all > > together was somewhat valuable. > > > > I'll grant a slight win to not allowing exceptions while setting a > > ScopedCrashKey. > > > > > http://codereview.chromium.org/7038010/diff/1012/chrome/browser/chrome_browse... > > chrome/browser/chrome_browser_application_mac.mm:374: // future > > exceptions are not masked. > > On 2011/05/17 22:23:27, Mark Mentovai wrote: > >> Can you at least say here that stuff will have unwound past the > > outermost > >> ScopedNSExceptionEnabler? > > > > OK. > > > > http://codereview.chromium.org/7038010/
LGTM http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... File base/mac/scoped_nsexception_enabler.h (right): http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:14: // ChromeBrowserApplication attempts to restrict throwing of Did you mean BrowserCrApplication? (This one is an oddity: the class name doesn’t match the filename.) http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:20: // Make it easy to safely allow NSException to be throw in a limited throw → thrown http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:21: // scope. Note that if an exception is throw, then this object will throw → thrown (again!) http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:32: ScopedNSExceptionEnabler(bool enable); Mark this constructor as |explicit|. http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:42: // the support code in ChromeBrowserApplication, other code should use BrowserCrApplication (again!). http://codereview.chromium.org/7038010/diff/7001/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/7001/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:333: ScopedCrashKey key(kActionKey, value); Yup, seeing this here, I agree that it’s fine. http://codereview.chromium.org/7038010/diff/7001/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:341: if (anAction == @selector(selectPDE:)) { If you know something about aTarget (or possibly even sender?), you can tighten this up a bit, by calling -[aTarget isKindOfClass:[ExceptionUsingClass class]], for example.
Since you initially chose Stuart as a reviewer, make sure he’s satisfied as well. Stuart?
http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... File base/mac/scoped_nsexception_enabler.h (right): http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:14: // ChromeBrowserApplication attempts to restrict throwing of On 2011/05/24 00:05:16, Mark Mentovai wrote: > Did you mean BrowserCrApplication? (This one is an oddity: the class name > doesn’t match the filename.) Right you are. I'll send you the code review to change that. [No, not really.] http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:20: // Make it easy to safely allow NSException to be throw in a limited On 2011/05/24 00:05:16, Mark Mentovai wrote: > throw → thrown Done. http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:21: // scope. Note that if an exception is throw, then this object will On 2011/05/24 00:05:16, Mark Mentovai wrote: > throw → thrown (again!) Done. Apologies, I must have reworded this somewhere. http://codereview.chromium.org/7038010/diff/7001/base/mac/scoped_nsexception_... base/mac/scoped_nsexception_enabler.h:32: ScopedNSExceptionEnabler(bool enable); On 2011/05/24 00:05:16, Mark Mentovai wrote: > Mark this constructor as |explicit|. But then I won't be able to declare ScopedNSExceptionEnabler e = 7! Oh, OK, that's actually a good idea. http://codereview.chromium.org/7038010/diff/7001/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/7001/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:341: if (anAction == @selector(selectPDE:)) { On 2011/05/24 00:05:16, Mark Mentovai wrote: > If you know something about aTarget (or possibly even sender?), you can tighten > this up a bit, by calling -[aTarget isKindOfClass:[ExceptionUsingClass class]], > for example. This info is from the backtrace on http://crbug.com/80686 . class-dump'ing around in the module list, the method is on class PMPDEController in PrintCocoaUI.bundle in the system PrintingPrivate.framework. So I'd likely have to do a string comparison on the name, which is a little sketchy, but this is far into sketchy to start with. WTYD? ["PDE" == "Printing Dialog Extension", I'm guessing this crash happens when the driver is prompted to load up a custom dialog and is doing something wrong.]
http://codereview.chromium.org/7038010/diff/7001/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_application_mac.mm (right): http://codereview.chromium.org/7038010/diff/7001/chrome/browser/chrome_browse... chrome/browser/chrome_browser_application_mac.mm:341: if (anAction == @selector(selectPDE:)) { shess wrote: > On 2011/05/24 00:05:16, Mark Mentovai wrote: > > If you know something about aTarget (or possibly even sender?), you can > tighten > > this up a bit, by calling -[aTarget isKindOfClass:[ExceptionUsingClass > class]], > > for example. > > This info is from the backtrace on http://crbug.com/80686 . class-dump'ing > around in the module list, the method is on class PMPDEController in > PrintCocoaUI.bundle in the system PrintingPrivate.framework. So I'd likely have > to do a string comparison on the name, which is a little sketchy, but this is > far into sketchy to start with. WTYD? > > ["PDE" == "Printing Dialog Extension", I'm guessing this crash happens when the > driver is prompted to load up a custom dialog and is doing something wrong.] Blech. Since it’s private, I’ll say “we don’t really know enough about aTarget,” so just forget it.
http://codereview.chromium.org/7038010/diff/13001/base/mac/scoped_nsexception... File base/mac/scoped_nsexception_enabler.h (right): http://codereview.chromium.org/7038010/diff/13001/base/mac/scoped_nsexception... base/mac/scoped_nsexception_enabler.h:31: // |scoped_ptr<ScopedNSExceptionEnabler>|). I'm not at all convinced that scoped_ptr is more contrived than an argument that causes the whole object not to do anything. I had to read the API, then the implementation, then the API again to actually have the right understanding of what passing false does, because I assumed I misunderstood the first time. To see if this was just me I showed this header to another engineer after explaining the basic goal of this class, and then asked what false did, and the answer was "I have no idea". I understand how you got here, but I think in practice the scoped_ptr version would be more readable in the code than a "false" argument anyway, and using that would make the API much less confusing. In other words, I think this has edged passed clever and into "clever" ;)
(If you feel strongly about keeping this structure, then the bool needs to at least become an enum for readability (see the recent chromium-dev thread). Maybe YES_REALLY and THE_CAKE_IS_A_LIE)
http://codereview.chromium.org/7038010/diff/13001/base/mac/scoped_nsexception... File base/mac/scoped_nsexception_enabler.h (right): http://codereview.chromium.org/7038010/diff/13001/base/mac/scoped_nsexception... base/mac/scoped_nsexception_enabler.h:31: // |scoped_ptr<ScopedNSExceptionEnabler>|). On 2011/05/24 00:54:42, stuartmorgan wrote: > I understand how you got here, but I think in practice the scoped_ptr version > would be more readable in the code than a "false" argument anyway, and using > that would make the API much less confusing. In other words, I think this has > edged passed clever and into "clever" ;) But then it's going to leak memory every time someone throws! Done.
LGTM
Change committed as 86503 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
