Closed Bug 993936 Opened 6 years ago Closed 6 years ago
Touch events don't reach DOM if one touchend gets lost in ns
People using Sailfish browser report quite often that browser suddenly stops handling touch input . The bug is not easy to reproduce, but most often it happens on JS-heavy sites when a user often clicks links while pinching not fully loaded content thus flooding the event loop with touch events. The problem seems to be that Gecko maintains global state in the list gCaptureTouchList inside layout/base/nsPresShell.cpp by adding touch points to the list upon "touchstart" and removing them upon "touchend" in . It may happen though that one "touchend" fails to reach . The root cause for that is still unclear. But when it happens the global state becomes inconsistent: gCaptureTouchList keeps a lost touchpoint forever. As result nsPresShell starts to remove all touch points from "touchstart" events in . The attached workaround resets the global list upon a new touch start/move/end sequence. The try build: https://tbpl.mozilla.org/?tree=Try&rev=cfc0acc918d5  https://together.jolla.com/question/1115/bug-browser-occasionally-gets-into-a-state-where-pages-dont-accept-any-touch-input/  http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7256  http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6748
Comment on attachment 8403819 [details] [diff] [review] Reset gCaptureTouchList upon new touch sequence Why isn't http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=6625164b8e4f&mark=7213-7213,7217-7217#7208 enough? Should we perhaps tweak http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=6625164b8e4f&mark=6282-6282#6282
Attachment #8403819 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #1) > Comment on attachment 8403819 [details] [diff] [review] > Reset gCaptureTouchList upon new touch sequence > > Why isn't > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell. > cpp?rev=6625164b8e4f&mark=7213-7213,7217-7217#7208 enough? Right. I've been working on mozilla29 and missed the bug 967236. With that fix cherry-picked I couldn't reproduce the problem. The only thing that worries me now is that the fix clears the list a bit too late. In this line http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6704 the variable anyTarget may become set and this code path http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6724 won't be triggered.
Ah, yes, that could be an issue. Want to fix?
How about making the check stricter?
Assignee: nobody → dmitry.rojkov
Status: NEW → ASSIGNED
Attachment #8404651 - Flags: review?(bugs)
Comment on attachment 8404651 [details] [diff] [review] Make check for a continuing session stricter Would be great if you could write a mochitest for this.
Attachment #8404651 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #5) > > Would be great if you could write a mochitest for this. Well, at least I tried to write it :) Please see the attachment. Unfortunately this test doesn't pass with my setup: for some reason the first multitouch event gets handled in the function listening to "touchstart" events twice. As result the last two assertions fail. I'm not sure if the test should be a part of this bug.
Tests are part of the bug they are testing. In general no fixes should be checked in without a test - that way catching regressions later is much simpler.
Added a test case checking that a lost touch point gets actually evicted in case of new single touch sequence. The try build with the test case: https://tbpl.mozilla.org/?tree=Try&rev=6ebe207adda2
Comment on attachment 8411680 [details] [diff] [review] Make check for a continuing session stricter Why the double setTimeout?
Right, the double setTimeout has been copy&pasted from another test case. I've removed one setTimeout - the test case still passes locally. Without timeouts it breaks.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.