372003 - Camino should remove tracking rects in viewWillMoveToWindow

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Cocoa supports the use of tracking rectangles to help keep track of
where the mouse is located -- it sends mouse-entered and mouse-exited
events whenever the mouse enters or exits one of these "tracking
rects".

The NSView class contains methods to add and remove tracking rects
(addTrackingRect and removeTrackingRect).  But the actual list of an
NSView's tracking rects is stored in the NSWindow object to whose view
hierachy the NSView object belongs.  So it's very important to remove
an NSView object's tracking rects when it's about to be deleted --
otherwise the NSWindow will keep sending it mouse-entered and
mouse-exited events, and crashes will result.  And it's also important
to call removeTrackingRect on an NSView object before it has been
removed from it's NSWindow's view hierarchy -- otherwise the call to
removeTrackingRect will have no effect (since the OS no longer knows
which NSWindow the NSView was attached to).

Intuitively, you'd think you should call removeTrackingRect when an
NSView object receives a removeFromSuperview message.  But I know from
personal experience that removeFromSuperview is sometimes called after
the NSView object has been removed from its NSWindow.

Apple has given a number of (coy and indirect) hints that it's best to
call removeTrackingRect from viewWillMoveToWindow.  This works for me
(I call removeTrackingRect when viewWillMoveToWindow is called with
its newWindow parameter set to nil).

In Apple's chapter on handling mouse tracking events
(http://developer.apple.com/documentation/Cocoa/Conceptual/EventOverview/MouseTrackingEvents/chapter_7_section_2.html),
listing 6-4 shows removeTrackingRect being called from
viewWillMoveToWindow, as an example of "removing a tracking rectangle
when a view is removed from its window".  In Apple's WebKit,
removeTrackingRect is called from the viewWillMoveToWindow method of
the WebBaseNetscapePluginView class.  And above this call there's the
following comment -- "We must remove the tracking rect before we move
to the new window.  Once we move to the new window, it will be too
late."

Cocoa widgets seems to handle this properly -- removeTrackingRect gets
called from nsChildView's viewWillMoveToWindow method.  But Camino
doesn't.

On the trunk, Camino calls removeTrackingRect from
RolloverImageButton's removeFromSuperview method.  And on the 1.8
branch Camino doesn't make any attempt to call removeTrackingRect as
an NSView object that might "contain" tracking rects is being
destroyed (the code seems to assume that these tracking rects will
always already have been removed by other means).

In my experience, crashes due to leftover tracking rects (ones that
still exist after their NSView object has been deleted) always take
place in [NSWindow sendEvent:].  A couple of examples that I've seen
recently have the following at the top of the thread 0 stack trace:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xa1b1c1f3

Thread 0 Crashed:
0   <<00000000>>        0xfffeff18 objc_msgSend_rtp + 24
1   com.apple.AppKit    0x93768fa0 -[NSWindow sendEvent:] + 6424
...

The crashes I saw were on OS X 10.4.8 in Camino 1.1b (there were two
in quick succession), while I had four or five tabs open (each
containing at least one Java applet), with the map24.com in the
current tab, while I was resizing the browser window.

At first I thought this might be a JEP bug.  But I recognized the
"lost tracking rect crash" signature.  And I found out that, while
Apple's JVM also uses tracking rects, it seems to always remove them
correctly (it seems to always call removeTrackingRect from
viewWillMoveToWindow).

I've found my crashes very difficult to reproduce -- I haven't seen
any more since the first two.  But I've noticed several other BMO bugs
that might be caused by "lost tracking rects" -- particularly bug
370954.  (Some other possibilities are bug 371550 and bug 359488.)

Comment 1

[Stuart Morgan]

(In reply to comment #0)

> Intuitively, you'd think you should call removeTrackingRect when an
> NSView object receives a removeFromSuperview message.  But I know from
> personal experience that removeFromSuperview is sometimes called after
> the NSView object has been removed from its NSWindow.

Nice. Thanks for the tip; I knew we were chasing a tracking rect problem in bug 370954, but I added new guards in the wrong place. I really should have thought to go back and look at that doc again, but I didn't.

I'll make a new patch to do the right thing.

> On the trunk, Camino calls removeTrackingRect from
> RolloverImageButton's removeFromSuperview method.  And on the 1.8
> branch Camino doesn't make any attempt to call removeTrackingRect as
> an NSView object that might "contain" tracking rects is being
> destroyed (the code seems to assume that these tracking rects will
> always already have been removed by other means).

Could you elaborate here? If we are talking just about RolloverImageButton, there shouldn't be any differences between trunk and branch.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

> Could you elaborate here?

When I say "1.8 branch" I mean "Camino 1.0.3" ... for which there's a
nice and convenient source download :-)

There actually isn't a RolloverImageButton in the Camino 1.0.3 source
download.  But since the only existing (indirect) call to
removeTrackingRect is called on BrowserTabBarView, that's where I'd
add a viewWillMoveToWindow method (from which I'd call
removeTrackingRect on "self").  (The existing call is "[tab
removeTrackingRectFromView: self]" in BrowserTabBarView's
unregisterTabButtonsForTracking).

