Allow repeated handler removal/addition with the TouchEventQueue

Allow repeated handler removal/addition with the TouchEventQueue

A valid scenario, observed in the wild, is a touchstart handler removing
itself and adding a touchmove handler. If those are the only touch
handlers on the document, the renderer will forward two distinct handler
existence notifications to the browser. This causes the TouchEventQueue
to effectively drop the remainder of the touch sequence, even though the
element targetted by the touchstart now has a touchmove.

Fix this scenario by making the TouchEventQueue effectively idempotent
to repeated addition and removal of touch handlers. In practice, this
means simplifying its statefulness and instead relying on the existing
pointer id state map to determine whether to forward touches for the
remainder of the sequence.

BUG=412723
Committed: https://crrev.com/c883176fd86dee2be74dd5e3c6a101f0e4f93bee
Cr-Commit-Position: refs/heads/master@{#296205}

[jdduke (slow), 2014-09-18 23:03:49]

jdduke@chromium.org changed reviewers:
+ rbyers@chromium.org

[jdduke (slow), 2014-09-18 23:03:50]

rbyers@: PTAL, thanks. This isn't quite as simple as I'd like, but it's a step.
I was hoping to get rid of the |drop_remaining_touches_in_sequence_| flag
entirely, instead relying solely on |touch_consumer_states_|. However, the
latter allows new touchstart events to pass through even if the first touchstart
lacked a consumer, but we need a stricter condition for synthetic cancellation.
Maybe someday we'll never have to synthetically cancel the sequence again...

[Rick Byers, 2014-09-19 17:54:07]

Seems like a nice improvement in complexity overall, thanks! I have to admit
though, after not working in this code for a few months, I find it difficult to
reason through all the possible states.  I hope we can find ways to simplify it
further...

LGTM.

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (left):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1123: // the
timeout will not apply until the *next* touch sequence. This affords a
I'm sad to see this go - it means touch-action: none is now a less reliably way
to minimize the pain caused by the timeout in an event-handling desktop app. 
But that's fine - what we really care about is mobile apps anyway.

FWIW I'm trying to ensure that the slimming paint work will let us easily
associate medadata like touch-action with compositor display lists.  Then we
should be able to easily do the touch-action hit testing on impl and address the
problem properly...

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (left):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:542:
DCHECK(!touchmove_slop_suppressor_->suppressing_touchmoves())
We added this DCHECK because we had at least one nasty bug where we'd never
offer the page a touchmove before scrolling.  Can't we still check that?

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:764:
DCHECK_GE(point.id, 0);
I'd prefer if the DCHECK_LT was in the bitset implementation instead of
sprinkled at every callsite.

Perhaps we should have signed overloads that do the DCHECK_GT too (or maybe
WebTouchPoint.id should really be unsigned), or if you want to just check that
here it's fine with me.

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:192: ui::BitSet32
touch_consumer_states_;
I wonder if we could simplify this further by keeping track only of whether
_any_ touch point has a consumer.  Blink already has to deal with getting events
for touch points it didn't know were down.

[jdduke (slow), 2014-09-22 16:30:14]

Thanks for review!

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (left):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1123: // the
timeout will not apply until the *next* touch sequence. This affords a
On 2014/09/19 17:54:06, Rick Byers wrote:
> I'm sad to see this go - it means touch-action: none is now a less reliably
way
> to minimize the pain caused by the timeout in an event-handling desktop app. 
> But that's fine - what we really care about is mobile apps anyway.
> 
> FWIW I'm trying to ensure that the slimming paint work will let us easily
> associate medadata like touch-action with compositor display lists.  Then we
> should be able to easily do the touch-action hit testing on impl and address
the
> problem properly...

Yup, agreed.

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (left):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:542:
DCHECK(!touchmove_slop_suppressor_->suppressing_touchmoves())
On 2014/09/19 17:54:06, Rick Byers wrote:
> We added this DCHECK because we had at least one nasty bug where we'd never
> offer the page a touchmove before scrolling.  Can't we still check that?

Hmm, you're right, I thought it would be tricky but I think we can just check
the consumer state.

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:764:
DCHECK_GE(point.id, 0);
On 2014/09/19 17:54:06, Rick Byers wrote:
> I'd prefer if the DCHECK_LT was in the bitset implementation instead of
> sprinkled at every callsite.
> 
> Perhaps we should have signed overloads that do the DCHECK_GT too (or maybe
> WebTouchPoint.id should really be unsigned), or if you want to just check that
> here it's fine with me.

That's much better, thanks.

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:192: ui::BitSet32
touch_consumer_states_;
On 2014/09/19 17:54:07, Rick Byers wrote:
> I wonder if we could simplify this further by keeping track only of whether
> _any_ touch point has a consumer.  Blink already has to deal with getting
events
> for touch points it didn't know were down.

Hmm, interesting, that is a tempting simplification, though I hesitate to to it
in this bug fix as we've had per-pointer filtering for as long as I can
remember. I'll add a TODO here and play with it as a follow-up.

[jdduke (slow), 2014-09-22 16:30:18]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-09-22 16:31:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/586553002/40001

[Rick Byers, 2014-09-22 16:37:11]

On 2014/09/22 16:30:14, jdduke wrote:
> Thanks for review!
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/input/input_router_impl_unittest.cc (left):
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> content/browser/renderer_host/input/input_router_impl_unittest.cc:1123: // the
> timeout will not apply until the *next* touch sequence. This affords a
> On 2014/09/19 17:54:06, Rick Byers wrote:
> > I'm sad to see this go - it means touch-action: none is now a less reliably
> way
> > to minimize the pain caused by the timeout in an event-handling desktop app.

> > But that's fine - what we really care about is mobile apps anyway.
> > 
> > FWIW I'm trying to ensure that the slimming paint work will let us easily
> > associate medadata like touch-action with compositor display lists.  Then we
> > should be able to easily do the touch-action hit testing on impl and address
> the
> > problem properly...
> 
> Yup, agreed.
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_event_queue.cc (left):
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue.cc:542:
> DCHECK(!touchmove_slop_suppressor_->suppressing_touchmoves())
> On 2014/09/19 17:54:06, Rick Byers wrote:
> > We added this DCHECK because we had at least one nasty bug where we'd never
> > offer the page a touchmove before scrolling.  Can't we still check that?
> 
> Hmm, you're right, I thought it would be tricky but I think we can just check
> the consumer state.
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_event_queue.cc (right):
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue.cc:764:
> DCHECK_GE(point.id, 0);
> On 2014/09/19 17:54:06, Rick Byers wrote:
> > I'd prefer if the DCHECK_LT was in the bitset implementation instead of
> > sprinkled at every callsite.
> > 
> > Perhaps we should have signed overloads that do the DCHECK_GT too (or maybe
> > WebTouchPoint.id should really be unsigned), or if you want to just check
that
> > here it's fine with me.
> 
> That's much better, thanks.
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_event_queue.h (right):
> 
>
https://codereview.chromium.org/586553002/diff/20001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue.h:192: ui::BitSet32
> touch_consumer_states_;
> On 2014/09/19 17:54:07, Rick Byers wrote:
> > I wonder if we could simplify this further by keeping track only of whether
> > _any_ touch point has a consumer.  Blink already has to deal with getting
> events
> > for touch points it didn't know were down.
> 
> Hmm, interesting, that is a tempting simplification, though I hesitate to to
it
> in this bug fix as we've had per-pointer filtering for as long as I can
> remember. I'll add a TODO here and play with it as a follow-up.

Thanks - all changes look good to me.

[commit-bot: I haz the power, 2014-09-22 17:14:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-22 17:14:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdduke (slow), 2014-09-22 17:32:47]

jdduke@chromium.org changed reviewers:
+ sadrul@chromium.org

[jdduke (slow), 2014-09-22 17:32:47]

sadrul@: Can you think of any problems this might introduce? That is, now we
forward the remainder of the touch sequence (if it had a handler upon
touchstart) even if all touch handlers are removed. I think the "no handler"
notification has been used as a flush signal when a new page is loaded, although
in theory that case should still be handled as the page should simply reject the
remaining touches.

In any case, could you take a look at 
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc? Thanks.

[sadrul, 2014-09-23 14:39:17]

On 2014/09/22 17:32:47, jdduke wrote:
> sadrul@: Can you think of any problems this might introduce? That is, now we
> forward the remainder of the touch sequence (if it had a handler upon
> touchstart) even if all touch handlers are removed. I think the "no handler"
> notification has been used as a flush signal when a new page is loaded,
although
> in theory that case should still be handled as the page should simply reject
the
> remaining touches.

I can't think of regressions this may cause.

> 
> In any case, could you take a look at 
> content/browser/renderer_host/render_widget_host_view_aura_unittest.cc?
Thanks.

The changes LGTM.

Another way to work around the multiple has-touch-handler notification from the
renderer to the browser in the scenario mentioned in the CL description would be
to update RenderWidget::hasTouchEventHandlers() to update some internal state
and not send the IPC when handling_input_event(), and add code in
::OnHandleInputEvent to look at that internal state before and after sending the
event to blink and send the IPC if necessary.

[jdduke (slow), 2014-09-23 15:07:08]

On 2014/09/23 14:39:17, sadrul wrote:
> Another way to work around the multiple has-touch-handler notification from
the
> renderer to the browser in the scenario mentioned in the CL description would
be
> to update RenderWidget::hasTouchEventHandlers() to update some internal state
> and not send the IPC when handling_input_event(), and add code in
> ::OnHandleInputEvent to look at that internal state before and after sending
the
> event to blink and send the IPC if necessary.

Yeah, that was my original proposal, but Rick pointed out that such an approach
may not be general enough, e.g., suppose some non-input-triggered JS removes the
touchstart handler then adds a touchmove handler (perhaps after the touchstart
has been handled). Then we're back to where we started.

Still, it might be worth doing that anyway (I actually have a corresponding CL).

[jdduke (slow), 2014-09-23 15:55:52]

On 2014/09/23 15:07:08, jdduke wrote:
> On 2014/09/23 14:39:17, sadrul wrote:
> > Another way to work around the multiple has-touch-handler notification from
> the
> > renderer to the browser in the scenario mentioned in the CL description
would
> be
> > to update RenderWidget::hasTouchEventHandlers() to update some internal
state
> > and not send the IPC when handling_input_event(), and add code in
> > ::OnHandleInputEvent to look at that internal state before and after sending
> the
> > event to blink and send the IPC if necessary.
> 
> Yeah, that was my original proposal, but Rick pointed out that such an
approach
> may not be general enough, e.g., suppose some non-input-triggered JS removes
the
> touchstart handler then adds a touchmove handler (perhaps after the touchstart
> has been handled). Then we're back to where we started.
> 
> Still, it might be worth doing that anyway (I actually have a corresponding
CL).

I wonder if any sites might have have been using this bug as an optimization,
i.e., they look at the touchstart, and if they didn't want to handle it, they
remove the touchstart handler, allowing the first touchmove to begin scrolling
(instead of forcing it to be forwarded to the main thread). That would be wrong
on so many levels, but I wonder :)

