New issue
Advanced search Search tips

Issue 851644 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 20
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ensure scrolling_touch_action_ has value when used

Project Member Reported by xidac...@chromium.org, Jun 11 2018

Issue description

We could end up having no value in the case when all the gesture event
are queued up and flushed after touch sequence end (finger lifted up).
 
Besides the touch ack timeout case, there are other two possibilities:
1. Start a fling with touchpad, then do fling boosting with touch screen.
2. Somehow TapDown is hit testing on a iframe, and GSB is targeted on the main frame.
I investigated a bit about the non-iframe timeout case, and that should be fine. This is because in the PassthroughTouchEventQueue::FlushQueue, we the ack state to INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS and that will set touch action to Auto.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40d56da5038844308b4d3921ae8aa0900d592650

commit 40d56da5038844308b4d3921ae8aa0900d592650
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Jul 25 13:31:26 2018

Handle iframe touch ack time out

When there is a touch ack time out, all the gesture events will be flushed
to the touch action filter. This case is already taken care of in the
non-iframe case, and this CL handles the iframe case.

When we see that a gesture event is flushed due to time out, we set the
effective touch action to auto which will allow all the gesture event.
A browser test is added to ensure the correctness.

This CL is break into few patches:
PS#22: The essential fix in line 1190 in the
render_widget_host_input_event_router.cc is commented out. When running the
newly added browser test, it will hit the DCHECK in touch_action_filter.cc
at line 55.

PS#23: Uncomment the essential fix in the
render_widget_host_input_event_router.cc and remove the DCHECK in
touch_action_filter.cc at line 55. Uncomment the temporary fix in
touch_action_filter.cc for bug 851664.

TBR=creis@chromium.org

