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

Review URL: https://codereview.chromium.org/586553002

Cr-Commit-Position: refs/heads/master@{#296205}

content/browser/renderer_host/input/input_router_impl_unittest.cc

@@ -284,7 +284,7 @@ class InputRouterImplTest : public testing::Test {
   }
 
   bool TouchEventTimeoutEnabled() const {
-    return input_router()->touch_event_queue_.ack_timeout_enabled();
+    return input_router()->touch_event_queue_.IsAckTimeoutEnabled();
   }
 
   void Flush() const {
@@ -1105,53 +1105,40 @@ TEST_F(InputRouterImplTest,
   // Start a touch sequence.
   PressTouchPoint(1, 1);
   SendTouchEvent();
+  EXPECT_EQ(1U, GetSentMessageCountAndResetSink());
 
   // TOUCH_ACTION_NONE should disable the timeout.
   OnSetTouchAction(TOUCH_ACTION_NONE);
   SendInputEventACK(WebInputEvent::TouchStart, INPUT_EVENT_ACK_STATE_CONSUMED);
+  EXPECT_EQ(1U, ack_handler_->GetAndResetAckCount());
   EXPECT_FALSE(TouchEventTimeoutEnabled());
 
-  // End the touch sequence.
-  ReleaseTouchPoint(0);
+  MoveTouchPoint(0, 1, 1);
   SendTouchEvent();
-  SendInputEventACK(WebInputEvent::TouchEnd, INPUT_EVENT_ACK_STATE_CONSUMED);
   EXPECT_FALSE(TouchEventTimeoutEnabled());
-  ack_handler_->GetAndResetAckCount();
-  GetSentMessageCountAndResetSink();
-
-  // Start another touch sequence.  While this does restore the touch timeout
-  // the timeout will not apply until the *next* touch sequence. This affords a
-  // touch-action response from the renderer without racing against the timeout.
-  PressTouchPoint(1, 1);
-  SendTouchEvent();
-  EXPECT_TRUE(TouchEventTimeoutEnabled());
   EXPECT_EQ(1U, GetSentMessageCountAndResetSink());
 
-  // Delay the ack.  The timeout should *not* fire.
+  // Delay the move ack. The timeout should not fire.
   RunTasksAndWait(base::TimeDelta::FromMilliseconds(timeout_ms + 1));
   EXPECT_EQ(0U, ack_handler_->GetAndResetAckCount());
   EXPECT_EQ(0U, GetSentMessageCountAndResetSink());
-
-  // Finally send the ack.  The touch sequence should not have been cancelled.
-  SendInputEventACK(WebInputEvent::TouchStart,
-                    INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
-  EXPECT_TRUE(TouchEventTimeoutEnabled());
+  SendInputEventACK(WebInputEvent::TouchEnd, INPUT_EVENT_ACK_STATE_CONSUMED);
   EXPECT_EQ(1U, ack_handler_->GetAndResetAckCount());
-  EXPECT_EQ(0U, GetSentMessageCountAndResetSink());
 
-  // End the sequence.
+  // End the touch sequence.
   ReleaseTouchPoint(0);
   SendTouchEvent();
   SendInputEventACK(WebInputEvent::TouchEnd, INPUT_EVENT_ACK_STATE_CONSUMED);
-  EXPECT_EQ(1U, ack_handler_->GetAndResetAckCount());
-  EXPECT_EQ(1U, GetSentMessageCountAndResetSink());
+  EXPECT_FALSE(TouchEventTimeoutEnabled());
+  ack_handler_->GetAndResetAckCount();
+  GetSentMessageCountAndResetSink();
 
-  // A new touch sequence should (finally) be subject to the timeout.
+  // Start another touch sequence.  This should restore the touch timeout.
   PressTouchPoint(1, 1);
   SendTouchEvent();
   EXPECT_TRUE(TouchEventTimeoutEnabled());
-  EXPECT_EQ(0U, ack_handler_->GetAndResetAckCount());
   EXPECT_EQ(1U, GetSentMessageCountAndResetSink());
+  EXPECT_EQ(0U, ack_handler_->GetAndResetAckCount());
 
   // Wait for the touch ack timeout to fire.
   RunTasksAndWait(base::TimeDelta::FromMilliseconds(timeout_ms + 1));

content/browser/renderer_host/input/touch_event_queue.h

@@ -14,6 +14,7 @@
 #include "content/common/content_export.h"
 #include "content/common/input/input_event_ack_state.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
+#include "ui/events/gesture_detection/bitset_32.h"
 #include "ui/gfx/geometry/point_f.h"
 
 namespace content {
@@ -114,6 +115,10 @@ class CONTENT_EXPORT TouchEventQueue {
   // it will take effect only for the following touch sequence.
   void SetAckTimeoutEnabled(bool enabled);
 
+  bool IsAckTimeoutEnabled() const;
+
+  bool IsForwardingTouches();
+
   bool empty() const WARN_UNUSED_RESULT {
     return touch_queue_.empty();
   }
@@ -122,13 +127,7 @@ class CONTENT_EXPORT TouchEventQueue {
     return touch_queue_.size();
   }
 
-  bool ack_timeout_enabled() const {
-    return ack_timeout_enabled_;
-  }
-
-  bool has_handlers() const {
-    return touch_filtering_state_ != DROP_ALL_TOUCHES;
-  }
+  bool has_handlers() const { return has_handlers_; }
 
  private:
   class TouchTimeoutHandler;
@@ -181,10 +180,8 @@ class CONTENT_EXPORT TouchEventQueue {
   // has no touch handler.
   PreFilterResult FilterBeforeForwarding(const blink::WebTouchEvent& event);
   void ForwardToRenderer(const TouchEventWithLatencyInfo& event);
-  void UpdateTouchAckStates(const blink::WebTouchEvent& event,
-                            InputEventAckState ack_result);
-  bool AllTouchAckStatesHaveState(InputEventAckState ack_state) const;
-
+  void UpdateTouchConsumerStates(const blink::WebTouchEvent& event,
+                                 InputEventAckState ack_result);
 
   // Handles touch event forwarding and ack'ed event dispatch.
   TouchEventQueueClient* client_;
@@ -192,9 +189,11 @@ class CONTENT_EXPORT TouchEventQueue {
   typedef std::deque<CoalescedWebTouchEvent*> TouchQueue;
   TouchQueue touch_queue_;
 
-  // Maintain the ACK status for each touch point.
-  typedef std::map<int, InputEventAckState> TouchPointAckStates;
-  TouchPointAckStates touch_ack_states_;
+  // Maps whether each active pointer has a consumer (i.e., a touch point has a
+  // valid consumer iff |touch_consumer_states[pointer.id]| is true.).
+  // TODO(jdduke): Consider simply tracking whether *any* touchstart had a
+  // consumer, crbug.com/416497.
+  ui::BitSet32 touch_consumer_states_;
 
   // Position of the first touch in the most recent sequence forwarded to the
   // client.
@@ -209,18 +208,18 @@ class CONTENT_EXPORT TouchEventQueue {
   // ack after forwarding a touch event to the client.
   bool dispatching_touch_;
 
-  enum TouchFilteringState {
-    FORWARD_ALL_TOUCHES,           // Don't filter at all - the default.
-    FORWARD_TOUCHES_UNTIL_TIMEOUT, // Don't filter unless we get an ACK timeout.
-    DROP_TOUCHES_IN_SEQUENCE,      // Filter all events until a new touch
-                                   // sequence is received.
-    DROP_ALL_TOUCHES,              // Filter all events, e.g., no touch handler.
-    TOUCH_FILTERING_STATE_DEFAULT = FORWARD_ALL_TOUCHES,
-  };
-  TouchFilteringState touch_filtering_state_;
+  // Whether the renderer has at least one touch handler.
+  bool has_handlers_;
+
+  // Whether to allow any remaining touches for the current sequence. Note that
+  // this is a stricter condition than an empty |touch_consumer_states_|, as it
+  // also prevents forwarding of touchstart events for new pointers in the
+  // current sequence. This is only used when the event is synthetically
+  // cancelled after a touch timeout, or after a scroll event when the
+  // mode is TOUCH_SCROLLING_MODE_TOUCHCANCEL.
+  bool drop_remaining_touches_in_sequence_;
 
   // Optional handler for timed-out touch event acks.
-  bool ack_timeout_enabled_;
   scoped_ptr<TouchTimeoutHandler> timeout_handler_;
 
   // Suppression of TouchMove's within a slop region when a sequence has not yet

content/browser/renderer_host/input/touch_event_queue_unittest.cc

@@ -196,8 +196,6 @@ class TouchEventQueueTest : public testing::Test,
 
   void SetAckTimeoutDisabled() { queue_->SetAckTimeoutEnabled(false); }
 
-  bool IsTimeoutEnabled() const { return queue_->ack_timeout_enabled(); }
-
   bool IsTimeoutRunning() const { return queue_->IsTimeoutRunningForTesting(); }
 
   bool HasPendingAsyncTouchMove() const {
@@ -285,9 +283,55 @@ TEST_F(TouchEventQueueTest, Basic) {
   EXPECT_TRUE(acked_event().cancelable);
 }
 
-// Tests that the touch-queue is emptied after the outstanding ack is received
-// if a page stops listening for touch events.
-TEST_F(TouchEventQueueTest, QueueFlushedOnAckAfterHandlersRemoved) {
+// Tests that touch-events with multiple points are queued properly.
+TEST_F(TouchEventQueueTest, BasicMultiTouch) {
+  const size_t kPointerCount = 10;
+  for (size_t i = 0; i < kPointerCount; ++i)
+    PressTouchPoint(i, i);
+
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+  EXPECT_EQ(0U, GetAndResetAckedEventCount());
+  EXPECT_EQ(kPointerCount, queued_event_count());
+
+  for (size_t i = 0; i < kPointerCount; ++i)
+    MoveTouchPoint(i, 1.f + i, 2.f + i);
+
+  EXPECT_EQ(0U, GetAndResetSentEventCount());
+  EXPECT_EQ(0U, GetAndResetAckedEventCount());
+  // All moves should coalesce.
+  EXPECT_EQ(kPointerCount + 1, queued_event_count());
+
+  for (size_t i = 0; i < kPointerCount; ++i)
+    ReleaseTouchPoint(kPointerCount - 1 - i);
+
+  EXPECT_EQ(0U, GetAndResetSentEventCount());
+  EXPECT_EQ(0U, GetAndResetAckedEventCount());
+  EXPECT_EQ(kPointerCount * 2 + 1, queued_event_count());
+
+  // Ack all presses.
+  for (size_t i = 0; i < kPointerCount; ++i)
+    SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
+
+  EXPECT_EQ(kPointerCount, GetAndResetAckedEventCount());
+  EXPECT_EQ(kPointerCount, GetAndResetSentEventCount());
+
+  // Ack the coalesced move.
+  SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
+  EXPECT_EQ(kPointerCount, GetAndResetAckedEventCount());
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+
+  // Ack all releases.
+  for (size_t i = 0; i < kPointerCount; ++i)
+    SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
+
+  EXPECT_EQ(kPointerCount, GetAndResetAckedEventCount());
+  EXPECT_EQ(kPointerCount - 1, GetAndResetSentEventCount());
+}
+
+// Tests that the touch-queue continues delivering events for an active pointer
+// after all handlers are removed, but acks new pointers immediately as having
+// no consumer.
+TEST_F(TouchEventQueueTest, NoNewTouchesForwardedAfterHandlersRemoved) {
   OnHasTouchEventHandlers(true);
   EXPECT_EQ(0U, queued_event_count());
   EXPECT_EQ(0U, GetAndResetSentEventCount());
@@ -309,37 +353,37 @@ TEST_F(TouchEventQueueTest, QueueFlushedOnAckAfterHandlersRemoved) {
   EXPECT_EQ(0U, queued_event_count());
   EXPECT_EQ(INPUT_EVENT_ACK_STATE_CONSUMED, acked_event_state());
 
-  // The release should not be forwarded.
-  ReleaseTouchPoint(0);
+  // Try forwarding a new pointer. It should be rejected immediately.
+  PressTouchPoint(2, 2);
   EXPECT_EQ(1U, GetAndResetAckedEventCount());
   EXPECT_EQ(0U, queued_event_count());
   EXPECT_EQ(INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, acked_event_state());
 
-  OnHasTouchEventHandlers(true);
+  // Further events for the pointer without a handler should not be forwarded.
+  MoveTouchPoint(1, 3, 3);
+  ReleaseTouchPoint(1);
+  EXPECT_EQ(2U, GetAndResetAckedEventCount());
+  EXPECT_EQ(0U, queued_event_count());
+  EXPECT_EQ(INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, acked_event_state());
 
-  // Events will be queued until the first sent event is ack'ed.
-  for (int i = 0; i < 10; ++i) {
-    PressTouchPoint(1, 1);
-    MoveTouchPoint(0, i, i);
-    ReleaseTouchPoint(0);
-  }
-  EXPECT_EQ(30U, queued_event_count());
+  // Events for the first pointer, that had a handler, should be forwarded, even
+  // if the renderer reports that no handlers exist.
+  MoveTouchPoint(0, 4, 4);
+  ReleaseTouchPoint(0);
   EXPECT_EQ(1U, GetAndResetSentEventCount());
+  EXPECT_EQ(2U, queued_event_count());
 
-  // Signal that all touch handlers have been removed. Note that flushing of
-  // the queue will not occur until *after* the outstanding ack is received.
-  OnHasTouchEventHandlers(false);
-  EXPECT_EQ(30U, queued_event_count());
-  EXPECT_EQ(0U, GetAndResetSentEventCount());
-  EXPECT_EQ(0U, GetAndResetAckedEventCount());
+  SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+  EXPECT_EQ(1U, queued_event_count());
+  EXPECT_EQ(INPUT_EVENT_ACK_STATE_CONSUMED, acked_event_state());
 
-  // Receive an ACK for the first touch-event. All remaining touch events should
-  // be flushed with the appropriate ack type.
   SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
-  EXPECT_EQ(0U, queued_event_count());
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
   EXPECT_EQ(0U, GetAndResetSentEventCount());
-  EXPECT_EQ(30U, GetAndResetAckedEventCount());
-  EXPECT_EQ(INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, acked_event_state());
+  EXPECT_EQ(0U, queued_event_count());
+  EXPECT_EQ(INPUT_EVENT_ACK_STATE_CONSUMED, acked_event_state());
 }
 
 // Tests that addition of a touch handler during a touch sequence will not cause
@@ -423,6 +467,34 @@ TEST_F(TouchEventQueueTest, ActiveSequenceDroppedWhenHandlersRemoved) {
   EXPECT_EQ(1U, GetAndResetSentEventCount());
 }
 
+// Tests that removal/addition of a touch handler without any intervening
+// touch activity has no affect on touch forwarding.
+TEST_F(TouchEventQueueTest,
+       ActiveSequenceUnaffectedByRepeatedHandlerRemovalAndAddition) {
+  // Send a touch-press event.
+  PressTouchPoint(1, 1);
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+  EXPECT_EQ(1U, queued_event_count());
+
+  // Simulate the case where the touchstart handler removes itself, and adds a
+  // touchmove handler.
+  OnHasTouchEventHandlers(false);
+  OnHasTouchEventHandlers(true);
+
+  // Queue a touch-move event.
+  MoveTouchPoint(0, 5, 5);
+  EXPECT_EQ(2U, queued_event_count());
+  EXPECT_EQ(0U, GetAndResetAckedEventCount());
+  EXPECT_EQ(0U, GetAndResetSentEventCount());
+
+  // The ack should trigger forwarding of the touchmove, as if no touch
+  // handler registration changes have occurred.
+  SendTouchEventAck(INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+  EXPECT_EQ(1U, queued_event_count());
+}
+
 // Tests that touch-events are coalesced properly in the queue.
 TEST_F(TouchEventQueueTest, Coalesce) {
   // Send a touch-press event.
@@ -733,10 +805,10 @@ TEST_F(TouchEventQueueTest, AckWithFollowupEvents) {
   // Create a touch event that will be queued synchronously by a touch ack.
   // Note, this will be triggered by all subsequent touch acks.
   WebTouchEvent followup_event;
-  followup_event.type = WebInputEvent::TouchStart;
+  followup_event.type = WebInputEvent::TouchMove;
   followup_event.touchesLength = 1;
-  followup_event.touches[0].id = 1;
-  followup_event.touches[0].state = WebTouchPoint::StatePressed;
+  followup_event.touches[0].id = 0;
+  followup_event.touches[0].state = WebTouchPoint::StateMoved;
   SetFollowupEvent(followup_event);
 
   // Receive an ACK for the press. This should cause the followup touch-move to
@@ -1175,14 +1247,12 @@ TEST_F(TouchEventQueueTest, NoTouchTimeoutIfDisabledAfterTouchStart) {
   // Send the ack immediately. The timeout should not have fired.
   SendTouchEventAck(INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
   EXPECT_FALSE(IsTimeoutRunning());
-  EXPECT_TRUE(IsTimeoutEnabled());
   EXPECT_EQ(1U, GetAndResetSentEventCount());
   EXPECT_EQ(1U, GetAndResetAckedEventCount());
 
   // Now explicitly disable the timeout.
   SetAckTimeoutDisabled();
   EXPECT_FALSE(IsTimeoutRunning());
-  EXPECT_FALSE(IsTimeoutEnabled());
 
   // A TouchMove should not start or trigger the timeout.
   MoveTouchPoint(0, 5, 5);
@@ -1203,19 +1273,6 @@ TEST_F(TouchEventQueueTest, NoTouchTimeoutIfAckIsSynchronous) {
   EXPECT_FALSE(IsTimeoutRunning());
 }
 
-// Tests that the timeout is disabled if the touch handler disappears.
-TEST_F(TouchEventQueueTest, NoTouchTimeoutIfTouchHandlerRemoved) {
-  SetUpForTimeoutTesting(DefaultTouchTimeoutDelay());
-
-  // Queue a TouchStart.
-  PressTouchPoint(0, 1);
-  ASSERT_TRUE(IsTimeoutRunning());
-
-  // Unload the touch handler.
-  OnHasTouchEventHandlers(false);
-  EXPECT_FALSE(IsTimeoutRunning());
-}
-
 // Tests that the timeout does not fire if explicitly disabled while an event
 // is in-flight.
 TEST_F(TouchEventQueueTest, NoTouchTimeoutIfDisabledWhileTimerIsActive) {
@@ -2131,4 +2188,42 @@ TEST_F(TouchEventQueueTest, TouchAbsorptionWithConsumedFirstMove) {
   EXPECT_EQ(0U, GetAndResetSentEventCount());
 }
 
+TEST_F(TouchEventQueueTest, UnseenTouchPointerIdsNotForwarded) {
+  SyntheticWebTouchEvent event;
+  event.PressPoint(0, 0);
+  SendTouchEvent(event);
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+  SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+
+  // Give the touchmove a previously unseen pointer id; it should not be sent.
+  int press_id = event.touches[0].id;
+  event.MovePoint(0, 1, 1);
+  event.touches[0].id = 7;
+  SendTouchEvent(event);
+  EXPECT_EQ(0U, GetAndResetSentEventCount());
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+
+  // Give the touchmove a valid id; it should be sent.
+  event.touches[0].id = press_id;
+  SendTouchEvent(event);
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+  SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+
+  // Do the same for release.
+  event.ReleasePoint(0);
+  event.touches[0].id = 11;
+  SendTouchEvent(event);
+  EXPECT_EQ(0U, GetAndResetSentEventCount());
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+
+  // Give the touchmove a valid id; it should be sent.
+  event.touches[0].id = press_id;
+  SendTouchEvent(event);
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+  SendTouchEventAck(INPUT_EVENT_ACK_STATE_CONSUMED);
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+}
+
 }  // namespace content

content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

@@ -919,7 +919,7 @@ TEST_F(RenderWidgetHostViewAuraTest, TouchEventState) {
   // Ack'ing the outstanding event should flush the pending touch queue.
   InputHostMsg_HandleInputEvent_ACK_Params ack;
   ack.type = blink::WebInputEvent::TouchStart;
-  ack.state = INPUT_EVENT_ACK_STATE_CONSUMED;
+  ack.state = INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS;
   widget_host_->OnMessageReceived(InputHostMsg_HandleInputEvent_ACK(0, ack));
   EXPECT_FALSE(widget_host_->ShouldForwardTouchEvent());
 

ui/events/gesture_detection/bitset_32.h

@@ -6,6 +6,7 @@
 #define UI_EVENTS_GESTURE_DETECTION_BITSET_32_H_
 
 #include "base/basictypes.h"
+#include "base/logging.h"
 
 namespace ui {
 
@@ -20,7 +21,10 @@ struct BitSet32 {
   explicit inline BitSet32(uint32_t value) : value(value) {}
 
   // Gets the value associated with a particular bit index.
-  static inline uint32_t value_for_bit(uint32_t n) { return 0x80000000 >> n; }
+  static inline uint32_t value_for_bit(uint32_t n) {
+    DCHECK_LE(n, 31U);
+    return 0x80000000 >> n;
+  }
 
   // Clears the bit set.
   inline void clear() { value = 0; }
@@ -86,6 +90,7 @@ struct BitSet32 {
   // Gets the inde of the specified bit in the set, which is the number of
   // marked bits that appear before the specified bit.
   inline uint32_t get_index_of_bit(uint32_t n) const {
+    DCHECK_LE(n, 31U);
     return popcnt(value & ~(0xffffffffUL >> n));
   }