[Mac] Inform reference counted objects that hold a weak Browser* when the Browser is being destroye…

chrome/browser/ui/cocoa/browser_window_controller.mm

Index: chrome/browser/ui/cocoa/browser_window_controller.mm
diff --git a/chrome/browser/ui/cocoa/browser_window_controller.mm b/chrome/browser/ui/cocoa/browser_window_controller.mm
index 022763ca1412a4b4ba2c130be2538e380bedeaf3..9826c86316fcab9a2ef96102b99ec4080bd84eac 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller.mm
@@ -302,7 +302,7 @@ using content::WebContents;
     // Create the overlayable contents controller.  This provides the switch
     // view that TabStripController needs.
     overlayableContentsController_.reset(
-        [[OverlayableContentsController alloc] initWithBrowser:browser]);
+        [[OverlayableContentsController alloc] init]);
     [[overlayableContentsController_ view]
         setFrame:[[devToolsController_ view] bounds]];
     [[devToolsController_ view]
@@ -319,10 +319,9 @@ using content::WebContents;
     // registering for the appropriate command state changes from the back-end.
     // Adds the toolbar to the content area.
     toolbarController_.reset([[ToolbarController alloc]
-              initWithCommands:browser->command_controller()->command_updater()
-                       profile:browser->profile()
-                       browser:browser
-                resizeDelegate:self]);
+        initWithCommands:browser->command_controller()->command_updater()
+                 profile:browser->profile()
+                 browser:browser]);
     [toolbarController_ setHasToolbar:[self hasToolbar]
                        hasLocationBar:[self hasLocationBar]];
 
@@ -425,7 +424,6 @@ using content::WebContents;
 
 - (void)dealloc {
   browser_->tab_strip_model()->CloseAllTabs();
-  [downloadShelfController_ exiting];
 
   // Explicitly release |presentationModeController_| here, as it may call back
   // to this BWC in |-dealloc|.  We are required to call |-exitPresentationMode|
@@ -439,6 +437,23 @@ using content::WebContents;
 
   [[NSNotificationCenter defaultCenter] removeObserver:self];
 
+  // Inform reference counted objects that the Browser will be destroyed. This

[erikchen, 2015/07/08 01:18:03]

This solution seems fragile. If someone adds a new controller, it's easy to
forget to add a reference here. 

Two ideas come to mind. 

1) Use weak pointers. 
Upside: Very easy.
Downside: You have less control over teardown.

2) Use some more explicit typing. (I'm horrible at nomenclature. All names
should be seen as placeholders).
  a) Make a protocol SupportsWeakBrowserPointer.
    - (void)receiveWeakBrowserPointer:(Browser*)browser;
    - (void)browserWillBeDestroyed;
  b) Have all these controllers inherit from this protocol.
  c) BrowserWindowController has methods:
    - (void)addWeakPointerRecipient:(id<SupportsWeakBrowserPointer> foo) {
         [foo receiveWeakBrowserPointer:self];
         [recipient_array_ addObject:foo];
     }
     - (void)dealloc {
         for (id foo in recipient_array_)
            [foo browserWillBeDestroyed];
     }
Upside: Allows other work to be done during teardown.
Downside: Requires more code writing.

What do you think?

[jackhou1, 2015/07/08 07:02:12]

I agree it's not obvious which objects implement -browserWillBeDestroyed and who
should be calling it. Patch set 12 adds a protocol HasWeakBrowserPointer
containing just -browserWillBeDestroyed. Most controllers take a Browser* in
their init method, so adding -receiveWeakBrowserPointer makes it more
complicated, and I'm not sure if there's any benefit. Similarly with
-addWeakPointerRecipient, adding a call to that is not much different to adding
a call to -browserWillBeDestroyed.

+  // ensures they invalidate their weak Browser* to prevent use-after-free.
+  // These may outlive the Browser if they are retained by something else. For
+  // example, since 10.10, the Nib loader internally creates an NSDictionary
+  // that retains NSViewControllers and is autoreleased, so there is no way to
+  // guarantee that the [super dealloc] call below will also call dealloc on the
+  // controllers.
+  [toolbarController_ browserWillBeDestroyed];
+  [tabStripController_ browserWillBeDestroyed];
+  // NOTE: |findBarCocoaController_| is additionally retained by FindBarBridge,
+  // but that is ultimately owned by Browser::find_bar_controller_ and will be
+  // released soon.
+  [findBarCocoaController_ browserWillBeDestroyed];
+  [downloadShelfController_ browserWillBeDestroyed];
+  [bookmarkBarController_ browserWillBeDestroyed];
+  [avatarButtonController_ browserWillBeDestroyed];
+
   [super dealloc];
 }