MacViews: Implement Tab Dragging

ui/views/cocoa/cocoa_window_move_loop.mm

+// Copyright 2016 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.
+
+#include "ui/views/cocoa/cocoa_window_move_loop.h"
+
+#import <Cocoa/Cocoa.h>
+
+#include "base/run_loop.h"
+#import "ui/gfx/mac/coordinate_conversion.mm"
+#import "ui/views/cocoa/bridged_content_view.h"
+#import "ui/views/cocoa/bridged_native_widget.h"
+
+// CocoaWindowMoveLoop can be deleted just before its local event monitor is
+// processed and in that case it's too late to call removeMonitor: and we have a
+// dangling this pointer. Use a proxy Obj-C class to store the weakptr.
+@interface WeakCocoaWindowMoveLoop : NSObject {

[tapted, 2016/04/13 08:28:13]

I still think this is unnecessary -- I think the issue is just that the event
monitor was only checking for WINDOW_DESTROYED but it also needs to check for
ENDED_EXTERNALLY -- either of those may indicate the destructor being called
(since End() prevents the destructor updating to ENDED_EXTERNALLY). *except* It
can't check for ENDED_EXTERNALLY since that's what the exit reason is
initialized with :p. So I think it needs a new enum value - that's probably
clearer anyway.

However, the weak_factory_ is needed for another reason - see below.

See also https://codereview.chromium.org/1877043003 where I've removed this
@interface

[themblsha, 2016/04/18 09:30:02]

In my testing the sometimes run_loop was Quit()'d, the monitor was unregistered,
the *whole stack frame* unwound, and only then I got the monitor notification.
Your approach would result access to the memory that was in the removed stack
frame, and results would be undetermined.

[tapted, 2016/04/20 07:30:22]

I'm not sure this is true - did asan confirm or deny this after applying the
code to remove the @interface.

We don't want the code to suggest the problem is more complex than it is.

Here, I think we would only require the @interface if calls could be made to the
EventMonitor after CocoaWindowMoveLoop::Run() has returned (regardless of
whether CocoaWindowMoveLoop itself has been destroyed).

Since CocoaWindowMoveLoop::Run() removes the EventMonitor before it returns,
that should be impossible. So, as long as EventMonitor verifies its state first
by using stack variables declared inside Run(), we should be able to avoid UAF.

[themblsha, 2016/04/20 13:38:40]

Sorry for not being clear in my explanations at what's happening here.

On 2016/04/20 07:30:22, tapted wrote:
I didn't try it on an ASAN build as it takes a while to recompile, but even
without ASAN it crashes fairly frequently to see the issue.
How the crash happens (http://pastie.org/10799043):
1. We get an ESC keypress event, -[NSApplication sendEvent:] calls
_NSSendEventToObservers and it *caches* the list of all registered event
monitors.
2. views::EventMonitorMac::EventMonitorMac()'s monitor gets the ESC keypress,
and it ends the drag and the CocoaMoveLoop.
3. CocoaMoveLoop::End() quits the run_loop.
4. CocoaMoveLoop event monitor is unregistered, but is still cached by
_NSSendEventToObservers.
5. _NSSendEventToObservers sends ESC keypress to the CocoaMoveLoop's monitor,
even though it's supposed to be unregistered at this point. At this point the
CocoaMoveLoop::Run()'s stack frame is unwound and the object is destroyed.

You might not be getting this crash because the ESC-cancelling of drag operation
is somehow not working for you, but the event propagation to the event monitors
should be similar.

[tapted, 2016/04/27 07:43:00]

Ah, that is likely. So it does sound like we need WeakCocoaWindowMoveLoop -
that's fine.

The bigger problem is that, for me, ESC is definitely not working. This is on a
machine running El Capital 10.11.4 -- does that match your configuration?

I've spent a good chunk of time tracing this. But the NSKeyDown event simply
isn't delivered to [NSApp sendEvent] during a window drag -- the EscapeTracker
in tab_drag_controller.cc never sees it.


I traced this further by adding code like:

  CGEventMask event_mask = CGEventMaskBit(kCGEventKeyDown) |
CGEventMaskBit(kCGEventLeftMouseUp);
  mouse_mach_port_.reset(CGEventTapCreate(
      kCGAnnotatedSessionEventTap,
      kCGHeadInsertEventTap,
      kCGEventTapOptionListenOnly,  // passive listener.
      event_mask,
      &CallBack,
      this));
  DLOG(INFO) << "made mach port";
  if (!mouse_mach_port_)
    return Widget::MOVE_LOOP_CANCELED;

  mouse_run_loop_source_.reset(
      CFMachPortCreateRunLoopSource(NULL, mouse_mach_port_, 0));
  if (!mouse_run_loop_source_)
    return Widget::MOVE_LOOP_CANCELED;
  DLOG(INFO) << "EventTAp!";

  CFRunLoopAddSource(
      CFRunLoopGetMain(), mouse_run_loop_source_, kCFRunLoopCommonModes);
  CGEventTapEnable(mouse_mach_port_, true);


This allows the key down event to be seen, but only when I run the application
as root, or with accessibility modes on. This lets me see the event with the
stack:

  * frame #0: 0x0000000111c4ac76 libbase.dylib`base::debug::BreakDebugger() + 38
at debugger_posix.cc:249
    frame #1: 0x00000001293f377a
libviews.dylib`CallBack(proxy=0x000000012a026d10, type=kCGEventKeyDown,
event=0x00000001002cf310, refcon=0x000000013af72c40) + 458 at
cocoa_window_move_loop.mm:30
    frame #2: 0x00007fff8d7b4e73 CoreGraphics`processEventTapData + 641
    frame #3: 0x00007fff8d696b2f CoreGraphics`_XPostEventTapData + 269
    frame #4: 0x00007fff8d7b4b9c CoreGraphics`eventTapMessageHandler + 133
    frame #5: 0x00007fff8e7a712c CoreFoundation`__CFMachPortPerform + 252
    frame #6: 0x00007fff8e7a7019
CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41
    frame #7: 0x00007fff8e7a6f89 CoreFoundation`__CFRunLoopDoSource1 + 473
    frame #8: 0x00007fff8e79e9bb CoreFoundation`__CFRunLoopRun + 2171
    frame #9: 0x00007fff8e79ded8 CoreFoundation`CFRunLoopRunSpecific + 296
    frame #10: 0x00007fff8cd1d935 HIToolbox`RunCurrentEventLoopInMode + 235
    frame #11: 0x00007fff8cd1d76f HIToolbox`ReceiveNextEventCommon + 432
    frame #12: 0x00007fff8cd1d5af
HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
    frame #13: 0x00007fff9bec6efa AppKit`_DPSNextEvent + 1067
    frame #14: 0x00007fff9bec632a AppKit`-[NSApplication
_nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
    frame #15: 0x0000000111bf86d9
libbase.dylib`base::MessagePumpNSApplication::DoRun(this=0x000000013af0ad90,
delegate=0x000000013af0ab20) + 505 at message_pump_mac.mm:671
    frame #16: 0x0000000111bf6ecd
libbase.dylib`base::MessagePumpCFRunLoopBase::Run(this=0x000000013af0ad90,
delegate=0x000000013af0ab20) + 125 at message_pump_mac.mm:238
    frame #17: 0x0000000111d22b7a
libbase.dylib`base::MessageLoop::RunHandler(this=0x000000013af0ab20) + 298 at
message_loop.cc:443
    frame #18: 0x0000000111dce525
libbase.dylib`base::RunLoop::Run(this=0x00007fff5fbfbd58) + 85 at run_loop.cc:35
    frame #19: 0x00000001293f4791
libviews.dylib`views::CocoaWindowMoveLoop::Run(this=0x000000013af72c40) + 1649
at cocoa_window_move_loop.mm:213


but then the event is swallowed while still inside CoreFoundation --
_nextEventMatchingEventMask:untilDate:inMode:dequeue only returns when another
event comes in, so the keydown is getting lost.

I'm suspecting this is something that has changed in 10.11. Sadly, I don't
really have time right now to tear apart the assembly code to diagnose it
further. Do you want to have a go at this, if you can get into a state where you
can reproduce the esc-not-working.


If we can't get this working with Esc, that might be a deal-breaker :(. I don't
like it, but we might need to fallback to the crappy method that
TabStripDragController uses in the Cocoa browser, which doesn't let you move
tabs between spaces.


Or.... we might be willing to sacrifice ESC. It's actually broken on Cocoa right
now -- ESC will cancel the drag, but the dragged-out tab doesn't move back into
the window it came from the way it does on other platforms. If we do that we
shouldn't bother with adding code to handle it at all (e.g. not bother adding
NSKeyDownMask to the event Mask.


Interestingly, it's actually a bug on the Cooca browser that it accepts key
events while a tab is being dragged. E.g. selecting an input field on some web
contents, then dragging that tab, then typing, then releasing will make the
typed text appear in the input field.

But of course dragging the whole window suppresses the keydowns "somewhere". Can
we figure out a way to switch that "off" perhaps?

[themblsha, 2016/04/28 17:39:29]

Turns out I had TextExpander 5 installed, and it made the Esc-drag-cancelling
work, so that's one mystery solved (it's not related to OS X version, I've
tested both on Yosemite and El Capitan).

Safari also accepts keyboard events into input fields while the window is being
dragged.

I've started reverse-engineering the -[NSApplication run], it consists of this:

      rbx = [NSAutoreleasePool new];
      [self setWindowsNeedUpdate:0x1];
      [rbx drain];
      while (self->_running != 0x0) {
        id pool = [NSAutoreleasePool new];
        [self nextEventMatchingMask:0xffffffffffffffff
                          untilDate:[NSDate distantFuture]
                             inMode:_kCFRunLoopDefaultMode
                            dequeue:YES];
        if (objc_collectingEnabled() != 0x0) {
          objc_clear_stack(0x0);
        }
        BOOL suddenTerminationEnabled = [self _disableSuddenTermination];
        id event = [self->_currentEvent retain];
        [self sendEvent:event];
        [event release];
        [NSUndoManager _endTopLevelGroupings];
        if (suddenTerminationEnabled != 0x0) {
          [self _enableSuddenTermination];
        }
        [pool drain];
      }

So it seems that the -nextEventMatchingMask:untilDate:inMode:dequeue: is the
actual culprit, will continue the hunt tomorrow. And then I'll be on a 2-week
vacation.

[themblsha, 2016/04/28 18:01:51]

Ah, I went on a false start as both the Esc keypress and the mouse dragging
events are both getting sent using the MessagePumpNSApplication::DoRun(). But
the cause for the error is still that the
-nextEventMatchingMask:untilDate:inMode:dequeue: is not returning the Esc
keypress for some reason.

So I'm going to investigate that.

+ @private
+  base::WeakPtr<views::CocoaWindowMoveLoop> weak_;
+}
+@end
+
+@implementation WeakCocoaWindowMoveLoop
+- (id)initWithWeakPtr:(const base::WeakPtr<views::CocoaWindowMoveLoop>&)weak {
+  if ((self = [super init])) {
+    weak_ = weak;
+  }
+  return self;
+}
+
+- (base::WeakPtr<views::CocoaWindowMoveLoop>&)weak {
+  return weak_;
+}
+@end
+
+namespace {
+
+// Returns current mouse position in screen coordinates.
+gfx::Point GetMousePosition() {
+  CGPoint cg_point = CGEventGetLocation(
+      base::ScopedCFTypeRef<CGEventRef>(CGEventCreate(nullptr)));
+  return gfx::Point(cg_point.x, cg_point.y);
+}
+
+// Send the mouse event of |type| to the |expected_window|, located at
+// |mouse_position|. |expected_window| could be nil if we don't know which
+// window is supposed to receive it.
+void SendCustomLeftMouseEvent(NSWindow* expected_window,
+                              gfx::Point mouse_position,

[tapted, 2016/04/13 08:28:13]

nit: const ref

[themblsha, 2016/04/18 09:30:02]

Done.

+                              CGEventType type) {
+  DCHECK(type == kCGEventLeftMouseDown || type == kCGEventLeftMouseUp);
+
+  // Check that we're sending the event to the window we expect.
+  if (expected_window) {
+    NSPoint ns_mouse_position = gfx::ScreenPointToNSPoint(mouse_position);

[tapted, 2016/04/13 08:28:13]

This should move to a separate function, since it's not used in a release build
and the compiler can't optimize out the calls because of possible side-effects.

E.g.

NSWindow* WindowAtScreenPoint(const gfx::Point& point) { ...
DCHECK_EQ(expected_window, WindowAtScreenPoint(mouse_position));

[themblsha, 2016/04/18 09:30:02]

Done.

+    NSInteger window_number = [NSWindow windowNumberAtPoint:ns_mouse_position
+                                belowWindowWithWindowNumber:0];
+    NSWindow* current_window = [[NSApplication sharedApplication]
+        windowWithWindowNumber:window_number];
+    DCHECK_EQ(expected_window, current_window);
+  }
+
+  base::ScopedCFTypeRef<CGEventRef> event(CGEventCreateMouseEvent(
+      nullptr, type, CGPointMake(mouse_position.x(), mouse_position.y()),
+      kCGMouseButtonLeft));
+
+  CGEventSetIntegerValueField(
+      event, kCGEventSourceUserData,
+      views::kCocoaWindowMoveLoopSimulatedEventUserData);
+  CGEventPost(kCGSessionEventTap, event);
+}
+
+}  // namespace
+
+namespace views {
+
+const int kCocoaWindowMoveLoopSimulatedEventUserData = 1;
+
+CocoaWindowMoveLoop::CocoaWindowMoveLoop(BridgedNativeWidget* owner,
+                                         gfx::Point initial_mouse_in_screen)
+    : owner_(owner),
+      run_loop_(new base::RunLoop),
+      initial_mouse_in_screen_(initial_mouse_in_screen),
+      quit_closure_(run_loop_->QuitClosure()),
+      weak_factory_(this) {
+  // AppKit continues to send mouse events to the window, but toolkit-views
+  // doesn't expect them during RunMoveLoop().
+  [BridgedContentView setIgnoreMouseEvents:YES];
+  owner_->SetDraggable(true);
+}
+
+CocoaWindowMoveLoop::~CocoaWindowMoveLoop() {
+  owner_->SetDraggable(false);
+  // Note the crafted events in Run() should not make their way through to
+  // toolkit-views, but regular events after that should be. So stop ignoring.
+  [BridgedContentView setIgnoreMouseEvents:NO];
+
+  // Handle the pathological case, where |this| is destroyed while running.
+  if (exit_reason_ref_) {
+    *exit_reason_ref_ = WINDOW_DESTROYED;
+    quit_closure_.Run();
+  }
+
+  owner_ = nullptr;
+}
+
+Widget::MoveLoopResult CocoaWindowMoveLoop::Run() {
+  LoopExitReason exit_reason = ENDED_EXTERNALLY;
+  exit_reason_ref_ = &exit_reason;
+  NSWindow* window = owner_->ns_window();
+
+  // Move the RunLoop to the stack so our destructor can be called while it is
+  // running.
+  scoped_ptr<base::RunLoop> run_loop(std::move(run_loop_));

[tapted, 2016/04/13 08:28:13]

I think this might not be necessary, and we can just do

  base::RunLoop run_loop;
  quit_closure_ = run_loop.QuitClosure();

(and remove the run_loop_ data member).

[themblsha, 2016/04/18 09:30:02]

Yep, works fine.

+
+  // A new window may have just been created, so post an event at the session
+  // tap level to initiate a window drag.
+  SendCustomLeftMouseEvent(window, initial_mouse_in_screen_,
+                           kCGEventLeftMouseDown);
+
+  // Will be retained by the monitor handler block.
+  WeakCocoaWindowMoveLoop* weak_cocoa_window_move_loop =
+      [[[WeakCocoaWindowMoveLoop alloc]
+          initWithWeakPtr:weak_factory_.GetWeakPtr()] autorelease];
+
+  NSEventMask mask = NSLeftMouseUpMask | NSKeyDownMask | NSLeftMouseDraggedMask;
+  auto handler = ^NSEvent*(NSEvent* event) {
+    CocoaWindowMoveLoop* strong = [weak_cocoa_window_move_loop weak].get();
+    if (!strong) {
+      // By this point CocoaWindowMoveLoop was deleted while processing this
+      // same event, and this event monitor was not unregistered in time. It is
+      // reproducible by the PressEscapeWhileDetached test.
+      // Continue processing the event.
+      return event;
+    }
+
+    if ([event type] == NSLeftMouseDragged) {
+      // AppKit doesn't supply position updates during a drag, so post a task to
+      // notify observers once AppKit has moved the window.
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&BridgedNativeWidget::OnPositionChanged,
+                                base::Unretained(strong->owner_)));

[tapted, 2016/04/13 08:28:13]

I don't think this can be Unretained - this should pass
weak_factory_.GetWeakPtr() and call a method on CocoaWindowMoveLoop which
invokes owner_->OnPositionChanged

see what I did in https://codereview.chromium.org/1877043003

[themblsha, 2016/04/18 09:30:02]

Nice idea, thanks.

+      return event;
+    }
+
+    strong->quit_closure_.Run();
+    if ([event type] == NSLeftMouseUp) {
+      *strong->exit_reason_ref_ = MOUSE_UP;
+      return event;  // Process the MouseUp.
+    }
+    *strong->exit_reason_ref_ = ESCAPE_PRESSED;
+    return nil;  // Swallow the keypress.
+  };
+  id monitor =
+      [NSEvent addLocalMonitorForEventsMatchingMask:mask handler:handler];
+
+  // NSKeyDownMask doesn't work inside addLocalMonitorForEventsMatchingMask:

[tapted, 2016/04/13 08:28:13]

This is still the case right?  Did you investigate ways to fix this and get
Escape to work properly? I think we need this to work. It "works" in tests
because the test event goes in at a different level, but when *actually*
dragging tabs and pressing ESC, the tab isn't going back into the window it was
previously in.

[themblsha, 2016/04/18 09:30:02]

For me it works fine on live Chromium. The block
views::CocoaMouseCapture::ActiveEventTap::Init() captures the Esc keypress and
correctly ends the drag. So I consider that comment to be accurate.

Here's my stacktrace: http://pastie.org/10799043

+  // the event is swallowed by the window move loop before it gets to -[NSApp
+  // sendEvent:]. To see an escape keypress, hook in an event tap lower.
+
+  run_loop->Run();
+  [NSEvent removeMonitor:monitor];
+
+  if (exit_reason != WINDOW_DESTROYED && exit_reason != ENDED_EXTERNALLY) {
+    exit_reason_ref_ = nullptr;  // Ensure End() doesn't replace the reason.
+    owner_->EndMoveLoop();       // Deletes |this|.
+  }
+
+  if (exit_reason != MOUSE_UP) {
+    // Tell AppKit to stop moving the window. Otherwise, AppKit will refuse to
+    // start a new window drag. Note the window being dragged is going away in
+    // this case, so it doesn't really matter where the event is located.
+
+    if (exit_reason == ENDED_EXTERNALLY) {
+      // When not canceled, the non-moving drag in the original window must
+      // resume. To do this, AppKit needs to see a mouseDown so that it sends
+      // the correct events. Ideally, it also needs to be at the original
+      // offset, so that it hits a non-draggable region of the original
+      // window: The tab being dragged may move some distance from the cursor
+      // when it "snaps in", so the cursor may not be over a tab. Sadly, this
+      // method doesn't know which window that is. But all that really needs
+      // to be done is to prevent a custom-dragging area from starting a
+      // window-drag. So hook into the logic in RepostEventIfHandledByWindow()
+      // by setting a flag here. Note this assumes the custom mouseDown event
+      // is guaranteed to hit another BridgedNativeWidget when it gets to the
+      // front of the event queue.
+      // TODO(tapted): A better fix would be to keep the temporary window
+      // around and never call EndMoveLoop() on Mac, making this block of code
+      // obsolete.
+      BridgedNativeWidget::IgnoreNextMouseDownForDraggableRegions();
+      SendCustomLeftMouseEvent(nil, GetMousePosition(),
+                               kCGEventLeftMouseDown);
+    } else {
+      SendCustomLeftMouseEvent(window, GetMousePosition(),
+                               kCGEventLeftMouseUp);
+    }
+  }
+
+  return exit_reason == ESCAPE_PRESSED ? Widget::MOVE_LOOP_CANCELED
+                                       : Widget::MOVE_LOOP_SUCCESSFUL;
+}
+
+void CocoaWindowMoveLoop::End() {
+  if (exit_reason_ref_) {
+    DCHECK_EQ(*exit_reason_ref_, ENDED_EXTERNALLY);
+    // Ensure the destructor doesn't replace the reason.
+    exit_reason_ref_ = nullptr;
+    quit_closure_.Run();
+  }
+}
+
+}  // namespace views