Address a crash under -[NSWindow close] via a WeakPtr PostTask from Browser::TabStripEmpty()

Address a crash under -[NSWindow close] via a WeakPtr PostTask from Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To (speculatively) fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701
Committed: https://crrev.com/7e91932ca0df94f1d3ebe9303e6ff31637751c17
Cr-Commit-Position: refs/heads/master@{#398474}

[tapted, 2016-06-07 00:21:42]

Description was changed from

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24.
The stacks all have in common a Posted Task that's triggering -[NSWindow close]
via a base::WeakPtr<Browser>. This only happens via Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled
window is destroyed, but this seems to not be guaranteed.

To fix, set a flag when the controlled window invokes
-[NSWindowController onWindowWillClose]. The window shouldn't be
accessed after this. Ensure the C++ BrowserWindowCocoa shim returns
nil in this case for the window, even if -[NSWindowController window]
doesn't.

BUG=616701
==========

to

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, set a flag when the controlled window invokes
-[NSWindowController onWindowWillClose]. The window shouldn't be
accessed after this. Ensure the C++ BrowserWindowCocoa shim returns nil
in this case for the window, even if -[NSWindowController window]
doesn't.

BUG=616701
==========

[tapted, 2016-06-07 00:21:42]

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

[tapted, 2016-06-07 00:24:37]

Hi Robert, please take a look. (and please CQ if it looks good - I might still
be asleep when the next build is cut).

[Robert Sesek, 2016-06-07 16:43:36]

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

https://codereview.chromium.org/2041213002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:546: }
What about:

[[self window] performSelector:@selector(autorelease) withObject:nil
afterDelay:0];
[self setWindow:nil];

instead?

[tapted, 2016-06-08 00:00:29]

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

https://codereview.chromium.org/2041213002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:546: }
On 2016/06/07 16:43:35, Robert Sesek wrote:
> What about:
> 
> [[self window] performSelector:@selector(autorelease) withObject:nil
> afterDelay:0];
> [self setWindow:nil];
> 
> instead?

So with this approach, I get a "guaranteed" double-free when the window is
closed -- both under __CFRunLoopDoTimer. The backtrace from dealloc is from
_ZN12_GLOBAL__N_119AutoreleasePoolPage3popEPv and the zombie is from
CFRunLoopTimerInvalidate (not under
__CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__).

So I think the autorelease is triggering a dealloc, then the strong reference in
the closure is trying to do another one.

I'm now thinking the part that is broken is the
performSelector:@selector(autorelease) bit. The runtime has no way of knowing a
delayed autorelease is hanging around when the autoreleasepool is drained, so
this seems fundamentally broken.

But if the closure has a strong reference it doesn't really matter what task we
post, so long as it isn't a release.  (but also if the closure has a strong
reference, why is the autorelease pool trying to dealloc it in the first place
:| ).

[tapted, 2016-06-08 00:07:43]

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

https://codereview.chromium.org/2041213002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:546: }
On 2016/06/08 00:00:29, tapted wrote:
> On 2016/06/07 16:43:35, Robert Sesek wrote:
> > What about:
> > 
> > [[self window] performSelector:@selector(autorelease) withObject:nil
> > afterDelay:0];
> > [self setWindow:nil];
> > 
> > instead?
> 
> So with this approach, I get a "guaranteed" double-free when the window is
> closed -- both under __CFRunLoopDoTimer. The backtrace from dealloc is from
> _ZN12_GLOBAL__N_119AutoreleasePoolPage3popEPv and the zombie is from
> CFRunLoopTimerInvalidate (not under
> __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__).
> 
> So I think the autorelease is triggering a dealloc, then the strong reference
in
> the closure is trying to do another one.
> 
> I'm now thinking the part that is broken is the
> performSelector:@selector(autorelease) bit. The runtime has no way of knowing
a
> delayed autorelease is hanging around when the autoreleasepool is drained, so
> this seems fundamentally broken.
> 
> But if the closure has a strong reference it doesn't really matter what task
we
> post, so long as it isn't a release.  (but also if the closure has a strong
> reference, why is the autorelease pool trying to dealloc it in the first place
> :| ). 

Ah - derp. No this is actually just a boring old double-free. We need to [window
retain] before we can [window autorelease] :p. Browser windows don't use
setReleasedWhenClosed:NO, so the NSWindow releases itself shortly after this
method returns.

Now go trybots go!

[Robert Sesek, 2016-06-08 00:09:00]

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

