Closed Bug 993936 Opened 10 years ago Closed 10 years ago

Touch events don't reach DOM if one touchend gets lost in nsPresShell

Categories

(Core :: DOM: Events, defect)

29 Branch
x86_64
Mer
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dmitry.rojkov, Assigned: dmitry.rojkov)

Details

Attachments

(1 file, 4 obsolete files)

People using Sailfish browser report quite often that browser suddenly stops handling touch input [1].
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 [2].

It may happen though that one "touchend" fails to reach [2]. 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 [3].

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

[1] https://together.jolla.com/question/1115/bug-browser-occasionally-gets-into-a-state-where-pages-dont-accept-any-touch-input/
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7256
[3] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6748
Attachment #8403819 - Flags: review?(bugs)
(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
Attachment #8403819 - Attachment is obsolete: true
Attachment #8404651 - Attachment is obsolete: true
Attachment #8406173 - Attachment is obsolete: true
Attachment #8411680 - Flags: review?(bugs)
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.
Attachment #8411680 - Attachment is obsolete: true
Attachment #8411680 - Flags: review?(bugs)
Attachment #8411704 - Flags: review?(bugs)
Attachment #8411704 - Flags: review?(bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/114aa4935a43
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.