New issue
Advanced search Search tips

Issue 672545 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

ReloadUrlWithAnchor test flaky on Android Site-Isolation bots

Project Member Reported by bokan@chromium.org, Dec 8 2016

Issue description

Comment 1 by creis@chromium.org, Dec 8 2016

Thanks for filing!  Example output, for posterity:

NavigationControllerBrowserTest.ReloadWithUrlAnchor (run #1):
[ RUN      ] NavigationControllerBrowserTest.ReloadWithUrlAnchor
[WARNING:dns_config_service_posix.cc(316)] Failed to read DnsConfig.
[ERROR:devtools_http_handler.cc(221)] Cannot start http server for devtools. Stop devtools.
referenceTable head length=54 1
../../content/browser/frame_host/navigation_controller_impl_browsertest.cc:1354: Failure
Value of: value
  Actual: 0
Expected: 100
[  FAILED  ] NavigationControllerBrowserTest.ReloadWithUrlAnchor, where TypeParam =  and GetParam() =  (545 ms)
[----------] 1 test from NavigationControllerBrowserTest (546 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (547 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] NavigationControllerBrowserTest.ReloadWithUrlAnchor, where TypeParam =  and GetParam() =


creis@, I tried run this locally but I did not get the failure.

```
$ adb_content_shell_command_line --enable-site-per-process
$ ninja -C out/Android content_browsertests_apk
$ out/Android/bin/run_content_browsertests --gtest-filter="NavigationControllerBrowserTest.ReloadWithUrlAnchor"
C   18.631s Main  ********************************************************************************
C   18.631s Main  Summary
C   18.631s Main  ********************************************************************************
C   18.631s Main  [==========] 1 test ran.
C   18.631s Main  [  PASSED  ] 1 test.
C   18.631s Main  ********************************************************************************

```

Comment 3 by creis@chromium.org, Dec 9 2016

The correct flag to use is --site-per-process.
creis@, I also got pass with --site-per-process

```
$ adb_content_shell_command_line --site-per-process                                                            
Wrote flags to /data/local/tmp/content-shell-command-line
  84B5T15A29018477 (angler-userdebug 6.0 MDB08Q 2462291 dev-keys): 'chrome --site-per-process'
$ out/Android/bin/run_content_browsertests --gtest-filter="NavigationControllerBrowserTest.ReloadWithUrlAnchor"
C   19.093s Main  ********************************************************************************
C   19.093s Main  Summary
C   19.093s Main  ********************************************************************************
C   19.093s Main  [==========] 1 test ran.
C   19.094s Main  [  PASSED  ] 1 test.
C   19.094s Main  ********************************************************************************

```

Comment 5 by bokan@chromium.org, Dec 9 2016

Do tests actually read the content_shell_command_line file or do flags have to be passed in via the test runner? I'd start by outputting in LOG whether that flag is on when the test runs to make sure you're actually running with it on.

Comment 6 by creis@chromium.org, Dec 9 2016

Summary: ReloadUrlWithAnchor test flaky on Android Site-Isolation bots (was: ReloadUrlWithAnchor test failing on Android Site-Isolation bots)
Comment 5: That's worth confirming.  I'd also suggest running multiple iterations, since the test is flaky on the Site Isolation Android bot:
https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Android?numbuilds=200

Failing builds: 2299, 2278, 2269, 2266, 2263, 2261, etc.

(Some of the other runs seem affected by  issue 672382 , where tons of tests are failing.  For example: 2297, 2296.)
I wanted to share my attempt to repro the intermittent failures from the bot:

$ git log --oneline | head -1  # Just trying to show at what revision I am building/testing.
465a1a9 Remove some keyboard cruft

$ . build/android/envsetup.sh

$ cat out/andr/args.gn
target_os = "android"
target_cpu = "x86"
is_debug = false
is_component_build = true
symbol_level = 1
use_goma = true

$ ninja -C out/andr -j 1500 -l 15 content_browsertests
...

$ build/android/avd.py run --sdcard-size 4096M --partition-size 4096M --api-level 23
...

$ out/andr/bin/run_content_browsertests --test-arguments=--site-per-process --gtest-filter=*NavigationControllerBrowserTest.ReloadWithUrlAnchor* --gtest-repeat=99


The test surprisingly passed 100 times.  I am not sure what hypothesis to form regarding the bot failures:

1. Maybe the test only fails intermittently when run on an ARM architecture (the emulator is x86)?
2. Maybe the bot has some other characteristics (slow?) that make the repro more likely?
3. Maybe state from other, previously run tests leaks and is needed for the repro (this seems unlikely given what I know about how browser tests run).
...


Please note that I double-checked that --site-per-process flag is taking effect by
1. Adding CHECK(!SiteIsolationPolicy::UseDedicatedProcessesForAllSites())
   to the NavigationControllerBrowserTest.ReloadWithUrlAnchor test
2. Verifying that the run_content_browsertests commandline above does pass --site-per-process
   (verified by observing a crash caused by the CHECK statement from above).
3. Verifying no crash without --test-arguments=--site-per-process
4. Removing the CHECK
I can easily reproduce this issue now. We can use same gn args with buildbot:

```
ffmpeg_branding = "Chrome"
is_component_build = false
is_debug = false
proprietary_codecs = true
symbol_level = 1
target_cpu = "arm64"
target_os = "android"
use_goma = true
```

Then build the content shell apk. Open http://chaopeng.me/htmltest/reload-with-url-anchor.html#d2, then reload repeatedly. It goes to incorrect position sometime. Also I can not reproduce this issue on chrome apk. 
Cc: lukasza@chromium.org alex...@chromium.org chaopeng@chromium.org
 Issue 677301  has been merged into this issue.
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 28 2016

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

commit d5c112c459321ac69ddf41d11ef3644ebfd275d0
Author: alexmos <alexmos@chromium.org>
Date: Wed Dec 28 19:42:01 2016

Disable flaky test: NavigationControllerBrowserTest.ReloadWithUrlAnchor

BUG= 672545 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2606963002
Cr-Commit-Position: refs/heads/master@{#440880}

[modify] https://crrev.com/d5c112c459321ac69ddf41d11ef3644ebfd275d0/content/browser/frame_host/navigation_controller_impl_browsertest.cc

Comment 12 Deleted

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| calls before |FrameLoader::processFragment|. And |FrameLoader::restoreScrollPositionAndViewState| sets |didRestoreFromHistory = true| to forbid |FrameLoader::processFragment|.

We call |FrameLoader::checkCompleted| in |FrameLoader::finishedParsing| when reload that should early return by checking |!shouldComplete(m_frame->document()) == true|.

But |Document::isDelayingLoadEvent| return false sometimes(need more investigation) that makes |shouldComplete(m_frame->document()) == true| then calls |FrameLoader::restoreScrollPositionAndViewState|. That makes |FrameLoader::restoreScrollPositionAndViewState| called before |FrameLoader::processFragment|.
Status: Started (was: Assigned)
Project Member

Comment 15 by bugdroid1@chromium.org, Feb 16 2017

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

commit 1033be6d096d430b3d1fe6439550448b2374ea4c
Author: chaopeng <chaopeng@chromium.org>
Date: Thu Feb 16 22:08:58 2017

Add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState restore
from default

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment| in |checkCompleted| and restore from
default.

So we add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState
restore from default.

BUG= 672545 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2628683003
Cr-Commit-Position: refs/heads/master@{#451111}

[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/content/common/page_state_serialization.cc
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/content/common/page_state_serialization.h
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/content/common/page_state_serialization_unittest.cc
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/content/renderer/history_serialization.cc
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/content/test/data/navigation_controller/reload-with-url-anchor.html
[add] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/content/test/data/page_state/serialized_v23.dat
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/third_party/WebKit/Source/core/loader/HistoryItem.cpp
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/third_party/WebKit/Source/core/loader/HistoryItem.h
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/third_party/WebKit/Source/web/WebHistoryItem.cpp
[modify] https://crrev.com/1033be6d096d430b3d1fe6439550448b2374ea4c/third_party/WebKit/public/web/WebHistoryItem.h

Status: Fixed (was: Started)

Sign in to add a comment