Clear last MouseMove root view in RWHIER if that view gets destroyed

Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/83e4f9ac895d5b42a4b114f3ef964676a40476ff
Cr-Commit-Position: refs/heads/master@{#424533}

[kenrb, 2016-10-06 15:56:40]

Description was changed from

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209, 647311
==========

to

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209, 647311
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-10-06 16:00:24]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 16:00:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 16:49:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 16:49:50]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-10-06 18:48:26]

kenrb@chromium.org changed reviewers:
+ nick@chromium.org

[kenrb, 2016-10-06 18:48:26]

Nick: PTAL?

[ncarter (slow), 2016-10-07 17:52:27]

https://codereview.chromium.org/2396083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2396083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:388:
CHECK(cur_view->IsRenderWidgetHostViewChildFrame());
If we can't guarantee this via static typing, then I think this approach makes
sense. A CHECK is way way better than a bad cast.

Statically typing this, would probably mean new functions to handle the root vs.
embedded frames, and updating the containers so that they explicitly held
RWHVChildFrames? In my experience the code usually winds up clearer after such
transformation.

[kenrb, 2016-10-07 18:41:14]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 18:41:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kenrb, 2016-10-07 18:44:24]

https://codereview.chromium.org/2396083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2396083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:388:
CHECK(cur_view->IsRenderWidgetHostViewChildFrame());
On 2016/10/07 17:52:27, ncarter wrote:
> If we can't guarantee this via static typing, then I think this approach makes
> sense. A CHECK is way way better than a bad cast.
> 
> Statically typing this, would probably mean new functions to handle the root
vs.
> embedded frames, and updating the containers so that they explicitly held
> RWHVChildFrames? In my experience the code usually winds up clearer after such
> transformation.

I'm not clear on how that would work. RWHVCF::GetParentView() can return any
kind of RenderWidgetHostView, so a static_cast is going to be necessary at some
point. Also, static typing makes it harder to adapt this later to deal with
RWHVMus.

I've uploaded what I think is a better approach to this guard that removes the
CHECK. Now there is a DCHECK and it is still resistant to bad cast bugs. If
you'd still prefer me to change this to static typing then we can chat about how
that could be done.

[ncarter (slow), 2016-10-07 19:40:57]

lgtm

thanks

[ncarter (slow), 2016-10-07 19:41:12]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2016-10-07 19:41:12]

The CQ bit was unchecked by nick@chromium.org

[kenrb, 2016-10-07 20:11:05]

Hmm, looks like I can't land this yet. I ran Windows bots 3 times yesterday and
they came up clean, but there is a red fullscreen test today so apparently it's
still flaky.

[kenrb, 2016-10-11 17:28:28]

Description was changed from

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209, 647311
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-10-11 18:09:53]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2016-10-11 18:09:54]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2396083002/#ps60001
(title: "Not re-enabling test")

[commit-bot: I haz the power, 2016-10-11 18:10:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 20:36:51]

Description was changed from

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-11 20:36:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-11 20:38:26]

Description was changed from

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clear last MouseMove root view in RWHIER if that view gets destroyed

Cluster-fuzz has reported some difficult-to-reproduce crashes in the
MouseEnter/Leave generation code in RenderWidgetHostInputEventRouter,
and there are some very sparse crash reports appearing for that also.

These might be caused by race conditions from RenderWidgetHostView
tree modifications that get slightly out of sync from the Surface
state that is used for hit testing (Surfaces aren't invalidated until
RWHVs are deleted, which for some RWHVs is not immediate upon them
having Destroy() called).

This CL speculatively tries to address the crashes by having
SendMouseEnterOrLeaveEvents abort when it discovers the RWHV tree out
of sync, and also clearing last_mouse_move_root_view_ when that
gets destroyed.

BUG=647821, 652209
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/83e4f9ac895d5b42a4b114f3ef964676a40476ff
Cr-Commit-Position: refs/heads/master@{#424533}
==========

[commit-bot: I haz the power, 2016-10-11 20:38:27]

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