New issue
Advanced search Search tips

Issue 875884 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 2
Type: Bug

Blocked on:
issue 877132



Sign in to add a comment

lifecycle/background-change-lifecycle-count.html is flaky on windows and linux

Project Member Reported by pdr@chromium.org, Aug 20

Issue description

This test is flaky on windows and linux:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_layout_tests&tests=lifecycle%2Fbackground-change-lifecycle-count.html

On linux, we're getting a synthetic mouse move event that's causing an additional lifecycle update.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 20

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

commit 0547b7af6432520151cf7190a9c6769e1cbf8513
Author: Philip Rogers <pdr@chromium.org>
Date: Mon Aug 20 17:00:19 2018

Mark background-change-lifecycle-count.html as flaky

TBR=chrishtr@chromium.org

Bug: 875884
Change-Id: I09e605403a5b068d9b2db0510289538a4f22b731
Reviewed-on: https://chromium-review.googlesource.com/1181507
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584477}
[modify] https://crrev.com/0547b7af6432520151cf7190a9c6769e1cbf8513/third_party/WebKit/LayoutTests/TestExpectations

Blockedon: 877132
This just ruined a cq run for me: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/win7_chromium_rel_ng/103711 Do virtual test suites need their own flaky entry?
I thought virtual inherited the non-virtual expectations but that's not the case (I just confirmed locally)! Sorry about that; I'll suppress this failure.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 10

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

commit 12c34c9a009317509c039ad3a5ad4cb88c78bd42
Author: Philip Rogers <pdr@chromium.org>
Date: Wed Oct 10 14:28:17 2018

Suppress virtual/threaded/.../background-change-lifecycle-count.html

virtual/threaded/lifecycle/background-change-lifecycle-count.html fails
on linux and windows (see: https://crbug.com/875884) and we had a
non-virtual expectation for this but were missing the virtual one. This
patch updates the virtual expectation.

TBR=thakis@chromium.org
NOTRY=true

Bug: 875884
Change-Id: I20e708e67e347aeee7603408f1d9778904d2268f
Reviewed-on: https://chromium-review.googlesource.com/c/1273072
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598312}
[modify] https://crrev.com/12c34c9a009317509c039ad3a5ad4cb88c78bd42/third_party/WebKit/LayoutTests/TestExpectations

Cc: nedngu...@google.com dcheng@chromium.org
I can repro this issue on a local windows device. It doesn't matter what count I set --gtest_repeat to, at least locally. The test fails with ~50% probability on the first run, and never fails on subsequent runs. This suggests a race condition during first run setup.

Looking at flakiness that happens on the CQ, we see a slightly different behavior. The test either always fails [including on repeats], or never fails [including on repeats]. 

This implies that there is some state that is leaking between tests.
If I modify the test runner to reset content shell between tests, then the tests will occasionally fail on repeat [using --gtest_repeat=10 or --gtest_repeat=30]. The reason I didn't land the CL to always reset content shell: https://chromium-review.googlesource.com/c/chromium/src/+/1259722 is because it roughly doubles the amount of time it takes to run the tests. 

However, if we're only retrying a small number of tests, then we should reset content shell for better consistency.
Erik, thats great! State leaking between tests is exactly the kind of bug this test is trying to catch. I bet we have a spurious timer or task that's active between tests. If you put a stacktrace in LocalFrameView::UpdateLifecyclePhases, it should reveal all callsites.

https://bugs.chromium.org/p/chromium/issues/detail?id=877132 is fixing the only flakiness we're aware of so far (the fake mouse event hover timer).
This flakiness is also caused by the fake mouse event hover timer. This timeout of 20ms: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/input/mouse_event_manager.cc?type=cs&sq=package:chromium&g=0&l=71 

causes the timer to fire at non-deterministic times during the test, causing flaky failures. I don't know why this is affected by whether we reset content shell between tests. I suspect that for a new content shell, there's a lot more tasks queued at the beginning of the test, and this causes the scheduler to have different behavior. 
Thanks for taking the time to find that timer Erik. This turns out to be the same issue they are working on in https://crbug.com/877132. we're removing the timer using a feature flag so it still exists right now but should disappear soon.
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 16

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

commit 5524af6018c603cd943c8f76ade1e407a5459ad7
Author: erikchen <erikchen@chromium.org>
Date: Tue Oct 16 19:21:27 2018

Add functionality to restart content shell between test runs.

Many layout tests will flakily fail when run in a clean content shell, but will
deterministically fail or pass when run in a reused content shell.

Unfortunately, we cannot restart content shell between all tests because that
significantly increases run time. [2X on Windows, 3X on Linux, 5X on macOS].

This CL adds the functionality to restart content shell between all test runs,
but then only enables it for tests with --gtest_repeat or --repeat-each with a
repeat count > 1.

Effects:
* For a local developer, this will only have an effect when using --gtest_repeat
  or --repeat-each. In this case, the developer is already trying to tease out
  some type of non-determinism between tests, and it's important to use a clean
  content shell. With this CL, flaky tests can be easily reproduced with
  --gtest_repeat, whereas they could not before. See
  https://bugs.chromium.org/p/chromium/issues/detail?id=894527#c5 for examples.
* On the CQ, this will only have an effect on 'retry without patch' and 'retry
  with patch', which retry failing tests looking for flakiness. This will help
  the CQ better detect tests that are flaky on ToT to reduce false rejects.

Change-Id: I4c04e382e733f8e1b40f4a6dde78292a41126a1b
Bug: 875884,   889036 
Reviewed-on: https://chromium-review.googlesource.com/c/1273810
Reviewed-by: Aleks Totic <atotic@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600077}
[modify] https://crrev.com/5524af6018c603cd943c8f76ade1e407a5459ad7/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/5524af6018c603cd943c8f76ade1e407a5459ad7/content/shell/common/layout_test/layout_test_switches.cc
[modify] https://crrev.com/5524af6018c603cd943c8f76ade1e407a5459ad7/content/shell/common/layout_test/layout_test_switches.h
[modify] https://crrev.com/5524af6018c603cd943c8f76ade1e407a5459ad7/third_party/blink/tools/blinkpy/web_tests/port/base.py
[modify] https://crrev.com/5524af6018c603cd943c8f76ade1e407a5459ad7/third_party/blink/tools/blinkpy/web_tests/run_webkit_tests.py

Sign in to add a comment