[jdduke (slow), 2014-09-23 15:56:08]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-09-23 15:57:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/586553002/100001

[Rick Byers, 2014-09-23 16:01:17]

On 2014/09/23 15:55:52, jdduke wrote:
> On 2014/09/23 15:07:08, jdduke wrote:
> > On 2014/09/23 14:39:17, sadrul wrote:
> > > Another way to work around the multiple has-touch-handler notification
from
> > the
> > > renderer to the browser in the scenario mentioned in the CL description
> would
> > be
> > > to update RenderWidget::hasTouchEventHandlers() to update some internal
> state
> > > and not send the IPC when handling_input_event(), and add code in
> > > ::OnHandleInputEvent to look at that internal state before and after
sending
> > the
> > > event to blink and send the IPC if necessary.
> > 
> > Yeah, that was my original proposal, but Rick pointed out that such an
> approach
> > may not be general enough, e.g., suppose some non-input-triggered JS removes
> the
> > touchstart handler then adds a touchmove handler (perhaps after the
touchstart
> > has been handled). Then we're back to where we started.
> > 
> > Still, it might be worth doing that anyway (I actually have a corresponding
> CL).
> 
> I wonder if any sites might have have been using this bug as an optimization,
> i.e., they look at the touchstart, and if they didn't want to handle it, they
> remove the touchstart handler, allowing the first touchmove to begin scrolling
> (instead of forcing it to be forwarded to the main thread). That would be
wrong
> on so many levels, but I wonder :)

Ha!  That would be very brittle.  Region-based cc touch hit testing doesn't
support this (there the decision is made at touchstart time), so any other
handlers anywhere on the page (even implicit ones like due to <input type=range>
and touch-action) would defeat this "optimization".  I think we can safely
ignore this.

[commit-bot: I haz the power, 2014-09-23 16:49:29]

Committed patchset #6 (id:100001) as b5e09f78c8732b03c45b49213366965825f2d138

[commit-bot: I haz the power, 2014-09-23 16:50:17]

Patchset 6 (id:??) landed as
https://crrev.com/c883176fd86dee2be74dd5e3c6a101f0e4f93bee
Cr-Commit-Position: refs/heads/master@{#296205}