GeckoTouchDispatcher generates improper event streams due to resampling

VERIFIED FIXED in Firefox 39, Firefox OS v2.2

Status

()

Core
Widget: Gonk
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla39
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.1 unaffected, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Created attachment 8574156 [details]
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.
See Also: → bug 1139575
Attachment #8574156 - Attachment mime type: text/plain → text/html
Created attachment 8574674 [details] [diff] [review]
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.
Attachment #8574674 - Flags: review?(mchang)
Created attachment 8574675 [details] [diff] [review]
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.
Attachment #8574675 - Flags: review?(mchang)
Created attachment 8574676 [details] [diff] [review]
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.
Attachment #8574676 - Flags: review?(mchang)
Created attachment 8574677 [details] [diff] [review]
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.
Attachment #8574677 - Flags: review?(mchang)
Created attachment 8574679 [details] [diff] [review]
Part 4 - Don't resample while we have inflight non-move events

Missed the .h file
Attachment #8574677 - Attachment is obsolete: true
Attachment #8574677 - Flags: review?(mchang)
Attachment #8574679 - Flags: review?(mchang)
Attachment #8574674 - Flags: review?(mchang) → review+
Attachment #8574675 - Flags: review?(mchang) → review+
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?
Attachment #8574679 - Flags: review?(mchang) → review+
Attachment #8574676 - Flags: review?(mchang) → review+
(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.
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
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.
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(drs.bugzilla)
Flags: needinfo?(dflanagan)
status-b2g-v2.1: --- → unaffected
status-b2g-master: affected → fixed
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(lissyx+mozillians)
Both the test app and the gallery app now work for me using a nightly build on Flame. Thanks, Kats!
Flags: needinfo?(dflanagan)
It runs really well now, especially in our Customizer app where these problems initially showed up.
Flags: needinfo?(dsherk)
Great, thanks! I'll request uplift to 2.2 for bug 1139575 and this.
Status: RESOLVED → VERIFIED
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
Attachment #8574674 - Flags: approval-mozilla-b2g37?
Blocks: 1062331

Updated

3 years ago
Attachment #8574674 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.