[omnibox] Fix TSAN issues with field tests in SearchProviderTest

Command line flags, field trials, and feature flags must be initialized
before BrowserTaskEnvironment to avoid a potential race condition, where
the browser is reading from the trial state at the same time that it is
being written to.

This isn't done in SearchProviderTest, which causes potential races in
two tests:
    SearchProviderTest.CommandLineOverrides
    SearchProviderTest.SendsWarmUpRequestOnFocus

To resolve, a new class SearchProviderFeatureTestComponent was created
to handle field trial state. It's initialized before the
BrowserTaskEnvironment member.

BrowserTaskEnvironment is responsible for spinning up the thread pool,
so everything before that point is synchronous. That's adding a thread
which is reading the field trial state, causing the race. C++
initializes it's member variables in order of declaration, so adding the
FeatureTestComponent before BrowserTaskEnvironment ensures that the
command line and field tests are set before any other threads are added.

This change is heavily based on mpearson's prior fix, which was
reverted. http://crrev.com/c/1263562

Bug: 891959
Change-Id: I5590579ad9aef73d2d055dec5712dfd5dc567ce9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1902786
Commit-Queue: Justin Gallagher <jugallag@microsoft.com>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714824}

chrome/browser/autocomplete/search_provider_unittest.cc

@@ -133,6 +133,54 @@ class TestAutocompleteProviderClient : public ChromeAutocompleteProviderClient {
 
 }  // namespace
 
+// SearchProviderFeatureTestComponent -----------------------------------------
+// Handles field trial, feature flag, and command line state for SearchProvider
+// tests. This is done as a base class, so that it runs before
+// BrowserTaskEnvironment is initialized.
+
+class SearchProviderFeatureTestComponent {
+ public:
+  SearchProviderFeatureTestComponent(
+      const base::Optional<bool> warm_up_on_focus,
+      const bool command_line_overrides);
+
+ private:
+  void ResetFieldTrialList();
+
+  std::unique_ptr<base::FieldTrialList> field_trial_list_;
+  base::test::ScopedFeatureList feature_list_;
+};
+
+SearchProviderFeatureTestComponent::SearchProviderFeatureTestComponent(
+    const base::Optional<bool> warm_up_on_focus,
+    const bool command_line_overrides) {
+  ResetFieldTrialList();
+
+  if (warm_up_on_focus.has_value()) {
+    if (warm_up_on_focus.value())
+      feature_list_.InitAndEnableFeature(omnibox::kSearchProviderWarmUpOnFocus);
+    else
+      feature_list_.InitAndDisableFeature(
+          omnibox::kSearchProviderWarmUpOnFocus);
+  }
+
+  if (command_line_overrides) {
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kGoogleBaseURL, "http://www.bar.com/");
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kExtraSearchQueryParams, "a=b");
+  }
+}
+
+void SearchProviderFeatureTestComponent::ResetFieldTrialList() {
+  // Destroy the existing FieldTrialList before creating a new one to avoid
+  // a DCHECK.
+  field_trial_list_.reset();
+  field_trial_list_.reset(new base::FieldTrialList(
+      std::make_unique<variations::SHA1EntropyProvider>("foo")));
+  variations::testing::ClearAllVariationParams();
+}
+
 // BaseSearchProviderTest -----------------------------------------------------
 
 // Base class that configures following environment:
@@ -181,23 +229,21 @@ class BaseSearchProviderTest : public testing::Test,
     bool allowed_to_be_default_match;
   };
 
-  BaseSearchProviderTest()
+  BaseSearchProviderTest(
+      const base::Optional<bool> warm_up_on_focus = base::nullopt,
+      const bool command_line_overrides = false)
       : default_t_url_(nullptr),
         term1_(ASCIIToUTF16("term1")),
         keyword_t_url_(nullptr),
         keyword_term_(ASCIIToUTF16("keyword")),
-        run_loop_(nullptr) {
-    ResetFieldTrialList();
-  }
+        feature_test_component_(warm_up_on_focus, command_line_overrides),
+        run_loop_(nullptr) {}
 
   void TearDown() override;
 
   void RunTest(TestData* cases, int num_cases, bool prefer_keyword);
 
  protected:
-  // Needed for AutocompleteFieldTrial::ActivateStaticTrials();
-  std::unique_ptr<base::FieldTrialList> field_trial_list_;
-
   // Default values used for testing.
   static const char kNotApplicable[];
   static const ExpectedMatch kEmptyExpectedMatch;
@@ -263,8 +309,6 @@ class BaseSearchProviderTest : public testing::Test,
                     const ExpectedMatch expected_matches[],
                     const ACMatches& matches);
 
-  void ResetFieldTrialList();
-
   // Enable or disable the specified Omnibox field trial rule.
   base::FieldTrial* CreateFieldTrial(const char* field_trial_rule,
                                      bool enabled);
