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

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 = Covered by views unittest RootViewTest.DeleteViewOnMouseExitDispatch

Committed: https://crrev.com/43dc9613e9badb5e0c6a11d25106271d2f23899f
Cr-Commit-Position: refs/heads/master@{#321414}

[ananta, 2015-03-17 00:57:37]

ananta@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2015-03-17 16:09:14]

I think any view (with children?) that destroys itself on ET_MOUSE_EXITED would
trigger this crash. The unit-test should probably do that to reproduce. I do not
know if there's anything in the UI that does that though. (perhaps
notifications?)

https://codereview.chromium.org/996103009/diff/1/ui/views/widget/root_view.cc
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/996103009/diff/1/ui/views/widget/root_view.cc...
ui/views/widget/root_view.cc:498: if (mouse_move_handler_) {
I think checking for !dispatch_details.target_destroyed would also work? That
would probably be clearer?
  if (!dispatch_details.target_destroyed) {
    CHECK(mouse_move_handler_);
    ....
  }

[ananta, 2015-03-18 01:11:06]

Added a views_unittest RootViewTest.DeleteViewOnMouseExitDispatch which tests
that we don't crash on return from the OnMouseExited handler from a deleted
child view.

https://codereview.chromium.org/996103009/diff/1/ui/views/widget/root_view.cc
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/996103009/diff/1/ui/views/widget/root_view.cc...
ui/views/widget/root_view.cc:498: if (mouse_move_handler_) {
On 2015/03/17 16:09:14, sadrul wrote:
> I think checking for !dispatch_details.target_destroyed would also work? That
> would probably be clearer?
>   if (!dispatch_details.target_destroyed) {
>     CHECK(mouse_move_handler_);
>     ....
>   }

Done.

[sadrul, 2015-03-18 22:30:00]

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
File ui/views/widget/root_view_unittest.cc (right):

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view_unittest.cc:382: child->RequestFocus();
Don't need focus, right?

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view_unittest.cc:392: root_view->OnMouseMoved(moved_event);
EXPECT_FALSE here that !got_mouse_exit?

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view_unittest.cc:401: EXPECT_TRUE(got_mouse_exit);
Also check that content->has_children() == false?

[ananta, 2015-03-19 00:02:55]

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
File ui/views/widget/root_view_unittest.cc (right):

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view_unittest.cc:382: child->RequestFocus();
On 2015/03/18 22:30:00, sadrul wrote:
> Don't need focus, right?

Done.

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view_unittest.cc:392: root_view->OnMouseMoved(moved_event);
On 2015/03/18 22:30:00, sadrul wrote:
> EXPECT_FALSE here that !got_mouse_exit?

Done.

https://codereview.chromium.org/996103009/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view_unittest.cc:401: EXPECT_TRUE(got_mouse_exit);
On 2015/03/18 22:30:00, sadrul wrote:
> Also check that content->has_children() == false?

Done.

[sadrul, 2015-03-19 09:13:33]

lgtm

[ananta, 2015-03-19 19:03:24]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-03-19 19:03:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/996103009/60001

[commit-bot: I haz the power, 2015-03-19 20:02:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-19 20:03:16]

Patchset 4 (id:??) landed as
https://crrev.com/43dc9613e9badb5e0c6a11d25106271d2f23899f
Cr-Commit-Position: refs/heads/master@{#321414}