Mac: reform our shutdown routine.

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.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44096

[Mark Mentovai, 2010-03-31 19:51:57]

The trybot failed unit_tests.  Can you check to see if it’s because of the patch
or if it’s flake?

[viettrungluu, 2010-04-01 15:59:10]

On 2010/03/31 19:51:57, Mark Mentovai wrote:
> The trybot failed unit_tests.  Can you check to see if it’s because of the
patch
> or if it’s flake?

Definitely the patch. I'll try to debug it today (and/or see if changes by
others have an effect).

[Scott Hess - ex-Googler, 2010-04-01 21:54:13]

http://codereview.chromium.org/1520006/diff/6001/4004
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4004#newcode845
chrome/browser/app_controller_mac.mm:845:
withReply:(NSAppleEventDescriptor*)reply { [NSApp terminate:nil]; }
Make it all pretty, with the ':' lined up and "{\n" and "\n}" and stuff.

http://codereview.chromium.org/1520006/diff/6001/4007
File chrome/browser/browser_list_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4007#newcode15
chrome/browser/browser_list_mac.mm:15: }
Does anyone still use browser_list_stub.cc?

http://codereview.chromium.org/1520006/diff/6001/4008
File chrome/browser/browser_shutdown.cc (right):

http://codereview.chromium.org/1520006/diff/6001/4008#newcode49
chrome/browser/browser_shutdown.cc:49: }  // anonymous namespace
I think it's safe to put this in the browser_shutdown namespace.  Most of the
other variables aren't exported anywhere, either.

http://codereview.chromium.org/1520006/diff/6001/4011
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4011#newcode126
chrome/browser/chrome_browser_application_mac.mm:126: [[NSApp delegate]
performSelector:selector withObject:NSApp];
Not clear why you coded it this way?  If you just called:
  [[NSApp delegate] stopTryingToTerminateApplication:NSApp];
and NSApp doesn't respond, it would crash.  As coded above, if NSApp does not
respond to that selector, it will crash.  I suppose there are cases where the
CHECK() crash provides a more obvious diagnostic in the crash dump, but it seems
marginal to me.

Actually, I'd kind of expect [NSApp cancelTerminate:nil], here, given the
implementation of Terminate().

[pink (ping after 24hrs), 2010-04-05 15:06:27]

All the "cancel terminate" work is nice, but it still gets us into a situation
where we would quit when we get down to zero browser windows sometime later in
the future? That sounds broken in a different way. Is there no way to avoid
that?

http://codereview.chromium.org/1520006/diff/6001/4004
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4004#newcode232
chrome/browser/app_controller_mac.mm:232: // (NSApplicationDelegate protocol)
Our mechanism for application terminate does
s/terminate/termination

http://codereview.chromium.org/1520006/diff/6001/4004#newcode247
chrome/browser/app_controller_mac.mm:247: // TODO(viettrungluu): Remove Apple
Event handlers here? (It's safe to leave
bug# for todo?

http://codereview.chromium.org/1520006/diff/6001/4004#newcode845
chrome/browser/app_controller_mac.mm:845:
withReply:(NSAppleEventDescriptor*)reply { [NSApp terminate:nil]; }
this makes it hard to find the body of the method

http://codereview.chromium.org/1520006/diff/6001/4009
File chrome/browser/browser_shutdown.h (right):

http://codereview.chromium.org/1520006/diff/6001/4009#newcode50
chrome/browser/browser_shutdown.h:50: // If the quit is aborted, then the flag
should be reset (but see notes below).
what notes where?

http://codereview.chromium.org/1520006/diff/6001/4009#newcode56
chrome/browser/browser_shutdown.h:56: void SetTryingToQuit(bool quitting);
do you want to make some notes about these not being threadsafe?

http://codereview.chromium.org/1520006/diff/6001/4011
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4011#newcode183
chrome/browser/chrome_browser_application_mac.mm:183: // within the application
can be redirected to not use -terminate:, but
is this still true? doesn't this cl redirect quits from outside?

[viettrungluu, 2010-04-07 16:30:31]

+rohitrao since he was also looking at this.

I haven't had time to work on this CL again, unfortunately. But let me give a
quick summary of the state of things.

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

My CL 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.

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.

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.

I'll write another message with alternatives....

[pink (ping after 24hrs), 2010-04-07 17:56:09]

I said this on IRC, but i'll echo it here for others.

This is a great summary, and clears a lot up at least for me. I am
comfortable moving ahead with trung's proposal (where we fix (1)
completely but (2) can still leave us in a "quits when all windows
close" state). I think it's better than where we are now and (1) is
probably much more common than (2). We can look at his
flag-reset-on-new-window suggestion in the future, but for now, I'm ok
with going forward with just this patch. I would like to see the
summary added to the code in a comment so we know what's going on and
why things ended up how they did.

I'm sure we'll have to revisit shutdown again at some point and
wouldn't want to go backwards because we've all forgotten where we
came from.

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[viettrungluu, 2010-04-08 22:29:30]

I think I've addressed all the comments.

The Mac unit test problems were due to the DCHECK()s, so I removed them. I'll
ponder what the right thing to do is (possibly nothing).

http://codereview.chromium.org/1520006/diff/6001/4004
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4004#newcode232
chrome/browser/app_controller_mac.mm:232: // (NSApplicationDelegate protocol)
Our mechanism for application terminate does
On 2010/04/05 15:06:28, pink wrote:
> s/terminate/termination

Done.

http://codereview.chromium.org/1520006/diff/6001/4004#newcode247
chrome/browser/app_controller_mac.mm:247: // TODO(viettrungluu): Remove Apple
Event handlers here? (It's safe to leave
On 2010/04/05 15:06:28, pink wrote:
> bug# for todo?

Done.

http://codereview.chromium.org/1520006/diff/6001/4004#newcode845
chrome/browser/app_controller_mac.mm:845:
withReply:(NSAppleEventDescriptor*)reply { [NSApp terminate:nil]; }
On 2010/04/01 21:54:13, shess wrote:
> Make it all pretty, with the ':' lined up and "{\n" and "\n}" and stuff.

Done. Train wreck presumably caused by operator error (and vim).

http://codereview.chromium.org/1520006/diff/6001/4007
File chrome/browser/browser_list_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4007#newcode15
chrome/browser/browser_list_mac.mm:15: }
On 2010/04/01 21:54:13, shess wrote:
> Does anyone still use browser_list_stub.cc?

Linux not satisfying
  'OS=="linux" and chromeos==0 and toolkit_views==0'
seems to *not* exclude it.

http://codereview.chromium.org/1520006/diff/6001/4008
File chrome/browser/browser_shutdown.cc (right):

http://codereview.chromium.org/1520006/diff/6001/4008#newcode49
chrome/browser/browser_shutdown.cc:49: }  // anonymous namespace
On 2010/04/01 21:54:13, shess wrote:
> I think it's safe to put this in the browser_shutdown namespace.  Most of the
> other variables aren't exported anywhere, either.

