Mac: Speculative fix for [Cocoa Zombie] -[BrowserWindowController release]

Reported crashes suggest a situation where we may double-free a
BrowserWindowController. Stacks show a UAF while trying to release a
reference to |self| held by an ObjC closure. The free occurs on an
autorelease triggered by a closure.

Most likely these are happening on the same closure. That is
__NSFireDelayedPerform "free()s" and its __delayedPerformCleanup
immediately tries to access to free again.

One hypothetical way this could occur would be if AppKit invoked
-[BrowserWindowController windowWillClose:] multiple times. Each time
would post a delayed autorelease closure. When the second is executed,
it would create the situation described.

Protect against this with an early exit in windowWillClose: if it's
already been called.

BUG=671213

Review-Url: https://codereview.chromium.org/2597023002
Cr-Commit-Position: refs/heads/master@{#441282}

chrome/browser/ui/cocoa/browser_window_controller.h

@@ -184,6 +184,10 @@ class Command;
   // being sent to the renderer, which causes the transition to be janky.
   BOOL blockLayoutSubviews_;
 
+  // Set when AppKit invokes -windowWillClose: to protect against possible
+  // crashes. See http://crbug.com/671213.
+  BOOL didWindowWillClose_;
+
   // The Extension Command Registry used to determine which keyboard events to
   // handle.
   std::unique_ptr<ExtensionKeybindingRegistryCocoa>

chrome/browser/ui/cocoa/browser_window_controller.mm

@@ -19,6 +19,7 @@
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/managed_bookmark_service_factory.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/extensions/extension_commands_global_registry.h"
 #include "chrome/browser/permissions/permission_request_manager.h"
@@ -55,13 +56,13 @@
 #import "chrome/browser/ui/cocoa/find_bar/find_bar_bridge.h"
 #import "chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.h"
 #import "chrome/browser/ui/cocoa/framed_browser_window.h"
+#import "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.h"
+#import "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_visibility_lock_controller.h"
 #include "chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h"
 #import "chrome/browser/ui/cocoa/fullscreen_window.h"
 #import "chrome/browser/ui/cocoa/infobars/infobar_container_controller.h"
-#import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.h"
-#import "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_visibility_lock_controller.h"
-#import "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.h"
 #include "chrome/browser/ui/cocoa/l10n_util.h"
+#import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.h"
 #import "chrome/browser/ui/cocoa/profiles/avatar_base_controller.h"
 #import "chrome/browser/ui/cocoa/profiles/avatar_button_controller.h"
 #import "chrome/browser/ui/cocoa/profiles/avatar_icon_controller.h"
@@ -510,14 +511,11 @@ bool IsTabDetachingInFullscreenEnabled() {
 - (void)destroyBrowser {
   [NSApp removeWindowsItem:[self window]];
 
-  // We need the window to go away now.
-  // We can't actually use |-autorelease| here because there's an embedded
-  // run loop in the |-performClose:| which contains its own autorelease pool.
-  // Instead call it after a zero-length delay, which gets us back to the main
-  // event loop.
-  [self performSelector:@selector(autorelease)
-             withObject:nil
-             afterDelay:0];
+  // This is invoked from chrome::SessionEnding() which will terminate the
+  // process without spinning another RunLoop. So no need to perform an
+  // autorelease. Note this is currently controlled by an experiment. See
+  // features::kDesktopFastShutdown in chrome/browser/features.cc.
+  DCHECK_EQ(browser_shutdown::GetShutdownType(), browser_shutdown::END_SESSION);
 }
 
 // Called when the window meets the criteria to be closed (ie,
@@ -525,13 +523,26 @@ bool IsTabDetachingInFullscreenEnabled() {
 // semantics of BrowserWindow::Close() and not call the Browser's dtor directly
 // from this method.
 - (void)windowWillClose:(NSNotification*)notification {
+  // Speculative fix for http://crbug.com/671213. It seems possible that AppKit
+  // may invoke -windowWillClose: twice under rare conditions. That would cause
+  // the logic below to post a second -autorelease, resulting in a double free.
+  // (Well, actually, a zombie access when the closure tries to call release on
+  // the strongly captured |self| pointer).
+  DCHECK(!didWindowWillClose_) << "If hit, please update crbug.com/671213.";
+  if (didWindowWillClose_)
+    return;
+
+  didWindowWillClose_ = YES;
+
   DCHECK_EQ([notification object], [self window]);
   DCHECK(browser_->tab_strip_model()->empty());
   [savedRegularWindow_ close];
+
   // We delete statusBubble here because we need to kill off the dependency
   // that its window has on our window before our window goes away.
   delete statusBubble_;
   statusBubble_ = NULL;
+
   // We can't actually use |-autorelease| here because there's an embedded
   // run loop in the |-performClose:| which contains its own autorelease pool.
   // Instead call it after a zero-length delay, which gets us back to the main