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

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

In 10.10, when ToolbarController's NSView is loaded, NSNib creates a
dictionary that retains ToolbarController. This dictionary is later
autoreleased. This causes ToolbarController to briefly outlive Browser
when the BrowserWindow closes. Objects whose lifetime is tied to
ToolbarController may hold a weak Browser* and attempt to access it
during destruction resulting in a use-after-free. This can also happen
with other similar reference-counted objects like WrenchMenuController.

This CL adds -browserWillBeDestroyed to many of these objects so they
can invalidate their Browser* and pass the notification onto other
similar objects. Any use-after-free becomes a null pointer dereference
which we can detect and fix.

BUG=506745, 505710
Committed: https://crrev.com/0e97f8a8934f34fb6f5aba7214eb8470862c52b6
Cr-Commit-Position: refs/heads/master@{#338198}

[jackhou1, 2015-07-06 06:58:07]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-07-06 06:58:08]

tapted, WDYT?

Main changes in BrowserWindowController, ToolbarController,
WrenchMenuController, in that order.

[tapted, 2015-07-06 07:26:18]

Just had a quick skim. I suspect this is what is needed.

Do we have something more concrete to say about the particular failure on 10.10.
That is, exactly what it is that has retained a reference to the controller, and
is causing it to be destroyed late?

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:443: // insures they
invalidate their weak Browser* to prevent use-after-free.
insures -> ensures?

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:120: -
(void)browserWillBeDestroyed {
should come after dealloc (init[WithFoo] and dealloc are usually paired together
in the .mm ... and I guess awakeFromNib is like an initWithFoo as well).

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:396: -
(void)browserWillBeDestroyed {
after dealloc

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:398: }
hm - toolbarActionsBar_ has a weak reference to Browser* as well.

I'm actually now thinking we should scrap all the dealloc methods, and replace
them with browserWillBeDestroyed (and, at the end, do the browser_ = nullptr).

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:90: -
(void)browserWillBeDestroyed {
after dealloc

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:133: -
(void)browserWillBeDestroyed {
after dealloc

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/overlayable_contents_controller.h
(right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/overlayable_contents_controller.h:27: -
(id)init;
this is now an override, so no need to redeclare

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:580: [tabStripView_
setController:nil];
Doing this for all the controlled views in other classes is probably a good idea
too...

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:44:
backForwardMenu_.reset();
This is a scoped_nsobject -- reset() may do nothing.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:239:
locationBarView_.reset();
e.g. [locationBarView_ setController:nil] might be a more reliable way to remove
references

[jackhou1, 2015-07-07 03:27:27]

>Do we have something more concrete to say about the particular failure on
10.10.
>That is, exactly what it is that has retained a reference to the controller,
and
>is causing it to be destroyed late?

It's the ToolbarController's NSView. Updated the CL description to make this
more clear. Also added a stack trace to the bug.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:443: // insures they
invalidate their weak Browser* to prevent use-after-free.
On 2015/07/06 07:26:17, tapted wrote:
> insures -> ensures?

Done.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:120: -
(void)browserWillBeDestroyed {
On 2015/07/06 07:26:18, tapted wrote:
> should come after dealloc (init[WithFoo] and dealloc are usually paired
together
> in the .mm ... and I guess awakeFromNib is like an initWithFoo as well).

Done.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:396: -
(void)browserWillBeDestroyed {
On 2015/07/06 07:26:18, tapted wrote:
> after dealloc

Done.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:398: }
On 2015/07/06 07:26:18, tapted wrote:
> hm - toolbarActionsBar_ has a weak reference to Browser* as well.
> 
> I'm actually now thinking we should scrap all the dealloc methods, and replace
> them with browserWillBeDestroyed (and, at the end, do the browser_ = nullptr).

Done in patch set 7.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:90: -
(void)browserWillBeDestroyed {
On 2015/07/06 07:26:18, tapted wrote:
> after dealloc

Done.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:133: -
(void)browserWillBeDestroyed {
On 2015/07/06 07:26:18, tapted wrote:
> after dealloc

Done.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/overlayable_contents_controller.h
(right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/overlayable_contents_controller.h:27: -
(id)init;
On 2015/07/06 07:26:18, tapted wrote:
> this is now an override, so no need to redeclare

Done.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:580: [tabStripView_
setController:nil];
On 2015/07/06 07:26:18, tapted wrote:
> Doing this for all the controlled views in other classes is probably a good
idea
> too...

I've just done this for things that might retain the owning object, i.e. the
owner passes |self| to the owned object.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:44:
backForwardMenu_.reset();
On 2015/07/06 07:26:18, tapted wrote:
> This is a scoped_nsobject -- reset() may do nothing.

Cleared more things, and reset model_.

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:239:
locationBarView_.reset();
On 2015/07/06 07:26:18, tapted wrote:
> e.g. [locationBarView_ setController:nil] might be a more reliable way to
remove
> references

locationBarView_ is a C++ object.

[tapted, 2015-07-07 04:25:59]

Maybe hit up rsesek for a second opinion too - I'm still a bit of a wuss
approving big changes in c/b/ui/cocoa :p

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:356: -
(void)dealloc {
There's a dealloc hiding down here, and a curious bug reference below.

To keep the diff sane, perhaps rename this definition to browserWillBeDestroyed
and move the declaration in .h down to below the menuController accessor. (then
replace [super dealloc]; with browser_ = nullptr)

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:428:
[downloadShelfController_ exiting];
I think this is redundant now that downloadShelfController_
browserWillBeDestroyed calls it.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:445: // an autorelease
NSView or an NSEvent.
You could mention here, something like,

For example, since 10.10, the Nib loader internally creates an NSDictionary
referencing views/controllers which is autoreleased, so there is no way to
guarantee that the [super dealloc] call below will also call dealloc on the
controllers.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:146: [self
exiting];
I think you can just move the contents of `exiting` into this function.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.h (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.h:66: -
(void)browserWillBeDestroyed;
Hm - I think this is OK to be consistent. But the lifetime is different.

FindBarCocoaController is owned by FindBarBridge which is owned by (wait for
it..) FindBarController, which is plonked into a scoped_ptr held by Browser.

FindBarBridge::~FindBarBridge() will be called reliably. It currently does
`[cocoa_controller_ release];`. It *could* be responsible for the
browserWillBeDestroyed call instead of browser_window_controller.mm. But
actually FindBarBridge and BrowserWindowController each have their own
reference. The one in browser_window_controller.mm will happen first, so I think
that's the way to go. But perhaps it needs a note to say FindBarBridge also has
a reference which will be released later.

I don't think it will make a difference, but only because
Browser::find_bar_controller_ is never destroyed once it is (lazily) created.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:44: [button_
setAttachedMenu:nullptr];
nit: nullptr -> nil

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:209:
resizeDelegate:(id<ViewResizer>)resizeDelegate
Can this be removed?

[jackhou1, 2015-07-07 08:33:06]

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:356: -
(void)dealloc {
On 2015/07/07 04:25:59, tapted wrote:
> There's a dealloc hiding down here, and a curious bug reference below.
> 
> To keep the diff sane, perhaps rename this definition to
browserWillBeDestroyed
> and move the declaration in .h down to below the menuController accessor.
(then
> replace [super dealloc]; with browser_ = nullptr)

Done.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:428:
[downloadShelfController_ exiting];
On 2015/07/07 04:25:59, tapted wrote:
> I think this is redundant now that downloadShelfController_
> browserWillBeDestroyed calls it.

Done.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:445: // an autorelease
NSView or an NSEvent.
On 2015/07/07 04:25:59, tapted wrote:
> You could mention here, something like,
> 
> For example, since 10.10, the Nib loader internally creates an NSDictionary
> referencing views/controllers which is autoreleased, so there is no way to
> guarantee that the [super dealloc] call below will also call dealloc on the
> controllers.

Done.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:146: [self
exiting];
On 2015/07/07 04:25:59, tapted wrote:
> I think you can just move the contents of `exiting` into this function.

Done.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.h (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.h:66: -
(void)browserWillBeDestroyed;
On 2015/07/07 04:25:59, tapted wrote:
> Hm - I think this is OK to be consistent. But the lifetime is different.
> 
> FindBarCocoaController is owned by FindBarBridge which is owned by (wait for
> it..) FindBarController, which is plonked into a scoped_ptr held by Browser.
> 
> FindBarBridge::~FindBarBridge() will be called reliably. It currently does
> `[cocoa_controller_ release];`. It *could* be responsible for the
> browserWillBeDestroyed call instead of browser_window_controller.mm. But
> actually FindBarBridge and BrowserWindowController each have their own
> reference. The one in browser_window_controller.mm will happen first, so I
think
> that's the way to go. But perhaps it needs a note to say FindBarBridge also
has
> a reference which will be released later.
> 
> I don't think it will make a difference, but only because
> Browser::find_bar_controller_ is never destroyed once it is (lazily) created.

Added note in BrowserWindowController::dealloc

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:44: [button_
setAttachedMenu:nullptr];
On 2015/07/07 04:25:59, tapted wrote:
> nit: nullptr -> nil

Done.

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:209:
resizeDelegate:(id<ViewResizer>)resizeDelegate
On 2015/07/07 04:25:59, tapted wrote:
> Can this be removed?

Done.

[tapted, 2015-07-07 12:45:10]

tapted@chromium.org changed reviewers:
+ rsesek@chromium.org

[tapted, 2015-07-07 12:45:11]

lgtm but +rsesek for a second opinion and since it's a bit of a curly change

[Robert Sesek, 2015-07-07 15:51:03]

rsesek@chromium.org changed reviewers:
+ erikchen@chromium.org

[Robert Sesek, 2015-07-07 15:51:04]

+erikchen who has been working to fix browser tests like
https://codereview.chromium.org/1229473002/

It seems like the way the autorelease pool plays with runloops has changed in
10.10. Can we solve this by modifying our use of MessageLoop/MessagePump and the
autorelease pool?

Something that's not clear to me from the CL description is when this NSNib
autoreleased dictionary is created and destroyed. If it's just when the view is
loaded, why doesn't the autoreleased reference get released as part of NSApp's
run loop and pool?

[tapted, 2015-07-08 00:11:54]

My perspective: after many days of head scratching while writing the tests for
ui/app_list/cocoa, I basically gave up on relying on dealloc for anything that
involves C++ objects as well. It's a trap :).

Aside from this autoreleased NSDictionary in the Nib loader, I think things like
NSEvents can also retain NSWindows and other objects while they're in the event
queue. Other references may be held by an NSAnimation, which has a strong
reference to itself after `startAnimation` is called, and typically a subclass
like NSViewAnimation with a dictionary holding references. There was no way to
predict what AppKit would do, and it's not easy to cleanup things like pending
animations. AppKit was also bemusingly non-deterministic for these things --
maybe windowserver data races, or stray events from
wherever-the-mouse-cursor-happened-to-be-on-screen-during-the-test. So, I gave
up on finding a general solution with autorelease pools and emptying event
queues, and went with `setDelegate:nullptr` for the app list.

For the browser, I see the `browserWillBeDestroyed` approach here as just a
formalization of existing approaches like -[DownloadShelfController exiting],
which came about to solve a similar lifetime issue.

We should still try to get insight into the cases where the NIB loader is
retaining or autoreleasing references to our views. However, I suspect it's only
part of the problem, and draining autorelease pools won't fix the underlying
issue. E.g. Maybe a browser window closes while some focus ring animation is
running; retaining an NSView -- this may cause a use-after-free and is unlikely
to get picked up in tests, or be helped by draining pools.

[erikchen, 2015-07-08 01:02:47]

On 2015/07/08 00:11:54, tapted wrote:
> My perspective: after many days of head scratching while writing the tests for
> ui/app_list/cocoa, I basically gave up on relying on dealloc for anything that
> involves C++ objects as well. It's a trap :).
> 
> Aside from this autoreleased NSDictionary in the Nib loader, I think things
like
> NSEvents can also retain NSWindows and other objects while they're in the
event
> queue. Other references may be held by an NSAnimation, which has a strong
> reference to itself after `startAnimation` is called, and typically a subclass
> like NSViewAnimation with a dictionary holding references. There was no way to
> predict what AppKit would do, and it's not easy to cleanup things like pending
> animations. AppKit was also bemusingly non-deterministic for these things --
> maybe windowserver data races, or stray events from
> wherever-the-mouse-cursor-happened-to-be-on-screen-during-the-test. So, I gave
> up on finding a general solution with autorelease pools and emptying event
> queues, and went with `setDelegate:nullptr` for the app list.
> 
> For the browser, I see the `browserWillBeDestroyed` approach here as just a
> formalization of existing approaches like -[DownloadShelfController exiting],
> which came about to solve a similar lifetime issue.
> 
> We should still try to get insight into the cases where the NIB loader is
> retaining or autoreleasing references to our views. However, I suspect it's
only
> part of the problem, and draining autorelease pools won't fix the underlying
> issue. E.g. Maybe a browser window closes while some focus ring animation is
> running; retaining an NSView -- this may cause a use-after-free and is
unlikely
> to get picked up in tests, or be helped by draining pools.


Some thoughts, no particular ordering:

1) This CL fixes the BrowserCloseManagerBrowserTest problems (at least on
10.10). https://codereview.chromium.org/1229473002/
2) This CL papers over a more pervasive problem, which is that tear down of the
Chrome application is poorly structured, and its interactions with AppKit add
non-determinism. I know of two causes of non-determinism.
  - AppKit retain/autoreleases NSObjects which Chrome assumes it can
synchronously destroy (via scoped_nsobject). The retain/autoreleasing is
unavoidable, but the problem is that our testing infrastructure doesn't flush
the autorelease pool at appropriate times. For more details, see my link in
comment #1.
  - AppKit retains NSObjects during animations. 
    + I don't think that a test should end if there are still views being
animated. It should wait for the animations to finish.
    + I think that all view transitions should have a flag animated:(BOOL) and
almost all tests should set this flag to NO, which both speeds up the test, and
avoids a lot of these problems.
3) I don't think either of my points in (2) have easy solutions. As such, I
think your CL is a reasonable way of shoving (some of) the problem under the
rug. I have specific comments to follow on your actual CL.

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

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:440: // Inform reference
counted objects that the Browser will be destroyed. This
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?

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:373: -
(void)browserWillBeDestroyed {
This method makes the assumption that this is guaranteed to be called before
teardown. This makes the class harder to unit test, and also adds implicit
behavior restrictions on consumers of the class.

How about:
1) Add an idempotent method -cleanUp.
2) Call -cleanUp from browserWillBeDestroyed and -dealloc.

Ditto for all other instances of this method.

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

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:440: // Inform reference
counted objects that the Browser will be destroyed. This
On 2015/07/08 01:18:03, erikchen wrote:
> 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?

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.

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/190001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:373: -
(void)browserWillBeDestroyed {
On 2015/07/08 01:18:03, erikchen wrote:
> This method makes the assumption that this is guaranteed to be called before
> teardown. This makes the class harder to unit test, and also adds implicit
> behavior restrictions on consumers of the class.
> 
> How about:
> 1) Add an idempotent method -cleanUp.
> 2) Call -cleanUp from browserWillBeDestroyed and -dealloc.
> 
> Ditto for all other instances of this method.

Yeah good point. For most of these it's fine to just call
-browserWillBeDestroyed in -dealloc. I think we can add -cleanUp for just those
classes that need it.

[erikchen, 2015-07-08 18:02:17]

lgtm with nits.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:154: [[self
animatableView] stopAnimation];
this logic looks copy-pasted from -exiting. Please dedupe.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm
(right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm:376:
[shelf_ browserWillBeDestroyed];
Why does this need to be changed from -exiting?

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:91: [self
browserWillBeDestroyed];
I recommended that you make a new method called -cleanUp. I understand
functionally why -dealloc would call -browserWillBeDestroyed, but semantically
it doesn't make much sense. 

That being said, I dislike quibbling on nomenclature, so I'll let it slide.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.h (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.h:12: #import
"chrome/browser/ui/cocoa/has_weak_browser_pointer.h"
ordering.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:406: [[self view]
removeTrackingArea:trackingArea_.get()];
It's not clean to me that -removeTrackingArea: is idempotent. Perhaps we should
be resetting trackingArea_ here?

Also, don't we need to call [trackingArea_ clearOwner]?

[Robert Sesek, 2015-07-08 22:15:18]

This approah is fine, so LG. Though you could potentially also do this with a
special C++ scoper class. The scoper would be a BrowserListObserver (or a
notification listener for NOTIFICATION_BROWSER_CLOSED, but notifications are
going away) and would then invoke a target-action whenever it recieved the
Closed notifcation for the Browser* to which it is attached. Classes that wanted
to participate would just add the scoper as an ivar and configure it for the
instance's Browser*. The advantage of this approach would be you don't have to
chain the notification down to all the various sub-controllers. I don't feel
strongly, so take or leave this as an alternative approach.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:154: [[self
animatableView] stopAnimation];
On 2015/07/08 18:02:17, erikchen wrote:
> this logic looks copy-pasted from -exiting. Please dedupe.

-editing was removed.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:91: [self
browserWillBeDestroyed];
On 2015/07/08 18:02:17, erikchen wrote:
> I recommended that you make a new method called -cleanUp. I understand
> functionally why -dealloc would call -browserWillBeDestroyed, but semantically
> it doesn't make much sense. 
> 
> That being said, I dislike quibbling on nomenclature, so I'll let it slide.

I kind of agree, but I don't know if adding this extra method indirection would
actually make things easier to follow.

[jackhou1, 2015-07-09 03:48:03]

On 2015/07/08 22:15:18, Robert Sesek wrote:
> This approah is fine, so LG. Though you could potentially also do this with a
> special C++ scoper class. The scoper would be a BrowserListObserver (or a
> notification listener for NOTIFICATION_BROWSER_CLOSED, but notifications are
> going away) and would then invoke a target-action whenever it recieved the
> Closed notifcation for the Browser* to which it is attached. Classes that
wanted
> to participate would just add the scoper as an ivar and configure it for the
> instance's Browser*. The advantage of this approach would be you don't have to
> chain the notification down to all the various sub-controllers. I don't feel
> strongly, so take or leave this as an alternative approach.

tapted suggested this too (http://crbug.com/506745#c5). It's also nice in that
the logic is encapsulated in each class that cares. I think ultimately we were
wary about having a large number of observers that might have interdependencies.
Calling -browserWillBeDestroyed explicitly makes it easier to trace the order
that the 'observers' are notified and lets us arbitrarily manipulate that order
if needed.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm
(right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm:376:
[shelf_ browserWillBeDestroyed];
On 2015/07/08 18:02:17, erikchen wrote:
> Why does this need to be changed from -exiting? 

They do exactly the same thing. Essentially DownloadShelfController was the only
class using the -browserWillBeDestroyed pattern introduced by this CL. Now that
we have -browserWillBeDestroyed on a bunch of similar classes, it makes sense to
unify it here too.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.h (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.h:12: #import
"chrome/browser/ui/cocoa/has_weak_browser_pointer.h"
On 2015/07/08 18:02:17, erikchen wrote:
> ordering.

Done.

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1221173003/diff/210001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:406: [[self view]
removeTrackingArea:trackingArea_.get()];
On 2015/07/08 18:02:17, erikchen wrote:
> It's not clean to me that -removeTrackingArea: is idempotent. Perhaps we
should
> be resetting trackingArea_ here?
> 
> Also, don't we need to call [trackingArea_ clearOwner]?

Done.

[Robert Sesek, 2015-07-09 15:13:09]

On 2015/07/09 03:48:03, jackhou1 wrote:
> On 2015/07/08 22:15:18, Robert Sesek wrote:
> > This approah is fine, so LG. Though you could potentially also do this with
a
> > special C++ scoper class. The scoper would be a BrowserListObserver (or a
> > notification listener for NOTIFICATION_BROWSER_CLOSED, but notifications are
> > going away) and would then invoke a target-action whenever it recieved the
> > Closed notifcation for the Browser* to which it is attached. Classes that
> wanted
> > to participate would just add the scoper as an ivar and configure it for the
> > instance's Browser*. The advantage of this approach would be you don't have
to
> > chain the notification down to all the various sub-controllers. I don't feel
> > strongly, so take or leave this as an alternative approach.
> 
> tapted suggested this too (http://crbug.com/506745#c5). It's also nice in that
> the logic is encapsulated in each class that cares. I think ultimately we were
> wary about having a large number of observers that might have
interdependencies.
> Calling -browserWillBeDestroyed explicitly makes it easier to trace the order
> that the 'observers' are notified and lets us arbitrarily manipulate that
order
> if needed.

That's a fair point. Certainly BrowserList doesn't guarantee a certain order
when delivering these notifications. LGTM

[jackhou1, 2015-07-09 23:33:47]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-07-09 23:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1221173003/#ps230001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-07-09 23:37:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1221173003/230001

[commit-bot: I haz the power, 2015-07-10 00:32:36]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-07-10 00:33:30]

Patchset 13 (id:??) landed as
https://crrev.com/0e97f8a8934f34fb6f5aba7214eb8470862c52b6
Cr-Commit-Position: refs/heads/master@{#338198}