From aaa47ee9d83f773d37aa4fd4a04097425ce62063 Mon Sep 17 00:00:00 2001 From: "dmaclach@chromium.org" Date: Thu, 5 Nov 2009 21:53:01 +0000 Subject: Cleans up our autorelease pool handling by making sure that an autorelease pool isn't created while the app is handling an event sent via -[NSApp sendEvent]. Branches browser/chrome_application_mac into browser/chrome_browser_application and base/chrome_application. Renderers will run as chrome_applications, and browsers will run as chrome_browser_applications. BUG=26418, 25462, 25463, 25465 TEST=1) See bug 25857. 2) Start up. Open 3+ windows. 3)Quit. See bugs for other repro cases. Review URL: http://codereview.chromium.org/345051 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@31135 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/app_controller_mac.mm | 1 - chrome/browser/browser_list.cc | 4 +- chrome/browser/browser_main_mac.mm | 13 +- chrome/browser/chrome_application_mac.h | 42 --- chrome/browser/chrome_application_mac.mm | 316 --------------------- chrome/browser/chrome_application_mac_unittest.mm | 81 ------ chrome/browser/chrome_browser_application_mac.h | 39 +++ chrome/browser/chrome_browser_application_mac.mm | 309 ++++++++++++++++++++ .../chrome_browser_application_mac_unittest.mm | 81 ++++++ chrome/browser/cocoa/cocoa_test_helper.h | 12 +- chrome/browser/cocoa/cocoa_test_helper.mm | 3 +- 11 files changed, 439 insertions(+), 462 deletions(-) delete mode 100644 chrome/browser/chrome_application_mac.h delete mode 100644 chrome/browser/chrome_application_mac.mm delete mode 100644 chrome/browser/chrome_application_mac_unittest.mm create mode 100644 chrome/browser/chrome_browser_application_mac.h create mode 100644 chrome/browser/chrome_browser_application_mac.mm create mode 100644 chrome/browser/chrome_browser_application_mac_unittest.mm (limited to 'chrome/browser') diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm index 50973f6..d24cd7a 100644 --- a/chrome/browser/app_controller_mac.mm +++ b/chrome/browser/app_controller_mac.mm @@ -16,7 +16,6 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/browser_shutdown.h" #include "chrome/browser/browser_window.h" -#import "chrome/browser/chrome_application_mac.h" #import "chrome/browser/cocoa/about_window_controller.h" #import "chrome/browser/cocoa/bookmark_menu_bridge.h" #import "chrome/browser/cocoa/browser_window_cocoa.h" diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc index dd7b9dc..f32ffdd 100644 --- a/chrome/browser/browser_list.cc +++ b/chrome/browser/browser_list.cc @@ -19,7 +19,7 @@ #include "chrome/common/result_codes.h" #if defined(OS_MACOSX) -#include "chrome/browser/chrome_application_mac.h" +#include "chrome/browser/chrome_browser_application_mac.h" #endif namespace { @@ -238,7 +238,7 @@ void BrowserList::CloseAllBrowsersAndExit() { // On the Mac, the application continues to run once all windows are closed. // Terminate will result in a CloseAllBrowsers(true) call, and additionally, // will cause the application to exit cleanly. - CrApplicationCC::Terminate(); + chrome_browser_application_mac::Terminate(); #endif } diff --git a/chrome/browser/browser_main_mac.mm b/chrome/browser/browser_main_mac.mm index ce5893b..65b6970 100644 --- a/chrome/browser/browser_main_mac.mm +++ b/chrome/browser/browser_main_mac.mm @@ -13,7 +13,7 @@ #import "chrome/app/keystone_glue.h" #import "chrome/browser/app_controller_mac.h" #include "chrome/browser/browser_main_win.h" -#import "chrome/browser/chrome_application_mac.h" +#import "chrome/browser/chrome_browser_application_mac.h" #include "chrome/browser/metrics/metrics_service.h" #include "chrome/common/main_function_params.h" #include "chrome/common/result_codes.h" @@ -28,15 +28,8 @@ namespace Platform { // MessageLoop API, which works out ok for us because it's a wrapper around // CFRunLoop. void WillInitializeMainMessageLoop(const MainFunctionParams& parameters) { - // Initialize NSApplication using the custom subclass. Check whether NSApp - // was already initialized using another class, because that would break - // some things. - [CrApplication sharedApplication]; - if (![NSApp isKindOfClass:[CrApplication class]]) { - LOG(ERROR) << "NSApp should be of type CrApplication, not " - << [[NSApp className] UTF8String]; - DCHECK(false) << "NSApp is of wrong type"; - } + // Initialize NSApplication using the custom subclass. + [BrowserCrApplication sharedApplication]; // Before we load the nib, we need to start up the resource bundle so we have // the strings avaiable for localization. diff --git a/chrome/browser/chrome_application_mac.h b/chrome/browser/chrome_application_mac.h deleted file mode 100644 index 60b6438..0000000 --- a/chrome/browser/chrome_application_mac.h +++ /dev/null @@ -1,42 +0,0 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CHROME_BROWSER_CHROME_APPLICATION_MAC_H_ -#define CHROME_BROWSER_CHROME_APPLICATION_MAC_H_ - -#ifdef __OBJC__ - -#import - -@interface CrApplication : NSApplication -@end - -// Namespace for exception-reporting helper functions. Exposed for -// testing purposes. -namespace CrApplicationNSException { - -// Bin for unknown exceptions. -extern const size_t kUnknownNSException; - -// Returns the histogram bin for |exception| if it is one we track -// specifically, or |kUnknownNSException| if unknown. -size_t BinForException(NSException* exception); - -// Use UMA to track exception occurance. -void RecordExceptionWithUma(NSException* exception); - -} // CrApplicationNSException - -#endif // __OBJC__ - -// CrApplicationCC provides access to CrApplication Objective-C selectors from -// C++ code. -namespace CrApplicationCC { - -// Calls -[NSApp terminate:]. -void Terminate(); - -} // namespace CrApplicationCC - -#endif // CHROME_BROWSER_CHROME_APPLICATION_MAC_H_ diff --git a/chrome/browser/chrome_application_mac.mm b/chrome/browser/chrome_application_mac.mm deleted file mode 100644 index 94c975b..0000000 --- a/chrome/browser/chrome_application_mac.mm +++ /dev/null @@ -1,316 +0,0 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#import "chrome/browser/chrome_application_mac.h" - -#import "base/histogram.h" -#import "base/logging.h" -#import "base/scoped_nsobject.h" -#import "base/sys_string_conversions.h" -#import "chrome/app/breakpad_mac.h" -#import "chrome/browser/cocoa/chrome_event_processing_window.h" -#import "chrome/browser/cocoa/objc_method_swizzle.h" -#import "chrome/browser/renderer_host/render_widget_host_view_mac.h" - -// The implementation of NSExceptions break various assumptions in the -// Chrome code. This category defines a replacement for -// -initWithName:reason:userInfo: for purposes of forcing a break in -// the debugger when an exception is raised. -raise sounds more -// obvious to intercept, but it doesn't catch the original throw -// because the objc runtime doesn't use it. -@interface NSException (NSExceptionSwizzle) -- (id)chromeInitWithName:(NSString *)aName - reason:(NSString *)aReason - userInfo:(NSDictionary *)someUserInfo; -@end - -static IMP gOriginalInitIMP = NULL; - -@implementation NSException (NSExceptionSwizzle) -- (id)chromeInitWithName:(NSString *)aName - reason:(NSString *)aReason - userInfo:(NSDictionary *)someUserInfo { - // Method only called when swizzled. - DCHECK(_cmd == @selector(initWithName:reason:userInfo:)); - - // Dear reader: something you just did provoked an NSException. - // Please check your backtrace and see if you can't file a bug with - // a repro case. You should be able to safely continue past the - // NOTREACHED(), but feel free to comment it out locally if it is - // making your job hard. - DLOG(ERROR) << "Someone is preparing to raise an exception! " - << base::SysNSStringToUTF8(aName) << " *** " - << base::SysNSStringToUTF8(aReason); - NOTREACHED(); - - // Forward to the original version. - return gOriginalInitIMP(self, _cmd, aName, aReason, someUserInfo); -} -@end - -namespace CrApplicationNSException { - -// Maximum number of known named exceptions we'll support. There is -// no central registration, but I only find about 75 possibilities in -// the system frameworks, and many of them are probably not -// interesting to track in aggregate (those relating to distributed -// objects, for instance). -const size_t kKnownNSExceptionCount = 25; - -const size_t kUnknownNSException = kKnownNSExceptionCount; - -size_t BinForException(NSException* exception) { - // A list of common known exceptions. The list position will - // determine where they live in the histogram, so never move them - // around, only add to the end. - static const NSString* kKnownNSExceptionNames[] = { - // ??? - NSGenericException, - - // Out-of-range on NSString or NSArray. - NSRangeException, - - // Invalid arg to method, unrecognized selector. - NSInvalidArgumentException, - - // malloc() returned null in object creation, I think. - NSMallocException, - - nil - }; - - // Make sure our array hasn't outgrown our abilities to track it. - DCHECK_LE(arraysize(kKnownNSExceptionNames), kKnownNSExceptionCount); - - const NSString* name = [exception name]; - for (int i = 0; kKnownNSExceptionNames[i]; ++i) { - if (name == kKnownNSExceptionNames[i]) { - return i; - } - } - return kUnknownNSException; -} - -void RecordExceptionWithUma(NSException* exception) { - static LinearHistogram histogram("OSX.NSException", 0, kUnknownNSException, - kUnknownNSException + 1); - histogram.SetFlags(kUmaTargetedHistogramFlag); - histogram.Add(BinForException(exception)); -} - -} // CrApplicationNSException - -namespace { - -// Helper to make it easy to get crash keys right. -// TODO(shess): Find a better home for this. app/breakpad_mac.h -// doesn't work. -class ScopedCrashKey { - public: - ScopedCrashKey(NSString* key, NSString* value) - : crash_key_([key retain]) { - SetCrashKeyValue(crash_key_.get(), value); - } - ~ScopedCrashKey() { - ClearCrashKeyValue(crash_key_.get()); - } - - private: - scoped_nsobject crash_key_; -}; - -// Do-nothing wrapper so that we can arrange to only swizzle -// -[NSException raise] when DCHECK() is turned on (as opposed to -// replicating the preprocess logic which turns DCHECK() on). -BOOL SwizzleNSExceptionInit() { - gOriginalInitIMP = ObjcEvilDoers::SwizzleImplementedInstanceMethods( - [NSException class], - @selector(initWithName:reason:userInfo:), - @selector(chromeInitWithName:reason:userInfo:)); - return YES; -} - -} // namespace - -@implementation CrApplication - -- init { - // TODO(shess): Push this somewhere where it can apply to the plugin - // and renderer processes, and where it can intercept uncaught - // exceptions. - DCHECK(SwizzleNSExceptionInit()); - return [super init]; -} - -// -terminate: is the entry point for orderly "quit" operations in Cocoa. -// This includes the application menu's quit menu item and keyboard -// equivalent, the application's dock icon menu's quit menu item, "quit" (not -// "force quit") in the Activity Monitor, and quits triggered by user logout -// and system restart and shutdown. -// -// The default NSApplication -terminate: implementation will end the process -// by calling exit(), and thus never leave the main run loop. This is -// unsuitable for Chrome's purposes. Chrome depends on leaving the main -// run loop to perform a proper orderly shutdown. This design is ingrained -// in the application and the assumptions that its code makes, and is -// entirely reasonable and works well on other platforms, but it's not -// compatible with the standard Cocoa quit sequence. Quits originated from -// within the application can be redirected to not use -terminate:, but -// quits from elsewhere cannot be. -// -// To allow the Cocoa-based Chrome to support the standard Cocoa -terminate: -// interface, and allow all quits to cause Chrome to shut down properly -// regardless of their origin, -terminate: is overriden. The custom -// -terminate: does not end the application with exit(). Instead, it simply -// returns after posting the normal NSApplicationWillTerminateNotification -// notification. The application is responsible for exiting on its own in -// whatever way it deems appropriate. In Chrome's case, the main run loop will -// end and the applicaton will exit by returning from main(). -// -// This implementation of -terminate: is scaled back and is not as -// fully-featured as the implementation in NSApplication, nor is it a direct -// drop-in replacement -terminate: in most applications. It is -// purpose-specific to Chrome. -- (void)terminate:(id)sender { - NSApplicationTerminateReply shouldTerminate = NSTerminateNow; - SEL selector = @selector(applicationShouldTerminate:); - if ([[self delegate] respondsToSelector:selector]) - shouldTerminate = [[self delegate] applicationShouldTerminate:self]; - - // If shouldTerminate is NSTerminateLater, the application is expected to - // call -replyToApplicationShouldTerminate: when it knows whether or not it - // should terminate. If the argument is YES, - // -replyToApplicationShouldTerminate: will call -terminate:. This will - // result in another call to the delegate's -applicationShouldTerminate:, - // which would be expected to return NSTerminateNow at that point. - if (shouldTerminate != NSTerminateNow) - return; - - [[NSNotificationCenter defaultCenter] - postNotificationName:NSApplicationWillTerminateNotification - object:self]; - - // Return, don't exit. The application is responsible for exiting on its - // own. -} - -- (BOOL)sendAction:(SEL)anAction to:(id)aTarget from:(id)sender { - // The Dock menu contains an automagic section where you can select - // amongst open windows. If a window is closed via JavaScript while - // the menu is up, the menu item for that window continues to exist. - // When a window is selected this method is called with the - // now-freed window as |aTarget|. Short-circuit the call if - // |aTarget| is not a valid window. - if (anAction == @selector(_selectWindow:)) { - // Not using -[NSArray containsObject:] because |aTarget| may be a - // freed object. - BOOL found = NO; - for (NSWindow* window in [self windows]) { - if (window == aTarget) { - found = YES; - break; - } - } - if (!found) { - return NO; - } - } - - // When a Cocoa control is wired to a freed object, we get crashers - // in the call to |super| with no useful information in the - // backtrace. Attempt to add some useful information. - static const NSString* kActionKey = @"sendaction"; - - // If the action is something generic like -commandDispatch:, then - // the tag is essential. - NSInteger tag = 0; - if ([sender isKindOfClass:[NSControl class]]) { - tag = [sender tag]; - if (tag == 0 || tag == -1) { - tag = [sender selectedTag]; - } - } else if ([sender isKindOfClass:[NSMenuItem class]]) { - tag = [sender tag]; - } - - NSString* actionString = NSStringFromSelector(anAction); - NSString* value = - [NSString stringWithFormat:@"%@ tag %d sending %@ to %p", - [sender className], tag, actionString, aTarget]; - - ScopedCrashKey key(kActionKey, value); - return [super sendAction:anAction to:aTarget from:sender]; -} - -- (void)sendEvent:(NSEvent*)event { - // The superclass's |sendEvent:| sends keyboard events to the menu and the key - // view loop before dispatching them to |keyDown:|. Since we want to send keys - // to the renderer before sending them to the menu, and we never want them to - // the kev view loop when the web is focussed, we change this behavior. - if ([[self keyWindow] - isKindOfClass:[ChromeEventProcessingWindow class]]) { - if ([static_cast([self keyWindow]) - shortcircuitEvent:event]) - return; - } - - [super sendEvent:event]; -} - -// NSExceptions which are caught by the event loop are logged here. -// NSException uses setjmp/longjmp, which can be very bad for C++, so -// we attempt to track and report them. -- (void)reportException:(NSException *)anException { - // If we throw an exception in this code, we can create an infinite - // loop. If we throw out of the if() without resetting - // |reportException|, we'll stop reporting exceptions for this run. - static BOOL reportingException = NO; - DCHECK(!reportingException); - if (!reportingException) { - reportingException = YES; - CrApplicationNSException::RecordExceptionWithUma(anException); - - // Store some human-readable information in breakpad keys in case - // there is a crash. Since breakpad does not provide infinite - // storage, we track two exceptions. The first exception thrown - // is tracked because it may be the one which caused the system to - // go off the rails. The last exception thrown is tracked because - // it may be the one most directly associated with the crash. - static const NSString* kFirstExceptionKey = @"firstexception"; - static BOOL trackedFirstException = NO; - static const NSString* kLastExceptionKey = @"lastexception"; - - // TODO(shess): It would be useful to post some stacktrace info - // from the exception. - // 10.6 has -[NSException callStackSymbols] - // 10.5 has -[NSException callStackReturnAddresses] - // 10.5 has backtrace_symbols(). - // I've tried to combine the latter two, but got nothing useful. - // The addresses are right, though, maybe we could train the crash - // server to decode them for us. - - NSString* value = [NSString stringWithFormat:@"%@ reason %@", - [anException name], [anException reason]]; - if (!trackedFirstException) { - SetCrashKeyValue(kFirstExceptionKey, value); - trackedFirstException = YES; - } else { - SetCrashKeyValue(kLastExceptionKey, value); - } - - reportingException = NO; - } - - [super reportException:anException]; -} - -@end - -namespace CrApplicationCC { - -void Terminate() { - [NSApp terminate:nil]; -} - -} // namespace CrApplicationCC diff --git a/chrome/browser/chrome_application_mac_unittest.mm b/chrome/browser/chrome_application_mac_unittest.mm deleted file mode 100644 index dbc1d90..0000000 --- a/chrome/browser/chrome_application_mac_unittest.mm +++ /dev/null @@ -1,81 +0,0 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#import - -#include "base/histogram.h" -#import "chrome/browser/chrome_application_mac.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace CrApplicationNSException { - -// Generate an NSException with the given name. -NSException* ExceptionNamed(NSString* name) { - return [NSException exceptionWithName:name - reason:@"No reason given" - userInfo:nil]; -} - -// Helper to keep binning expectations readible. -size_t BinForExceptionNamed(NSString* name) { - return BinForException(ExceptionNamed(name)); -} - -TEST(ChromeApplicationMacTest, ExceptionBinning) { - // These exceptions must be in this order. - EXPECT_EQ(BinForExceptionNamed(NSGenericException), 0U); - EXPECT_EQ(BinForExceptionNamed(NSRangeException), 1U); - EXPECT_EQ(BinForExceptionNamed(NSInvalidArgumentException), 2U); - EXPECT_EQ(BinForExceptionNamed(NSMallocException), 3U); - - // Random other exceptions map to |kUnknownNSException|. - EXPECT_EQ(BinForExceptionNamed(@"CustomName"), kUnknownNSException); - EXPECT_EQ(BinForExceptionNamed(@"Custom Name"), kUnknownNSException); - EXPECT_EQ(BinForExceptionNamed(@""), kUnknownNSException); - EXPECT_EQ(BinForException(nil), kUnknownNSException); -} - -TEST(ChromeApplicationMacTest, RecordException) { - // Start up a histogram recorder. - StatisticsRecorder recorder; - - StatisticsRecorder::Histograms histograms; - StatisticsRecorder::GetSnapshot("OSX.NSException", &histograms); - EXPECT_EQ(0U, histograms.size()); - - // Record some known exceptions. - RecordExceptionWithUma(ExceptionNamed(NSGenericException)); - RecordExceptionWithUma(ExceptionNamed(NSGenericException)); - RecordExceptionWithUma(ExceptionNamed(NSGenericException)); - RecordExceptionWithUma(ExceptionNamed(NSGenericException)); - RecordExceptionWithUma(ExceptionNamed(NSRangeException)); - RecordExceptionWithUma(ExceptionNamed(NSInvalidArgumentException)); - RecordExceptionWithUma(ExceptionNamed(NSInvalidArgumentException)); - RecordExceptionWithUma(ExceptionNamed(NSInvalidArgumentException)); - RecordExceptionWithUma(ExceptionNamed(NSMallocException)); - RecordExceptionWithUma(ExceptionNamed(NSMallocException)); - - // Record some unknown exceptions. - RecordExceptionWithUma(ExceptionNamed(@"CustomName")); - RecordExceptionWithUma(ExceptionNamed(@"Custom Name")); - RecordExceptionWithUma(ExceptionNamed(@"")); - RecordExceptionWithUma(nil); - - // We should have exactly the right number of exceptions. - StatisticsRecorder::GetSnapshot("OSX.NSException", &histograms); - EXPECT_EQ(1U, histograms.size()); - EXPECT_EQ(kUmaTargetedHistogramFlag, histograms[0]->flags()); - Histogram::SampleSet sample; - histograms[0]->SnapshotSample(&sample); - EXPECT_EQ(4, sample.counts(0)); - EXPECT_EQ(1, sample.counts(1)); - EXPECT_EQ(3, sample.counts(2)); - EXPECT_EQ(2, sample.counts(3)); - - // The unknown exceptions should end up in the overflow bucket. - EXPECT_EQ(kUnknownNSException + 1, histograms[0]->bucket_count()); - EXPECT_EQ(4, sample.counts(kUnknownNSException)); -} - -} // CrApplicationNSException diff --git a/chrome/browser/chrome_browser_application_mac.h b/chrome/browser/chrome_browser_application_mac.h new file mode 100644 index 0000000..9a1ab9e --- /dev/null +++ b/chrome/browser/chrome_browser_application_mac.h @@ -0,0 +1,39 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_CHROME_BROWSER_APPLICATION_MAC_H_ +#define CHROME_BROWSER_CHROME_BROWSER_APPLICATION_MAC_H_ + +#ifdef __OBJC__ + +#import "base/chrome_application_mac.h" + +@interface BrowserCrApplication : CrApplication +@end + +namespace chrome_browser_application_mac { + +// Bin for unknown exceptions. Exposed for testing purposes. +extern const size_t kUnknownNSException; + +// Returns the histogram bin for |exception| if it is one we track +// specifically, or |kUnknownNSException| if unknown. Exposed for testing +// purposes. +size_t BinForException(NSException* exception); + +// Use UMA to track exception occurance. Exposed for testing purposes. +void RecordExceptionWithUma(NSException* exception); + +} // namespace chrome_browser_application_mac + +#endif // __OBJC__ + +namespace chrome_browser_application_mac { + +// Calls -[NSApp terminate:]. +void Terminate(); + +} // namespace chrome_browser_application_mac + +#endif // CHROME_BROWSER_CHROME_BROWSER_APPLICATION_MAC_H_ diff --git a/chrome/browser/chrome_browser_application_mac.mm b/chrome/browser/chrome_browser_application_mac.mm new file mode 100644 index 0000000..57c6fab --- /dev/null +++ b/chrome/browser/chrome_browser_application_mac.mm @@ -0,0 +1,309 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#import "chrome/browser/chrome_browser_application_mac.h" + +#import "base/histogram.h" +#import "base/logging.h" +#import "base/scoped_nsobject.h" +#import "base/sys_string_conversions.h" +#import "chrome/app/breakpad_mac.h" +#import "chrome/browser/cocoa/chrome_event_processing_window.h" +#import "chrome/browser/cocoa/objc_method_swizzle.h" + +// The implementation of NSExceptions break various assumptions in the +// Chrome code. This category defines a replacement for +// -initWithName:reason:userInfo: for purposes of forcing a break in +// the debugger when an exception is raised. -raise sounds more +// obvious to intercept, but it doesn't catch the original throw +// because the objc runtime doesn't use it. +@interface NSException (NSExceptionSwizzle) +- (id)chromeInitWithName:(NSString *)aName + reason:(NSString *)aReason + userInfo:(NSDictionary *)someUserInfo; +@end + +static IMP gOriginalInitIMP = NULL; + +@implementation NSException (NSExceptionSwizzle) +- (id)chromeInitWithName:(NSString *)aName + reason:(NSString *)aReason + userInfo:(NSDictionary *)someUserInfo { + // Method only called when swizzled. + DCHECK(_cmd == @selector(initWithName:reason:userInfo:)); + + // Dear reader: something you just did provoked an NSException. + // Please check your backtrace and see if you can't file a bug with + // a repro case. You should be able to safely continue past the + // NOTREACHED(), but feel free to comment it out locally if it is + // making your job hard. + DLOG(ERROR) << "Someone is preparing to raise an exception! " + << base::SysNSStringToUTF8(aName) << " *** " + << base::SysNSStringToUTF8(aReason); + NOTREACHED(); + + // Forward to the original version. + return gOriginalInitIMP(self, _cmd, aName, aReason, someUserInfo); +} +@end + +namespace chrome_browser_application_mac { + +// Maximum number of known named exceptions we'll support. There is +// no central registration, but I only find about 75 possibilities in +// the system frameworks, and many of them are probably not +// interesting to track in aggregate (those relating to distributed +// objects, for instance). +const size_t kKnownNSExceptionCount = 25; + +const size_t kUnknownNSException = kKnownNSExceptionCount; + +size_t BinForException(NSException* exception) { + // A list of common known exceptions. The list position will + // determine where they live in the histogram, so never move them + // around, only add to the end. + static const NSString* kKnownNSExceptionNames[] = { + // ??? + NSGenericException, + + // Out-of-range on NSString or NSArray. + NSRangeException, + + // Invalid arg to method, unrecognized selector. + NSInvalidArgumentException, + + // malloc() returned null in object creation, I think. + NSMallocException, + + nil + }; + + // Make sure our array hasn't outgrown our abilities to track it. + DCHECK_LE(arraysize(kKnownNSExceptionNames), kKnownNSExceptionCount); + + const NSString* name = [exception name]; + for (int i = 0; kKnownNSExceptionNames[i]; ++i) { + if (name == kKnownNSExceptionNames[i]) { + return i; + } + } + return kUnknownNSException; +} + +void RecordExceptionWithUma(NSException* exception) { + static LinearHistogram histogram("OSX.NSException", 0, kUnknownNSException, + kUnknownNSException + 1); + histogram.SetFlags(kUmaTargetedHistogramFlag); + histogram.Add(BinForException(exception)); +} + +void Terminate() { + [NSApp terminate:nil]; +} + +} // namespace chrome_browser_application_mac + +namespace { + +// Helper to make it easy to get crash keys right. +// TODO(shess): Find a better home for this. app/breakpad_mac.h +// doesn't work. +class ScopedCrashKey { + public: + ScopedCrashKey(NSString* key, NSString* value) + : crash_key_([key retain]) { + SetCrashKeyValue(crash_key_.get(), value); + } + ~ScopedCrashKey() { + ClearCrashKeyValue(crash_key_.get()); + } + + private: + scoped_nsobject crash_key_; +}; + +// Do-nothing wrapper so that we can arrange to only swizzle +// -[NSException raise] when DCHECK() is turned on (as opposed to +// replicating the preprocess logic which turns DCHECK() on). +BOOL SwizzleNSExceptionInit() { + gOriginalInitIMP = ObjcEvilDoers::SwizzleImplementedInstanceMethods( + [NSException class], + @selector(initWithName:reason:userInfo:), + @selector(chromeInitWithName:reason:userInfo:)); + return YES; +} + +} // namespace + +@implementation BrowserCrApplication + +- init { + DCHECK(SwizzleNSExceptionInit()); + return [super init]; +} + +// -terminate: is the entry point for orderly "quit" operations in Cocoa. +// This includes the application menu's quit menu item and keyboard +// equivalent, the application's dock icon menu's quit menu item, "quit" (not +// "force quit") in the Activity Monitor, and quits triggered by user logout +// and system restart and shutdown. +// +// The default NSApplication -terminate: implementation will end the process +// by calling exit(), and thus never leave the main run loop. This is +// unsuitable for Chrome's purposes. Chrome depends on leaving the main +// run loop to perform a proper orderly shutdown. This design is ingrained +// in the application and the assumptions that its code makes, and is +// entirely reasonable and works well on other platforms, but it's not +// compatible with the standard Cocoa quit sequence. Quits originated from +// within the application can be redirected to not use -terminate:, but +// quits from elsewhere cannot be. +// +// To allow the Cocoa-based Chrome to support the standard Cocoa -terminate: +// interface, and allow all quits to cause Chrome to shut down properly +// regardless of their origin, -terminate: is overriden. The custom +// -terminate: does not end the application with exit(). Instead, it simply +// returns after posting the normal NSApplicationWillTerminateNotification +// notification. The application is responsible for exiting on its own in +// whatever way it deems appropriate. In Chrome's case, the main run loop will +// end and the applicaton will exit by returning from main(). +// +// This implementation of -terminate: is scaled back and is not as +// fully-featured as the implementation in NSApplication, nor is it a direct +// drop-in replacement -terminate: in most applications. It is +// purpose-specific to Chrome. +- (void)terminate:(id)sender { + NSApplicationTerminateReply shouldTerminate = NSTerminateNow; + SEL selector = @selector(applicationShouldTerminate:); + if ([[self delegate] respondsToSelector:selector]) + shouldTerminate = [[self delegate] applicationShouldTerminate:self]; + + // If shouldTerminate is NSTerminateLater, the application is expected to + // call -replyToApplicationShouldTerminate: when it knows whether or not it + // should terminate. If the argument is YES, + // -replyToApplicationShouldTerminate: will call -terminate:. This will + // result in another call to the delegate's -applicationShouldTerminate:, + // which would be expected to return NSTerminateNow at that point. + if (shouldTerminate != NSTerminateNow) + return; + + [[NSNotificationCenter defaultCenter] + postNotificationName:NSApplicationWillTerminateNotification + object:self]; + + // Return, don't exit. The application is responsible for exiting on its + // own. +} + +- (BOOL)sendAction:(SEL)anAction to:(id)aTarget from:(id)sender { + // The Dock menu contains an automagic section where you can select + // amongst open windows. If a window is closed via JavaScript while + // the menu is up, the menu item for that window continues to exist. + // When a window is selected this method is called with the + // now-freed window as |aTarget|. Short-circuit the call if + // |aTarget| is not a valid window. + if (anAction == @selector(_selectWindow:)) { + // Not using -[NSArray containsObject:] because |aTarget| may be a + // freed object. + BOOL found = NO; + for (NSWindow* window in [self windows]) { + if (window == aTarget) { + found = YES; + break; + } + } + if (!found) { + return NO; + } + } + + // When a Cocoa control is wired to a freed object, we get crashers + // in the call to |super| with no useful information in the + // backtrace. Attempt to add some useful information. + static const NSString* kActionKey = @"sendaction"; + + // If the action is something generic like -commandDispatch:, then + // the tag is essential. + NSInteger tag = 0; + if ([sender isKindOfClass:[NSControl class]]) { + tag = [sender tag]; + if (tag == 0 || tag == -1) { + tag = [sender selectedTag]; + } + } else if ([sender isKindOfClass:[NSMenuItem class]]) { + tag = [sender tag]; + } + + NSString* actionString = NSStringFromSelector(anAction); + NSString* value = + [NSString stringWithFormat:@"%@ tag %d sending %@ to %p", + [sender className], tag, actionString, aTarget]; + + ScopedCrashKey key(kActionKey, value); + return [super sendAction:anAction to:aTarget from:sender]; +} + +- (void)sendEvent:(NSEvent*)event { + chrome_application_mac::ScopedSendingEvent scoper(self); + // The superclass's |sendEvent:| sends keyboard events to the menu and the key + // view loop before dispatching them to |keyDown:|. Since we want to send keys + // to the renderer before sending them to the menu, and we never want them to + // the kev view loop when the web is focussed, we change this behavior. + if ([[self keyWindow] + isKindOfClass:[ChromeEventProcessingWindow class]]) { + if ([static_cast([self keyWindow]) + shortcircuitEvent:event]) + return; + } + + [super sendEvent:event]; +} + +// NSExceptions which are caught by the event loop are logged here. +// NSException uses setjmp/longjmp, which can be very bad for C++, so +// we attempt to track and report them. +- (void)reportException:(NSException *)anException { + // If we throw an exception in this code, we can create an infinite + // loop. If we throw out of the if() without resetting + // |reportException|, we'll stop reporting exceptions for this run. + static BOOL reportingException = NO; + DCHECK(!reportingException); + if (!reportingException) { + reportingException = YES; + chrome_browser_application_mac::RecordExceptionWithUma(anException); + + // Store some human-readable information in breakpad keys in case + // there is a crash. Since breakpad does not provide infinite + // storage, we track two exceptions. The first exception thrown + // is tracked because it may be the one which caused the system to + // go off the rails. The last exception thrown is tracked because + // it may be the one most directly associated with the crash. + static const NSString* kFirstExceptionKey = @"firstexception"; + static BOOL trackedFirstException = NO; + static const NSString* kLastExceptionKey = @"lastexception"; + + // TODO(shess): It would be useful to post some stacktrace info + // from the exception. + // 10.6 has -[NSException callStackSymbols] + // 10.5 has -[NSException callStackReturnAddresses] + // 10.5 has backtrace_symbols(). + // I've tried to combine the latter two, but got nothing useful. + // The addresses are right, though, maybe we could train the crash + // server to decode them for us. + + NSString* value = [NSString stringWithFormat:@"%@ reason %@", + [anException name], [anException reason]]; + if (!trackedFirstException) { + SetCrashKeyValue(kFirstExceptionKey, value); + trackedFirstException = YES; + } else { + SetCrashKeyValue(kLastExceptionKey, value); + } + + reportingException = NO; + } + + [super reportException:anException]; +} + +@end diff --git a/chrome/browser/chrome_browser_application_mac_unittest.mm b/chrome/browser/chrome_browser_application_mac_unittest.mm new file mode 100644 index 0000000..7fc134e --- /dev/null +++ b/chrome/browser/chrome_browser_application_mac_unittest.mm @@ -0,0 +1,81 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#import + +#include "base/histogram.h" +#import "chrome/browser/chrome_browser_application_mac.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace chrome_browser_application_mac { + +// Generate an NSException with the given name. +NSException* ExceptionNamed(NSString* name) { + return [NSException exceptionWithName:name + reason:@"No reason given" + userInfo:nil]; +} + +// Helper to keep binning expectations readible. +size_t BinForExceptionNamed(NSString* name) { + return BinForException(ExceptionNamed(name)); +} + +TEST(ChromeApplicationMacTest, ExceptionBinning) { + // These exceptions must be in this order. + EXPECT_EQ(BinForExceptionNamed(NSGenericException), 0U); + EXPECT_EQ(BinForExceptionNamed(NSRangeException), 1U); + EXPECT_EQ(BinForExceptionNamed(NSInvalidArgumentException), 2U); + EXPECT_EQ(BinForExceptionNamed(NSMallocException), 3U); + + // Random other exceptions map to |kUnknownNSException|. + EXPECT_EQ(BinForExceptionNamed(@"CustomName"), kUnknownNSException); + EXPECT_EQ(BinForExceptionNamed(@"Custom Name"), kUnknownNSException); + EXPECT_EQ(BinForExceptionNamed(@""), kUnknownNSException); + EXPECT_EQ(BinForException(nil), kUnknownNSException); +} + +TEST(ChromeApplicationMacTest, RecordException) { + // Start up a histogram recorder. + StatisticsRecorder recorder; + + StatisticsRecorder::Histograms histograms; + StatisticsRecorder::GetSnapshot("OSX.NSException", &histograms); + EXPECT_EQ(0U, histograms.size()); + + // Record some known exceptions. + RecordExceptionWithUma(ExceptionNamed(NSGenericException)); + RecordExceptionWithUma(ExceptionNamed(NSGenericException)); + RecordExceptionWithUma(ExceptionNamed(NSGenericException)); + RecordExceptionWithUma(ExceptionNamed(NSGenericException)); + RecordExceptionWithUma(ExceptionNamed(NSRangeException)); + RecordExceptionWithUma(ExceptionNamed(NSInvalidArgumentException)); + RecordExceptionWithUma(ExceptionNamed(NSInvalidArgumentException)); + RecordExceptionWithUma(ExceptionNamed(NSInvalidArgumentException)); + RecordExceptionWithUma(ExceptionNamed(NSMallocException)); + RecordExceptionWithUma(ExceptionNamed(NSMallocException)); + + // Record some unknown exceptions. + RecordExceptionWithUma(ExceptionNamed(@"CustomName")); + RecordExceptionWithUma(ExceptionNamed(@"Custom Name")); + RecordExceptionWithUma(ExceptionNamed(@"")); + RecordExceptionWithUma(nil); + + // We should have exactly the right number of exceptions. + StatisticsRecorder::GetSnapshot("OSX.NSException", &histograms); + EXPECT_EQ(1U, histograms.size()); + EXPECT_EQ(kUmaTargetedHistogramFlag, histograms[0]->flags()); + Histogram::SampleSet sample; + histograms[0]->SnapshotSample(&sample); + EXPECT_EQ(4, sample.counts(0)); + EXPECT_EQ(1, sample.counts(1)); + EXPECT_EQ(3, sample.counts(2)); + EXPECT_EQ(2, sample.counts(3)); + + // The unknown exceptions should end up in the overflow bucket. + EXPECT_EQ(kUnknownNSException + 1, histograms[0]->bucket_count()); + EXPECT_EQ(4, sample.counts(kUnknownNSException)); +} + +} // chrome_browser_application_mac diff --git a/chrome/browser/cocoa/cocoa_test_helper.h b/chrome/browser/cocoa/cocoa_test_helper.h index 5b05505..5f51c54 100644 --- a/chrome/browser/cocoa/cocoa_test_helper.h +++ b/chrome/browser/cocoa/cocoa_test_helper.h @@ -8,6 +8,7 @@ #import #include +#import "base/chrome_application_mac.h" #include "base/debug_util.h" #include "base/file_path.h" #include "base/mac_util.h" @@ -138,14 +139,7 @@ class CocoaTest : public PlatformTest { class CocoaNoWindowTestHelper { public: CocoaNoWindowTestHelper() { - // Look in the framework bundle for resources. - FilePath path; - PathService::Get(base::DIR_EXE, &path); - path = path.Append(chrome::kFrameworkName); - mac_util::SetOverrideAppBundlePath(path); - - // Bootstrap Cocoa. It's very unhappy without this. - [NSApplication sharedApplication]; + CocoaTest::BootstrapCocoa(); // Set the duration of AppKit-evaluated animations (such as frame changes) // to zero for testing purposes. That way they take effect immediately. @@ -155,7 +149,7 @@ class CocoaNoWindowTestHelper { // DEPRECATED // TODO(dmaclach): remove as soon as I can get my other CLs in that get rid -// of any dependencies on this. 10/30/09 at the latest. +// of any dependencies on this. 11/30/09 at the latest. class CocoaTestHelper : public CocoaNoWindowTestHelper { public: CocoaTestHelper() { diff --git a/chrome/browser/cocoa/cocoa_test_helper.mm b/chrome/browser/cocoa/cocoa_test_helper.mm index 462b10a..17bba55 100644 --- a/chrome/browser/cocoa/cocoa_test_helper.mm +++ b/chrome/browser/cocoa/cocoa_test_helper.mm @@ -3,6 +3,7 @@ // found in the LICENSE file. #import "chrome/browser/cocoa/cocoa_test_helper.h" +#import "chrome/browser/chrome_browser_application_mac.h" #import "base/logging.h" @implementation CocoaTestHelperWindow @@ -68,7 +69,7 @@ void CocoaTest::BootstrapCocoa() { mac_util::SetOverrideAppBundlePath(path); // Bootstrap Cocoa. It's very unhappy without this. - [NSApplication sharedApplication]; + [CrApplication sharedApplication]; } void CocoaTest::TearDown() { -- cgit v1.1