WebContentsViewMac: Simplify deferring close during event loops

This issue is best docuemnted in https://crrev.com/31183. The best
repro is to
- Nativate to https://crbug.com/25462
- Download test.html
- Open test.html from the downloads bar
- Tab-drag test.html out, and hold the drag for at least 5 seconds

The implmenetation of this functionality effectively uses
WebContentsViewCocoa (via cancelPreviousPerformRequestsWithTarget and
performSelector) as a weak pointer for WebContentsViewMac. This is not
the right tool for the job, and is guaranteed to not work when
WebContentsViewCocoa is in another process.

Change this to use a weak pointer for weak pointer functionality.

Merge WebContentsView::IsEventTracking and CloseTabAfterEventTracking
into a single function.

Bug: Cleaning up WebContentsView
Change-Id: I215744f048d5c49be00ae94ecdbfdbbdbd62415a
Reviewed-on: https://chromium-review.googlesource.com/c/1492754Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636914}

content/browser/web_contents/web_contents_impl.cc

@@ -5726,10 +5726,8 @@ void WebContentsImpl::Close(RenderViewHost* rvh) {
   // TODO(shess): This could get more fine-grained.  For instance,
   // closing a tab in another window while selecting text in the
   // current window's Omnibox should be just fine.
-  if (view_->IsEventTracking()) {
-    view_->CloseTabAfterEventTracking();
+  if (view_->CloseTabAfterEventTrackingIfNeeded())
     return;
-  }
 #endif
 
   // Ignore this if it comes from a RenderViewHost that we aren't showing.

content/browser/web_contents/web_contents_ns_view_bridge.mm

@@ -33,7 +33,6 @@ WebContentsNSViewBridge::~WebContentsNSViewBridge() {
   // while the user was operating a UI control which resulted in a
   // close.  In that case, the Cocoa view outlives the
   // WebContentsViewMac instance due to Cocoa retain count.
-  [cocoa_view_ cancelDeferredClose];
   [cocoa_view_ clearWebContentsView];
   [cocoa_view_ removeFromSuperview];
 }

content/browser/web_contents/web_contents_view.h

@@ -123,13 +123,12 @@ class WebContentsView {
   virtual void SetOverscrollControllerEnabled(bool enabled) = 0;
 
 #if defined(OS_MACOSX)
-  // If we close the tab while a UI control is in an event-tracking
-  // loop, the control may message freed objects and crash.
-  // WebContents::Close() calls IsEventTracking(), and if it returns
-  // true CloseTabAfterEventTracking() is called and the close is not
-  // completed.
-  virtual bool IsEventTracking() const = 0;
-  virtual void CloseTabAfterEventTracking() = 0;
+  // If we close the tab while a UI control is in an event-tracking loop, the
+  // the control may message freed objects and crash. WebContents::Close will
+  // call this. If it returns true, then WebContents::Close will early-out, and
+  // it will be the responsibility of |this| to call CloseTab when the nested
+  // loop has ended.
+  virtual bool CloseTabAfterEventTrackingIfNeeded() = 0;
 #endif
 };
 

content/browser/web_contents/web_contents_view_child_frame.cc

@@ -110,13 +110,9 @@ void WebContentsViewChildFrame::SetOverscrollControllerEnabled(bool enabled) {
 }
 
 #if defined(OS_MACOSX)
-bool WebContentsViewChildFrame::IsEventTracking() const {
+bool WebContentsViewChildFrame::CloseTabAfterEventTrackingIfNeeded() {
   return false;
 }
-
-void WebContentsViewChildFrame::CloseTabAfterEventTracking() {
-  NOTREACHED();
-}
 #endif
 
 void WebContentsViewChildFrame::RestoreFocus() {

content/browser/web_contents/web_contents_view_child_frame.h

@@ -50,8 +50,7 @@ class WebContentsViewChildFrame : public WebContentsView,
                              RenderViewHost* new_host) override;
   void SetOverscrollControllerEnabled(bool enabled) override;
 #if defined(OS_MACOSX)
-  bool IsEventTracking() const override;
-  void CloseTabAfterEventTracking() override;
+  bool CloseTabAfterEventTrackingIfNeeded() override;
 #endif
 
   // Backend implementation of RenderViewHostDelegateView.

content/browser/web_contents/web_contents_view_cocoa.h

@@ -60,9 +60,7 @@ CONTENT_EXPORT
             dragOperationMask:(NSDragOperation)operationMask
                         image:(NSImage*)image
                        offset:(NSPoint)offset;
-- (void)cancelDeferredClose;
 - (void)clearWebContentsView;
-- (void)closeTabAfterEvent;
 - (void)updateWebContentsVisibility;
 - (void)viewDidBecomeFirstResponder:(NSNotification*)notification;
 - (content::WebContentsImpl*)webContents;

content/browser/web_contents/web_contents_view_cocoa.mm

@@ -47,9 +47,6 @@ using content::WebContentsViewMac;
 }
 
 - (void)dealloc {
-  // Cancel any deferred tab closes, just in case.
-  [self cancelDeferredClose];
-
   // This probably isn't strictly necessary, but can't hurt.
   [self unregisterDraggedTypes];
 
@@ -235,24 +232,12 @@ using content::WebContentsViewMac;
   return result;
 }
 
-- (void)cancelDeferredClose {
-  SEL aSel = @selector(closeTabAfterEvent);
-  [NSObject cancelPreviousPerformRequestsWithTarget:self
-                                           selector:aSel
-                                             object:nil];
-}
-
 - (void)clearWebContentsView {
   webContentsView_ = nullptr;
   client_ = nullptr;
   [dragSource_ clearWebContentsView];
 }
 
-- (void)closeTabAfterEvent {
-  if (webContentsView_)
-    webContentsView_->CloseTab();
-}
-
 - (void)viewDidBecomeFirstResponder:(NSNotification*)notification {
   if (!client_)
     return;

content/browser/web_contents/web_contents_view_guest.cc

@@ -169,12 +169,9 @@ void WebContentsViewGuest::SetOverscrollControllerEnabled(bool enabled) {
 }
 
 #if defined(OS_MACOSX)
-bool WebContentsViewGuest::IsEventTracking() const {
+bool WebContentsViewGuest::CloseTabAfterEventTrackingIfNeeded() {
   return false;
 }
-
-void WebContentsViewGuest::CloseTabAfterEventTracking() {
-}
 #endif
 
 WebContents* WebContentsViewGuest::web_contents() {

content/browser/web_contents/web_contents_view_guest.h

@@ -68,8 +68,7 @@ class WebContentsViewGuest : public WebContentsView,
                              RenderViewHost* new_host) override;
   void SetOverscrollControllerEnabled(bool enabled) override;
 #if defined(OS_MACOSX)
-  bool IsEventTracking() const override;
-  void CloseTabAfterEventTracking() override;
+  bool CloseTabAfterEventTrackingIfNeeded() override;
 #endif
 
   // Backend implementation of RenderViewHostDelegateView.

content/browser/web_contents/web_contents_view_mac.h

@@ -83,8 +83,7 @@ class WebContentsViewMac : public WebContentsView,
   void RenderViewHostChanged(RenderViewHost* old_host,
                              RenderViewHost* new_host) override;
   void SetOverscrollControllerEnabled(bool enabled) override;
-  bool IsEventTracking() const override;
-  void CloseTabAfterEventTracking() override;
+  bool CloseTabAfterEventTrackingIfNeeded() override;
 
   // RenderViewHostDelegateView:
   void StartDragging(const DropData& drop_data,
@@ -195,6 +194,9 @@ class WebContentsViewMac : public WebContentsView,
   mojo::AssociatedBinding<mojom::WebContentsNSViewClient>
       ns_view_client_binding_;
 
+  // Used by CloseTabAfterEventTrackingIfNeeded.
+  base::WeakPtrFactory<WebContentsViewMac> deferred_close_weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(WebContentsViewMac);
 };
 

content/browser/web_contents/web_contents_view_mac.mm

@@ -80,7 +80,8 @@ WebContentsViewMac::WebContentsViewMac(WebContentsImpl* web_contents,
     : web_contents_(web_contents),
       delegate_(delegate),
       ns_view_id_(ui::NSViewIds::GetNewId()),
-      ns_view_client_binding_(this) {}
+      ns_view_client_binding_(this),
+      deferred_close_weak_ptr_factory_(this) {}
 
 WebContentsViewMac::~WebContentsViewMac() {
   if (views_host_)
@@ -395,19 +396,21 @@ void WebContentsViewMac::RenderViewHostChanged(RenderViewHost* old_host,
 void WebContentsViewMac::SetOverscrollControllerEnabled(bool enabled) {
 }
 
-bool WebContentsViewMac::IsEventTracking() const {
-  return base::MessagePumpMac::IsHandlingSendEvent();
-}
-
 // Arrange to call CloseTab() after we're back to the main event loop.
-// The obvious way to do this would be PostNonNestableTask(), but that
-// will fire when the event-tracking loop polls for events.  So we
-// need to bounce the message via Cocoa, instead.
-void WebContentsViewMac::CloseTabAfterEventTracking() {
-  [cocoa_view() cancelDeferredClose];
-  [cocoa_view() performSelector:@selector(closeTabAfterEvent)
-                     withObject:nil
-                     afterDelay:0.0];
+// The obvious way to do this would be to post a NonNestable task, but that
+// would fire when the event-tracking loop polls for events.  So we need to
+// bounce the message via Cocoa, instead.
+bool WebContentsViewMac::CloseTabAfterEventTrackingIfNeeded() {
+  if (!base::MessagePumpMac::IsHandlingSendEvent())
+    return false;
+
+  deferred_close_weak_ptr_factory_.InvalidateWeakPtrs();
+  auto weak_ptr = deferred_close_weak_ptr_factory_.GetWeakPtr();
+  CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopDefaultMode, ^{
+    if (weak_ptr)
+      weak_ptr->CloseTab();
+  });
+  return true;
 }
 
 void WebContentsViewMac::CloseTab() {