Disallow FeatureList overrides after early setup

This changes BrowserTestBase::SetUp to disallow FeatureList overrides
for the remaining duration of the test. Catches incorrect usage of
ScopedFeatureList which can cause data races.

Bug: 846380
Change-Id: Ie76d59e191edc3ac05c96131e83e5aa0b73dfda7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1842111
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705389}

base/feature_list.cc

@@ -29,6 +29,17 @@ FeatureList* g_feature_list_instance = nullptr;
 // Tracks whether the FeatureList instance was initialized via an accessor.
 bool g_initialized_from_accessor = false;
 
+#if DCHECK_IS_ON()
+const char* g_reason_overrides_disallowed = nullptr;
+
+void DCheckOverridesAllowed() {
+  const bool feature_overrides_allowed = !g_reason_overrides_disallowed;
+  DCHECK(feature_overrides_allowed) << g_reason_overrides_disallowed;
+}
+#else
+void DCheckOverridesAllowed() {}
+#endif
+
 // An allocator entry for a feature in shared memory. The FeatureEntry is
 // followed by a base::Pickle object that contains the feature and trial name.
 struct FeatureEntry {
@@ -86,6 +97,23 @@ FeatureList::FeatureList() = default;
 
 FeatureList::~FeatureList() = default;
 
+FeatureList::ScopedDisallowOverrides::ScopedDisallowOverrides(
+    const char* reason)
+#if DCHECK_IS_ON()
+    : previous_reason_(g_reason_overrides_disallowed) {
+  g_reason_overrides_disallowed = reason;
+}
+#else
+{
+}
+#endif
+
+FeatureList::ScopedDisallowOverrides::~ScopedDisallowOverrides() {
+#if DCHECK_IS_ON()
+  g_reason_overrides_disallowed = previous_reason_;
+#endif
+}
+
 void FeatureList::InitializeFromCommandLine(
     const std::string& enable_features,
     const std::string& disable_features) {
@@ -385,6 +413,7 @@ void FeatureList::RegisterOverride(StringPiece feature_name,
                                    OverrideState overridden_state,
                                    FieldTrial* field_trial) {
   DCHECK(!initialized_);
+  DCheckOverridesAllowed();
   if (field_trial) {
     DCHECK(IsValidFeatureOrFieldTrialName(field_trial->trial_name()))
         << field_trial->trial_name();

base/feature_list.h

@@ -100,6 +100,21 @@ class BASE_EXPORT FeatureList {
   FeatureList();
   ~FeatureList();
 
+  // Used by common test fixture classes to prevent abuse of ScopedFeatureList
+  // after multiple threads have started.
+  class BASE_EXPORT ScopedDisallowOverrides {
+   public:
+    explicit ScopedDisallowOverrides(const char* reason);
+    ~ScopedDisallowOverrides();
+
+   private:
+#if DCHECK_IS_ON()
+    const char* const previous_reason_;
+#endif
+
+    DISALLOW_COPY_AND_ASSIGN(ScopedDisallowOverrides);
+  };
+
   // Specifies whether a feature override enables or disables the feature.
   enum OverrideState {
     OVERRIDE_USE_DEFAULT,

base/test/scoped_feature_list.h

@@ -26,13 +26,19 @@ namespace test {
 // list and initialize it anew, destroy an existing scoped list and init
 // a new one.
 //
+// If multiple instances of this class are used in a nested fashion, they
+// should be destroyed in the opposite order of their Init*() methods being
+// called.
+//
 // ScopedFeatureList needs to be initialized (via one of Init*() methods)
 // before running code that inspects the state of features, such as in the
 // constructor of the test harness.
 //
-// If multiple instances of this class are used in a nested fashion, they
-// should be destroyed in the opposite order of their Init*() methods being
-// called.
+// WARNING: To be clear, in multithreaded test environments (such as browser
+// tests) there may background threads using FeatureList before the test body is
+// even entered. In these cases it is imperative that ScopedFeatureList be
+// initialized BEFORE those threads are started, hence the recommendation to do
+// initialization in the test harness's constructor.
 class ScopedFeatureList final {
  public:
   ScopedFeatureList();

chrome/browser/spellchecker/spell_check_host_chrome_impl_win_browsertest.cc

@@ -20,13 +20,14 @@
 
 class SpellCheckHostChromeImplWinBrowserTest : public InProcessBrowserTest {
  public:
+  SpellCheckHostChromeImplWinBrowserTest() {
+    feature_list_.InitAndEnableFeature(spellcheck::kWinUseBrowserSpellChecker);
+  }
+
   void SetUpOnMainThread() override {
     content::BrowserContext* context = browser()->profile();
     renderer_.reset(new content::MockRenderProcessHost(context));
 
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitAndEnableFeature(spellcheck::kWinUseBrowserSpellChecker);
-
     SpellCheckHostChromeImpl::Create(
         renderer_->GetID(), spell_check_host_.BindNewPipeAndPassReceiver());
   }
@@ -58,6 +59,7 @@ class SpellCheckHostChromeImplWinBrowserTest : public InProcessBrowserTest {
   }
 
  protected:
+  base::test::ScopedFeatureList feature_list_;
   std::unique_ptr<content::MockRenderProcessHost> renderer_;
   mojo::Remote<spellcheck::mojom::SpellCheckHost> spell_check_host_;
 
@@ -72,9 +74,6 @@ IN_PROC_BROWSER_TEST_F(SpellCheckHostChromeImplWinBrowserTest,
   if (base::win::GetVersion() < base::win::Version::WIN8)
     return;
 
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(spellcheck::kWinUseBrowserSpellChecker);
-
   spellcheck_platform::SetLanguage(
       "en-US", base::BindOnce(&SpellCheckHostChromeImplWinBrowserTest::
                                   SetLanguageCompletionCallback,

content/public/test/browser_test_base.cc

@@ -427,6 +427,12 @@ void BrowserTestBase::SetUp() {
     InitializeBrowserMemoryInstrumentationClient();
   }
 
+  // All FeatureList overrides should have been registered prior to browser test
+  // SetUp().
+  base::FeatureList::ScopedDisallowOverrides disallow_feature_overrides(
+      "FeatureList overrides must happen in the test constructor, before "
+      "BrowserTestBase::SetUp() has run.");
+
   auto discardable_shared_memory_manager =
       std::make_unique<discardable_memory::DiscardableSharedMemoryManager>();
   auto service_manager_env = std::make_unique<ServiceManagerEnvironment>(
@@ -581,6 +587,14 @@ std::string GetDefaultTraceFilaneme() {
 }  // namespace
 
 void BrowserTestBase::ProxyRunTestOnMainThreadLoop() {
+#if !defined(OS_ANDROID)
+  // All FeatureList overrides should have been registered prior to browser test
+  // SetUp(). Note that on Android, this scoper lives in SetUp() above.
+  base::FeatureList::ScopedDisallowOverrides disallow_feature_overrides(
+      "FeatureList overrides must happen in the test constructor, before "
+      "BrowserTestBase::SetUp() has run.");
+#endif
+
   // Install a RunLoop timeout if none is present but do not override tests that
   // set a ScopedRunTimeoutForTest from their fixture's constructor (which
   // happens as part of setting up the test factory in gtest while

content/public/test/browser_test_base.h

@@ -32,21 +32,12 @@ class BrowserTestBase : public testing::Test {
   BrowserTestBase();
   ~BrowserTestBase() override;
 
-  // Configures everything for an in process browser test (i.e. feature list,
-  // thread pool, etc.) by invoking ContentMain (or manually on OS_ANDROID). As
-  // such all single-threaded initialization (such as
-  // base::test::ScopedFeatureList) must be done before this step, i.e.:
-  //   class MyFixture : public content::BrowserTestBase {
-  //    public:
-  //     void SetUp() override {
-  //       scoped_feature_list_.InitWithFeatures(features::kMyFeature);
-  //       content::BrowserTestBase::SetUp();
-  //     }
+  // Configures everything for an in process browser test (e.g. thread pool,
+  // etc.) by invoking ContentMain (or manually on OS_ANDROID). As such all
+  // single-threaded initialization must be done before this step.
   //
-  //    private:
-  //     base::test::ScopedFeatureList scoped_feature_list_;
-  //   };
-  // ContentMain then ends up invoking RunTestOnMainThreadLoop.
+  // ContentMain then ends up invoking RunTestOnMainThreadLoop with browser
+  // threads already running.
   void SetUp() override;
 
   // Restores state configured in SetUp.