Done.

http://codereview.chromium.org/1520006/diff/6001/4008#newcode240
chrome/browser/browser_shutdown.cc:240:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
It's these DCHECK()s which are fouling up the unit tests (and it's not this CL
which messes that up, really). (Hrm, do the unit tests have a named UI thread?)

http://codereview.chromium.org/1520006/diff/6001/4009
File chrome/browser/browser_shutdown.h (right):

http://codereview.chromium.org/1520006/diff/6001/4009#newcode50
chrome/browser/browser_shutdown.h:50: // If the quit is aborted, then the flag
should be reset (but see notes below).
On 2010/04/05 15:06:28, pink wrote:
> what notes where?

The notes in front of |SetTryingToQuit()|; I added a clarification.

http://codereview.chromium.org/1520006/diff/6001/4009#newcode56
chrome/browser/browser_shutdown.h:56: void SetTryingToQuit(bool quitting);
On 2010/04/05 15:06:28, pink wrote:
> do you want to make some notes about these not being threadsafe?

I think that's implicit in nearly everything in src/chrome/browser (i.e., not
thread-safe unless otherwise specified).

http://codereview.chromium.org/1520006/diff/6001/4011
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/1520006/diff/6001/4011#newcode126
chrome/browser/chrome_browser_application_mac.mm:126: [[NSApp delegate]
performSelector:selector withObject:NSApp];
On 2010/04/01 21:54:13, shess wrote:
> Not clear why you coded it this way?  If you just called:
>   [[NSApp delegate] stopTryingToTerminateApplication:NSApp];
> and NSApp doesn't respond, it would crash.  As coded above, if NSApp does not
> respond to that selector, it will crash.  I suppose there are cases where the
> CHECK() crash provides a more obvious diagnostic in the crash dump, but it
seems
> marginal to me.
> 
> Actually, I'd kind of expect [NSApp cancelTerminate:nil], here, given the
> implementation of Terminate().

Done (to both).

http://codereview.chromium.org/1520006/diff/6001/4011#newcode183
chrome/browser/chrome_browser_application_mac.mm:183: // within the application
can be redirected to not use -terminate:, but
On 2010/04/05 15:06:28, pink wrote:
> is this still true? doesn't this cl redirect quits from outside?

I guess it's a bit more complicated than that, since the standard AE handler for
quitting does more than just call -terminate:. But let me update the whole
comment.

[Scott Hess - ex-Googler, 2010-04-08 22:59:24]

LGTM, with minor nits.  I'm tired and grumpy and while this is a convoluted
problem, it feels like we could drop some indirection without any loss.

http://codereview.chromium.org/1520006/diff/23001/24001
File chrome/browser/app_controller_mac.h (right):

http://codereview.chromium.org/1520006/diff/23001/24001#newcode60
chrome/browser/app_controller_mac.h:60: // and if that succeeds then quit.
<channel pedantic reviewer> "Try to terminate the application" and
-tryToTerminateApplication: sound eerily similar.  Feel free to snip the first
sentence of this comment (and the comment below), and lose "That is, ".

No need to alphabetize them, though :-).

http://codereview.chromium.org/1520006/diff/23001/24002
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/1520006/diff/23001/24002#newcode239
chrome/browser/app_controller_mac.mm:239: NOTREACHED();
Non-blocking suggestion: I think you should consider a CHECK() here, because if
this is going wrong, how will we ever know?  Alternately, you could drop a
histogram in, and only add the CHECK() if it's not happening that much.

Would there be any real downside to calling -tryToTerminateApplication: and
returning NSTerminateCancel.

http://codereview.chromium.org/1520006/diff/23001/24002#newcode846
chrome/browser/app_controller_mac.mm:846:
withReply:(NSAppleEventDescriptor*)reply {
Does the calling client block until they get a reply?

http://codereview.chromium.org/1520006/diff/23001/24010
File chrome/browser/js_modal_dialog_mac.mm (right):

http://codereview.chromium.org/1520006/diff/23001/24010#newcode76
chrome/browser/js_modal_dialog_mac.mm:76: 
CancelTerminate() exists to forward to -cancelTerminate: which forwards to
-stopTryingToTerminate: which calls IsTryingToQuit() which calls
SetTryingToQuit().

I can't see anyone else calling that.  WDYT of just having this code like:
  if (bridge->is_before_unload_dialog())
    browser_shutdown::SetTryingToQuit(false);
and getting rid of all the other stuff.