[Mac] Update tab close button when user drags outside

ui/base/cocoa/hover_button.mm

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ui/base/cocoa/hover_button.h"
 
 @implementation HoverButton
 
 @synthesize hoverState = hoverState_;
 
 - (id)initWithFrame:(NSRect)frameRect {
   if ((self = [super initWithFrame:frameRect])) {
-    [self setTrackingEnabled:YES];
-    hoverState_ = kHoverStateNone;
-    [self updateTrackingAreas];
+    [self commonInit];
   }
   return self;
 }
 
 - (void)awakeFromNib {
+  [self commonInit];
+}
+
+- (void)commonInit {
   [self setTrackingEnabled:YES];
   self.hoverState = kHoverStateNone;
   [self updateTrackingAreas];
 }
 
 - (void)dealloc {
   [self setTrackingEnabled:NO];
   [super dealloc];
 }
 
 - (void)mouseEntered:(NSEvent*)theEvent {
   if (trackingArea_.get())
     self.hoverState = kHoverStateMouseOver;
 }
 
+- (void)mouseMoved:(NSEvent*)theEvent {
+  [self checkImageState];
+}
+
 - (void)mouseExited:(NSEvent*)theEvent {
   if (trackingArea_.get())
     self.hoverState = kHoverStateNone;
 }
 
-- (void)mouseMoved:(NSEvent*)theEvent {
-  [self checkImageState];
-}
+- (void)mouseDown:(NSEvent*)theEvent {
+  mouseDown_ = YES;
+  self.hoverState = kHoverStateMouseDown;
 
-- (void)mouseDown:(NSEvent*)theEvent {
-  self.hoverState = kHoverStateMouseDown;
-  // The hover button needs to hold onto itself here for a bit.  Otherwise,
-  // it can be freed while |super mouseDown:| is in its loop, and the
-  // |checkImageState| call will crash.
-  // http://crbug.com/28220

[Avi (use Gerrit), 2017/05/25 03:59:54]

The crash in this bug seems plausible with your new code; do we still need to
have the same precaution to prevent a regression?

[shrike, 2017/05/25 17:07:22]

The comment says that the button can get freed within the call to [super
mouseDown:], but with my change we no longer call [super mouseDown:]. It seems
like we should no longer need the retain?

[Avi (use Gerrit), 2017/05/25 18:24:23]

Right, but this moves the functionality of [super mouseDown:] here; it doesn't
solve the issue.

The issue is what happens if the user is clicking and tracking the mouse down
while the tab is being animated closed. In that situation, in your new code, you
have the same problem: you're in a loop in -mouseDown: while the object is
getting deleted from under you.

If you work through the repro steps of the bug and it doesn't crash with the new
code, OK, but it looks like it's a valid concern.

[shrike, 2017/05/25 18:31:00]

I see. I thought the issue was what was happening inside of [super mouseDown:].

I tried working through the repro steps but couldn't figure them out - do they
make sense to you? I'm not sure how to close the tab while I'm clicking down on
the button (if I'm understanding things correctly). I'm fine with putting the
retain (and comment) back, but I'm wondering how to actually repro the release
bug.

-  base::scoped_nsobject<HoverButton> myself([self retain]);
+  // Begin tracking the mouse.
+  if ([theEvent type] == NSLeftMouseDown) {
+    NSWindow* window = [self window];
+    NSEvent* nextEvent = nil;
 
-  [super mouseDown:theEvent];
-  // We need to check the image state after the mouseDown event loop finishes.
-  // It's possible that we won't get a mouseExited event if the button was
-  // moved under the mouse during tab resize, instead of the mouse moving over
-  // the button.
-  // http://crbug.com/31279
-  [self checkImageState];
+    while ((nextEvent = [window
+                nextEventMatchingMask:(NSLeftMouseDraggedMask |
+                                       NSLeftMouseUpMask | NSKeyDownMask)])) {
+      // Update the image state, which will change if the user moves the mouse
+      // into or out of the button.
+      [self checkImageState];
+
+      if ([nextEvent type] == NSLeftMouseUp) {
+        break;
+      }
+    }
+  }
+
+  // If the mouse is still over the button, it means the user clicked the
+  // button.
+  if (self.hoverState == kHoverStateMouseDown) {
+    [self performClick:nil];
+  }
+
+  // Clean up.
+  mouseDown_ = NO;
 }
 
 - (void)setAccessibilityTitle:(NSString*)accessibilityTitle {
   NSCell* cell = [self cell];
   [cell accessibilitySetOverrideValue:accessibilityTitle
                          forAttribute:NSAccessibilityTitleAttribute];
 }
 
 - (void)setTrackingEnabled:(BOOL)enabled {
   if (enabled) {
@@ skipping 31 matching lines @@
   [self checkImageState];
 }
 
 - (void)checkImageState {
   if (!trackingArea_.get())
     return;
 
   // Update the button's state if the button has moved.
   NSPoint mouseLoc = [[self window] mouseLocationOutsideOfEventStream];
   mouseLoc = [self convertPoint:mouseLoc fromView:nil];
-  self.hoverState = NSPointInRect(mouseLoc, [self bounds]) ?
-      kHoverStateMouseOver : kHoverStateNone;
+  BOOL mouseInBounds = NSPointInRect(mouseLoc, [self bounds]);
+  if (mouseDown_ && mouseInBounds) {
+    self.hoverState = kHoverStateMouseDown;
+  } else {
+    self.hoverState = mouseInBounds ? kHoverStateMouseOver : kHoverStateNone;
+  }
 }
 
 - (void)setHoverState:(HoverState)state {
   BOOL stateChanged = (hoverState_ != state);
   hoverState_ = state;
   [self setNeedsDisplay:stateChanged];
 }
 
 @end