Fix crash which occurs when a widget destroys itself as a result of ET_GESTURE_TAP_DOWN

BUG=342867
TEST=WidgetTest.WidgetDeleted_InDispatchGestureEvent
TBR=sadrul

Review URL: https://codereview.chromium.org/180803006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@253962 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 143 insertions, 116 deletions

diff --git a/ui/views/widget/root_view.cc b/ui/views/widget/root_view.cc
index 2e4ea9e..51b6bdc 100644
--- a/ui/views/widget/root_view.cc
+++ b/ui/views/widget/root_view.cc
@@ -19,7 +19,6 @@
 #include "ui/views/views_switches.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
-#include "ui/views/widget/widget_deletion_observer.h"
 
 #if defined(USE_AURA)
 #include "ui/base/cursor/cursor.h"
@@ -139,7 +138,11 @@ void RootView::DispatchKeyEvent(ui::KeyEvent* event) {
 void RootView::DispatchScrollEvent(ui::ScrollEvent* event) {
   for (View* v = GetEventHandlerForPoint(event->location());
        v && v != this && !event->stopped_propagation(); v = v->parent()) {
-    DispatchEventToTarget(v, event);
+    ui::EventDispatchDetails dispatch_details = DispatchEventToTarget(v, event);
+    if (dispatch_details.dispatcher_destroyed ||
+        dispatch_details.target_destroyed) {
+      return;
+    }
   }
 
   if (event->handled() || event->type() != ui::ET_SCROLL)
@@ -163,11 +166,14 @@ void RootView::DispatchTouchEvent(ui::TouchEvent* event) {
   if (touch_pressed_handler_) {
     ui::TouchEvent touch_event(*event, static_cast<View*>(this),
                                touch_pressed_handler_);
-    DispatchEventToTarget(touch_pressed_handler_, &touch_event);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(touch_pressed_handler_, &touch_event);
     if (touch_event.handled())
       event->SetHandled();
     if (touch_event.stopped_propagation())
       event->StopPropagation();
+    if (dispatch_details.dispatcher_destroyed)
+      return;
     return;
   }
 
@@ -183,11 +189,14 @@ void RootView::DispatchTouchEvent(ui::TouchEvent* event) {
     // See if this view wants to handle the touch
     ui::TouchEvent touch_event(*event, static_cast<View*>(this),
                                touch_pressed_handler_);
-    DispatchEventToTarget(touch_pressed_handler_, &touch_event);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(touch_pressed_handler_, &touch_event);
     if (touch_event.handled())
       event->SetHandled();
     if (touch_event.stopped_propagation())
       event->StopPropagation();
+    if (dispatch_details.dispatcher_destroyed)
+      return;
 
     // The view could have removed itself from the tree when handling
     // OnTouchEvent(). So handle as per OnMousePressed. NB: we
@@ -222,7 +231,10 @@ void RootView::DispatchGestureEvent(ui::GestureEvent* event) {
         (event->IsScrollGestureEvent() || event->IsFlingScrollEvent())  ?
             scroll_gesture_handler_ : gesture_handler_;
     ui::GestureEvent handler_event(*event, static_cast<View*>(this), handler);
-    DispatchEventToTarget(handler, &handler_event);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(handler, &handler_event);
+    if (dispatch_details.dispatcher_destroyed)
+      return;
 
     if (event->type() == ui::ET_GESTURE_END &&
         event->details().touch_points() <= 1) {
@@ -259,13 +271,17 @@ void RootView::DispatchGestureEvent(ui::GestureEvent* event) {
           scroll_gesture_handler_ = scroll_gesture_handler_->parent()) {
         ui::GestureEvent gesture_event(*event, static_cast<View*>(this),
                                        scroll_gesture_handler_);
-        DispatchEventToTarget(scroll_gesture_handler_, &gesture_event);
+        ui::EventDispatchDetails dispatch_details =
+            DispatchEventToTarget(scroll_gesture_handler_, &gesture_event);
         if (gesture_event.stopped_propagation()) {
           event->StopPropagation();
           return;
         } else if (gesture_event.handled()) {
           event->SetHandled();
           return;
+        } else if (dispatch_details.dispatcher_destroyed ||
+                   dispatch_details.target_destroyed) {
+          return;
         }
       }
       scroll_gesture_handler_ = NULL;
@@ -310,7 +326,10 @@ void RootView::DispatchGestureEvent(ui::GestureEvent* event) {
     // See if this view wants to handle the Gesture.
     ui::GestureEvent gesture_event(*event, static_cast<View*>(this),
                                    gesture_handler_);
-    DispatchEventToTarget(gesture_handler_, &gesture_event);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(gesture_handler_, &gesture_event);
+    if (dispatch_details.dispatcher_destroyed)
+      return;
 
     // The view could have removed itself from the tree when handling
     // OnGestureEvent(). So handle as per OnMousePressed. NB: we
@@ -417,7 +436,10 @@ bool RootView::OnMousePressed(const ui::MouseEvent& event) {
     ui::MouseEvent mouse_pressed_event(event, static_cast<View*>(this),
                                        mouse_pressed_handler_);
     drag_info_.Reset();
-    DispatchEventToTarget(mouse_pressed_handler_, &mouse_pressed_event);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(mouse_pressed_handler_, &mouse_pressed_event);
+    if (dispatch_details.dispatcher_destroyed)
+      return true;
     return true;
   }
   DCHECK(!explicit_mouse_handler_);
@@ -445,12 +467,10 @@ bool RootView::OnMousePressed(const ui::MouseEvent& event) {
       mouse_pressed_event.set_flags(event.flags() & ~ui::EF_IS_DOUBLE_CLICK);
 
     drag_info_.Reset();
-    {
-      WidgetDeletionObserver widget_deletion_observer(widget_);
-      DispatchEventToTarget(mouse_pressed_handler_, &mouse_pressed_event);
-      if (!widget_deletion_observer.IsWidgetAlive())
-        return mouse_pressed_event.handled();
-    }
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(mouse_pressed_handler_, &mouse_pressed_event);
+    if (dispatch_details.dispatcher_destroyed)
+      return mouse_pressed_event.handled();
 
     // The view could have removed itself from the tree when handling
     // OnMousePressed().  In this case, the removal notification will have
@@ -493,7 +513,10 @@ bool RootView::OnMouseDragged(const ui::MouseEvent& event) {
 
     ui::MouseEvent mouse_event(event, static_cast<View*>(this),
                                mouse_pressed_handler_);
-    DispatchEventToTarget(mouse_pressed_handler_, &mouse_event);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(mouse_pressed_handler_, &mouse_event);
+    if (dispatch_details.dispatcher_destroyed)
+      return false;
   }
   return false;
 }
@@ -508,8 +531,10 @@ void RootView::OnMouseReleased(const ui::MouseEvent& event) {
     // configure state such that we're done first, then call View.
     View* mouse_pressed_handler = mouse_pressed_handler_;
     SetMouseHandler(NULL);
-    DispatchEventToTarget(mouse_pressed_handler, &mouse_released);
-    // WARNING: we may have been deleted.
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(mouse_pressed_handler, &mouse_released);
+    if (dispatch_details.dispatcher_destroyed)
+      return;
   }
 }
 
@@ -553,7 +578,10 @@ void RootView::OnMouseMoved(const ui::MouseEvent& event) {
           (!mouse_move_handler_->notify_enter_exit_on_child() ||
            !mouse_move_handler_->Contains(v))) {
         MouseEnterExitEvent exit(event, ui::ET_MOUSE_EXITED);
-        DispatchEventToTarget(mouse_move_handler_, &exit);
+        ui::EventDispatchDetails dispatch_details =
+            DispatchEventToTarget(mouse_move_handler_, &exit);
+        if (dispatch_details.dispatcher_destroyed)
+          return;
         NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
             mouse_move_handler_, v);
       }
@@ -564,7 +592,10 @@ void RootView::OnMouseMoved(const ui::MouseEvent& event) {
         MouseEnterExitEvent entered(event, ui::ET_MOUSE_ENTERED);
         entered.ConvertLocationToTarget(static_cast<View*>(this),
                                         mouse_move_handler_);
-        DispatchEventToTarget(mouse_move_handler_, &entered);
+        ui::EventDispatchDetails dispatch_details =
+            DispatchEventToTarget(mouse_move_handler_, &entered);
+        if (dispatch_details.dispatcher_destroyed)
+          return;
         NotifyEnterExitOfDescendant(entered, ui::ET_MOUSE_ENTERED, v,
             old_handler);
       }
@@ -576,7 +607,10 @@ void RootView::OnMouseMoved(const ui::MouseEvent& event) {
       widget_->SetCursor(mouse_move_handler_->GetCursor(moved_event));
   } else if (mouse_move_handler_ != NULL) {
     MouseEnterExitEvent exited(event, ui::ET_MOUSE_EXITED);
-    DispatchEventToTarget(mouse_move_handler_, &exited);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(mouse_move_handler_, &exited);
+    if (dispatch_details.dispatcher_destroyed)
+      return;
     NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
         mouse_move_handler_, v);
     // On Aura the non-client area extends slightly outside the root view for
@@ -591,7 +625,10 @@ void RootView::OnMouseMoved(const ui::MouseEvent& event) {
 void RootView::OnMouseExited(const ui::MouseEvent& event) {
   if (mouse_move_handler_ != NULL) {
     MouseEnterExitEvent exited(event, ui::ET_MOUSE_EXITED);
-    DispatchEventToTarget(mouse_move_handler_, &exited);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(mouse_move_handler_, &exited);
+    if (dispatch_details.dispatcher_destroyed)
+      return;
     NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
         mouse_move_handler_, NULL);
     mouse_move_handler_ = NULL;
@@ -600,8 +637,14 @@ void RootView::OnMouseExited(const ui::MouseEvent& event) {
 
 bool RootView::OnMouseWheel(const ui::MouseWheelEvent& event) {
   for (View* v = GetEventHandlerForPoint(event.location());
-       v && v != this && !event.handled(); v = v->parent())
-    DispatchEventToTarget(v, const_cast<ui::MouseWheelEvent*>(&event));
+       v && v != this && !event.handled(); v = v->parent()) {
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(v, const_cast<ui::MouseWheelEvent*>(&event));
+    if (dispatch_details.dispatcher_destroyed ||
+        dispatch_details.target_destroyed) {
+      return event.handled();
+    }
+  }
   return event.handled();
 }
 
@@ -700,12 +743,14 @@ void RootView::SetMouseLocationAndFlags(const ui::MouseEvent& event) {
   last_mouse_event_y_ = event.y();
 }
 
-void RootView::DispatchEventToTarget(View* target, ui::Event* event) {
+ui::EventDispatchDetails RootView::DispatchEventToTarget(View* target,
+                                                         ui::Event* event) {
   View* old_target = event_dispatch_target_;
   event_dispatch_target_ = target;
   ui::EventDispatchDetails details = DispatchEvent(target, event);
   if (!details.dispatcher_destroyed)
     event_dispatch_target_ = old_target;
+  return details;
 }
 
 void RootView::NotifyEnterExitOfDescendant(const ui::MouseEvent& event,
@@ -721,20 +766,24 @@ void RootView::NotifyEnterExitOfDescendant(const ui::MouseEvent& event,
     // of the callbacks can mark the event as handled, and that would cause
     // incorrect event dispatch.
     MouseEnterExitEvent notify_event(event, type);
-    DispatchEventToTarget(p, &notify_event);
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(p, &notify_event);
+    if (dispatch_details.dispatcher_destroyed ||
+        dispatch_details.target_destroyed) {
+      return;
+    }
   }
 }
 
 
 void RootView::DispatchKeyEventStartAt(View* view, ui::KeyEvent* event) {
-  if (event->handled() || !view)
-    return;
-
-  for (; view && view != this; view = view->parent()) {
-    DispatchEventToTarget(view, event);
-    // Do this check here rather than in the if as |view| may have been deleted.
-    if (event->handled())
+  for (; view && view != this && !event->handled(); view = view->parent()) {
+    ui::EventDispatchDetails dispatch_details =
+        DispatchEventToTarget(view, event);
+    if (dispatch_details.dispatcher_destroyed ||
+        dispatch_details.target_destroyed) {
       return;
+    }
   }
 }
 
diff --git a/ui/views/widget/root_view.h b/ui/views/widget/root_view.h
index 391f714..c6a09c4 100644
--- a/ui/views/widget/root_view.h
+++ b/ui/views/widget/root_view.h
@@ -145,7 +145,9 @@ class VIEWS_EXPORT RootView : public View,
   // be applied to the point prior to calling this).
   void SetMouseLocationAndFlags(const ui::MouseEvent& event);
 
-  void DispatchEventToTarget(View* target, ui::Event* event);
+  ui::EventDispatchDetails DispatchEventToTarget(
+      View* target,
+      ui::Event* event) WARN_UNUSED_RESULT;
 
   // |view| is the view receiving |event|. This function sends the event to all
   // the Views up the hierarchy that has |notify_enter_exit_on_child_| flag
diff --git a/ui/views/widget/widget_unittest.cc b/ui/views/widget/widget_unittest.cc
index 27583c6..adecf03 100644
--- a/ui/views/widget/widget_unittest.cc
+++ b/ui/views/widget/widget_unittest.cc
@@ -12,6 +12,12 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/client/aura_constants.h"
+#include "ui/aura/client/window_tree_client.h"
+#include "ui/aura/test/event_generator.h"
+#include "ui/aura/test/test_window_delegate.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_event_dispatcher.h"
 #include "ui/base/hit_test.h"
 #include "ui/events/event_utils.h"
 #include "ui/gfx/native_widget_types.h"
@@ -21,24 +27,16 @@
 #include "ui/views/test/test_views_delegate.h"
 #include "ui/views/test/widget_test.h"
 #include "ui/views/views_delegate.h"
+#include "ui/views/widget/native_widget_aura.h"
 #include "ui/views/widget/native_widget_delegate.h"
 #include "ui/views/widget/root_view.h"
+#include "ui/views/widget/widget_deletion_observer.h"
 #include "ui/views/window/dialog_delegate.h"
 #include "ui/views/window/native_frame_view.h"
 
-#if defined(USE_AURA)
-#include "ui/aura/client/aura_constants.h"
-#include "ui/aura/client/window_tree_client.h"
-#include "ui/aura/test/test_window_delegate.h"
-#include "ui/aura/window.h"
-#include "ui/aura/window_event_dispatcher.h"
-#include "ui/views/widget/native_widget_aura.h"
 #if !defined(OS_CHROMEOS)
 #include "ui/views/widget/desktop_aura/desktop_native_widget_aura.h"
 #endif
-#elif defined(OS_WIN)
-#include "ui/views/widget/native_widget_win.h"
-#endif
 
 #if defined(OS_WIN)
 #include "ui/views/win/hwnd_util.h"
@@ -162,6 +160,33 @@ class EventCountHandler : public ui::EventHandler {
   DISALLOW_COPY_AND_ASSIGN(EventCountHandler);
 };
 
+// Class that closes the widget (which ends up deleting it immediately) when the
+// appropriate event is received.
+class CloseWidgetView : public View {
+ public:
+  explicit CloseWidgetView(ui::EventType event_type)
+      : event_type_(event_type) {
+  }
+
+  // ui::EventHandler override:
+  virtual void OnEvent(ui::Event* event) OVERRIDE {
+    if (event->type() == event_type_) {
+      // Go through NativeWidgetPrivate to simulate what happens if the OS
+      // deletes the NativeWindow out from under us.
+      GetWidget()->native_widget_private()->CloseNow();
+    } else {
+      View::OnEvent(event);
+      if (!event->IsTouchEvent())
+        event->SetHandled();
+    }
+  }
+
+ private:
+  const ui::EventType event_type_;
+
+  DISALLOW_COPY_AND_ASSIGN(CloseWidgetView);
+};
+
 ui::WindowShowState GetWidgetShowState(const Widget* widget) {
   // Use IsMaximized/IsMinimized/IsFullScreen instead of GetWindowPlacement
   // because the former is implemented on all platforms but the latter is not.
@@ -605,12 +630,6 @@ class WidgetWithDestroyedNativeViewTest : public ViewsTestBase {
     widget->ReleaseCapture();
     widget->HasCapture();
     widget->GetWorkAreaBoundsInScreen();
-    // These three crash with NativeWidgetWin, so I'm assuming we don't need
-    // them to work for the other NativeWidget impls.
-    // widget->CenterWindow(gfx::Size(50, 60));
-    // widget->GetRestoredBounds();
-    // widget->ShowInactive();
-    // widget->Show();
   }
 
  private:
@@ -1269,54 +1288,6 @@ TEST_F(WidgetTest, TestWindowVisibilityAfterHide) {
 // nested message loops from such events, nor has the code ever really dealt
 // with this situation.
 
-// Class that closes the widget (which ends up deleting it immediately) when the
-// appropriate event is received.
-class CloseWidgetView : public View {
- public:
-  explicit CloseWidgetView(ui::EventType event_type)
-      : event_type_(event_type) {
-  }
-
-  // View overrides:
-  virtual bool OnMousePressed(const ui::MouseEvent& event) OVERRIDE {
-    if (!CloseWidget(event))
-      View::OnMousePressed(event);
-    return true;
-  }
-  virtual bool OnMouseDragged(const ui::MouseEvent& event) OVERRIDE {
-    if (!CloseWidget(event))
-      View::OnMouseDragged(event);
-    return true;
-  }
-  virtual void OnMouseReleased(const ui::MouseEvent& event) OVERRIDE {
-    if (!CloseWidget(event))
-      View::OnMouseReleased(event);
-  }
-  virtual void OnMouseMoved(const ui::MouseEvent& event) OVERRIDE {
-    if (!CloseWidget(event))
-      View::OnMouseMoved(event);
-  }
-  virtual void OnMouseEntered(const ui::MouseEvent& event) OVERRIDE {
-    if (!CloseWidget(event))
-      View::OnMouseEntered(event);
-  }
-
- private:
-  bool CloseWidget(const ui::LocatedEvent& event) {
-    if (event.type() == event_type_) {
-      // Go through NativeWidgetPrivate to simulate what happens if the OS
-      // deletes the NativeWindow out from under us.
-      GetWidget()->native_widget_private()->CloseNow();
-      return true;
-    }
-    return false;
-  }
-
-  const ui::EventType event_type_;
-
-  DISALLOW_COPY_AND_ASSIGN(CloseWidgetView);
-};
-
 // Generates two moves (first generates enter, second real move), a press, drag
 // and release stopping at |last_event_type|.
 void GenerateMouseEvents(Widget* widget, ui::EventType last_event_type) {
@@ -1715,37 +1686,42 @@ TEST_F(WidgetTest, SetTopLevelCorrectly) {
   EXPECT_TRUE(delegate->is_top_level());
 }
 
-// A scumbag View that deletes its owning widget OnMousePressed.
-class WidgetDeleterView : public View {
- public:
-  WidgetDeleterView() : View() {}
+TEST_F(WidgetTest, WidgetDeleted_InOnMousePressed) {
+  Widget* widget = new Widget;
+  Widget::InitParams params =
+      CreateParams(views::Widget::InitParams::TYPE_POPUP);
+  widget->Init(params);
 
-  // Overridden from View.
-  virtual bool OnMousePressed(const ui::MouseEvent& event) OVERRIDE {
-    delete GetWidget();
-    return true;
-  }
+  widget->SetContentsView(new CloseWidgetView(ui::ET_MOUSE_PRESSED));
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(WidgetDeleterView);
-};
+  widget->SetSize(gfx::Size(100, 100));
+  widget->Show();
 
-TEST_F(WidgetTest, TestWidgetDeletedInOnMousePressed) {
+  aura::test::EventGenerator generator(GetContext(), widget->GetNativeWindow());
+
+  WidgetDeletionObserver deletion_observer(widget);
+  generator.ClickLeftButton();
+  EXPECT_FALSE(deletion_observer.IsWidgetAlive());
+
+  // Yay we did not crash!
+}
+
+TEST_F(WidgetTest, WidgetDeleted_InDispatchGestureEvent) {
   Widget* widget = new Widget;
   Widget::InitParams params =
       CreateParams(views::Widget::InitParams::TYPE_POPUP);
-  params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
   widget->Init(params);
 
-  widget->SetContentsView(new WidgetDeleterView);
+  widget->SetContentsView(new CloseWidgetView(ui::ET_GESTURE_TAP_DOWN));
 
   widget->SetSize(gfx::Size(100, 100));
   widget->Show();
 
-  gfx::Point click_location(45, 15);
-  ui::MouseEvent press(ui::ET_MOUSE_PRESSED, click_location, click_location,
-                       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON);
-  widget->OnMouseEvent(&press);
+  aura::test::EventGenerator generator(GetContext());
+
+  WidgetDeletionObserver deletion_observer(widget);
+  generator.GestureTapAt(widget->GetWindowBoundsInScreen().CenterPoint());
+  EXPECT_FALSE(deletion_observer.IsWidgetAlive());
 
   // Yay we did not crash!
 }