[Mac] Create CrTrackingArea and use it in TabStripController.

[Mac] Create CrTrackingArea and use it in TabStripController.

CrTrackingArea can prevent messages from being sent to the tracking area's
owner, which is the source of a significant number of zombie crashes. This hopes
to prevent that.

BUG=48709
TEST=Crash reports go down.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74940

[Robert Sesek, 2011-02-10 18:09:43]

I'm only doing this to TabStripController right now to see what the effect is.

[Scott Hess - ex-Googler, 2011-02-11 01:17:12]

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
File chrome/browser/ui/cocoa/chrome_tracking_area.h (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
chrome/browser/ui/cocoa/chrome_tracking_area.h:18:
scoped_nsobject<CrTrackingAreaOwnerProxy> ownerProxy_;
Rather than using |ownerProxy_|, you can override -owner to return nil after
clear has been called.

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
File chrome/browser/ui/cocoa/chrome_tracking_area.mm (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
chrome/browser/ui/cocoa/chrome_tracking_area.mm:74: userInfo:userInfo]))) {
No need for the empty if().  Actually, the static_cast<id>() shouldn't be
needed, either.

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/tabs/ta...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/tabs/ta...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:34: #import
"chrome/browser/ui/cocoa/chrome_tracking_area.h"
It occurs to me that "chrome" is redundant, and it defines CrTrackingArea
anyhow, not ChromeTrackingArea.

[Robert Sesek, 2011-02-11 02:30:50]

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
File chrome/browser/ui/cocoa/chrome_tracking_area.h (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
chrome/browser/ui/cocoa/chrome_tracking_area.h:18:
scoped_nsobject<CrTrackingAreaOwnerProxy> ownerProxy_;
On 2011/02/11 01:17:12, shess wrote:
> Rather than using |ownerProxy_|, you can override -owner to return nil after
> clear has been called.

I went with this approach because it affords us better debugging options (e.g.,
you mentioned doing backtrace serialization). The proxy approach is also useful
for probing live in the debugger, assuming you can repro. And, NSProxy is cool
;-).

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
File chrome/browser/ui/cocoa/chrome_tracking_area.mm (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
chrome/browser/ui/cocoa/chrome_tracking_area.mm:74: userInfo:userInfo]))) {
On 2011/02/11 01:17:12, shess wrote:
> No need for the empty if().  Actually, the static_cast<id>() shouldn't be
> needed, either.

if() is just for convention. The static_cast<> is necessary here because Apple
declared the initializer as returning NSTrackingArea. I should probably file a
Radar about that.

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/tabs/ta...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/tabs/ta...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:34: #import
"chrome/browser/ui/cocoa/chrome_tracking_area.h"
On 2011/02/11 01:17:12, shess wrote:
> It occurs to me that "chrome" is redundant, and it defines CrTrackingArea
> anyhow, not ChromeTrackingArea.

Yea, I agree. I was following CrApplication. I'll move the file to
tracking_area.{h,mm}.

[Robert Sesek, 2011-02-11 19:53:58]

I've moved the file and ditched the Proxy. I also added a test that makes a
zombie.

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
File chrome/browser/ui/cocoa/chrome_tracking_area.h (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
chrome/browser/ui/cocoa/chrome_tracking_area.h:18:
scoped_nsobject<CrTrackingAreaOwnerProxy> ownerProxy_;
On 2011/02/11 01:17:12, shess wrote:
> Rather than using |ownerProxy_|, you can override -owner to return nil after
> clear has been called.

Switched to this approach. Closer examination showed that
|-methodSignatureForSelector:| can still end up messaging a zombie. Which left
me with three options:

1) Retain the owner in ownerProxy_. That diverges from the expectation of
NSTrackingArea, which I don't like.
2) Override |-doesNotRecognizeSelector:| and do nothing if |!alive_|.
3) Ditch the proxy and just wrap |-owner|.