Bug:  851644 
Change-Id: Ic381dd17fa531c1e9f7d649b48dc9f0f0d7d9c21
Reviewed-on: https://chromium-review.googlesource.com/1102729
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577858}
[modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/input/touch_action_filter_unittest.cc
[modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_targeter.cc
[modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_targeter.h
[modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/site_per_process_hit_test_browsertest.cc
[add] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/test/data/frame_tree/page_with_janky_frame.html
[add] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/test/data/page_with_touch_start_janking_main_thread.html

Status: Fixed (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a48983eff56446dba26a010834780667362eaafd

commit a48983eff56446dba26a010834780667362eaafd
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Jul 25 20:03:27 2018

Revert "Handle iframe touch ack time out"

This reverts commit 40d56da5038844308b4d3921ae8aa0900d592650.

Reason for revert: <INSERT REASONING HERE>
The test seems to be flaky, it failed sometimes:
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/47384
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/47378

Original change's description:
> Handle iframe touch ack time out
> 
> When there is a touch ack time out, all the gesture events will be flushed
> to the touch action filter. This case is already taken care of in the
> non-iframe case, and this CL handles the iframe case.
> 
> When we see that a gesture event is flushed due to time out, we set the
> effective touch action to auto which will allow all the gesture event.
> A browser test is added to ensure the correctness.
> 
> This CL is break into few patches:
> PS#22: The essential fix in line 1190 in the
> render_widget_host_input_event_router.cc is commented out. When running the
> newly added browser test, it will hit the DCHECK in touch_action_filter.cc
> at line 55.
> 
> PS#23: Uncomment the essential fix in the
> render_widget_host_input_event_router.cc and remove the DCHECK in
> touch_action_filter.cc at line 55. Uncomment the temporary fix in
> touch_action_filter.cc for bug 851664.
> 
> TBR=creis@chromium.org
> 
> Bug:  851644 
> Change-Id: Ic381dd17fa531c1e9f7d649b48dc9f0f0d7d9c21
> Reviewed-on: https://chromium-review.googlesource.com/1102729
> Commit-Queue: Xida Chen <xidachen@chromium.org>
> Reviewed-by: Timothy Dresser <tdresser@chromium.org>
> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577858}

TBR=tdresser@chromium.org,xidachen@chromium.org,nzolghadr@chromium.org

Change-Id: I266946883f56f95fa2fdf0ee56d9b678221a7d60
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  851644 
Reviewed-on: https://chromium-review.googlesource.com/1150583
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578028}
[modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/input/touch_action_filter_unittest.cc
[modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_targeter.cc
[modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_targeter.h
[modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/site_per_process_hit_test_browsertest.cc
[delete] https://crrev.com/a2c9d0a944b7aab058845d061e877104702534cf/content/test/data/frame_tree/page_with_janky_frame.html
[delete] https://crrev.com/a2c9d0a944b7aab058845d061e877104702534cf/content/test/data/page_with_touch_start_janking_main_thread.html

Status: Assigned (was: Fixed)
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38642faa718b019b824e178b73ae72d60d017892

commit 38642faa718b019b824e178b73ae72d60d017892
Author: Xida Chen <xidachen@chromium.org>
Date: Mon Jul 30 17:11:43 2018

Reland of: Handle iframe touch ack time out

The previous CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1102729
was reverted. The reason is that in the test we have two gesture scroll,
and the second gesture starts before the first TapDown finishes.

This CL solve the problem. PS#1 is exactly the same as the previous CL
which was reverted, so that it is easier for review.

TBR=creis@chromium.org

Bug:  851644 
Change-Id: Ife9bf8f9a37892380e6587b80bcc03366cc0e80d
Reviewed-on: https://chromium-review.googlesource.com/1151094
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579074}
[modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/input/touch_action_filter_unittest.cc
[modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_targeter.cc
[modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_targeter.h
[modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/site_per_process_hit_test_browsertest.cc
[add] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/test/data/frame_tree/page_with_janky_frame.html
[add] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/test/data/page_with_touch_start_janking_main_thread.html

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3

commit 205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Jul 31 20:04:57 2018

Revert "Reland of: Handle iframe touch ack time out"

This reverts commit 38642faa718b019b824e178b73ae72d60d017892.

Reason for revert: <INSERT REASONING HERE>
Crashes on Win canary

Original change's description:
> Reland of: Handle iframe touch ack time out
> 
> The previous CL:
> https://chromium-review.googlesource.com/c/chromium/src/+/1102729
> was reverted. The reason is that in the test we have two gesture scroll,
> and the second gesture starts before the first TapDown finishes.
> 
> This CL solve the problem. PS#1 is exactly the same as the previous CL
> which was reverted, so that it is easier for review.
> 
> TBR=creis@chromium.org
> 
> Bug:  851644 
> Change-Id: Ife9bf8f9a37892380e6587b80bcc03366cc0e80d
> Reviewed-on: https://chromium-review.googlesource.com/1151094
> Commit-Queue: Xida Chen <xidachen@chromium.org>
> Reviewed-by: Xida Chen <xidachen@chromium.org>
> Reviewed-by: Timothy Dresser <tdresser@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#579074}

TBR=creis@chromium.org,tdresser@chromium.org,xidachen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  851644 
Change-Id: I8d1c4753963f22e55f1b338bf2b264376b683660
Reviewed-on: https://chromium-review.googlesource.com/1157036
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579540}
[modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/input/touch_action_filter_unittest.cc
[modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_targeter.cc
[modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_targeter.h
[modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/site_per_process_hit_test_browsertest.cc
[delete] https://crrev.com/687b3af44494680e7f4d0bacb45d09d6ec6dc084/content/test/data/frame_tree/page_with_janky_frame.html
[delete] https://crrev.com/687b3af44494680e7f4d0bacb45d09d6ec6dc084/content/test/data/page_with_touch_start_janking_main_thread.html

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee78df3802eebbbea595e48e714a4348a06ee43e

commit ee78df3802eebbbea595e48e714a4348a06ee43e
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Aug 03 17:40:40 2018

Reland of: Handle iframe touch ack time out

The previous CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1151094
was reverted. The reason is that there were some crashes.

This CL solve the problem. PS#1 is exactly the same as the previous CL
which was reverted, so that it is easier for review.

TBR=creis@chromium.org

Bug:  851644 
Change-Id: I80a8c3b7dfd94e7abace4e3281b959f9fc1dcd2a
Reviewed-on: https://chromium-review.googlesource.com/1157698
Reviewed-by: Xida Chen <xidachen@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580589}
[modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_targeter.cc
[modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_targeter.h
[modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/site_per_process_hit_test_browsertest.cc
[add] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/test/data/frame_tree/page_with_janky_frame.html
[add] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/test/data/page_with_touch_start_janking_main_thread.html

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b945abbef17031ed77ba22808289ff39a88735d

commit 3b945abbef17031ed77ba22808289ff39a88735d
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Aug 14 23:55:51 2018

Add detailed debugging in TouchActionFilter

Previously we added some debugging instrumentation in TouchActionFilter,
when the allowed_touch_action_ is set or reset to non-Auto. The results
that we see is not enough to diagose the crash.

This CL adds more detailed debugging. In particular, whenever it is set
or reset, we add a string.

Bug:  851644 
Change-Id: I2d19055c1008659365f9c3f54fcbdb30760b2166
Reviewed-on: https://chromium-review.googlesource.com/1174696
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583087}
[modify] https://crrev.com/3b945abbef17031ed77ba22808289ff39a88735d/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/3b945abbef17031ed77ba22808289ff39a88735d/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/3b945abbef17031ed77ba22808289ff39a88735d/content/browser/renderer_host/input/touch_action_filter.h

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/097ccfa873338b4d96dcd060782b0f3aecbcb971

commit 097ccfa873338b4d96dcd060782b0f3aecbcb971
Author: Xida Chen <xidachen@chromium.org>
Date: Thu Aug 16 15:18:53 2018

Add some more debugging in TouchActionFilter

This is a follow up for
https://chromium-review.googlesource.com/c/chromium/src/+/1174696

TBR=dtapuska@chromium.org

Bug:  851644 
Change-Id: I702bd9afc80a82d0b7db02cb8f06a1765c69aaaf
Reviewed-on: https://chromium-review.googlesource.com/1177631
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583655}
[modify] https://crrev.com/097ccfa873338b4d96dcd060782b0f3aecbcb971/content/browser/renderer_host/input/touch_action_filter.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67e36d282d8a39011b62688c915b9aa79601ea24

commit 67e36d282d8a39011b62688c915b9aa79601ea24
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Aug 17 20:08:44 2018

Fix a crash when pinch is sent to root while targeting the iframe

When there is a pinch zoom gesture, it will always be sent to the root
frame even if the fingers are targeting the child frame. In this case,
the TouchActionFilter::allowed_touch_action_ of the root frame may not
have a value and results in a crash.

This CL fixes it by force the allowed_touch_action_ of the root frame
to auto in this case. A new browser test is added.

TBR=creis@chromium.org

Bug:  851644 
Change-Id: I1c9d87a1b5fecd7140fdc0ef3bfcfc4b0ea930ea
Reviewed-on: https://chromium-review.googlesource.com/1178634
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584165}
[modify] https://crrev.com/67e36d282d8a39011b62688c915b9aa79601ea24/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/67e36d282d8a39011b62688c915b9aa79601ea24/content/browser/site_per_process_hit_test_browsertest.cc
[add] https://crrev.com/67e36d282d8a39011b62688c915b9aa79601ea24/content/test/data/div_with_touch_action_auto.html

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34

commit 7a7759f7643b3c052c87a30ce1dcc99d7a85fe34
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Aug 22 16:42:31 2018

Mark gesture starts on GestureTapDown

Right now in TouchActionFilter, the member gesture_sequence_in_progress_
turns to true at GestureScrollBegin and false at GestureScrollEnd.
When this is true, we do not reset the |scrolling_touch_action_| because
being true means there are more incoming gesture events.

However, when the renderer is busy, it could happen that we have
GestureTapDown, and then we receive the OnHasTouchEventHandlers(true)
message and reset the |scrolling_touch_action_|. In this case, processing
the incoming gestures such as GSB can cause a crash because the
|scrolling_touch_action_| has no value.

Bug:  851644 
Change-Id: Idd9de11fc59ef87d7fa8f2d9e3d951fb4db5c2c5
Reviewed-on: https://chromium-review.googlesource.com/1180560
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585072}
[modify] https://crrev.com/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d326b48bd894605b4e41e22f6f085af7c12274b

commit 0d326b48bd894605b4e41e22f6f085af7c12274b
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Aug 22 20:39:26 2018

Reset touch action at TouchActionFilter's constructor

When we start processing a gesture event, the
OnHasTouchEventHandler(false) IPC message may not yet
be received which means the |scrolling_touch_action_|
has no value and that results a crash.

This CL fixes the problem by call ResetTouchAction()
in TouchActionFilter's constructor. At that moment, the
|has_touch_event_handler_| is default to false which
will set both |allowed_touch_action_| and
|scrolling_touch_action_| to Auto.

Bug:  850238 ,  851644 
Change-Id: I36f3edf2b9292347f184458aa059904af1a2f597
Reviewed-on: https://chromium-review.googlesource.com/1180592
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585234}
[modify] https://crrev.com/0d326b48bd894605b4e41e22f6f085af7c12274b/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/0d326b48bd894605b4e41e22f6f085af7c12274b/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/0d326b48bd894605b4e41e22f6f085af7c12274b/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/866afbedf0b038cffb6287a8e735277b2ef00d46

commit 866afbedf0b038cffb6287a8e735277b2ef00d46
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Aug 24 12:16:28 2018

Remove temp fix for a crash in TouchActionFilter

Previously in order to prevent a crash, we add some temporary fix in
touch_action_filter.cc. With some of the recent committed changes, there
should be no more crashing.

This CL removes the temporary code for suppressing the crash but keeps
DumpWithoutCrashing so that we can get debugging info for the crash.

Bug:  851644 
Change-Id: I614ae6d20a24b402141563451c51b36d52302d70
Reviewed-on: https://chromium-review.googlesource.com/1185627
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585803}
[modify] https://crrev.com/866afbedf0b038cffb6287a8e735277b2ef00d46/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/866afbedf0b038cffb6287a8e735277b2ef00d46/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/866afbedf0b038cffb6287a8e735277b2ef00d46/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Status: Fixed (was: Assigned)
Status: Assigned (was: Fixed)
re-open, more clean up is needed.
Project Member

Comment 18 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a648ed8345adc49563301bde7071ae0f16b128e0

commit a648ed8345adc49563301bde7071ae0f16b128e0
Author: Xida Chen <xidachen@chromium.org>
Date: Mon Aug 27 18:47:16 2018

[Debugging] Added debugging instrumentation in TouchActionFilter

Right now there's crash at TouchActionFilter when filtering a gesture
scroll update generated by fling controller. This CL adds debugging
instrumentation to record the gesture sequence.

TBR=dtapuska@chromium.org

Bug:  851644 
Change-Id: Ifb9b246eab063e1df9563501b7c31d69ae45c16d
Reviewed-on: https://chromium-review.googlesource.com/1191024
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586338}
[modify] https://crrev.com/a648ed8345adc49563301bde7071ae0f16b128e0/content/browser/renderer_host/input/touch_action_filter.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9001acb31e5c937b6b66ffaeffeaa23157d83ea

commit b9001acb31e5c937b6b66ffaeffeaa23157d83ea
Author: Xida Chen <xidachen@chromium.org>
Date: Mon Aug 27 18:57:50 2018

Mark gesture sequence in progress true for GestureTapCancel

Right now in TouchActionFilter, we set |gesture_sequence_in_progress_|
false when processing GestureTap and GestureTapCancel. This is wrong
with considering the following gesture sequence:
1. A simple tap: GestureTapDown-->GestureTapUnconfirmed-->GestureTap
2. Double tap: GestureTapDown-->GestureTapUnconfirmed-->GestureTapCancel
-->GestureTapDown-->GestureTapCancel-->GestureDoubleTap
3. A gesture scroll: GestureTapDown-->GestureTapCancel
-->GestureScrollBegin-->GestureScrollUpdate-->GestureScrollEnd
4. A fling: GestureTapDown-->GestureTapCancel-->GestureScrollBegin
-->GestureScrollUpdate-->GestureScrollEnd

All these sequence indicates that the gesture_sequence_in_progress_
should be true when filtering a GestureTapCancel and false when
GestureTap. This CL does that.

In additional to that, this CL also mark gesture_sequence_in_progress_
false when it is GestureLongTap or GestureTwoFingerTap. Unit tests
are added for both cases.

TBR=dtapuska@chromium.org

Bug:  851644 
Change-Id: I40e70cb95a93c20b13952c571d1e1108405c6051
Reviewed-on: https://chromium-review.googlesource.com/1190423
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586351}
[modify] https://crrev.com/b9001acb31e5c937b6b66ffaeffeaa23157d83ea/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/b9001acb31e5c937b6b66ffaeffeaa23157d83ea/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45630997c15b80dec5d37add5a2f685e91548327

commit 45630997c15b80dec5d37add5a2f685e91548327
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Aug 29 19:54:20 2018

Set touch action to Auto in GSB if there is no GestureTapDown

In chrome VR, we could get GestureScrollBegin without receiving a
GestureTapDown. In this case we should set touch action to Auto.

As a result of this, we need to add a TapDown in many unit tests
because they send GSB without TapDown.

TBR=dtapuska@chromium.org

Bug:  851644 
Change-Id: Ifa04259b68524e96b0a4faa2f8e1d0bd9b35c81c
Reviewed-on: https://chromium-review.googlesource.com/1195649
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587259}
[modify] https://crrev.com/45630997c15b80dec5d37add5a2f685e91548327/content/browser/renderer_host/input/input_router_impl_unittest.cc
[modify] https://crrev.com/45630997c15b80dec5d37add5a2f685e91548327/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/45630997c15b80dec5d37add5a2f685e91548327/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3811c0a0b69929b3013172732e6b2beb73cfe5c7

commit 3811c0a0b69929b3013172732e6b2beb73cfe5c7
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Aug 29 20:01:50 2018

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}
[modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/578be39046de0eb8ad2c103b3a98dda76b5651b2

commit 578be39046de0eb8ad2c103b3a98dda76b5651b2
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Aug 29 20:07:04 2018

Make InputRouterImpl::AllowedTouchAction return the actual touch action

Right now this function either returns the actual allowed touch action
when it has value, or return Auto when it has no value. It is written
this way to temporary fix a crash. Now that the crash is properly fixed,
we can return the actual allowed touch action.

TBR=dtapuska@chromium.org

Bug:  851644 
Change-Id: I6ae91f5f26c327377fbb0d0cbbfede925ac59680
Reviewed-on: https://chromium-review.googlesource.com/1189136
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587266}
[modify] https://crrev.com/578be39046de0eb8ad2c103b3a98dda76b5651b2/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/578be39046de0eb8ad2c103b3a98dda76b5651b2/content/browser/site_per_process_hit_test_browsertest.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Aug 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ee2335137207b20e2dd3878adff3b1c8aac484c

commit 0ee2335137207b20e2dd3878adff3b1c8aac484c
Author: Xida Chen <xidachen@chromium.org>
Date: Thu Aug 30 22:31:40 2018

Count number of active touches in TouchActionFilter

Right now we have a active_touch_in_progress_ to indicate whether there
is an active touch or not. It is true when we receive ACK for touch
start and false at touch end.

From the current crash report, we are seeing multiple consecutive ack
for touch end, without any ack for touch start in between.

This CL changes the active_touch_in_progress_ to num_of_active_touches_.
It is increased by one when we receive ack for touch start and decrease
by one at touch end. At touch end, we only reset touch action when this
number is zero.

In theory, touch start and associated touch end should arrive in pair.
This CL does a DumpWithoutCrashing if this number is ever larger than
one.

Bug:  851644 
Change-Id: I584f1aad56bf5bc5c08e20e3ae4ca36d5f9e3586
Reviewed-on: https://chromium-review.googlesource.com/1196696
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587813}
[modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b

commit 284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Sep 05 07:51:48 2018

Revert "Count number of active touches in TouchActionFilter"

This reverts commit 0ee2335137207b20e2dd3878adff3b1c8aac484c.

Reason for revert: Top crash in canary and blocks dev release
per bug 880021.

Original change's description:
> Count number of active touches in TouchActionFilter
> 
> Right now we have a active_touch_in_progress_ to indicate whether there
> is an active touch or not. It is true when we receive ACK for touch
> start and false at touch end.
> 
> From the current crash report, we are seeing multiple consecutive ack
> for touch end, without any ack for touch start in between.
> 
> This CL changes the active_touch_in_progress_ to num_of_active_touches_.
> It is increased by one when we receive ack for touch start and decrease
> by one at touch end. At touch end, we only reset touch action when this
> number is zero.
> 
> In theory, touch start and associated touch end should arrive in pair.
> This CL does a DumpWithoutCrashing if this number is ever larger than
> one.
> 
> Bug:  851644 
> Change-Id: I584f1aad56bf5bc5c08e20e3ae4ca36d5f9e3586
> Reviewed-on: https://chromium-review.googlesource.com/1196696
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Commit-Queue: Xida Chen <xidachen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587813}

TBR=dtapuska@chromium.org,xidachen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  851644 , 880021
Change-Id: I265b573cef100ff6210683e7f5ec01dfcb28ea9f
Reviewed-on: https://chromium-review.googlesource.com/1205923
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588791}
[modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Sep 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52121d9dedda38ffda872776a58cf0a705b4e1c5

commit 52121d9dedda38ffda872776a58cf0a705b4e1c5
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Sep 07 16:04:15 2018

Reland: Count number of active touches in TouchActionFilter

The original CL was reverted because there were many DumpWithoutCrashing,
and the reason is that the num_of_active_touches_ goes below 0.

The problem is that in InputRouterImpl::TouchEventHandled, we increment
this num only when it is ack from the main thread. In other words, we
missed other cases such as when there is a passive event listerner.

This CL fixes the problem. PS#1 is exactly the same as the original CL,
so that it is easier for review.

PS#2 fixes the problem and added some more unit tests. It also removes
the temporary fix to prevent crashes.

TBR=tdresser@chromium.org

Bug:  851644 
Change-Id: I4d668ab322c94ea2704e3e220e2afd1a44833dca
Reviewed-on: https://chromium-review.googlesource.com/1209545
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589533}
[modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/input_router_impl.h
[modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/input_router_impl_unittest.cc
[modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/touch_action_filter_unittest.cc
[modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Sep 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8eea72eee6acae9a39be6775ed7332e8b837e6a

commit e8eea72eee6acae9a39be6775ed7332e8b837e6a
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Sep 11 18:34:53 2018

[Debugging] Add InputEventAckState to the debug string for TouchAction

Right now in InputRouterImpl::OnTouchEventAck, we append a "T" to the
debug string in TouchActionFilter when the touch event is a touch
sequence start and that the input event ack state is NO_CONSUMER_EXISTS.

This CL make changes to append the input event ack state to the debug
string. The reason is that all the current crashes seems to be the case
that is not acked from the main thread, but we do not know the ack state
yet.

Bug:  851644 
Change-Id: Ia57de7aa2c63e104008893dc8809a5eac679460d
Reviewed-on: https://chromium-review.googlesource.com/1219781
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590415}
[modify] https://crrev.com/e8eea72eee6acae9a39be6775ed7332e8b837e6a/content/browser/renderer_host/input/input_router_impl.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Sep 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06614c433f82c637514f25f44f42ce461bb14661

commit 06614c433f82c637514f25f44f42ce461bb14661
Author: Xida Chen <xidachen@chromium.org>
Date: Thu Sep 13 21:17:51 2018

Fix a crash at TouchActionFilter::FilterGestureEvent

All crash reports can be put in two category:
1.  The InputEventAckState can be NON_BLOCKING at
InputRouterImpl::OnTouchEventAck, in this case, we should just set
the allowed touch action to Auto.
2. TouchActionFilter can start filtering gesture event
without an ack for a touch sequence start.

In both cases, we set the allowed touch action to Auto.

Bug:  851644 
Change-Id: I630a5d7e8804078c6f39a9102476bb484127ae61
Reviewed-on: https://chromium-review.googlesource.com/1223247
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591158}
[modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/input_router_impl_unittest.cc
[modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Sep 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55

commit cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Sep 14 20:24:04 2018

[Debugging] Append unique_touch_event_id to debug string in TouchActionFilter

In this CL, we append the unique_touch_event_id to a debug string, at
the time when we get ACK for touch sequence start and touch sequence end,
this way we'd know their correspondences.

Bug:  851644 
Change-Id: I2ee486bb2a2348a14e74a17e32f495aceaaab374
Reviewed-on: https://chromium-review.googlesource.com/1226820
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591454}
[modify] https://crrev.com/cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55/content/browser/renderer_host/input/touch_action_filter.cc

Project Member

Comment 29 by bugdroid1@chromium.org, Sep 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f05759040106ea0aa09a35074431c35b84f4cbc

commit 1f05759040106ea0aa09a35074431c35b84f4cbc
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Sep 18 22:46:12 2018

Set touch action Auto in corner cases

WidgetInputHandlerManager::DispatchEvent is the function where we run
the callback that sets the touch action. There are some corner cases
where the touch action is set to base::nullopt. This is fine previously
when the TouchActionFilter::allowed_touch_action_ has a default value
of kTouchActionAuto.

However, now that we changed the |allowed_touch_action_| to Optional
instead of having a default value (Auto), and rely on the fact that
it will be set by the renderer when ack from the main thread, we have
to set |allowed_touch_action_| to Auto for these corner cases.

This CL sets it at receiver end, which is in
InputRouterImpl::OnTouchEventAck. When receiving the ack for touch
sequence start, if the |allowed_touch_action_| is not set, then we
maybe hitting these corner cases.

Bug:  851644 
Change-Id: If1bb393580619eb7b7ba60ac2b01099c83c71f26
Reviewed-on: https://chromium-review.googlesource.com/1228295
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592235}
[modify] https://crrev.com/1f05759040106ea0aa09a35074431c35b84f4cbc/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/1f05759040106ea0aa09a35074431c35b84f4cbc/content/browser/renderer_host/input/input_router_impl_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment