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

commit c593c9f2d244392c11794edab72218ed544f1bd3
Author: Alexey Baskakov <loyso@chromium.org>
Date: Thu Oct 04 01:34:04 2018

Sync: Disable SearchProviderTest.SendsWarmUpRequestOnFocus on TSan.

The test is flaky on Linux TSan builder.

Example:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/26934

TBR=jdonnelly@chromium.org

Bug: 891959
Change-Id: Ia29f4d01145588f97a6e34b1e1fa833186238dfb
Reviewed-on: https://chromium-review.googlesource.com/c/1260524
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596472}
[modify] https://crrev.com/c593c9f2d244392c11794edab72218ed544f1bd3/chrome/browser/autocomplete/search_provider_unittest.cc

Assigning to random search_provider_unittest.cc owner. Feel free to reassign.

-> untriaged, so the omnibox triage process picks it up

FYI, it looks like the issue is with how the test sets the feature list.  Either something changed in the underlying metrics code or we're doing it wrong.

This flakiness was introduced by https://chromium-review.googlesource.com/c/chromium/src/+/1257912

The patch is unrelated to the test, but it does introduce a feature toggle that a backend thread uses. This thread is kicked off with TestingProfile.

The test in question overrides features during runtime (although not the one that causes the race), which is questionable, but then it boils down to whether ScopedFeatureList should support this scenario without TSAN warnings.

There are other workarounds like preventing the backend thread from starting, but they'd be too hacky and I would rather keep the test disabled for TSAN. 

Removing Sheriff-Chromium since this is disabled now.

mpearson, when were you thinking we might remove OmniboxWarmUpSearchProviderOnFocus? Setting it is apparently causing this test flakiness.

Also 891906 for another version of the same error.

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

commit 6111392df90a20002baa8d04ab06cf4e8fb8b0e7
Author: Mark Pearson <mpearson@chromium.org>
Date: Fri Oct 05 19:01:40 2018

Refactor SearchProviderTest.SendsWarmUpRequestOnFocus to Deflake It

and re-enable it

Call the scoped feature list earlier in test setup instead of in
the middle of a test.  Calling scoped feature list in the middle of
a test will clobber existing features, which could result in problems
as behavior changes mid-run.

(I am not ready to review this feature flag yet.  This feature has
caused problems in the past, so I'd like to keep the flag around for
longer.  It has little overhead in the code.)

Bug: 891959
Change-Id: I2cee38ff6ff2cc7de16ba1d260f831cee5a68349
Reviewed-on: https://chromium-review.googlesource.com/c/1263562
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597222}
[modify] https://crrev.com/6111392df90a20002baa8d04ab06cf4e8fb8b0e7/chrome/browser/autocomplete/search_provider_unittest.cc
[modify] https://crrev.com/6111392df90a20002baa8d04ab06cf4e8fb8b0e7/components/omnibox/browser/search_provider.h

Hopefully fixed; please reopen if it flakes again.

Still flaking in https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/27064

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

commit b584f23f110128fb50e2cddf85724fad225fa11e
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Fri Oct 05 22:07:30 2018

Sync: Disable SearchProviderTest.SendsWarmUpRequestOnFocus again on TSan.

The test is still flaky on Linux TSan builder.

Tbr: mpearson@chromium.org
Bug: 891959
Change-Id: I1233fb7a8fb60465064e5e4bd80fcc5a57e41858
Reviewed-on: https://chromium-review.googlesource.com/c/1265823
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597332}
[modify] https://crrev.com/b584f23f110128fb50e2cddf85724fad225fa11e/chrome/browser/autocomplete/search_provider_unittest.cc

jdonnelly@, upon further thought, I think I'll just leave this test disabled under ASAN.  It might not be worth the time to deflake it.  Do you have any objections?

(I'll also revert my minor refactoring.)

jdonnelly@, upon further thought, I think I'll just leave this test disabled under ASAN.  It might not be worth the time to deflake it.  Do you have any objections?

(I'll also revert my minor refactoring.)

(I don't see an obvious solution to deflake with major refactoring such as by more dramatic changes to the class hierarchy.)

I don't object to leaving it disabled under ASAN.

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

commit a181995ae3af099f20974710d90d1558d635a5eb
Author: Mark Pearson <mpearson@chromium.org>
Date: Tue Nov 13 22:40:27 2018

Revert "Refactor SearchProviderTest.SendsWarmUpRequestOnFocus to Deflake It"

This reverts commit 6111392df90a20002baa8d04ab06cf4e8fb8b0e7.

Refactoring did do something, but the issue remained.  The core problem
I think is that the parent class fields are initialized first.  This
means the parent class's fields like content::TestBrowserThreadBundle
get created, which I think will end up looking at field trial state.
Adding a base::test::ScopedFeatureList after that happens is too late;
the initial field trial state has already been inspected.

I could create a separate class that lists the base::test::ScopedFeatureList
first, and then all the other variables such as TestBrowserThreadBundle
later.  That might work.  Or not; I think there are other reasons the
TestBrowserThreadBundle wants to be listed first in the fields of a testing
class.

It's not worth the trouble to investigate.  I'm happy to have coverage
of the single MAYBE_ test on regular bot.  I don't think we also need
coverage of it on the ASAN (THREAD_SANITIZER) bots as well.

Bug: 891959
Change-Id: Iae816dde484982060121c3e667b779d70da00e65
Reviewed-on: https://chromium-review.googlesource.com/c/1334269
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607776}
[modify] https://crrev.com/a181995ae3af099f20974710d90d1558d635a5eb/chrome/browser/autocomplete/search_provider_unittest.cc
[modify] https://crrev.com/a181995ae3af099f20974710d90d1558d635a5eb/components/omnibox/browser/search_provider.h

Punting back into low-priority omnibox queue.

Removing sync tag, as this doesn't really have to do with sync.

It being flaky under TSAN still means there's a data race in non-TSAN...

ScopedFeatureList should be initialized before the threading environment (ScopedTaskEnvironment or TestBrowserThreadBundle) and outlive it.

It's not feasible to safely toggle feature flags while threads are querying the feature state.

It's a design flaw of ScopedFeatureList that this is even allowed. Ideally we would DCHECK(!base::TaskScheduler::GetInstance()) in ScopedFeatureList's constructor and destructor but issue 846380 was discovered too late and we're now stuck with a plethora of tests doing it wrong, including this one :(

+1 to make it a higher priority.

The NextAction date has arrived: 2019-01-08