The last option seemed like the sanest. My only concern is if they use the ivar
directly rather than the accessor anywhere.

[Scott Hess - ex-Googler, 2011-02-11 20:08:19]

LGTM.  I think doing it the old-fashioned way is absolutely reasonable, as it's
not like we need an open-ended set of methods to work.

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
File chrome/browser/ui/cocoa/chrome_tracking_area.h (right):

http://codereview.chromium.org/6486002/diff/1/chrome/browser/ui/cocoa/chrome_...
chrome/browser/ui/cocoa/chrome_tracking_area.h:18:
scoped_nsobject<CrTrackingAreaOwnerProxy> ownerProxy_;
On 2011/02/11 19:53:58, rsesek wrote:
> On 2011/02/11 01:17:12, shess wrote:
> > Rather than using |ownerProxy_|, you can override -owner to return nil after
> > clear has been called.
> 
> Switched to this approach. Closer examination showed that
> |-methodSignatureForSelector:| can still end up messaging a zombie. Which left
> me with three options:
> 
> 1) Retain the owner in ownerProxy_. That diverges from the expectation of
> NSTrackingArea, which I don't like.
> 2) Override |-doesNotRecognizeSelector:| and do nothing if |!alive_|.
> 3) Ditch the proxy and just wrap |-owner|.
> 
> The last option seemed like the sanest. My only concern is if they use the
ivar
> directly rather than the accessor anywhere.

AFAICT from tracing messages, -mouseMoved: calls -owner a couple times, so I'd
be surprised if it directly uses the iVar.  That said, we could always override
-mouseMoved: itself to short-circuit things.

I'm not so concerned (at this point) about solving the problem of messaging the
zombie object as I am about figuring out if there really are cases where
removing a tracking rect can result in later tracking messages, and trying to
get an angle on how things get to this spot.

[Robert Sesek, 2011-02-14 17:28:35]

It turns out just doing it the simple way (overriding |-owner|) does not work.
The framework calls |-[owner respondsToSelector:@selector(mouseMoved:)]| which
of course returns NO when |owner==nil|. When that happens, the framework asserts
(stack below). I've switched back to using the proxy because that seems to be
the only recourse to get around that.

What I find interesting about this stack is that it occurs after the window has
closed...

