New issue
Advanced search Search tips

Issue 845150 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

smoothness.pathological_mobile_sites/http://www.wowwiki.com/World_of_Warcraft:_Mists_of_Pandaria and 1 other(s) in smoothness.pathological_mobile_sites failing on chromium.perf/Android One Perf

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, May 21 2018

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of eyaich@google.com

smoothness.pathological_mobile_sites/http://www.wowwiki.com/World_of_Warcraft:_Mists_of_Pandaria and 1 other(s) in smoothness.pathological_mobile_sites failing on chromium.perf/Android One Perf

Builders failed on: 
- Android One Perf: 
  https://ci.chromium.org/buildbot/chromium.perf/Android%20One%20Perf


 

Comment 2 by eyaich@chromium.org, May 21 2018

Cc: eyaich@chromium.org
Components: Speed>Benchmarks>Waterfall
Owner: picksi@chromium.org
Looks like it clearly started failing around build 1555: 
https://ci.chromium.org/buildbot/chromium.perf/Android%20One%20Perf/1555

Link to failing log: 
https://chrome-swarming.appspot.com/task?id=3d8f83890cb0b010&refresh=10&show_raw=1

I kicked off a bisect
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, May 23 2018

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/14c39f3c240000

'JobState' object has no attribute '_comparison_mode'
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, May 28 2018

Cc: xidac...@chromium.org
Owner: xidac...@chromium.org
Status: Assigned (was: Available)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14a12e62240000

Make TouchActionFilter::allowed_touch_action_ optional by xidachen@chromium.org
https://chromium.googlesource.com/chromium/src/+/ffd43addc29a9c7433657cba3df93f8a87887ac7

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
The CL was reverted, but I will keep this open so that I can keep track of all bugs when re-landing the CL
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 7 2018

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

commit 2eec314f52989e78e862da1675b0d095e9629197
Author: Xida Chen <xidachen@chromium.org>
Date: Thu Jun 07 11:42:42 2018

Reland of: Make TouchActionFilter::allowed_touch_action_ optional

The original CL was reverted because of multiple failures, this CL fixes
them one by one and lists them here. The first patch set is exactly the
same as the one that was reverted, so that it is easier to review.

1.  crbug.com/845012 
The original CL caused 3 tests to fail on multiple bots because of
a illegal downcast from MockInputRouter to InputRouter. PS#3 should
have that fixed.

2. crbug.com/845063, crbug.com/844858,  crbug.com/845150 
The problem is due to calling ResetTouchAction at GSE in cases that
it should not.
In a typical fling sequence, we have GSB, then a bunch of GSU, then
a FlingStart, then another bunch of GSU, and then a GSE. If we have a
in-flight fling and we start a second fling sequence before GSE of
the first fling, then OnSetTouchAction is called for the second
gesture sequence and that sets the |allowed_touch_action_|. After
that, GSE of the first fling comes which resets the |allowed_touch_action_|.
Then when the GSB of the second fling comes it doesn't have a valid
touch action to start with which results in the crash.

This is now fixed by using another member |scrolling_touch_action_|,
which takes over the value in the |allowed_touch_action_| when there
is a touch sequence end. So we reset the |allowed_touch_action_| at
each touch sequence end, and if that is not the gesture sequence end,
we will keep using the |scrolling_touch_action_| until the next TapDown.

3.  crbug.com/845153 
Tested this locally. The problem is JS injecting a touch event handler
in the middle of gesture sequence. In my original CL, when we saw a
touch event handler, we call ResetTouchAction(). If that happens in the
middle of a gesture sequence, then allowed_touch_action_ has no value
when subsequence gesture events comes which results in the crash.

This is also solved by using the |scrolling_touch_action_| which will
not be reset when JS inject a touch event handler.

TBR=pdr@chromium.org, piman@chromium.org

Bug:  845012 , 845063, 844858,  845153 ,  845150 
Change-Id: I4f1e1deff71db4587fb42c38f87ccb1cb7311cdc
Reviewed-on: https://chromium-review.googlesource.com/1068204
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@{#565233}
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/frame_host/cross_process_frame_connector.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/compositor_event_ack_browsertest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/input_router.h
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/input_router_impl.h
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/input_router_impl_unittest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/touch_action_filter_unittest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/content/public/test/browser_test_utils.h
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/third_party/blink/public/platform/web_gesture_event.h
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/third_party/blink/renderer/core/exported/web_page_popup_impl.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/third_party/blink/renderer/core/input/touch_action_test.cc
[modify] https://crrev.com/2eec314f52989e78e862da1675b0d095e9629197/third_party/blink/renderer/core/input/touch_event_manager.cc

Status: Fixed (was: Assigned)

Sign in to add a comment