Try to fix top crasher....

Try to fix top crasher.

The problem is timing related so I am unable to write a unit test which proves this fixes things.  The essence of the change is to make sure we don't send messages to objects which no longer exist by clearing the delegate pointer in objects we own before we go away (where "we" is an object).  The delegate pattern does not get retained, which gives certain objects to hold dangling pointers.  

BUG=53205
http://crbug.com/53205


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

[John Grabowski, 2010-08-25 01:52:01]

Going to try and get this in M6; please review carefully

[dhollowa, 2010-08-25 17:15:42]

http://codereview.chromium.org/3158031/diff/1/5
File chrome/browser/cocoa/bookmark_button.mm (right):

http://codereview.chromium.org/3158031/diff/1/5#newcode146
chrome/browser/cocoa/bookmark_button.mm:146: inside_ = YES;
Is it possible for the button to be created with the mouse already inside?  Then
dealloc might think we're not inside when we are.  Wondering if the initializer
should set |inside_| state explicitly based on mouse location and not rely on
movement to set it.

Another way to say it is... can we have a |mouseExited| fire prior to a
|mouseEntered|?

http://codereview.chromium.org/3158031/diff/1/6
File chrome/browser/cocoa/bookmark_button_cell.mm (right):

http://codereview.chromium.org/3158031/diff/1/6#newcode184
chrome/browser/cocoa/bookmark_button_cell.mm:184: // the retainCount of the cell
or it's owning control.
nit: s/it's/its/

[mrossetti, 2010-08-25 17:15:53]

http://codereview.chromium.org/3158031/diff/1/2
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/3158031/diff/1/2#newcode1376
chrome/browser/cocoa/bookmark_bar_controller.mm:1376: [button setDelegate:nil];
Since you probably want to perform the setDelegate:nil and removeFromSuperview
as a pair always, then perhaps you should consider either introducing a new
function (-[BookmarkButton detachFromParent]) or override removeFromSuperview so
that one does not accidentally call removeFromSuperview without also calling
setDelegate:nil.

http://codereview.chromium.org/3158031/diff/1/2#newcode1445
chrome/browser/cocoa/bookmark_bar_controller.mm:1445: [button setDelegate:nil];
If you add a consolidated function for de-delegating and removing then you could
retain the makeObjectsPerformSelector.

http://codereview.chromium.org/3158031/diff/1/3
File chrome/browser/cocoa/bookmark_bar_folder_controller.mm (right):

http://codereview.chromium.org/3158031/diff/1/3#newcode1364
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:1364: [oldButton
removeFromSuperview];
Do you need to setDelegate:nil here?

[John Grabowski, 2010-08-25 20:28:30]

new diffs up

Dave I wasn't able to repro your crash case but I did fix it in the code
(removal of the inside_ change.

http://codereview.chromium.org/3158031/diff/1/2
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/3158031/diff/1/2#newcode1376
chrome/browser/cocoa/bookmark_bar_controller.mm:1376: [button setDelegate:nil];
On 2010/08/25 17:15:53, mrossetti wrote:
> Since you probably want to perform the setDelegate:nil and removeFromSuperview
> as a pair always, then perhaps you should consider either introducing a new
> function (-[BookmarkButton detachFromParent]) or override removeFromSuperview
so
> that one does not accidentally call removeFromSuperview without also calling
> setDelegate:nil.

I think the only case I really care about is in dealloc.  Since we are going
away, we make sure buttons no longer have a pointer to us.  The rest of the
calls are defensive given the schedule.

 There is at least one case (the chevron) where we removeFromSuperview of a
bookmark button but don't want to clear the delegate.

http://codereview.chromium.org/3158031/diff/1/3
File chrome/browser/cocoa/bookmark_bar_folder_controller.mm (right):

http://codereview.chromium.org/3158031/diff/1/3#newcode1364
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:1364: [oldButton
removeFromSuperview];
On 2010/08/25 17:15:53, mrossetti wrote:
> Do you need to setDelegate:nil here?

yes; thx

http://codereview.chromium.org/3158031/diff/1/5
File chrome/browser/cocoa/bookmark_button.mm (right):

http://codereview.chromium.org/3158031/diff/1/5#newcode146
chrome/browser/cocoa/bookmark_button.mm:146: inside_ = YES;
On 2010/08/25 17:15:42, dhollowa wrote:
> Is it possible for the button to be created with the mouse already inside? 
Then
> dealloc might think we're not inside when we are.  Wondering if the
initializer
> should set |inside_| state explicitly based on mouse location and not rely on
> movement to set it.
> 
> Another way to say it is... can we have a |mouseExited| fire prior to a
> |mouseEntered|?

no.

In any case, as your experiment has shown, we don't want to call mouseExited: in
our dealloc, so the use of inside_ has been removed.

[mrossetti, 2010-08-25 21:10:34]

LGTM

[dhollowa, 2010-08-25 21:30:34]

I patched in the change and have been beating on it.  Can't trigger a zombie
now.  So LGTM.