https://codereview.chromium.org/2041213002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:546: }
On 2016/06/08 00:00:29, tapted wrote:
> On 2016/06/07 16:43:35, Robert Sesek wrote:
> > What about:
> > 
> > [[self window] performSelector:@selector(autorelease) withObject:nil
> > afterDelay:0];
> > [self setWindow:nil];
> > 
> > instead?
> 
> So with this approach, I get a "guaranteed" double-free when the window is
> closed -- both under __CFRunLoopDoTimer. The backtrace from dealloc is from
> _ZN12_GLOBAL__N_119AutoreleasePoolPage3popEPv and the zombie is from
> CFRunLoopTimerInvalidate (not under
> __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__).
> 
> So I think the autorelease is triggering a dealloc, then the strong reference
in
> the closure is trying to do another one.
> 
> I'm now thinking the part that is broken is the
> performSelector:@selector(autorelease) bit. The runtime has no way of knowing
a
> delayed autorelease is hanging around when the autoreleasepool is drained, so
> this seems fundamentally broken.
> 
> But if the closure has a strong reference it doesn't really matter what task
we
> post, so long as it isn't a release.  (but also if the closure has a strong
> reference, why is the autorelease pool trying to dealloc it in the first place
> :| ). 

That makes sense, actually. What I wrote would be an over-release because we
transfer ownership of the window to the controller when it's created
(https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/tabs/tab_window_...).
So calling setWindow:nil will release that ref.

To correct that, I think it should be:

NSWindow* window = [[self window] retain];
[self setWindow:nil];
[window performSelector:@selector(autorelease) withObject:nil afterDelay:0];

[Robert Sesek, 2016-06-08 00:10:08]

race condition LGTM

[tapted, 2016-06-08 00:14:48]

Description was changed from

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, set a flag when the controlled window invokes
-[NSWindowController onWindowWillClose]. The window shouldn't be
accessed after this. Ensure the C++ BrowserWindowCocoa shim returns nil
in this case for the window, even if -[NSWindowController window]
doesn't.

BUG=616701
==========

to

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, clear the controller's reference to the NSWindow inside
-[NSWindowController onWindowWillClose]. The window shouldn't be
accessed after this. This ensures the C++ BrowserWindowCocoa shim
returns nil in this case for its window().

BUG=616701
==========

[Robert Sesek, 2016-06-08 00:26:17]

Description was changed from

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, clear the controller's reference to the NSWindow inside
-[NSWindowController onWindowWillClose]. The window shouldn't be
accessed after this. This ensures the C++ BrowserWindowCocoa shim
returns nil in this case for its window().

BUG=616701
==========

to

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, clear the controller's reference to the NSWindow inside
-[NSWindowController windowWillClose:]. The window shouldn't be
accessed after this. This ensures the C++ BrowserWindowCocoa shim
returns nil in this case for its window().

BUG=616701
==========

[tapted, 2016-06-08 01:09:31]

On 2016/06/08 00:10:08, Robert Sesek wrote:
> race condition LGTM

Lots of crashes in browser_tests sadly -- under [_NSWindowTransformAnimation
dealloc] of all things.

I haven't thought about it deeply, but it looks like -[NSWindow dealloc] does
[[NSWindow contentView] setWindow:nil]; But most of the BrowserWindow subviews
ignore the argument to setWindow, and just assume that they can get their window
from [self window], but since this is happening in view*Will*MoveToWindow:,
[NSView window] hasn't updated yet yet, so it returns the window that's in the
process of being dealloc'd.

One approach might be to use setReleasedWhenClosed:NO, and store the browser
NSWindow as a scoped_nsobject on the controller so it gets destroyed after the
scoped_nsobjects holding the subviews are released. But that won't necessarily
guarantee the order since there may be other references to the subviews.

At this point I'm inclined to go back to the first approach, in patchset 2.
Altering the lifetime of things on BrowserWindowCocoa is too fraught with danger
-- certainly not a thing we should rush into a beta channel merge. Setting that
flag feels like the safest approach.

[tapted, 2016-06-08 01:15:06]

Description was changed from

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, clear the controller's reference to the NSWindow inside
-[NSWindowController windowWillClose:]. The window shouldn't be
accessed after this. This ensures the C++ BrowserWindowCocoa shim
returns nil in this case for its window().

BUG=616701
==========

to

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701
==========

[tapted, 2016-06-08 01:22:30]

PTAL - went back to patchset 2... It's the best I can come up with that doesn't
make me think of this:
http://www.gifbin.com/bin/012014/1389122551_poking_a_spider_nest.gif

[Robert Sesek, 2016-06-08 02:00:32]

I'm not totally convinced this will fix the issue, but let's go with it as a
speculative fix. LGTM.

Do we have enough samples on canary/dev to verify that this works before needing
to merge it to beta?

On 2016/06/08 01:22:30, tapted wrote:
> PTAL - went back to patchset 2... It's the best I can come up with that
doesn't
> make me think of this:
> http://www.gifbin.com/bin/012014/1389122551_poking_a_spider_nest.gif

DO NOT LIKE!!!!

https://codereview.chromium.org/2041213002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

https://codereview.chromium.org/2041213002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_cocoa.h:175: // Called when the
controller's window invokes -onWindowWillClose:.
nit: just windowWillClose: (and on line 206).

[tapted, 2016-06-08 02:33:04]

