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

chrome/browser/ui/cocoa/browser_window_controller.mm

 // Copyright 2012 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/ui/cocoa/browser_window_controller.h"
 
 #include <cmath>
 #include <numeric>
 
 #include "base/command_line.h"
@@ skipping 284 matching lines @@
 
     // Create a sub-controller for the docked devTools and add its view to the
     // hierarchy.
     devToolsController_.reset([[DevToolsController alloc] init]);
     [[devToolsController_ view] setFrame:[[self tabContentArea] bounds]];
     [[self tabContentArea] addSubview:[devToolsController_ view]];
 
     // 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]
         addSubview:[overlayableContentsController_ view]];
 
     // Create a controller for the tab strip, giving it the model object for
     // this window's Browser and the tab strip view. The controller will handle
     // registering for the appropriate tab notifications from the back-end and
     // managing the creation of new tabs.
     [self createTabStripController];
 
     // Create a controller for the toolbar, giving it the toolbar model object
     // and the toolbar view from the nib. The controller will handle
     // 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]];
 
     // Create a sub-controller for the bookmark bar.
     bookmarkBarController_.reset(
         [[BookmarkBarController alloc]
             initWithBrowser:browser_.get()
                initialWidth:NSWidth([[[self window] contentView] frame])
                    delegate:self
              resizeDelegate:self]);
@@ skipping 82 matching lines @@
             windowShim_.get()));
 
     // We are done initializing now.
     initializing_ = NO;
   }
   return self;
 }
 
 - (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|
   // before releasing the controller.
   [presentationModeController_ exitPresentationMode];
   presentationModeController_.reset();
 
   // Under certain testing configurations we may not actually own the browser.
   if (ownsBrowser_ == NO)
     ignore_result(browser_.release());
 
   [[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];
 }
 
 - (gfx::Rect)enforceMinWindowSize:(gfx::Rect)bounds {
   gfx::Rect checkedBounds = bounds;
 
   NSSize minSize = [[self window] minSize];
   if (bounds.width() < minSize.width)
       checkedBounds.set_width(minSize.width);
   if (bounds.height() < minSize.height)
@@ skipping 1793 matching lines @@
 
 - (BOOL)supportsBookmarkBar {
   return [self supportsWindowFeature:Browser::FEATURE_BOOKMARKBAR];
 }
 
 - (BOOL)isTabbedWindow {
   return browser_->is_type_tabbed();
 }
 
 @end  // @implementation BrowserWindowController(WindowType)