Mac: reform our shutdown routine.

Make shutdown be more like other platforms. Moreover:
- Cancelling quit from an onbeforeunload dialog shouldn't mess up the browser.
- Having quit cancelled due to a window pop up on the closure of another window
  shouldn't break the browser. [With this patch, it will result in the browser
  being in a quirky state in which the closure of the last browser window will
  cause a quit. But the browser won't be broken.]

BUG=34384,37813,37927
TEST=See bugs.

Review URL: http://codereview.chromium.org/1520006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@44096 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 217 insertions, 71 deletions

diff --git a/chrome/browser/app_controller_mac.h b/chrome/browser/app_controller_mac.h
index 11f705f..86cf2d9 100644
--- a/chrome/browser/app_controller_mac.h
+++ b/chrome/browser/app_controller_mac.h
@@ -56,6 +56,13 @@ class Profile;
 - (void)didEndMainMessageLoop;
 - (Profile*)defaultProfile;
 
+// Try to close all browser windows, and if that succeeds then quit.
+- (BOOL)tryToTerminateApplication:(NSApplication*)app;
+
+// Stop trying to terminate the application. That is, prevent the final browser
+// window closure from causing the application to quit.
+- (void)stopTryingToTerminateApplication:(NSApplication*)app;
+
 // Show the preferences window, or bring it to the front if it's already
 // visible.
 - (IBAction)showPreferences:(id)sender;
diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 8985761..5da68a3 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -138,6 +138,8 @@ void RecordLastRunAppBundlePath() {
 
 @interface AppController(Private)
 - (void)initMenuState;
+- (void)handleQuitEvent:(NSAppleEventDescriptor*)event
+              withReply:(NSAppleEventDescriptor*)reply;
 - (void)openUrls:(const std::vector<GURL>&)urls;
 - (void)getUrl:(NSAppleEventDescriptor*)event
      withReply:(NSAppleEventDescriptor*)reply;
@@ -217,30 +219,66 @@ void RecordLastRunAppBundlePath() {
   }
 }
 
+// (NSApplicationDelegate protocol) This is the Apple-approved place to override
+// the default handlers.
+- (void)applicationWillFinishLaunching:(NSNotification*)notification {
+  NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
+  [em setEventHandler:self
+          andSelector:@selector(handleQuitEvent:withReply:)
+        forEventClass:kCoreEventClass
+           andEventID:kAEQuitApplication];
+}
+
+// (NSApplicationDelegate protocol) Our mechanism for application termination
+// does not go through |-applicationShouldTerminate:|, so it should not be
+// called. In a release build, cancelling termination will prevent a crash (but
+// if things go really wrong, the user may have to force-terminate the
+// application).
 - (NSApplicationTerminateReply)applicationShouldTerminate:
