Don't reset touch action when there is a active touch sequence

Right now when the browser receives the OnHasTouchEventHandlers IPC
message, it resets the |scrolling_touch_action_| as long as both of
these are true:
1. it is not in the middle of a gesture sequence.
2. the IPC message indicates that there is a touch event handler.

Consider the following case: we have received the touch start ACK from
the renderer, which assgins a value to |allowed_touch_action_|, then
before GestureTapDown comes, JS removes the touch event listener and
re-added it. In this case, it is not in the middle of a gesture
sequence beause GestureTapDown has not arrived yet, so our code
resets both the |allowed_touch_action_| and the
|scrolling_touch_action_|. Later on when GestureScrollBegin comes,
the browser crashes.

This CL fixes the issue by adding a |active_touch_in_progress_|
member variable. It is true when we receive a touch sequence
started, and false at touch end. When we receive the
OnHasTouchEventHandler(true), we should reset the touch actions
only when there is no active touch and no gesture sequence.

TBR=dtapuska@chromium.org

Bug: 851644
Change-Id: I269d77dff0ae0e577a55734550cc639363267e50
Reviewed-on: https://chromium-review.googlesource.com/1195146
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587262}

content/browser/renderer_host/input/input_router_impl.cc

@@ -352,12 +352,14 @@ void InputRouterImpl::OnTouchEventAck(const TouchEventWithLatencyInfo& event,
     touch_action_filter_.AppendToGestureSequenceForDebugging("T");
     // Touch action must be auto when there is no consumer
     touch_action_filter_.OnSetTouchAction(cc::kTouchActionAuto);
+    touch_action_filter_.SetActiveTouchInProgress(true);
     UpdateTouchAckTimeoutEnabled();
   }
   disposition_handler_->OnTouchEventAck(event, ack_source, ack_result);
 
   if (WebTouchEventTraits::IsTouchSequenceEnd(event.event)) {
     touch_action_filter_.ReportAndResetTouchAction();
+    touch_action_filter_.SetActiveTouchInProgress(false);
     UpdateTouchAckTimeoutEnabled();
   }
 }
@@ -613,6 +615,7 @@ void InputRouterImpl::OnHasTouchEventHandlers(bool has_handlers) {
 void InputRouterImpl::ForceSetTouchActionAuto() {
   touch_action_filter_.AppendToGestureSequenceForDebugging("F");
   touch_action_filter_.OnSetTouchAction(cc::kTouchActionAuto);
+  touch_action_filter_.SetActiveTouchInProgress(true);
 }
 
 void InputRouterImpl::OnHasTouchEventHandlersForTest(bool has_handlers) {
@@ -632,6 +635,7 @@ void InputRouterImpl::OnSetTouchAction(cc::TouchAction touch_action) {
   touch_action_filter_.AppendToGestureSequenceForDebugging(
       std::to_string(touch_action).c_str());
   touch_action_filter_.OnSetTouchAction(touch_action);
+  touch_action_filter_.SetActiveTouchInProgress(true);
 
   // kTouchActionNone should disable the touch ack timeout.
   UpdateTouchAckTimeoutEnabled();

content/browser/renderer_host/input/touch_action_filter.cc

@@ -237,6 +237,11 @@ void TouchActionFilter::OnSetTouchAction(cc::TouchAction touch_action) {
   scrolling_touch_action_ = allowed_touch_action_;
 }
 
+void TouchActionFilter::SetActiveTouchInProgress(
+    bool active_touch_in_progress) {
+  active_touch_in_progress_ = active_touch_in_progress;
+}
+
 void TouchActionFilter::ReportAndResetTouchAction() {
   if (has_touch_event_handler_)
     gesture_sequence_.append("R1");
@@ -349,14 +354,14 @@ void TouchActionFilter::OnHasTouchEventHandlers(bool has_handlers) {
     gesture_sequence_.append("L1");
   else
     gesture_sequence_.append("L0");
-  ResetTouchAction();
-  // If a page has a touch event handler, this function can be called twice with
-  // has_handlers = false first and then true later. When it is true, we need to
-  // reset the |scrolling_touch_action_|. However, we do not want to reset it if
-  // there is an active gesture sequence in progress. For example, the
-  // OnHasTouchEventHandlers(true) can be received after a GestureTapDown.
-  if (has_touch_event_handler_ && !gesture_sequence_in_progress_)
-    scrolling_touch_action_.reset();
+  // We have set the associated touch action if the touch start already happened
+  // or there is a gesture in progress. In these cases, we should not reset the
+  // associated touch action.
+  if (!gesture_sequence_in_progress_ && !active_touch_in_progress_) {
+    ResetTouchAction();
+    if (has_touch_event_handler_)
+      scrolling_touch_action_.reset();
+  }
 }
 
 }  // namespace content

content/browser/renderer_host/input/touch_action_filter.h

@@ -67,6 +67,8 @@ class CONTENT_EXPORT TouchActionFilter {
 
   void OnHasTouchEventHandlers(bool has_handlers);
 
+  void SetActiveTouchInProgress(bool active_touch_in_progress);
+
   // Debugging only.
   void AppendToGestureSequenceForDebugging(const char* str);
 
@@ -105,6 +107,9 @@ class CONTENT_EXPORT TouchActionFilter {
   // before GSE.
   bool gesture_sequence_in_progress_ = false;
 
+  // True at touch start and false at touch end.
+  bool active_touch_in_progress_ = false;
+
   // What touch actions are currently permitted.
   base::Optional<cc::TouchAction> allowed_touch_action_;
 

content/browser/renderer_host/input/touch_action_filter_unittest.cc

@@ -1072,6 +1072,22 @@ TEST_F(TouchActionFilterTest,
   EXPECT_FALSE(ScrollingTouchAction().has_value());
 }
 
+TEST_F(TouchActionFilterTest, OnHasTouchEventHandlersReceivedAfterTouchStart) {
+  filter_.OnHasTouchEventHandlers(true);
+  EXPECT_FALSE(ScrollingTouchAction().has_value());
+  EXPECT_FALSE(filter_.allowed_touch_action().has_value());
+
+  // Receive a touch start ack, set the touch action.
+  filter_.OnSetTouchAction(cc::kTouchActionPanY);
+  filter_.SetActiveTouchInProgress(true);
+  filter_.OnHasTouchEventHandlers(false);
+  EXPECT_EQ(ScrollingTouchAction().value(), cc::kTouchActionPanY);
+  EXPECT_EQ(filter_.allowed_touch_action().value(), cc::kTouchActionPanY);
+  filter_.OnHasTouchEventHandlers(true);
+  EXPECT_EQ(ScrollingTouchAction().value(), cc::kTouchActionPanY);
+  EXPECT_EQ(filter_.allowed_touch_action().value(), cc::kTouchActionPanY);
+}
+
 // If the renderer is busy, the gesture event might have come before the
 // OnHasTouchEventHanlders IPC is received. In this case, we should allow all
 // the gestures.