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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[dmitry.rojkov]

Attachment: Reset gCaptureTouchList upon new touch sequence

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[dmitry.rojkov]

(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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Ah, yes, that could be an issue. Want to fix?

Comment 4

[dmitry.rojkov]

Attachment: Make check for a continuing session stricter

How about making the check stricter?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[dmitry.rojkov]

Attachment: Test eviction of lost touch points

(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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[dmitry.rojkov]

Attachment: Make check for a continuing session stricter

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 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8411680 [details] [diff] [review]
Make check for a continuing session stricter

Why the double setTimeout?

Comment 10

[dmitry.rojkov]

Attachment: Make check for a continuing session stricter

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/114aa4935a43

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/114aa4935a43