Description was changed from

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701
==========

to

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To (speculatively) fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701
==========

[tapted, 2016-06-08 02:47:27]

On 2016/06/08 02:00:32, Robert Sesek wrote:
> I'm not totally convinced this will fix the issue, but let's go with it as a
> speculative fix. LGTM.

updated CL description with "speculative" :)

> Do we have enough samples on canary/dev to verify that this works before
needing
> to merge it to beta?

I've added a go/crash search link to the bug that I'll monitor. There seems to
be a few crashes in each Canary going back pretty far. But there's a big uptick
(60!) in the most recent Canary -- 53.0.2754.0. Spread across a whole bunch of
Client IDs too. I'll see if I can extract any new insights with this nugget of
information.

It's possible we have missed the latest beta spin, so it might have a bit more
time to bake.

https://codereview.chromium.org/2041213002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

https://codereview.chromium.org/2041213002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_cocoa.h:175: // Called when the
controller's window invokes -onWindowWillClose:.
On 2016/06/08 02:00:32, Robert Sesek wrote:
> nit: just windowWillClose: (and on line 206).

Whoops - Done. (I noticed you updated the CL description earlier - thanks :).

[tapted, 2016-06-08 03:00:41]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-06-08 03:00:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2041213002/#ps100001
(title: "windowWillClose")

[tapted, 2016-06-08 03:01:34]

The CQ bit was unchecked by tapted@chromium.org

[commit-bot: I haz the power, 2016-06-08 03:01:34]

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

[tapted, 2016-06-08 03:21:38]

On 2016/06/08 02:47:27, tapted wrote:
> On 2016/06/08 02:00:32, Robert Sesek wrote:
> > I'm not totally convinced this will fix the issue, but let's go with it as a
> > speculative fix. LGTM.
> 
> updated CL description with "speculative" :)
> 
> > Do we have enough samples on canary/dev to verify that this works before
> needing
> > to merge it to beta?
> 
> I've added a go/crash search link to the bug that I'll monitor. There seems to
> be a few crashes in each Canary going back pretty far. But there's a big
uptick
> (60!) in the most recent Canary -- 53.0.2754.0. Spread across a whole bunch of
> Client IDs too. I'll see if I can extract any new insights with this nugget of
> information.

So 53.0.2754.0 is not actually the most recent Canary, but it is the last one
that we have crashes for :(.
And that's actually a few days old. So there's the possibility that this no
longer crashes in Canary, but I'm stumped why that would be.

Could this fix beta.... maybe? I'm kinda out of ideas at this point.

I've scanned both

https://chromium.googlesource.com/chromium/src/+log/53.0.2754.0..53.0.2756.0?...
and
https://chromium.googlesource.com/chromium/src/+log/53.0.2753.0..53.0.2754.0?...

but couldn't find anything. Note there doesn't seem to be a 53.0.2755.? tag.

Let's get this in so it can bake and leave our options open for the beta. But if
there's no blip in the crash reporter on m53, we should probably revert it in
Canary to get more data.

[tapted, 2016-06-08 03:22:02]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-06-08 03:22:22]

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

[commit-bot: I haz the power, 2016-06-08 03:55:07]

Description was changed from

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To (speculatively) fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701
==========

to

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To (speculatively) fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701
==========

[commit-bot: I haz the power, 2016-06-08 03:55:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-08 03:57:03]

Description was changed from

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To (speculatively) fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701
==========

to

==========
Address a crash under -[NSWindow close] via a WeakPtr PostTask from
Browser::TabStripEmpty()

This is the #1 browser crash for Mac in current Beta - 52.0.2743.24. The
stacks all have in common a Posted Task that's triggering -[NSWindow
close] via a base::WeakPtr<Browser>. This only happens via
Browser::TabStripEmpty().

The WeakPtr is only nerfed when BrowserWindowController's dealloc fully
completes. It seems plausible that this can be after the NSWindow's
dealloc fully completes, leading to an invalid access. One would hope
that [NSWindowController window] returns nil once the controlled window
is destroyed, but this seems to not be guaranteed.

To (speculatively) fix, set a flag when the controlled window invokes
-[NSWindowController windowWillClose]. The window shouldn't be accessed
after this. Ensure the C++ BrowserWindowCocoa shim returns nil in this
case for the window, even if -[NSWindowController window] doesn't.

BUG=616701

Committed: https://crrev.com/7e91932ca0df94f1d3ebe9303e6ff31637751c17
Cr-Commit-Position: refs/heads/master@{#398474}
==========

[commit-bot: I haz the power, 2016-06-08 03:57:05]

Patchset 6 (id:??) landed as
https://crrev.com/7e91932ca0df94f1d3ebe9303e6ff31637751c17
Cr-Commit-Position: refs/heads/master@{#398474}

[tapted, 2016-06-09 05:58:57]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2042123005/ by tapted@chromium.org.

The reason for reverting is: Fix didn't work..