1140578 - GeckoTouchDispatcher generates improper event streams due to resampling

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test page

This is a follow-up from bug 1139575. :djf created a test page (which I've attached to this bug) which detects unexpected events in the touch event stream and reports it (see bug 1139575 comment 33 for a fuller description, but it's pretty self-explanatory). Pinching around this page on a Flame often triggers the warning. I traced the problem to the GeckoTouchDispatcher. Sequences like so:

touchstart (1 point)
touchmove (1 point)
touchstart (2 points)
vsync

will result in the following events generated:

touchstart (1 point)
touchstart (2 points)
touchmove (1 point)

which is unexpected, because once the second touchstart happens all touchmoves should have two points. There may be a corresponding problem with touchend.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Remove redundant code

DispatchTouchMoveEvents already checks the size of mTouchMoveEvents, so there's no need to check it again in NotifyVsync except for the return value... which isn't used.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Remove more redundant code

DispatchTouchMoveEvents takes care of clearing the mTouchMoveEvents queue in all branches anyway, so there's no need to do it here.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Add a flag to track pending touchmoves

Sometimes the resampling code leaves a touchmove in the queue, and sometimes it doesn't. It makes it hard to reason about whether or not we actually need to dispatch a move event given stuff in the queue. It probably works in all cases now, but only if we never call DispatchTouchMoveEvents outside of NotifyVsync. In part 4 I'm going to break that assumption so I need this explicit flag to track state.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Don't resample while we have inflight non-move events

Basically this prevents resampling of touchmove events across non-move events. All move events should still be getting preserved and dispatched to content, it's just that some of the move events that come immediately after a non-move event may not be resampled, so as to preserve the ordering of events.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Don't resample while we have inflight non-move events

Missed the .h file

Comment 6

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8574679 [details] [diff] [review]
Part 4 - Don't resample while we have inflight non-move events

Review of attachment 8574679 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +142,5 @@
> +}
> +
> +void
> +GeckoTouchDispatcher::DispatchTouchNonMoveEvent(MultiTouchInput aInput)
> +{

nit: AssertOnControllerThread().

@@ +152,5 @@
>    }
> +  DispatchTouchEvent(aInput);
> +
> +  MutexAutoLock lock(mTouchQueueLock);
> +  mInflightNonMoveEvents--;

nit: Can we assert that mInflightNonMoveEvents is >= 0?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #6)
> > +  DispatchTouchEvent(aInput);
> > +
> > +  MutexAutoLock lock(mTouchQueueLock);
> > +  mInflightNonMoveEvents--;
> 
> nit: Can we assert that mInflightNonMoveEvents is >= 0?

If I wrap it in a "if (mResamplingEnabled)" block then yes. I'll do that, and add the other assert as well.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/5e8615919055
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/027665c616dd
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/394a63d190e0
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/49bbfe7887d5

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e8615919055
https://hg.mozilla.org/mozilla-central/rev/027665c616dd
https://hg.mozilla.org/mozilla-central/rev/394a63d190e0
https://hg.mozilla.org/mozilla-central/rev/49bbfe7887d5

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Doug/Alex/David - please let me know if you are still seeing issues with bad touch event streams on a build with these patches and the patches from bug 1139575.

Comment 11

[:gerard-majax]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Doug/Alex/David - please let me know if you are still seeing issues with bad
> touch event streams on a build with these patches and the patches from bug
> 1139575.

No issue at all on Gallery app, and the test page proposed never reported issued even with 6 or 7 fingers.

Comment 12

[David Flanagan [:djf]]

Both the test app and the gallery app now work for me using a nightly build on Flame. Thanks, Kats!

Comment 13

[Doug Sherk (:drs) (inactive)]

It runs really well now, especially in our Customizer app where these problems initially showed up.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Great, thanks! I'll request uplift to 2.2 for bug 1139575 and this.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8574674 [details] [diff] [review]
Part 1 - Remove redundant code

NOTE: Approval request applies to all 4 patches. Just flagging one since it's easier.
=====

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): touch resampling (part of project silk), specifically bug 1062331 turned it on
User impact if declined: APZ and touch listeners in web content can get bad touch event streams, with touch events coming out-of-order.
Testing completed: verified on master by the people who were seeing the problem (see last few comments above)
Risk to taking this patch (and alternatives if risky): fairly low risk; first two patches are just minor cleanup and the other two are also pretty small.
String or UUID changes made by this patch: none

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Rebased and uplifted:

remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/02cd68669367
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a9998097ac85
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4cb1dcc73c38
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cc165b6fe33d