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

Issue 796236 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

UkmBrowserTest.HistoryDeleteCheck test is flaky on Windows

Project Member Reported by r...@chromium.org, Dec 19 2017

Issue description

This is the #1 failure on the MSVC bots. Ignore the name CrWinClang, it's using MSVC, and the bot will be renamed after the clang switch sticks.
https://ci.chromium.org/buildbot/chromium.clang/CrWinClang/

The flakiness dashboard shows that this is mostly specific to the MSVC release bots, which are CrWinClang and CrWinClang64:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=UkmBrowserTest.HistoryDeleteCheck

The test was recently added in https://chromium-review.googlesource.com/c/chromium/src/+/818202


 

Comment 1 by r...@chromium.org, Dec 19 2017

Summary: UkmBrowserTest.HistoryDeleteCheck test is flaky on Windows (was: UkmBrowserTest.HistoryDeleteCheck test is flaky on Windows, maybe MSVC-only)
I guess the failures are not MSVC-specific, I see another instance on ToTWin, which uses clang:
https://ci.chromium.org/buildbot/chromium.clang/ToTWin/648

Here are the test logs, which are more or less the same. The has_source_data() check is what's failing.

[ RUN      ] UkmBrowserTest.HistoryDeleteCheck
[5368:6300:1212/121726.186:29172545:WARNING:loopback_server.cc(638)] Loopback sync persistent state file does not exist.
...
[5368:5596:1212/121726.470:WARNING:syncer_proto_util.cc(338)] Error posting from syncer: Response Code (bogus on error): -1 Content-Length (bogus on error): -1 Server Status: SYNC_AUTH_ERROR
[5368:5596:1212/121726.472:WARNING:sync_encryption_handler_impl.cc(970)] Nigori had empty encryption keybag.
[5368:5596:1212/121726.473:WARNING:sync_encryption_handler_impl.cc(970)] Nigori had empty encryption keybag.
[5368:5596:1212/121726.487:WARNING:sync_encryption_handler_impl.cc(346)] Ignoring new implicit passphrase. Keystore migration already performed.
../../chrome/browser/metrics/ukm_browsertest.cc(323): error: Value of: has_source_data()
  Actual: true
Expected: false
[5368:6300:1212/121727.245:ERROR:shared_change_processor.cc(210)] Sessions datatype error was encountered: Change processor disconnected.
[5368:6300:1212/121727.278:INFO:sync_scheduler_impl.cc(133)] Scheduling next sync for CryptAuth Enrollment:
    Strategy: Aggressive Recovery
    Time Delta: 0 seconds
    Previous Failures: 1
[  FAILED  ] UkmBrowserTest.HistoryDeleteCheck, where TypeParam =  and GetParam() =  (1118 ms)



Comment 2 by r...@chromium.org, Dec 21 2017

Cc: -siggi@chromium.org
Components: Internals>Metrics
I prepared a revert: https://chromium-review.googlesource.com/c/chromium/src/+/840862
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 21 2017

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

commit fdf4d5eb8e20a0a66bd659400bd2729f3ca2072c
Author: Reid Kleckner <rnk@google.com>
Date: Thu Dec 21 20:43:07 2017

Revert "Add test coverage of UKM History delete handling."

This reverts commit 2a7fbdc52318f6169ae27ef251c52d91b5480aa9.

Reason for revert: This new test is flaky on Windows.

Original change's description:
> Add test coverage of UKM History delete handling.
> 
> Coverage on iOS left for a follow up.
> 
> Bug:  793470 
> Change-Id: I4844e667c421f7df3c3412362d50ad74078e8223
> Reviewed-on: https://chromium-review.googlesource.com/818202
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Commit-Queue: Steven Holte <holte@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#523256}

TBR=asvitkine@chromium.org,bcwhite@chromium.org,holte@chromium.org

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

Bug:  793470 , 796236 
Change-Id: I43356b1e4f5bea9b71b206ffe7ef1496d9d41b7d
Reviewed-on: https://chromium-review.googlesource.com/840862
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Reviewed-by: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525795}
[modify] https://crrev.com/fdf4d5eb8e20a0a66bd659400bd2729f3ca2072c/chrome/browser/metrics/ukm_browsertest.cc

Comment 4 by holte@chromium.org, Dec 21 2017

Looking through the code a bit, I suspect that this might be due to the call to NotifyDeleteURLs not being run from message loop yet:

https://cs.chromium.org/chromium/src/components/history/core/browser/history_service.cc?rcl=984f604436155b09389c7b2e396b4513dfc09a88&l=161 

I think BrowsingDataRemoverCompletionObserver::BlockUntilCompletion might only block until the task that posts that task gets run?

https://chromium-review.googlesource.com/c/chromium/src/+/840734
adds an extra content::RunAllPendingInMessageLoop(), not sure if there is a cleaner way to make sure that it flushed.

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 21 2017

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

commit 8ed93d0f69704598d0eae6ed6d2a1c85d2ed6a52
Author: Steven Holte <holte@google.com>
Date: Thu Dec 21 21:50:47 2017

Reland UKM history delete test.

This adds a content::RunAllPendingInMessageLoop call to avoid flakiness.

Bug:  796236 
Change-Id: I45f8748315b3509b917a1dd552deabca753b0321
Reviewed-on: https://chromium-review.googlesource.com/840734
Reviewed-by: Reid Kleckner <rnk@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525819}
[modify] https://crrev.com/8ed93d0f69704598d0eae6ed6d2a1c85d2ed6a52/chrome/browser/metrics/ukm_browsertest.cc

Comment 6 by r...@chromium.org, Dec 27 2017

Looks like it's still flaking:
https://ci.chromium.org/buildbot/chromium.clang/CrWinClang/1309

Comment 8 by holte@chromium.org, Jan 8 2018

Status: Fixed (was: Unconfirmed)
I don't see new instances of this after:

https://chromium-review.googlesource.com/852831

Sign in to add a comment