2011-02-11 14:32:07.781 unit_tests[2814:903] *** Assertion failure in
-[FramedBrowserWindow
_setTrackingRect:inside:owner:userData:useTrackingNum:install:],
/SourceCache/AppKit/AppKit-1038.35/AppKit.subproj/NSWindow.m:3944
2011-02-11 14:32:07.782 unit_tests[2814:903] An uncaught exception was raised
2011-02-11 14:32:07.782 unit_tests[2814:903] Window: target of tracking rect
doesnt understand -mouseEntered: or -mouseExited messages
2011-02-11 14:32:07.882 unit_tests[2814:903] *** Terminating app due to uncaught
exception 'NSInternalInconsistencyException', reason: 'Window: target of
tracking rect doesnt understand -mouseEntered: or -mouseExited messages'
*** Call stack at first throw:
(
	0   CoreFoundation                      0x90fb86ba __raiseError + 410
	1   libobjc.A.dylib                     0x92624509 objc_exception_throw + 56
	2   CoreFoundation                      0x90fb83e8 +[NSException
raise:format:arguments:] + 136
	3   Foundation                          0x93143bb3 -[NSAssertionHandler
handleFailureInMethod:object:file:lineNumber:description:] + 116
	4   AppKit                              0x91780153 -[NSWindow
_setTrackingRect:inside:owner:userData:useTrackingNum:install:] + 596
	5   AppKit                              0x9177fef6 -[NSWindow
_setTrackingRect:inside:owner:userData:useTrackingNum:] + 99
	6   AppKit                              0x9177fcc9 -[NSView(NSInternal)
_installTrackingArea:] + 384
	7   AppKit                              0x91737b33 -[NSView(NSInternal)
_updateTrackingAreas] + 1176
	8   CoreFoundation                      0x90f192c0 CFArrayApplyFunction + 224
	9   AppKit                              0x91737bdc -[NSView(NSInternal)
_updateTrackingAreas] + 1345
	10  AppKit                              0x917375bb
_handleInvalidCursorRectsNote + 392
	11  CoreFoundation                      0x90f64e02 __CFRunLoopDoObservers +
1186
	12  CoreFoundation                      0x90f20d8d __CFRunLoopRun + 557
	13  CoreFoundation                      0x90f20464 CFRunLoopRunSpecific + 452
	14  CoreFoundation                      0x90f20291 CFRunLoopRunInMode + 97
	15  HIToolbox                           0x93a24004 RunCurrentEventLoopInMode +
392
	16  HIToolbox                           0x93a23cf7 ReceiveNextEventCommon + 158
	17  HIToolbox                           0x93a23c40
BlockUntilNextEventMatchingListInMode + 81
	18  AppKit                              0x9170d78d _DPSNextEvent + 847
	19  AppKit                              0x9170cfce -[NSApplication
nextEventMatchingMask:untilDate:inMode:dequeue:] + 156
	20  unit_tests                          0x00ad5b62 _ZN9CocoaTest8TearDownEv +
360
	21  unit_tests                          0x00acdf29
_ZN37BrowserWindowFullScreenControllerTest8TearDownEv + 55
	22  unit_tests                          0x01e851b8
_ZN7testingL35HandleExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS3_FS2_vEPKc
+ 107
	23  unit_tests                          0x01e8805f _ZN7testing4Test3RunEv + 271
	24  unit_tests                          0x01e8bc41 _ZN7testing8TestInfo3RunEv +
249
	25  unit_tests                          0x01e8bdca _ZN7testing8TestCase3RunEv +
230
	26  unit_tests                          0x01e8ca7a
_ZN7testing8internal12UnitTestImpl11RunAllTestsEv + 658
	27  unit_tests                          0x01e84fd0
_ZN7testingL35HandleExceptionsInMethodIfSupportedINS_8internal12UnitTestImplEbEET0_PT_MS4_FS3_vEPKc
+ 113
	28  unit_tests                          0x01e8504b _ZN7testing8UnitTest3RunEv +
111
	29  unit_tests                          0x03b176ca _ZN4base9TestSuite3RunEv +
216
	30  unit_tests                          0x01c773dc main + 75
	31  unit_tests                          0x001114aa start + 54
)

[Scott Hess - ex-Googler, 2011-02-14 19:25:38]

You results make me suspect that maybe when things are already tracking and the
window closes, there is a case where it unwinds with a -mouseMoved: and
-mouseExited: combo to close the stream, but the owner has been deallocated as
part of the -windowWillClose:.  Maybe it would be worth calling something like:

  CHECK(![[theView trackingAreas] containsObject:self]);