@@ -279,6 +323,9 @@ class BaseSearchProviderTest : public testing::Test,
   const base::string16 keyword_term_;
   GURL keyword_url_;
 
+  // SearchProviderFeatureTestComponent must come before BrowserTaskEnvironment,
+  // to avoid a possible race.
+  SearchProviderFeatureTestComponent feature_test_component_;
   content::BrowserTaskEnvironment task_environment_;
 
   network::TestURLLoaderFactory test_url_loader_factory_;
@@ -297,6 +344,11 @@ class BaseSearchProviderTest : public testing::Test,
 // Test environment with valid suggest and search URL.
 class SearchProviderTest : public BaseSearchProviderTest {
  public:
+  SearchProviderTest(
+      const base::Optional<bool> warm_up_on_focus = base::nullopt,
+      const bool command_line_overrides = false)
+      : BaseSearchProviderTest(warm_up_on_focus, command_line_overrides) {}
+
   void SetUp() override {
     CustomizableSetUp(
         /* search_url */ "http://defaultturl/{searchTerms}",
@@ -579,15 +631,6 @@ void BaseSearchProviderTest::CheckMatches(
   }
 }
 
-void BaseSearchProviderTest::ResetFieldTrialList() {
-  // Destroy the existing FieldTrialList before creating a new one to avoid
-  // a DCHECK.
-  field_trial_list_.reset();
-  field_trial_list_.reset(new base::FieldTrialList(
-      std::make_unique<variations::SHA1EntropyProvider>("foo")));
-  variations::testing::ClearAllVariationParams();
-}
-
 base::FieldTrial* BaseSearchProviderTest::CreateFieldTrial(
     const char* field_trial_rule,
     bool enabled) {
@@ -1206,39 +1249,6 @@ TEST_F(SearchProviderTest, KeywordVerbatim) {
   RunTest(cases, base::size(cases), true);
 }
 
-// Ensures command-line flags are reflected in the URLs the search provider
-// generates.
-TEST_F(SearchProviderTest, CommandLineOverrides) {
-  TemplateURLService* turl_model =
-      TemplateURLServiceFactory::GetForProfile(&profile_);
-
-  TemplateURLData data;
-  data.SetShortName(ASCIIToUTF16("default"));
-  data.SetKeyword(data.short_name());
-  data.SetURL("{google:baseURL}{searchTerms}");
-  default_t_url_ = turl_model->Add(std::make_unique<TemplateURL>(data));
-  turl_model->SetUserSelectedDefaultSearchProvider(default_t_url_);
-
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      switches::kGoogleBaseURL, "http://www.bar.com/");
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      switches::kExtraSearchQueryParams, "a=b");
-
-  TestData cases[] = {
-    { ASCIIToUTF16("k a"), 2,
-      { ResultInfo(GURL("http://keyword/a"),
-                   AutocompleteMatchType::SEARCH_OTHER_ENGINE,
-                   true,
-                   ASCIIToUTF16("k a")),
-        ResultInfo(GURL("http://www.bar.com/k%20a?a=b"),
-                   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
-                   false,
-                   ASCIIToUTF16("k a")) } },
-  };
-
-  RunTest(cases, base::size(cases), false);
-}
-
 // Verifies Navsuggest results don't set a TemplateURL, which Instant relies on.
 // Also verifies that just the *first* navigational result is listed as a match
 // if suggested relevance scores were not sent.
@@ -3762,37 +3772,44 @@ TEST_F(SearchProviderTest, DoesNotProvideOnFocus) {
   EXPECT_TRUE(provider_->matches().empty());
 }
 
-#if defined(THREAD_SANITIZER)
-// SearchProviderTest.SendsWarmUpRequestOnFocus is flaky on Linux TSan Tests
-// crbug.com/891959.
-#define MAYBE_SendsWarmUpRequestOnFocus DISABLED_SendsWarmUpRequestOnFocus
-#else
-#define MAYBE_SendsWarmUpRequestOnFocus SendsWarmUpRequestOnFocus
-#endif  // defined(THREAD_SANITIZER)
-TEST_F(SearchProviderTest, MAYBE_SendsWarmUpRequestOnFocus) {
+TEST_F(InvalidSearchProviderTest, DoesNotSendSuggestRequests) {
+  base::string16 query = ASCIIToUTF16("query");
+  QueryForInput(query, false, false);
+
+  // Make sure the default provider's suggest service was not queried.
+  EXPECT_FALSE(test_url_loader_factory_.IsPending(prefix + "query"));
+}
+
+// SearchProviderWarmUpTest --------------------------------------------------
+//
+// Like SearchProviderTest.  The only addition is that it's a
+// TestWithParam<bool>, where the boolean parameter represents whether the
+// omnibox::kSearchProviderWarmUpOnFocus feature flag should be enabled.
+class SearchProviderWarmUpTest : public SearchProviderTest,
+                                 public testing::WithParamInterface<bool> {
+ public:
+  SearchProviderWarmUpTest() : SearchProviderTest(GetParam(), false) {}
+
+  SearchProviderWarmUpTest(SearchProviderWarmUpTest const&) = delete;
+  SearchProviderWarmUpTest& operator=(SearchProviderWarmUpTest const&) = delete;
+};
+
+TEST_P(SearchProviderWarmUpTest, SendsWarmUpRequestOnFocus) {
   AutocompleteInput input(base::ASCIIToUTF16("f"),
                           metrics::OmniboxEventProto::OTHER,
                           ChromeAutocompleteSchemeClassifier(&profile_));
   input.set_prefer_keyword(true);
   input.set_from_omnibox_focus(true);
 
-  {
-    // First, verify that without the warm-up feature enabled, the provider
-    // immediately terminates with no matches.
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitAndDisableFeature(omnibox::kSearchProviderWarmUpOnFocus);
+  if (!GetParam()) {  // The warm-up feature ought to be disabled.
+    // The provider immediately terminates with no matches.
     provider_->Start(input, false);
     // RunUntilIdle so that SearchProvider has a chance to create the
     // URLFetchers (if it wants to, which it shouldn't in this case).
     base::RunLoop().RunUntilIdle();
     EXPECT_TRUE(provider_->done());
     EXPECT_TRUE(provider_->matches().empty());
-  }
-
-  {
-    // Then, check the behavior with the warm-up feature enabled.
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitAndEnableFeature(omnibox::kSearchProviderWarmUpOnFocus);
+  } else {  // The warm-up feature ought to be enabled.
     provider_->Start(input, false);
     // RunUntilIdle so that SearchProvider create the URLFetcher.
     base::RunLoop().RunUntilIdle();
@@ -3811,10 +3828,46 @@ TEST_F(SearchProviderTest, MAYBE_SendsWarmUpRequestOnFocus) {
   }
 }
 
-TEST_F(InvalidSearchProviderTest, DoesNotSendSuggestRequests) {
-  base::string16 query = ASCIIToUTF16("query");
-  QueryForInput(query, false, false);
+INSTANTIATE_TEST_CASE_P(SearchProviderTest,
+                        SearchProviderWarmUpTest,
+                        testing::Values(false, true));
 
-  // Make sure the default provider's suggest service was not queried.
-  EXPECT_FALSE(test_url_loader_factory_.IsPending(prefix + "query"));
+// SearchProviderCommandLineOverrideTest -------------------------------------
+//
+// Like SearchProviderTest.  The only addition is that it sets additional
+// command line flags in SearchProviderFeatureTestComponent.
+class SearchProviderCommandLineOverrideTest : public SearchProviderTest {
+ public:
+  SearchProviderCommandLineOverrideTest()
+      : SearchProviderTest(base::nullopt, true) {}
+
+  SearchProviderCommandLineOverrideTest(
+      SearchProviderCommandLineOverrideTest const&) = delete;
+  SearchProviderCommandLineOverrideTest& operator=(
+      SearchProviderCommandLineOverrideTest const&) = delete;
+};
+
+TEST_F(SearchProviderCommandLineOverrideTest, CommandLineOverrides) {
+  TemplateURLService* turl_model =
+      TemplateURLServiceFactory::GetForProfile(&profile_);
+
+  TemplateURLData data;
+  data.SetShortName(ASCIIToUTF16("default"));
+  data.SetKeyword(data.short_name());
+  data.SetURL("{google:baseURL}{searchTerms}");
+  default_t_url_ = turl_model->Add(std::make_unique<TemplateURL>(data));
+  turl_model->SetUserSelectedDefaultSearchProvider(default_t_url_);
+
+  TestData cases[] = {
+      {ASCIIToUTF16("k a"),
+       2,
+       {ResultInfo(GURL("http://keyword/a"),
+                   AutocompleteMatchType::SEARCH_OTHER_ENGINE, true,
+                   ASCIIToUTF16("k a")),
+        ResultInfo(GURL("http://www.bar.com/k%20a?a=b"),
+                   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, false,
+                   ASCIIToUTF16("k a"))}},
+  };
+
+  RunTest(cases, base::size(cases), false);
 }

components/omnibox/browser/search_provider.h

@@ -102,13 +102,13 @@ class SearchProvider : public BaseSearchProvider,
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, AnswersCache);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, RemoveExtraAnswers);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoesNotProvideOnFocus);
-  FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SendsWarmUpRequestOnFocus);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoTrimHttpScheme);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest,
                            DontTrimHttpSchemeIfInputHasScheme);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest,
                            DontTrimHttpsSchemeIfInputHasScheme);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoTrimHttpsScheme);
+  FRIEND_TEST_ALL_PREFIXES(SearchProviderWarmUpTest, SendsWarmUpRequestOnFocus);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedPrefetchTest, ClearPrefetchedResults);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedPrefetchTest, SetPrefetchQuery);