Clear motion event cache and reset in OverlayPanelEventFilter

Clear motion event cache and reset in OverlayPanelEventFilter

This change clears the recorded event cache in the
OverlayPanelEventFilter when it does not consume a touch event. This
should prevent beginning event streams with something other than
ACTION_DOWN.

BUG=613069
Committed: https://crrev.com/a4e1e069dcd16629d605668b1fbf3cbb84a12edf
Cr-Commit-Position: refs/heads/master@{#419306}

[mdjones, 2016-09-16 15:56:59]

mdjones@chromium.org changed reviewers:
+ donnd@chromium.org, twellington@chromium.org

[mdjones, 2016-09-16 15:57:00]

This is the most likely of two possible causes that I can see right now that
have been causing crashes. ptal.

[Donn Denman, 2016-09-16 16:39:29]

LGTM with some caveats:
1) I'm not too familiar with this Event Filter code.  If you have doubts and
would like another set of eyes on this I suggest asking Pedro -- since it's a
small CL I'll bet he'd be happy to look.
2) The crashes that I see look like they are caused by null event.get()
operations, which you've already fixed and merged, right?  Maybe there are other
crashes that I'm missing.

[mdjones, 2016-09-16 17:37:47]

On 2016/09/16 16:39:29, Donn Denman wrote:
> LGTM with some caveats:
> 1) I'm not too familiar with this Event Filter code.  If you have doubts and
> would like another set of eyes on this I suggest asking Pedro -- since it's a
> small CL I'll bet he'd be happy to look.
> 2) The crashes that I see look like they are caused by null event.get()
> operations, which you've already fixed and merged, right?  Maybe there are
other
> crashes that I'm missing.

Yes, the crash is caused by event.getX(), but that is ultimately triggered by
GestureDetector.java passing a null motion event to chrome. Android will only do
this if the first motion event in the stream is not an ACTION_DOWN.

[Donn Denman, 2016-09-16 18:10:37]

On 2016/09/16 17:37:47, mdjones wrote:
> On 2016/09/16 16:39:29, Donn Denman wrote:
> > LGTM with some caveats:
> > 1) I'm not too familiar with this Event Filter code.  If you have doubts and
> > would like another set of eyes on this I suggest asking Pedro -- since it's
a
> > small CL I'll bet he'd be happy to look.
> > 2) The crashes that I see look like they are caused by null event.get()
> > operations, which you've already fixed and merged, right?  Maybe there are
> other
> > crashes that I'm missing.
> 
> Yes, the crash is caused by event.getX(), but that is ultimately triggered by
> GestureDetector.java passing a null motion event to chrome. Android will only
do
> this if the first motion event in the stream is not an ACTION_DOWN.

This doesn't make sense to me because on TOT the only call that I see to
isDistanceGreaterThanTouchSlop with two events is in handleScroll(), and you
check that they are not null there (with your recent fix). So how is that
possible?

[mdjones, 2016-09-16 20:18:07]

On 2016/09/16 18:10:37, Donn Denman wrote:
> On 2016/09/16 17:37:47, mdjones wrote:
> > On 2016/09/16 16:39:29, Donn Denman wrote:
> > > LGTM with some caveats:
> > > 1) I'm not too familiar with this Event Filter code.  If you have doubts
and
> > > would like another set of eyes on this I suggest asking Pedro -- since
it's
> a
> > > small CL I'll bet he'd be happy to look.
> > > 2) The crashes that I see look like they are caused by null event.get()
> > > operations, which you've already fixed and merged, right?  Maybe there are
> > other
> > > crashes that I'm missing.
> > 
> > Yes, the crash is caused by event.getX(), but that is ultimately triggered
by
> > GestureDetector.java passing a null motion event to chrome. Android will
only
> do
> > this if the first motion event in the stream is not an ACTION_DOWN.
> 
> This doesn't make sense to me because on TOT the only call that I see to
> isDistanceGreaterThanTouchSlop with two events is in handleScroll(), and you
> check that they are not null there (with your recent fix). So how is that
> possible?

Are you looking at the most recent crash reports? This bug has had a number of
iterations that are ultimately the same issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=613069#c35

[Donn Denman, 2016-09-16 20:26:13]

On 2016/09/16 20:18:07, mdjones wrote:
> On 2016/09/16 18:10:37, Donn Denman wrote:
> > On 2016/09/16 17:37:47, mdjones wrote:
> > > On 2016/09/16 16:39:29, Donn Denman wrote:
> > > > LGTM with some caveats:
> > > > 1) I'm not too familiar with this Event Filter code.  If you have doubts
> and
> > > > would like another set of eyes on this I suggest asking Pedro -- since
> it's
> > a
> > > > small CL I'll bet he'd be happy to look.
> > > > 2) The crashes that I see look like they are caused by null event.get()
> > > > operations, which you've already fixed and merged, right?  Maybe there
are
> > > other
> > > > crashes that I'm missing.
> > > 
> > > Yes, the crash is caused by event.getX(), but that is ultimately triggered
> by
> > > GestureDetector.java passing a null motion event to chrome. Android will
> only
> > do
> > > this if the first motion event in the stream is not an ACTION_DOWN.
> > 
> > This doesn't make sense to me because on TOT the only call that I see to
> > isDistanceGreaterThanTouchSlop with two events is in handleScroll(), and you
> > check that they are not null there (with your recent fix). So how is that
> > possible?
> 
> Are you looking at the most recent crash reports? This bug has had a number of
> iterations that are ultimately the same issue:
> https://bugs.chromium.org/p/chromium/issues/detail?id=613069#c35

Yes, I was trying to look at the latest ones.  Maybe we can look together f2f, I
might learn something....

Still LGTM

[gone, 2016-09-16 21:12:37]

dfalcantara@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[gone, 2016-09-16 21:12:37]

rs lgtm

[mdjones, 2016-09-16 21:42:26]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2016-09-16 21:43:30]

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-09-16 22:34:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-16 22:37:25]

Description was changed from

==========
Clear motion event cache and reset in OverlayPanelEventFilter

This change clears the recorded event cache in the
OverlayPanelEventFilter when it does not consume a touch event. This
should prevent beginning event streams with something other than
ACTION_DOWN.

BUG=613069
==========

to

==========
Clear motion event cache and reset in OverlayPanelEventFilter

This change clears the recorded event cache in the
OverlayPanelEventFilter when it does not consume a touch event. This
should prevent beginning event streams with something other than
ACTION_DOWN.

BUG=613069

Committed: https://crrev.com/a4e1e069dcd16629d605668b1fbf3cbb84a12edf
Cr-Commit-Position: refs/heads/master@{#419306}
==========

[commit-bot: I haz the power, 2016-09-16 22:37:25]

Patchset 1 (id:??) landed as
https://crrev.com/a4e1e069dcd16629d605668b1fbf3cbb84a12edf
Cr-Commit-Position: refs/heads/master@{#419306}