in the code doing the deallocation, to check if window-closing somehow
short-circuits a bunch of this stuff.  Another case I wonder about is adding
that same line before the call to -addTrackingArea: (can you add yourself
twice?).

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
File chrome/browser/ui/cocoa/tracking_area.mm (right):

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:12: @interface CrTrackingAreaOwnerProxy
: NSProxy {
I'm a tiny bit nervous about using NSProxy.  I think you're doing it right, but
it's not common in Chromium code, and the informal protocol we're supporting is
so small that a manual proxy would be pretty easy to implement, and probably no
more code than the NSProxy version.

I think implementing -[CrTrackingAreaOwnerProxy mouseMoved:] is possible with
your design, right?  That's the main case I imagine wanting to override to do
something interesting for debugging.

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:58: return YES;
Seems like [ownerClass_ instancesRespondToSelector:aSelector] would be better,
just in case.

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:77:
ownerProxy_.reset([[CrTrackingAreaOwnerProxy alloc] initWithOwner:owner]);
You can get away with this, but I don't think you should try.  Put a local
scoped_nsobject<> out here, and initialize ownerProxy_ if the super init
succeeds.

[Robert Sesek, 2011-02-14 20:46:35]

On 2011/02/14 19:25:38, shess wrote:
> You results make me suspect that maybe when things are already tracking and
the
> window closes, there is a case where it unwinds with a -mouseMoved: and
> -mouseExited: combo to close the stream, but the owner has been deallocated as
> part of the -windowWillClose:.

So the tracking areas are removed in |-dealloc| and the BWC is autoreleased in
|-windowWillClose:|, but on a 0-delay timer (to schedule on the main run loop,
which is key, apparently). I thought about the crash I posted, and I think it
may be a test-specific issue. The crash has CocoaTest::TearDown in it,
specifically the parts where we pump the event queue manually. That potentially
means that the objects aren't going to be cleaned up like they would be in
Chromium. I added some logging to demonstrate what I think is happening.

Normal Chromium:
Chromium[91533:903] BrowserWindowController windowWillClose:
Chromium[91533:903] CrTrackingArea clearOwner
Chromium[91533:903] CrTrackingArea clearOwner
Chromium[91533:903] BrowserWindowController dealloc
Chromium[91533:903] TabStripController dealloc
Chromium[91533:903] CrTrackingArea dealloc
Chromium[91533:903] CrTrackingArea dealloc

BrowserWindowControllerTest.TestStatusBubblePositioning:
unit_tests[91526:903] BrowserWindowController windowWillClose:
unit_tests[91526:903] CrTrackingArea clearOwner
unit_tests[91526:903] CrTrackingArea clearOwner
unit_tests[91526:903] pump event queue spin #0
unit_tests[91526:903] pump event queue spin #1
unit_tests[91526:903] BrowserWindowController dealloc
unit_tests[91526:903] TabStripController dealloc
unit_tests[91526:903] CrTrackingArea dealloc
unit_tests[91526:903] CrTrackingArea dealloc

BrowserWindowFullScreenControllerTest.TestFullscreen (where I saw the crash):
unit_tests[91526:903] BrowserWindowController windowWillClose:
unit_tests[91526:903] CrTrackingArea clearOwner
unit_tests[91526:903] CrTrackingArea clearOwner
unit_tests[91526:903] pump event queue spin #0
unit_tests[91526:903] *** Assertion failure in -[FramedBrowserWindow
_setTrackingRect:inside:owner:userData:useTrackingNum:install:],
/SourceCache/AppKit/AppKit-1038.35/AppKit.subproj/NSWindow.m:3944

This says to me that what happens between window close (and the scheduled
autorelease) and the next invocation of the main run loop, followed by the
draining of the main autorelease pool, are critical. And our tests muck with
that, which is causing the crash. When I experimented using the "simple" way in
just Chromium, I didn't see any issues. Still, the proxy does give us coverage
over this  edge case that the test hits, just in case it somehow get triggered
in Chromium.

>   CHECK(![[theView trackingAreas] containsObject:self]);
> 
> in the code doing the deallocation, to check if window-closing somehow
> short-circuits a bunch of this stuff.  Another case I wonder about is adding
> that same line before the call to -addTrackingArea: (can you add yourself
> twice?).

I haven't checked if it's possible to add yourself twice, but in this instance,
I don't see how it's possible. The two tracking areas constructed in |-init...|
and aren't ever modified. In other places where we see the tracking area
crashes, this may be a worthwhile CHECK to add.

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
File chrome/browser/ui/cocoa/tracking_area.mm (right):

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:12: @interface CrTrackingAreaOwnerProxy
: NSProxy {
On 2011/02/14 19:25:38, shess wrote:
> I'm a tiny bit nervous about using NSProxy.  I think you're doing it right,
but
> it's not common in Chromium code, and the informal protocol we're supporting
is
> so small that a manual proxy would be pretty easy to implement, and probably
no
> more code than the NSProxy version.

At this point, with the three dynamic messaging methods overridden, there's no
material difference between using NSObject and NSProxy. Since you're a little
nervous about NSProxy, I've switched it over to be an NSObject subclass.

> I think implementing -[CrTrackingAreaOwnerProxy mouseMoved:] is possible with
> your design, right?  That's the main case I imagine wanting to override to do
> something interesting for debugging.

Yes, this would be possible regardless of the superclass.

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:58: return YES;
On 2011/02/14 19:25:38, shess wrote:
> Seems like [ownerClass_ instancesRespondToSelector:aSelector] would be better,
> just in case.

Do'h. I was looking for that method but couldn't find it (I thought it was
instanceResponds...). Done.

http://codereview.chromium.org/6486002/diff/9001/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:77:
ownerProxy_.reset([[CrTrackingAreaOwnerProxy alloc] initWithOwner:owner]);
On 2011/02/14 19:25:38, shess wrote:
> You can get away with this, but I don't think you should try.  Put a local
> scoped_nsobject<> out here, and initialize ownerProxy_ if the super init
> succeeds.

Done.

[Scott Hess - ex-Googler, 2011-02-14 21:13:45]

LGTM, with the minor nits addressed (no need to re-review, unless you want me
to).

I concur that paranoid checks shouldn't be necessary, but at this point I kind
of feel like this is one of those Sherlock Holmes situations: "When you have
eliminated all which is impossible, then whatever remains, however improbable,
must be the truth."

As far as the testing infrastructure ... I hate that stuff.  I can understand
how Cocoa got the way it is (I was hanging around watching it happen!), but I
still wish they had designed better for testability.  Meanwhile we muddle
along...

http://codereview.chromium.org/6486002/diff/7004/chrome/browser/ui/cocoa/trac...
File chrome/browser/ui/cocoa/tracking_area.mm (right):

http://codereview.chromium.org/6486002/diff/7004/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:33: - (id)initWithOwner:(id)owner {
OK, but now you need if ((self = [super init])) {...}

http://codereview.chromium.org/6486002/diff/7004/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area.mm:78:
ownerProxy_.reset(ownerProxy.release());
Suggest ownerProxy_.swap(ownerProxy);.  I think your existing code is right, but
I definitely find that I have to think about it.  I think that swap() has
identical impact.

[Scott Hess - ex-Googler, 2011-05-28 00:00:32]

Saw this when reviewing to see if any unit tests were mucking with the zombie
code.

http://codereview.chromium.org/6486002/diff/7004/chrome/browser/ui/cocoa/trac...
File chrome/browser/ui/cocoa/tracking_area_unittest.mm (right):

http://codereview.chromium.org/6486002/diff/7004/chrome/browser/ui/cocoa/trac...
chrome/browser/ui/cocoa/tracking_area_unittest.mm:72: TEST_F(CrTrackingAreaTest,
ZombieOwner) {
Apologies for not catching this when first reviewed, but ... this doesn't do
what you want it to do.  The |NO| parameter means that the zombie code will call
-shouldBecomeCrZombie before turning an object into a zombie, and since nobody
overrides it, in this case the default is returned (NO).  That method is saying
"How I want to be treated", not "You should act this way".  So as written no
object will ever be made into a zombie, they'll always go through the default
paths.

Fortunately, if the zombie code _had_ kicked in, the line above "Don't crash"
would crash :-).

[Scott Hess - ex-Googler, 2011-05-28 00:07:59]

On 2011/05/28 00:00:32, shess wrote:
> Fortunately, if the zombie code _had_ kicked in, the line above "Don't crash"
> would crash :-).

Sorry, isAlive_ would prevent this.  Anyhow, I don't think the test is testing
anything.