Fix for a crasher in the browser seen while dispatching mouse enter or mouse...

Fix for a crasher in the browser seen while dispatching mouse enter or mouse exit messages via the root view.

Based on a number of crash dumps I looked at starting from M40 onwards, the crash occurs in the RootView::NotifyEnterExitOfDescendant function while dereferencing a NULL view parameter.

The parameter passed as the view, is the mouse_move_handler_ member which is checked for validity before calling this
function. However the disassembly in the crash dump clearly suggests that the parameter passed on the stack is NULL.

Looking at the code the only way that could happen if we end up in a nested invocation to the root view, which could
potentially happen in the context of a modal loop. I could not repro that hypothesis however.

Given that this is a browser crash and seems to be occurring frequently enough, I think this warrants a NULL check
for the mouse_move_handler_ before calling the RootView::NotifyEnterExitOfDescendant function.

I added a CHECK for the view parameter in the NotifyEnterExitOfDescendant  function in case there are additional callsites
added in the future.

BUG=467356
TEST=No test at the moment as I could not verify the hypothesis with actions in the UI.

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

Cr-Commit-Position: refs/heads/master@{#321414}

ui/views/widget/root_view.cc

@@ -493,8 +493,13 @@ void RootView::OnMouseMoved(const ui::MouseEvent& event) {
             DispatchEvent(mouse_move_handler_, &exit);
         if (dispatch_details.dispatcher_destroyed)
           return;
-        NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
-            mouse_move_handler_, v);
+        // The mouse_move_handler_ could have been destroyed in the context of
+        // the mouse exit event.
+        if (!dispatch_details.target_destroyed) {
+          CHECK(mouse_move_handler_);
+          NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
+              mouse_move_handler_, v);
+        }
       }
       View* old_handler = mouse_move_handler_;
       mouse_move_handler_ = v;
@@ -507,8 +512,13 @@ void RootView::OnMouseMoved(const ui::MouseEvent& event) {
             DispatchEvent(mouse_move_handler_, &entered);
         if (dispatch_details.dispatcher_destroyed)
           return;
-        NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_ENTERED,
-            mouse_move_handler_, old_handler);
+        // The mouse_move_handler_ could have been destroyed in the context of
+        // the mouse exit event.
+        if (!dispatch_details.target_destroyed) {
+          CHECK(mouse_move_handler_);
+          NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_ENTERED,
+              mouse_move_handler_, old_handler);
+        }
       }
     }
     ui::MouseEvent moved_event(event, static_cast<View*>(this),
@@ -525,8 +535,13 @@ void RootView::OnMouseMoved(const ui::MouseEvent& event) {
         DispatchEvent(mouse_move_handler_, &exited);
     if (dispatch_details.dispatcher_destroyed)
       return;
-    NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
-        mouse_move_handler_, v);
+    // The mouse_move_handler_ could have been destroyed in the context of the
+    // mouse exit event.
+    if (!dispatch_details.target_destroyed) {
+      CHECK(mouse_move_handler_);
+      NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
+          mouse_move_handler_, v);
+    }
     // On Aura the non-client area extends slightly outside the root view for
     // some windows.  Let the non-client cursor handling code set the cursor
     // as we do above.
@@ -543,8 +558,13 @@ void RootView::OnMouseExited(const ui::MouseEvent& event) {
         DispatchEvent(mouse_move_handler_, &exited);
     if (dispatch_details.dispatcher_destroyed)
       return;
-    NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
-        mouse_move_handler_, NULL);
+    // The mouse_move_handler_ could have been destroyed in the context of the
+    // mouse exit event.
+    if (!dispatch_details.target_destroyed) {
+      CHECK(mouse_move_handler_);
+      NotifyEnterExitOfDescendant(event, ui::ET_MOUSE_EXITED,
+          mouse_move_handler_, NULL);
+    }
     mouse_move_handler_ = NULL;
   }
 }

ui/views/widget/root_view_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "ui/views/widget/root_view.h"
 
+#include "ui/events/event_utils.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/view_targeter.h"
@@ -337,5 +338,67 @@ TEST_F(RootViewTest, ContextMenuFromLongPressOnDisabledView) {
   EXPECT_EQ(0, controller.show_context_menu_calls());
 }
 
+// This view class provides functionality to delete itself in the context of
+// mouse exit event and helps test that we don't crash when we return from
+// the mouse exit handler.
+class DeleteViewOnMouseExit : public View {
+ public:
+  explicit DeleteViewOnMouseExit(bool* got_mouse_exit)
+      : got_mouse_exit_(got_mouse_exit) {
+  }
+
+  ~DeleteViewOnMouseExit() override {}
+
+  void OnMouseExited(const ui::MouseEvent& event) override {
+    *got_mouse_exit_ = true;
+    delete this;
+  }
+
+ private:
+  // Set to true in OnMouseExited().
+  bool* got_mouse_exit_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeleteViewOnMouseExit);
+};
+
+// Verifies deleting a View in OnMouseExited() doesn't crash.
+TEST_F(RootViewTest, DeleteViewOnMouseExitDispatch) {
+  Widget widget;
+  Widget::InitParams init_params =
+      CreateParams(Widget::InitParams::TYPE_POPUP);
+  init_params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  widget.Init(init_params);
+  widget.SetBounds(gfx::Rect(10, 10, 500, 500));
+
+  View* content = new View;
+  widget.SetContentsView(content);
+
+  bool got_mouse_exit = false;
+  View* child = new DeleteViewOnMouseExit(&got_mouse_exit);
+  content->AddChildView(child);
+  child->SetBounds(10, 10, 500, 500);
+
+  internal::RootView* root_view =
+      static_cast<internal::RootView*>(widget.GetRootView());
+
+  // Generate a mouse move event which ensures that the mouse_moved_handler_
+  // member is set in the RootView class.
+  ui::MouseEvent moved_event(ui::ET_MOUSE_MOVED, gfx::Point(15, 15),
+                             gfx::Point(100, 100), ui::EventTimeForNow(), 0,
+                             0);
+  root_view->OnMouseMoved(moved_event);
+  EXPECT_FALSE(got_mouse_exit);
+
+  // Generate a mouse exit event which in turn will delete the child view which
+  // was the target of the mouse move event above. This should not crash when
+  // the mouse exit handler returns from the child.
+  ui::MouseEvent exit_event(ui::ET_MOUSE_EXITED, gfx::Point(), gfx::Point(),
+                            ui::EventTimeForNow(), 0, 0);
+  root_view->OnMouseExited(exit_event);
+
+  EXPECT_TRUE(got_mouse_exit);
+  EXPECT_FALSE(content->has_children());
+}
+
 }  // namespace test
 }  // namespace views