-    (NSApplication *)sender {
-  // Check for in-progress downloads, and prompt the user if they really want to
-  // quit (and thus cancel the downloads).
-  if (![self shouldQuitWithInProgressDownloads])
-    return NSTerminateCancel;
+    (NSApplication*)sender {
+  NOTREACHED();
+  return NSTerminateCancel;
+}
+
+- (BOOL)tryToTerminateApplication:(NSApplication*)app {
+  // Set the state to "trying to quit", so that closing all browser windows will
+  // lead to termination.
+  browser_shutdown::SetTryingToQuit(true);
+
+  // TODO(viettrungluu): Remove Apple Event handlers here? (It's safe to leave
+  // them in, but I'm not sure about UX; we'd also want to disable other things
+  // though.) http://crbug.com/40861
+
+  if (!BrowserList::size())
+    return YES;
+
+  // Try to close all the windows.
+  BrowserList::CloseAllBrowsers(true);
+
+  return NO;
+}
+
+- (void)stopTryingToTerminateApplication:(NSApplication*)app {
+  if (browser_shutdown::IsTryingToQuit()) {
+    // Reset the "trying to quit" state, so that closing all browser windows
+    // will no longer lead to termination.
+    browser_shutdown::SetTryingToQuit(false);
 
-  return NSTerminateNow;
+    // TODO(viettrungluu): Were we to remove Apple Event handlers above, we
+    // would have to reinstall them here. http://crbug.com/40861
+  }
 }
 
 // Called when the app is shutting down. Clean-up as appropriate.
-- (void)applicationWillTerminate:(NSNotification *)aNotification {
+- (void)applicationWillTerminate:(NSNotification*)aNotification {
   NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
   [em removeEventHandlerForEventClass:kInternetEventClass
                            andEventID:kAEGetURL];
   [em removeEventHandlerForEventClass:'WWW!'
                            andEventID:'OURL'];
 
-  // Close all the windows.
-  BrowserList::CloseAllBrowsers(true);
-
-  // On Windows, this is done in Browser::OnWindowClosing, but that's not
-  // appropriate on Mac since we don't shut down when we reach zero windows.
-  browser_shutdown::OnShutdownStarting(browser_shutdown::BROWSER_EXIT);
+  // There better be no browser windows left at this point.
+  CHECK_EQ(BrowserList::size(), 0u);
 
   // Release the reference to the browser process. Once all the browsers get
   // dealloc'd, it will stop the RunLoop and fall back into main().
@@ -395,7 +433,7 @@ void RecordLastRunAppBundlePath() {
   CFPropertyListRef plist = CFPreferencesCopyAppValue(checkInterval, app);
   if (!plist) {
     const float fiveHoursInSeconds = 5.0 * 60.0 * 60.0;
-    NSNumber *value = [NSNumber numberWithFloat:fiveHoursInSeconds];
+    NSNumber* value = [NSNumber numberWithFloat:fiveHoursInSeconds];
     CFPreferencesSetAppValue(checkInterval, value, app);
     CFPreferencesAppSynchronize(app);
   }
@@ -422,7 +460,7 @@ void RecordLastRunAppBundlePath() {
   // call this from awakeFromNib.
   NSMenu* view_menu = [[[NSApp mainMenu] itemWithTag:IDC_VIEW_MENU] submenu];
   NSMenuItem* encoding_menu_item = [view_menu itemWithTag:IDC_ENCODING_MENU];
-  NSMenu *encoding_menu = [encoding_menu_item submenu];
+  NSMenu* encoding_menu = [encoding_menu_item submenu];
   EncodingMenuControllerDelegate::BuildEncodingMenu([self defaultProfile],
                                                     encoding_menu);
 
@@ -802,6 +840,13 @@ void RecordLastRunAppBundlePath() {
   return NULL;
 }
 
+// (Private) Never call |-applicationShouldTerminate:|; just make everything go
+// through |-terminate:|.
+- (void)handleQuitEvent:(NSAppleEventDescriptor*)event
+              withReply:(NSAppleEventDescriptor*)reply {
+  [NSApp terminate:nil];
+}
+
 // Various methods to open URLs that we get in a native fashion. We use
 // BrowserInit here because on the other platforms, URLs to open come through
 // the ProcessSingleton, and it calls BrowserInit. It's best to bottleneck the
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 1c510dc..dffd1cd 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -636,14 +636,18 @@ void Browser::OnWindowClosing() {
 
   bool exiting = false;
 
-#if defined(OS_WIN) || defined(OS_LINUX)
-  // We don't want to do this on Mac since closing all windows isn't a sign
-  // that the app is shutting down.
-  if (BrowserList::size() == 1) {
+#if defined(OS_MACOSX)
+  // On Mac, closing the last window isn't usually a sign that the app is
+  // shutting down.
+  bool should_quit_if_last_browser = browser_shutdown::IsTryingToQuit();
+#else
+  bool should_quit_if_last_browser = true;
+#endif
+
+  if (should_quit_if_last_browser && BrowserList::size() == 1) {
     browser_shutdown::OnShutdownStarting(browser_shutdown::WINDOW_CLOSE);
     exiting = true;
   }
-#endif
 
   // Don't use HasSessionService here, we want to force creation of the
   // session service so that user can restore what was open.
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index ec7865d1..94b0c6b 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -253,8 +253,8 @@ void BrowserList::CloseAllBrowsersAndExit() {
   CloseAllBrowsers(true);
 #else
   // 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.
+  // Terminate will result in a CloseAllBrowsers(true) call, and once (and if)
+  // that is done, will cause the application to exit cleanly.
   chrome_browser_application_mac::Terminate();
 #endif
 }
diff --git a/chrome/browser/browser_list_mac.mm b/chrome/browser/browser_list_mac.mm
new file mode 100644
index 0000000..f65a3e5
--- /dev/null
+++ b/chrome/browser/browser_list_mac.mm
@@ -0,0 +1,15 @@
+// Copyright (c) 2010 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.
+
+#include "chrome/browser/browser_list.h"
+
+#include "chrome/browser/browser_shutdown.h"
+#import "chrome/browser/chrome_browser_application_mac.h"
+
+// static
+void BrowserList::AllBrowsersClosed() {
+  // Only terminate after all browsers are closed if trying quit.
+  if (browser_shutdown::IsTryingToQuit())
+    chrome_browser_application_mac::Terminate();
+}
diff --git a/chrome/browser/browser_shutdown.cc b/chrome/browser/browser_shutdown.cc
index 5d24596..c052790 100644
--- a/chrome/browser/browser_shutdown.cc
+++ b/chrome/browser/browser_shutdown.cc
@@ -41,6 +41,11 @@ using base::TimeDelta;
 
 namespace browser_shutdown {
 
+#if defined(OS_MACOSX)
+// Whether the browser is trying to quit (e.g., Quit chosen from menu).
+bool g_trying_to_quit = false;
+#endif  // OS_MACOSX
+
 Time shutdown_started_;
 ShutdownType shutdown_type_ = NOT_VALID;
 int shutdown_num_processes_;
@@ -226,4 +231,14 @@ void ReadLastShutdownInfo() {
           &ReadLastShutdownFile, type, num_procs, num_procs_slow));
 }
 
+#if defined(OS_MACOSX)
+void SetTryingToQuit(bool quitting) {
+  g_trying_to_quit = quitting;
+}
+
+bool IsTryingToQuit() {
+  return g_trying_to_quit;
+}
+#endif
+
 }  // namespace browser_shutdown
diff --git a/chrome/browser/browser_shutdown.h b/chrome/browser/browser_shutdown.h
index 49d800f..3ab2c2e 100644
--- a/chrome/browser/browser_shutdown.h
+++ b/chrome/browser/browser_shutdown.h
@@ -44,6 +44,22 @@ void Shutdown();
 // Called at startup to create a histogram from our previous shutdown time.
 void ReadLastShutdownInfo();
 
+#if defined(OS_MACOSX)
+// On Mac, closing the last window does not automatically quit the application.
+// To actually quit, set a flag which makes final window closure trigger a quit.
+// If the quit is aborted, then the flag should be reset (but see notes below on
+// the proper way to do this, i.e., usually not using |SetTryingToQuit()|).
+
+// This is a low-level mutator; in general, don't call it, except from
+// appropriate places in the app controller. To quit, use usual means, e.g.,
+// using |chrome_browser_application_mac::Terminate()|. To stop quitting, use
+// |chrome_browser_application_mac::CancelTerminate()|.
+void SetTryingToQuit(bool quitting);
+
+// General accessor.
+bool IsTryingToQuit();
+#endif  // OS_MACOSX
+
 }  // namespace browser_shutdown
 
 #endif  // CHROME_BROWSER_BROWSER_SHUTDOWN_H__
diff --git a/chrome/browser/chrome_browser_application_mac.h b/chrome/browser/chrome_browser_application_mac.h
index 9a1ab9e..67b4230 100644
--- a/chrome/browser/chrome_browser_application_mac.h
+++ b/chrome/browser/chrome_browser_application_mac.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -10,6 +10,10 @@
 #import "base/chrome_application_mac.h"
 
 @interface BrowserCrApplication : CrApplication
+// Our implementation of |-terminate:| only attempts to terminate the
+// application, i.e., begins a process which may lead to termination. This
+// method cancels that process.
+- (void)cancelTerminate:(id)sender;
 @end
 
 namespace chrome_browser_application_mac {
@@ -34,6 +38,9 @@ namespace chrome_browser_application_mac {
 // Calls -[NSApp terminate:].
 void Terminate();
 
+// Cancels a termination started by |Terminate()|.
+void CancelTerminate();
+
 }  // 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
index f373e11..07aac53 100644
--- a/chrome/browser/chrome_browser_application_mac.mm
+++ b/chrome/browser/chrome_browser_application_mac.mm
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -9,6 +9,7 @@
 #import "base/scoped_nsobject.h"
 #import "base/sys_string_conversions.h"
 #import "chrome/app/breakpad_mac.h"
+#import "chrome/browser/app_controller_mac.h"
 #import "chrome/browser/cocoa/objc_method_swizzle.h"
 
 // The implementation of NSExceptions break various assumptions in the
@@ -120,6 +121,10 @@ void Terminate() {
   [NSApp terminate:nil];
 }
 
+void CancelTerminate() {
+  [NSApp cancelTerminate:nil];
+}
+
 }  // namespace chrome_browser_application_mac
 
 namespace {
@@ -161,56 +166,80 @@ BOOL 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.
+////////////////////////////////////////////////////////////////////////////////
+// HISTORICAL COMMENT (by viettrungluu, from
+// http://codereview.chromium.org/1520006 with mild editing):
+//
+// A quick summary of the state of things (before the changes to shutdown):
+//
+// Currently, we are totally hosed (put in a bad state in which Cmd-W does the
+// wrong thing, and which will probably eventually lead to a crash) if we begin
+// quitting but termination is aborted for some reason.
+//
+// I currently know of two ways in which termination can be aborted:
+// (1) Common case: a window has an onbeforeunload handler which pops up a
+//     "leave web page" dialog, and the user answers "no, don't leave".
+// (2) Uncommon case: popups are enabled (in Content Settings, i.e., the popup
+//     blocker is disabled), and some nasty web page pops up a new window on
+//     closure.
+//
+// I don't know of other ways in which termination can be aborted, but they may
+// exist (or may be added in the future, for that matter).
 //
-// 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.
+// My CL [see above] does the following:
+// a. Should prevent being put in a bad state (which breaks Cmd-W and leads to
+//    crash) under all circumstances.
+// b. Should completely handle (1) properly.
+// c. Doesn't (yet) handle (2) properly and puts it in a weird state (but not
+//    that bad).
+// d. Any other ways of aborting termination would put it in that weird state.
 //
-// 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().
+// c. can be fixed by having the global flag reset on browser creation or
+// similar (and doing so might also fix some possible d.'s as well). I haven't
+// done this yet since I haven't thought about it carefully and since it's a
+// corner case.
 //
-// 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.
+// The weird state: a state in which closing the last window quits the browser.
+// This might be a bit annoying, but it's not dangerous in any way.
+////////////////////////////////////////////////////////////////////////////////
+
+// |-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 |-terminate:| implementation ends the process by calling exit(),
+// and thus never leaves the main run loop. This is unsuitable for Chrome since
+// Chrome depends on leaving the main run loop to perform an orderly shutdown.
+// We support the normal |-terminate:| interface by overriding the default
+// implementation. Our implementation, which is very specific to the needs of
+// Chrome, works by asking the application delegate to terminate using its
+// |-tryToTerminateApplication:| method.
+//
+// |-tryToTerminateApplication:| differs from the standard
+// |-applicationShouldTerminate:| in that no special event loop is run in the
+// case that immediate termination is not possible (e.g., if dialog boxes
+// allowing the user to cancel have to be shown). Instead, this method sets a
+// flag and tries to close all browsers. This flag causes the closure of the
+// final browser window to begin actual tear-down of the application.
+// Termination is cancelled by resetting this flag. The standard
+// |-applicationShouldTerminate:| is not supported, and code paths leading to it
+// must be redirected.
 - (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.
+  AppController* appController = static_cast<AppController*>([NSApp delegate]);
+  if ([appController tryToTerminateApplication:self]) {
+    [[NSNotificationCenter defaultCenter]
+        postNotificationName:NSApplicationWillTerminateNotification
+                      object:self];
+  }
+
+  // Return, don't exit. The application is responsible for exiting on its own.
+}
+
+- (void)cancelTerminate:(id)sender {
+  AppController* appController = static_cast<AppController*>([NSApp delegate]);
+  [appController stopTryingToTerminateApplication:self];
 }
 
 - (BOOL)sendAction:(SEL)anAction to:(id)aTarget from:(id)sender {
diff --git a/chrome/browser/js_modal_dialog_mac.mm b/chrome/browser/js_modal_dialog_mac.mm
index 3bdc9aa..d15e6ad 100644
--- a/chrome/browser/js_modal_dialog_mac.mm
+++ b/chrome/browser/js_modal_dialog_mac.mm
@@ -10,6 +10,7 @@
 #include "app/message_box_flags.h"
 #import "base/cocoa_protocols_mac.h"
 #include "base/sys_string_conversions.h"
+#import "chrome/browser/chrome_browser_application_mac.h"
 #include "grit/app_strings.h"
 #include "grit/generated_resources.h"
 
@@ -68,6 +69,11 @@
       break;
     }
     case NSAlertSecondButtonReturn:  {  // Cancel
+      // If the user wants to stay on this page, stop quitting (if a quit is in
+      // progress).
+      if (bridge->is_before_unload_dialog())
+        chrome_browser_application_mac::CancelTerminate();
+
       bridge->OnCancel();
       break;
     }
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index b323f802..715fc39 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -250,6 +250,7 @@
         'browser/browser_init.cc',
         'browser/browser_init.h',
         'browser/browser_list_gtk.cc',
+        'browser/browser_list_mac.mm',
         'browser/browser_list_stub.cc',
         'browser/browser_list_win.cc',
         'browser/browser_list.cc',
@@ -2544,6 +2545,7 @@
             'browser/automation/automation_provider_list_generic.cc',
             'browser/bookmarks/bookmark_context_menu.cc',
             'browser/bookmarks/bookmark_drop_info.cc',
+            'browser/browser_list_stub.cc',
             'browser/dock_info.cc',
             'browser/importer/nss_decryptor_system_nss.cc',
             'browser/importer/nss_decryptor_system_nss.h',