New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 773848 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Fix WebViewTest.testPauseResumeTimers

Project Member Reported by yolandyan@chromium.org, Oct 11 2017

Issue description

WebViewTest.testPauseResumeTimers is broken after this CL has landed (https://chromium-review.googlesource.com/c/chromium/src/+/702356)

Since pauseTimers API calls into setWebKitSharedTimersSuspended on native side.

First failures: https://build.chromium.org/p/chromium.android/builders/Android%20WebView%20L%20%28dbg%29/builds/9446


testPauseResumeTimers source code:
https://android.googlesource.com/platform/cts/+/0a83759/tests/tests/webkit/src/android/webkit/cts/WebViewTest.java#1769

https://cs.chromium.org/search/?q=setWebKitSharedTimersSuspended&sq=package:chromium&type=cs

The revert failed bacause the revert CL lead to compiling issue due to other changes between your CL and the revert.

Assigning it to the CL owner

`android_webview/tools/run_cts.py --arch arm_64 --platform L -f android.webkit.cts.WebViewTest#testPauseResumeTimers -v`
is the command to test CTS locally
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 11 2017

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

commit b8c2a13cd103cd09d539d754b3d15ce8a7d94d9f
Author: Yoland Yan <yolandyan@chromium.org>
Date: Wed Oct 11 22:15:47 2017

Disable testPauseResumeTimers

TBR: changwan@chromium.org,boliu@chromium.org
Bug:  773848 
Change-Id: Icb9d95bcc4257d795a4980e6a63dcdc8499393bc
Reviewed-on: https://chromium-review.googlesource.com/713940
Reviewed-by: Yoland Yan <yolandyan@chromium.org>
Commit-Queue: Yoland Yan <yolandyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508144}
[modify] https://crrev.com/b8c2a13cd103cd09d539d754b3d15ce8a7d94d9f/android_webview/tools/cts_config/expected_failure_on_bot.json

Comment 2 by boliu@chromium.org, Oct 11 2017

Components: Mobile>WebView
Labels: -Pri-2 ReleaseBlock-Stable M-63 OS-Android Pri-1
CTS failure is release-blocking 
Status: Assigned (was: Untriaged)
Comment in CL:

Replace all usages of RendererScheduler::PauseTaskQueue with
RendererScheduler::PauseRenderer, given that all callsites expect all
javascript callbacks to be paused.

Appears to be at odds with CTS comment:

        // Test that JavaScript is executed even with timers paused.

Alex: could you please take a look.

Comment 4 by boliu@chromium.org, Oct 12 2017

Well, the cts test does not match documentation on webview.pausetimers. doc says page loading stops as well, and the test clearly expects it to still work...
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 17 2017

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

commit 4b61522379090f524b8c56d7ce4e9481f42beebd
Author: Alexander Timin <altimin@chromium.org>
Date: Tue Oct 17 20:34:06 2017

[scheduler] Reintroduce concept of pausing timers for Android WebView.

Android WebView has an API to pause timers and we have to support it
for the time being because changing WebView API and CTS tests is hard.

This patch relands parts of
https://chromium-review.googlesource.com/c/chromium/src/+/702356.

R=alexclarke@chromium.org,skyostil@chromium.org,jochen@chromium.org,yolandyan@chromium.org
BUG= 773848 

Change-Id: I728961bc4852ccefc9d5cc0ce45c872cd6dd840e
Reviewed-on: https://chromium-review.googlesource.com/718939
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Yoland Yan <yolandyan@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509504}
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/android_webview/tools/cts_config/expected_failure_on_bot.json
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/content/renderer/render_thread_impl.h
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/Source/platform/scheduler/test/fake_renderer_scheduler.cc
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/public/platform/scheduler/DEPS
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/public/platform/scheduler/test/fake_renderer_scheduler.h
[modify] https://crrev.com/4b61522379090f524b8c56d7ce4e9481f42beebd/third_party/WebKit/public/platform/scheduler/test/mock_renderer_scheduler.h

Status: Fixed (was: Assigned)
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-63; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-63 label, otherwise remove Merge-TBD label. Thanks.
Labels: -Merge-TBD Merge-Request-63
As per c#2, CTS failure is release blocking.
Project Member

Comment 9 by sheriffbot@chromium.org, Oct 19 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Review -Merge-Review-63 Merge-Approved-63
Thanks for fixing this. 
Status: Started (was: Fixed)
This still needs to be merged to M63 branch 3239.  Please merge asap.
Project Member

Comment 12 by sheriffbot@chromium.org, Oct 27 2017

Cc: cma...@chromium.org candr...@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 13 by sheriffbot@chromium.org, Oct 30 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
altimin@,

Please merge the change as asap. Merge has been approved on Oct 23rd.

Thanks!
altimin@ are you merging this?
Merge the change, please!
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 8 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efdf10bd51757ab69abb00c484b066e374657aad

commit efdf10bd51757ab69abb00c484b066e374657aad
Author: Alexander Timin <altimin@chromium.org>
Date: Wed Nov 08 22:26:33 2017

[scheduler] Reintroduce concept of pausing timers for Android WebView.

Android WebView has an API to pause timers and we have to support it
for the time being because changing WebView API and CTS tests is hard.

This patch relands parts of
https://chromium-review.googlesource.com/c/chromium/src/+/702356.

R=alexclarke@chromium.org, jochen@chromium.org, skyostil@chromium.org, yolandyan@chromium.org
TBR=altimin@chromium.org
BUG= 773848 

(cherry picked from commit 4b61522379090f524b8c56d7ce4e9481f42beebd)

Change-Id: I728961bc4852ccefc9d5cc0ce45c872cd6dd840e
Reviewed-on: https://chromium-review.googlesource.com/718939
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Yoland Yan <yolandyan@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#509504}
Reviewed-on: https://chromium-review.googlesource.com/758403
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#423}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/android_webview/tools/cts_config/expected_failure_on_bot.json
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/content/renderer/render_thread_impl.h
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/Source/platform/scheduler/test/fake_renderer_scheduler.cc
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/public/platform/scheduler/DEPS
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/public/platform/scheduler/test/fake_renderer_scheduler.h
[modify] https://crrev.com/efdf10bd51757ab69abb00c484b066e374657aad/third_party/WebKit/public/platform/scheduler/test/mock_renderer_scheduler.h

Status: Fixed (was: Started)
Closing since fix has been merged to M63.

Sign in to add a comment