Since BrowserTabBarView is (I think) the only view that uses tracking
rects even on trunk, it might be a good idea to add a
viewWillMoveToWindow method to that class on the trunk, too (and
replace the indirect call to removeTrackingRect in
RolloverImageButton's removeFromSuperview with a direct call in
BrowserTabBarView's new viewWillMoveToWindow method).

Comment 3

[Stuart Morgan]

Attachment: fix RolloverImageButton

> Since BrowserTabBarView is (I think) the only view that uses tracking
> rects even on trunk

Definitely not true on trunk; RolloverImageButton manages its own rects, and given that that's the code that started being used more when things got worse, that's probably most/all of the crashes.

This should be the right fix for RIB, using viewWillMoveToWindow:. It also moves the tracking rect to self, rather than superview; I'm not clear on why it wasn't already that way, and having things based on the superview when we don't know the tear-down order seems like a recipe for pain.

I'll leave this open to audit the other uses.

Comment 4

[Stuart Morgan]

(In reply to comment #3)
> Definitely not true on trunk

By which I actually mean, definitely not true on trunk or 1.8 branch. 1.0.3 is not 1.8 branch, it's a mini-branch off of the version of 1.8 that was current when 1.0 was released.

Comment 5

[Samuel Sidler (old account; do not CC)]

(In reply to comment #4)
> (In reply to comment #3)
> > Definitely not true on trunk
> 
> By which I actually mean, definitely not true on trunk or 1.8 branch. 1.0.3 is
> not 1.8 branch, it's a mini-branch off of the version of 1.8 that was current
> when 1.0 was released.

By which you mean, it's a minibranch off the 1.8.0 branch each time we release a new 1.0.x release.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

>> Since BrowserTabBarView is (I think) the only view that uses tracking
>> rects even on trunk
>
> Definitely not true on trunk;

As far as I can tell, BrowserTabBarView is RolloverImageButton's
superview.  If so, BrowserTabBarView _is_ the only view that uses
tracking rects (on trunk or the branches) ... though with your patch
it will now be RolloverImageButton on the trunk.

You patch should work fine on the trunk.  But I suspect it's only an
accident that crashes haven't been reported on the branches.
viewWillMoveToWindow should also be used on the branches -- on
whatever view(s) that tracking rects are added to or removed from.

Comment 7

[Stuart Morgan]

(In reply to comment #6)
> As far as I can tell, BrowserTabBarView is RolloverImageButton's
> superview.

The popup blocker uses it as well.

> You patch should work fine on the trunk.  But I suspect it's only an
> accident that crashes haven't been reported on the branches.
> viewWillMoveToWindow should also be used on the branches

My patch is for both trunk and the 1.8 branch (which is the only branch that is under active development; 1.0.x is pretty much just security fixes only). Crashes have been reported on the 1.8 branch.

As I said, I'll leave this open to deal with the tabs themselves.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #7)

I stand corrected on both counts (assuming that there will never be
another 1.0.X branch release).

I took another look at the trunk code and noticed that there's a
mBlockedPopupCloseButton RolloverImageButton created in a nib file and
(presumably) added to a mBlockedPopupView NSView.

Sneaky, sneaky :-)

Comment 9

[Stuart Morgan]

(In reply to comment #8)
> I stand corrected on both counts (assuming that there will never be
> another 1.0.X branch release).

There will be, and I'll probably back-port the tab patch. On the other hand, I think accidents of the complex code paths that tabs use left us safe, since we weren't really seeing any of these crashes in the 1.0 timeframe (AFAIK).

Comment 10

[Josh Aas]

Comment on attachment 256712 [details] [diff] [review]
fix RolloverImageButton

+- (void)setBounds:(NSRect)inBounds
+{
+  NSLog(@"calling setBounds:");

Do you really want to leave a live NSLog in there? Probably either take that out or mark it with a debug macro.

Comment 11

[Stuart Morgan]

Comment on attachment 256712 [details] [diff] [review]
fix RolloverImageButton

I'll pull the NSLog out on checkin.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 256712 [details] [diff] [review]
fix RolloverImageButton

sr=pink

Comment 13

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH (sans NSLog). Again, thanks Steven! You saved us a lot of debugging here.

Leaving open until I check over the tab tracking rects.

Comment 14

[Stuart Morgan]

It looks like the tab code is safe, but not in a robust way. The tracking rects are removed in a method called from a windowWillClose handler (this method is called windowClosed, confusingly enough), and hiding the tabs just shrinks the view, rather than removing it, so windowWillClose should cover the only case.

So while our current tab usage works, it's not hard to imagine usage where it wouldn't be, so I'll definitely fix it. It won't need a 1.0.x port though. Marking as 1.2 since it should definitely be done before then, but I'll likely fix it for 1.1

Comment 15

[Stuart Morgan]

Attachment: fix BrowserTabBarView

Here's the BrowserTabBarView fix. It removes the now-unnecessary windowClosed method on BTBV.

Comment 16

[Mike Pinkerton (not reading bugmail)]

why have we switched from doing this when the window closes to doing it when it moves to a view? shouldn't we be checking the param for nil here, not just always doing it?

Comment 17

[Stuart Morgan]

If they are moving to another window, then all the tracking rects are going to be hosed since they are, under the covers, associated with a specific window. They would be built up again correctly in the new window.

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 263428 [details] [diff] [review]
fix BrowserTabBarView

sr=pink

Comment 19

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH.