[Mac] Update tab close button when user drags outside

ui/base/cocoa/hover_button.mm

Index: ui/base/cocoa/hover_button.mm
diff --git a/ui/base/cocoa/hover_button.mm b/ui/base/cocoa/hover_button.mm
index e763c0cdb69bf8281dd0b981dab4a676bbf7487a..962a26cb5f650e1d44f9382c769bd19325257128 100644
--- a/ui/base/cocoa/hover_button.mm
+++ b/ui/base/cocoa/hover_button.mm
@@ -10,14 +10,16 @@
 
 - (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];
@@ -33,30 +35,45 @@
     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;
-  // 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]);
-
-  [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];
+
+  // Begin tracking the mouse.
+  if ([theEvent type] == NSLeftMouseDown) {
+    NSWindow* window = [self window];
+    NSEvent* nextEvent = nil;
+
+    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 {
@@ -108,8 +125,12 @@
   // 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 {