Closed
Bug 1140578
Opened 9 years ago
Closed 9 years ago
GeckoTouchDispatcher generates improper event streams due to resampling
Categories
(Core Graveyard :: Widget: Gonk, defect)
Tracking
(firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.1 unaffected, b2g-v2.2 fixed, b2g-master fixed)
VERIFIED
FIXED
mozilla39
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(5 files, 1 obsolete file)
2.53 KB,
text/html
|
Details | |
2.38 KB,
patch
|
mchang
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
mchang
:
review+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
mchang
:
review+
|
Details | Diff | Splinter Review |
4.43 KB,
patch
|
mchang
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8574156 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
Missed the .h file
Attachment #8574677 -
Attachment is obsolete: true
Attachment #8574677 -
Flags: review?(mchang)
Attachment #8574679 -
Flags: review?(mchang)
Updated•9 years ago
|
Attachment #8574674 -
Flags: review?(mchang) → review+
Updated•9 years ago
|
Attachment #8574675 -
Flags: review?(mchang) → review+
Comment 6•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8574676 -
Flags: review?(mchang) → review+
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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
Updated•9 years ago
|
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
status-b2g-v2.1:
--- → unaffected
Comment 11•9 years ago
|
||
(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)
Comment 12•9 years ago
|
||
Both the test app and the gallery app now work for me using a nightly build on Flame. Thanks, Kats!
Flags: needinfo?(dflanagan)
Comment 13•9 years ago
|
||
It runs really well now, especially in our Customizer app where these problems initially showed up.
Flags: needinfo?(dsherk)
Assignee | ||
Comment 14•9 years ago
|
||
Great, thanks! I'll request uplift to 2.2 for bug 1139575 and this.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 15•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8574674 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Assignee | ||
Comment 16•9 years ago
|
||
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
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•