Fix misc ScopedFeatureList usage (2/8)

ScopedFeatureList is unsafe to use after browser threads have been
started. This constraint will imminently be enforced by DCHECK to
prevent further erroneous usage from landing.

This CL corrects usage within miscellaneous Chrome feature tests,
including Password Manager, payments, page load metrics, etc.

This is split from a larger CL where in some rare cases, correction
was too complex to resolve before landing the DCHECK, so corresponding
test(s) may be disabled instead of fixed.

Bug: 846380
Change-Id: Ic4944308327312038d1e92523ac1188530f54b0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1850732Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: John Delaney <johnidel@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#705133}

chrome/browser/page_load_metrics/observers/ad_metrics/ads_page_load_metrics_observer_browsertest.cc

@@ -915,12 +915,22 @@ IN_PROC_BROWSER_TEST_F(AdsPageLoadMetricsObserverResourceBrowserTest,
       blink::mojom::WebFeature::kHeavyAdIntervention, 1);
 }
 
+class AdsPageLoadMetricsObserverResourceBrowserTestWithoutHeavyAdIntervention
+    : public AdsPageLoadMetricsObserverResourceBrowserTest {
+ public:
+  AdsPageLoadMetricsObserverResourceBrowserTestWithoutHeavyAdIntervention() {
+    feature_list_.InitAndDisableFeature(features::kHeavyAdIntervention);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Check that when the heavy ad feature is disabled we don't navigate
 // the frame.
-IN_PROC_BROWSER_TEST_F(AdsPageLoadMetricsObserverResourceBrowserTest,
-                       HeavyAdInterventionDisabled_ErrorPageNotLoaded) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndDisableFeature(features::kHeavyAdIntervention);
+IN_PROC_BROWSER_TEST_F(
+    AdsPageLoadMetricsObserverResourceBrowserTestWithoutHeavyAdIntervention,
+    ErrorPageNotLoaded) {
   base::HistogramTester histogram_tester;
   auto incomplete_resource_response =
       std::make_unique<net::test_server::ControllableHttpResponse>(

chrome/browser/password_manager/password_manager_browsertest.cc

@@ -2457,7 +2457,21 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, ChangePwd1AccountStored) {
   CheckThatCredentialsStored("temp", "new_pw");
 }
 
-IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
+// This fixture disable autofill. If a password is autofilled, then all the
+// Javascript changes are discarded and test below would not be able to feed a
+// new password to the form.
+class PasswordManagerBrowserTestWithAutofillDisabled
+    : public PasswordManagerBrowserTest {
+ public:
+  PasswordManagerBrowserTestWithAutofillDisabled() {
+    feature_list_.InitAndEnableFeature(features::kFillOnAccountSelect);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestWithAutofillDisabled,
                        PasswordOverridenUpdateBubbleShown) {
   // At first let us save credentials to the PasswordManager.
   scoped_refptr<password_manager::TestPasswordStore> password_store =
@@ -2471,12 +2485,6 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
   signin_form.password_value = base::ASCIIToUTF16("pw");
   password_store->AddLogin(signin_form);
 
-  // Disable autofill. If a password is autofilled then all the Javacript
-  // changes are discarded. The test would not be able to feed the new password
-  // below.
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kFillOnAccountSelect);
-
   // Check that password update bubble is shown.
   NavigateToFile("/password/password_form.html");
   NavigationObserver observer(WebContents());

chrome/browser/payments/has_enrolled_instrument_query_quota_browsertest.cc

@@ -57,23 +57,29 @@ class HasEnrolledInstrumentQueryQuotaTest : public PlatformBrowserTest {
     return chrome_test_utils::GetActiveWebContents(this);
   }
 
- protected:
-  base::test::ScopedFeatureList features_;
-
  private:
   net::EmbeddedTestServer https_server_;
 
   DISALLOW_COPY_AND_ASSIGN(HasEnrolledInstrumentQueryQuotaTest);
 };
 
+class HasEnrolledInstrumentQueryQuotaTestNoFlags
+    : public HasEnrolledInstrumentQueryQuotaTest {
+ public:
+  HasEnrolledInstrumentQueryQuotaTestNoFlags() {
+    features_.InitWithFeatures(
+        /*enabled_features=*/{}, /*disabled_features=*/{
+            features::kStrictHasEnrolledAutofillInstrument,
+            features::kWebPaymentsPerMethodCanMakePaymentQuota});
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+};
+
 // Payment options do not trigger query quota when the strict autofill data
 // check is disabled. Per-method query quota is also disabled in this test.
-IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTest, NoFlags) {
-  features_.InitWithFeatures(
-      /*enabled_features=*/{}, /*disabled_features=*/{
-          features::kStrictHasEnrolledAutofillInstrument,
-          features::kWebPaymentsPerMethodCanMakePaymentQuota});
-
+IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTestNoFlags, NoFlags) {
   EXPECT_EQ(false,
             content::EvalJs(GetActiveWebContents(), "hasEnrolledInstrument()"));
   EXPECT_EQ(false,
@@ -81,13 +87,24 @@ IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTest, NoFlags) {
                             "hasEnrolledInstrument({requestShipping:true})"));
 }
 
+class HasEnrolledInstrumentQueryQuotaTestPerMethodQuota
+    : public HasEnrolledInstrumentQueryQuotaTest {
+ public:
+  HasEnrolledInstrumentQueryQuotaTestPerMethodQuota() {
+    features_.InitWithFeatures(
+        /*enabled_features=*/{features::
+                                  kWebPaymentsPerMethodCanMakePaymentQuota},
+        /*disabled_features=*/{features::kStrictHasEnrolledAutofillInstrument});
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+};
+
 // Payment options do not trigger query quota when the strict autofill data
 // check is disabled. Per-method query quota is enabled in this test.
-IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTest, PerMethodQuota) {
-  features_.InitWithFeatures(
-      /*enabled_features=*/{features::kWebPaymentsPerMethodCanMakePaymentQuota},
-      /*disabled_features=*/{features::kStrictHasEnrolledAutofillInstrument});
-
+IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTestPerMethodQuota,
+                       PerMethodQuota) {
   EXPECT_EQ(false,
             content::EvalJs(GetActiveWebContents(), "hasEnrolledInstrument()"));
   EXPECT_EQ(false,
@@ -95,15 +112,25 @@ IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTest, PerMethodQuota) {
                             "hasEnrolledInstrument({requestShipping:true})"));
 }
 
+class HasEnrolledInstrumentQueryQuotaTestStrictAutofillDataCheck
+    : public HasEnrolledInstrumentQueryQuotaTest {
+ public:
+  HasEnrolledInstrumentQueryQuotaTestStrictAutofillDataCheck() {
+    features_.InitWithFeatures(
+        /*enabled_features=*/{features::kStrictHasEnrolledAutofillInstrument},
+        /*disabled_features=*/{
+            features::kWebPaymentsPerMethodCanMakePaymentQuota});
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+};
+
 // Payment options trigger query quota for Basic Card when the strict autofill
 // data check is enabled. Per-method query quota is disabled in this test.
-IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTest,
-                       StrictAutofillDataCheck) {
-  features_.InitWithFeatures(
-      /*enabled_features=*/{features::kStrictHasEnrolledAutofillInstrument},
-      /*disabled_features=*/{
-          features::kWebPaymentsPerMethodCanMakePaymentQuota});
-
+IN_PROC_BROWSER_TEST_F(
+    HasEnrolledInstrumentQueryQuotaTestStrictAutofillDataCheck,
+    StrictAutofillDataCheck) {
   EXPECT_EQ(false,
             content::EvalJs(GetActiveWebContents(), "hasEnrolledInstrument()"));
   EXPECT_EQ("NotAllowedError: Exceeded query quota for hasEnrolledInstrument",
@@ -111,14 +138,25 @@ IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTest,
                             "hasEnrolledInstrument({requestShipping:true})"));
 }
 
+class HasEnrolledInstrumentQueryQuotaTestBothFlags
+    : public HasEnrolledInstrumentQueryQuotaTest {
+ public:
+  HasEnrolledInstrumentQueryQuotaTestBothFlags() {
+    features_.InitWithFeatures(
+        /*enabled_features=*/{features::kStrictHasEnrolledAutofillInstrument,
+                              features::
+                                  kWebPaymentsPerMethodCanMakePaymentQuota},
+        /*disabled_features=*/{});
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+};
+
 // Payment options trigger query quota for Basic Card when the strict autofill
 // data check is enabled. Per-method query quota is also enabled in this test.
-IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTest, BothFlags) {
-  features_.InitWithFeatures(
-      /*enabled_features=*/{features::kStrictHasEnrolledAutofillInstrument,
-                            features::kWebPaymentsPerMethodCanMakePaymentQuota},
-      /*disabled_features=*/{});
-
+IN_PROC_BROWSER_TEST_F(HasEnrolledInstrumentQueryQuotaTestBothFlags,
+                       BothFlags) {
   EXPECT_EQ(false,
             content::EvalJs(GetActiveWebContents(), "hasEnrolledInstrument()"));
   EXPECT_EQ("NotAllowedError: Exceeded query quota for hasEnrolledInstrument",

chrome/browser/payments/payment_request_can_make_payment_browsertest.cc

@@ -167,7 +167,6 @@ class PaymentRequestCanMakePaymentTestBase : public PlatformBrowserTest,
  private:
   PaymentRequestTestController payment_request_controller_;
 
-  base::test::ScopedFeatureList feature_list_;
   syncer::TestSyncService sync_service_;
   std::unique_ptr<net::EmbeddedTestServer> https_server_;
   std::unique_ptr<autofill::EventWaiter<TestEvent>> event_waiter_;
@@ -229,13 +228,23 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryTest,
   ExpectBodyContains({"false"});
 }
 
+class PaymentRequestCanMakePaymentQueryTestWithGooglePayCardsDisabled
+    : public PaymentRequestCanMakePaymentQueryTest {
+ public:
+  PaymentRequestCanMakePaymentQueryTestWithGooglePayCardsDisabled() {
+    feature_list_.InitAndDisableFeature(
+        payments::features::kReturnGooglePayInBasicCard);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Visa is required, and user has a masked visa instrument, and Google Pay cards
 // in basic-card is disabled.
-IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryTest,
-                       CanMakePayment_Supported_GooglePayCardsDisabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(
-      payments::features::kReturnGooglePayInBasicCard);
+IN_PROC_BROWSER_TEST_F(
+    PaymentRequestCanMakePaymentQueryTestWithGooglePayCardsDisabled,
+    CanMakePayment_Supported) {
   NavigateTo("/payment_request_can_make_payment_query_test.html");
   autofill::CreditCard card = autofill::test::GetMaskedServerCard();
   card.SetNumber(base::ASCIIToUTF16("4111111111111111"));  // We need a visa.
@@ -252,13 +261,23 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryTest,
   ExpectBodyContains({"false"});
 }
 
+class PaymentRequestCanMakePaymentQueryTestWithGooglePayCardsEnabled
+    : public PaymentRequestCanMakePaymentQueryTest {
+ public:
+  PaymentRequestCanMakePaymentQueryTestWithGooglePayCardsEnabled() {
+    feature_list_.InitAndEnableFeature(
+        payments::features::kReturnGooglePayInBasicCard);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Visa is required, and user has a masked visa instrument, and Google Pay cards
 // in basic-card is enabled.
-IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryTest,
-                       CanMakePayment_Supported_GooglePayCardsEnabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      payments::features::kReturnGooglePayInBasicCard);
+IN_PROC_BROWSER_TEST_F(
+    PaymentRequestCanMakePaymentQueryTestWithGooglePayCardsEnabled,
+    CanMakePayment_Supported) {
   NavigateTo("/payment_request_can_make_payment_query_test.html");
   autofill::CreditCard card = autofill::test::GetMaskedServerCard();
   card.SetNumber(base::ASCIIToUTF16("4111111111111111"));  // We need a visa.
@@ -371,8 +390,6 @@ class PaymentRequestCanMakePaymentQueryCCTest
   }
 
  private:
-  base::test::ScopedFeatureList feature_list_;
-
   DISALLOW_COPY_AND_ASSIGN(PaymentRequestCanMakePaymentQueryCCTest);
 };
 
@@ -597,13 +614,22 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITest,
   ExpectBodyContains({"NotAllowedError"});
 }
 
+class PaymentRequestCanMakePaymentQueryPMITestWithPaymentQuota
+    : public PaymentRequestCanMakePaymentQueryPMITest {
+ public:
+  PaymentRequestCanMakePaymentQueryPMITestWithPaymentQuota() {
+    feature_list_.InitAndEnableFeature(
+        features::kWebPaymentsPerMethodCanMakePaymentQuota);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // If the device does not have any payment apps installed, canMakePayment() and
 // hasEnrolledInstrument() should return false for them.
-IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITest,
+IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITestWithPaymentQuota,
                        QueryQuotaForPaymentApps) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      features::kWebPaymentsPerMethodCanMakePaymentQuota);
   NavigateTo("/payment_request_payment_method_identifier_test.html");
 
   CallCanMakePayment(CheckFor::ALICE_PAY);
@@ -664,17 +690,29 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITest,
   ExpectBodyContains({"NotAllowedError"});
 }
 
+class
+    PaymentRequestCanMakePaymentQueryPMITestWithPaymentQuotaAndServiceWorkerPayment
+    : public PaymentRequestCanMakePaymentQueryPMITest {
+ public:
+  PaymentRequestCanMakePaymentQueryPMITestWithPaymentQuotaAndServiceWorkerPayment() {
+    feature_list_.InitWithFeatures(
+        /*enable_features=*/{::features::kServiceWorkerPaymentApps,
+                             features::
+                                 kWebPaymentsPerMethodCanMakePaymentQuota},
+        /*disable_features=*/{});
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Querying for payment apps in incognito returns result as normal mode to avoid
 // incognito mode detection. Multiple queries for different apps are rejected
 // with NotSupportedError to avoid user fingerprinting.
-IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITest,
-                       QueryQuotaForPaymentAppsInIncognitoMode) {
+IN_PROC_BROWSER_TEST_F(
+    PaymentRequestCanMakePaymentQueryPMITestWithPaymentQuotaAndServiceWorkerPayment,
+    QueryQuotaForPaymentAppsInIncognitoMode) {
   NavigateTo("/payment_request_payment_method_identifier_test.html");
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatures(
-      /*enable_features=*/{::features::kServiceWorkerPaymentApps,
-                           features::kWebPaymentsPerMethodCanMakePaymentQuota},
-      /*disable_features=*/{});
 
   SetIncognito(true);
 
@@ -703,11 +741,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITest,
 // as in normal mode to avoid incognito mode detection. Multiple queries for
 // different payment methods are rejected with NotSupportedError to avoid user
 // fingerprinting.
-IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITest,
+IN_PROC_BROWSER_TEST_F(PaymentRequestCanMakePaymentQueryPMITestWithPaymentQuota,
                        NoQueryQuotaForPaymentAppsAndCardsInIncognito) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      features::kWebPaymentsPerMethodCanMakePaymentQuota);
   NavigateTo("/payment_request_payment_method_identifier_test.html");
   SetIncognito(true);
 

chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc

@@ -68,7 +68,11 @@ const int64_t kIdShift = 1 << 13;
 // to UKM logs.
 class TabActivityWatcherTest : public InProcessBrowserTest {
  protected:
-  TabActivityWatcherTest() = default;
+  TabActivityWatcherTest() {
+    scoped_feature_list_.InitAndEnableFeatureWithParameters(
+        features::kTabRanker,
+        {{"disable_background_log_with_TabRanker", "false"}});
+  }
 
   // TabActivityWatcherTest:
   void PreRunTestOnMainThread() override {
@@ -79,9 +83,6 @@ class TabActivityWatcherTest : public InProcessBrowserTest {
 
   void SetUpOnMainThread() override {
     InProcessBrowserTest::SetUpOnMainThread();
-    scoped_feature_list_.InitAndEnableFeatureWithParameters(
-        features::kTabRanker,
-        {{"disable_background_log_with_TabRanker", "false"}});
     ASSERT_TRUE(embedded_test_server()->Start());
     test_urls_ = {embedded_test_server()->GetURL("/title1.html"),
                   embedded_test_server()->GetURL("/title2.html"),
@@ -173,20 +174,31 @@ class TabActivityWatcherTest : public InProcessBrowserTest {
   }
 
   std::vector<GURL> test_urls_;
-  base::test::ScopedFeatureList scoped_feature_list_;
   std::unique_ptr<UkmEntryChecker> ukm_entry_checker_;
   std::unique_ptr<ukm::TestAutoSetUkmRecorder> test_ukm_recorder_;
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(TabActivityWatcherTest);
 };
 
+class TabActivityWatcherTestWithBackgroundLogDisabled
+    : public TabActivityWatcherTest {
+ public:
+  TabActivityWatcherTestWithBackgroundLogDisabled() {
+    feature_list_.InitAndEnableFeatureWithParameters(
+        features::kTabRanker,
+        {{"disable_background_log_with_TabRanker", "true"}});
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Tests calculating tab scores using the Tab Ranker.
-IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest, CalculateReactivationScore) {
-  base::test::ScopedFeatureList scoped_feature_list_overrides;
-  scoped_feature_list_overrides.InitAndEnableFeatureWithParameters(
-      features::kTabRanker,
-      {{"disable_background_log_with_TabRanker", "true"}});
+IN_PROC_BROWSER_TEST_F(TabActivityWatcherTestWithBackgroundLogDisabled,
+                       CalculateReactivationScore) {
   // Use test clock so tabs have non-zero backgrounded times.
   base::SimpleTestTickClock test_clock;
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing(&test_clock);
@@ -389,15 +401,23 @@ IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest, TabDrag) {
   EXPECT_EQ(0, ukm_entry_checker_->NumNewEntriesRecorded(kFOCEntryName));
 }
 
+class TabActivityWatcherTestWithBackgroundLogEnabled
+    : public TabActivityWatcherTest {
+ public:
+  TabActivityWatcherTestWithBackgroundLogEnabled() {
+    feature_list_.InitAndEnableFeatureWithParameters(
+        features::kTabRanker,
+        {{"number_of_oldest_tabs_to_log_with_TabRanker", "1"},
+         {"disable_background_log_with_TabRanker", "false"}});
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Tests discarded tab is recorded correctly.
-IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest,
+IN_PROC_BROWSER_TEST_F(TabActivityWatcherTestWithBackgroundLogEnabled,
                        DiscardedTabGetsPreviousSourceId) {
-  base::test::ScopedFeatureList scoped_feature_list_overrides;
-  scoped_feature_list_overrides.InitAndEnableFeatureWithParameters(
-      features::kTabRanker,
-      {{"number_of_oldest_tabs_to_log_with_TabRanker", "1"},
-       {"disable_background_log_with_TabRanker", "false"}});
-
   ukm::SourceId ukm_source_id_for_tab_0 = 0;
   ukm::SourceId ukm_source_id_for_tab_1 = 0;
 
@@ -506,19 +526,29 @@ IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest, AllWindowMetricsArePopulated) {
   }
 }
 
-// Test the query time logging is correct.
-IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest, LogOldestNTabFeatures) {
-  // Set Feature params for this test.
-  // (1) background log is disabled, so that only query time logging and
-  // corresponding labels should be logged.
-  // (2) number of oldest tabs to log is set to 1, so that only 1 tab should be
-  // logged.
-  base::test::ScopedFeatureList scoped_feature_list_overrides;
-  scoped_feature_list_overrides.InitAndEnableFeatureWithParameters(
-      features::kTabRanker,
-      {{"number_of_oldest_tabs_to_log_with_TabRanker", "1"},
-       {"disable_background_log_with_TabRanker", "true"}});
+class TabActivityWatcherTestWithBackgroundLogDisabledAndOnlyOneOldestTab
+    : public TabActivityWatcherTest {
+ public:
+  TabActivityWatcherTestWithBackgroundLogDisabledAndOnlyOneOldestTab() {
+    // Set Feature params for this test.
+    // (1) background log is disabled, so that only query time logging and
+    // corresponding labels should be logged.
+    // (2) number of oldest tabs to log is set to 1, so that only 1 tab should
+    // be logged.
+    feature_list_.InitAndEnableFeatureWithParameters(
+        features::kTabRanker,
+        {{"number_of_oldest_tabs_to_log_with_TabRanker", "1"},
+         {"disable_background_log_with_TabRanker", "true"}});
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
 
+// Test the query time logging is correct.
+IN_PROC_BROWSER_TEST_F(
+    TabActivityWatcherTestWithBackgroundLogDisabledAndOnlyOneOldestTab,
+    LogOldestNTabFeatures) {
   // Use test clock so tabs have non-zero backgrounded times.
   base::SimpleTestTickClock test_clock;
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing(&test_clock);
@@ -610,13 +640,9 @@ IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest, LogOldestNTabFeatures) {
 }
 
 // Tests label id is recorded correctly for discarded tabs.
-IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest, DiscardedTabGetsCorrectLabelId) {
-  base::test::ScopedFeatureList scoped_feature_list_overrides;
-  scoped_feature_list_overrides.InitAndEnableFeatureWithParameters(
-      features::kTabRanker,
-      {{"number_of_oldest_tabs_to_log_with_TabRanker", "1"},
-       {"disable_background_log_with_TabRanker", "true"}});
-
+IN_PROC_BROWSER_TEST_F(
+    TabActivityWatcherTestWithBackgroundLogDisabledAndOnlyOneOldestTab,
+    DiscardedTabGetsCorrectLabelId) {
   ui_test_utils::NavigateToURL(browser(), test_urls_[0]);
   AddTabAtIndex(1, test_urls_[1], ui::PAGE_TRANSITION_LINK);
   // No TabMetrics events are logged till now.
@@ -692,14 +718,9 @@ IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest, DiscardedTabGetsCorrectLabelId) {
 
 // Tests label_id is incremented if the LogOldestNTabFeatures is called second
 // times without logging the label first.
-IN_PROC_BROWSER_TEST_F(TabActivityWatcherTest,
-                       TabsAlreadyHaveLabelIdGetIncrementalLabelIds) {
-  base::test::ScopedFeatureList scoped_feature_list_overrides;
-  scoped_feature_list_overrides.InitAndEnableFeatureWithParameters(
-      features::kTabRanker,
-      {{"number_of_oldest_tabs_to_log_with_TabRanker", "1"},
-       {"disable_background_log_with_TabRanker", "true"}});
-
+IN_PROC_BROWSER_TEST_F(
+    TabActivityWatcherTestWithBackgroundLogDisabledAndOnlyOneOldestTab,
+    TabsAlreadyHaveLabelIdGetIncrementalLabelIds) {
   ui_test_utils::NavigateToURL(browser(), test_urls_[0]);
   AddTabAtIndex(1, test_urls_[1], ui::PAGE_TRANSITION_LINK);
   // No TabMetrics events are logged till now.

chrome/browser/sessions/session_restore_browsertest.cc

@@ -939,9 +939,13 @@ std::vector<base::Optional<TabGroupId>> GetTabGroups(
 // to do a command reset when quitting and restoring.
 class SessionRestoreTabGroupsTest : public SessionRestoreTest,
                                     public testing::WithParamInterface<bool> {
+ public:
+  SessionRestoreTabGroupsTest() {
+    feature_override_.InitAndEnableFeature(features::kTabGroups);
+  }
+
  protected:
   void SetUpOnMainThread() override {
-    feature_override_.InitAndEnableFeature(features::kTabGroups);
     SessionRestoreTest::SetUpOnMainThread();
   }
 
@@ -1031,8 +1035,11 @@ INSTANTIATE_TEST_SUITE_P(WithAndWithoutReset,
 
 // Ensure tab groups aren't restored if |features::kTabGroups| is disabled.
 // Regression test for crbug.com/983962.
+//
+// TODO(https://crbug.com/1012605): Find a way to cover this regression without
+// relying on dynamic FeatureList overrides mid-test.
 IN_PROC_BROWSER_TEST_F(SessionRestoreTest,
-                       GroupsNotRestoredWhenFeatureDisabled) {
+                       DISABLED_GroupsNotRestoredWhenFeatureDisabled) {
   auto feature_override = std::make_unique<base::test::ScopedFeatureList>();
   feature_override->InitAndEnableFeature(features::kTabGroups);
 
@@ -1866,7 +1873,12 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreWithURLInCommandLineTest,
 class SecFetchSiteSessionRestoreTest : public SessionRestoreTest {
  public:
   SecFetchSiteSessionRestoreTest()
-      : https_test_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}
+      : https_test_server_(net::EmbeddedTestServer::TYPE_HTTPS) {
+    feature_list_.InitWithFeatures(
+        {network::features::kFetchMetadata,
+         network::features::kFetchMetadataDestination},
+        {});
+  }
 
   void SetUpOnMainThread() override {
     SessionRestoreTest::SetUpOnMainThread();
@@ -1877,11 +1889,6 @@ class SecFetchSiteSessionRestoreTest : public SessionRestoreTest {
     https_test_server_.SetSSLConfig(net::EmbeddedTestServer::CERT_OK);
     ASSERT_TRUE(https_test_server_.Start());
     ASSERT_TRUE(embedded_test_server()->Start());
-
-    feature_list_.InitWithFeatures(
-        {network::features::kFetchMetadata,
-         network::features::kFetchMetadataDestination},
-        {});
   }
 
   content::WebContents* GetTab(Browser* browser, int tab_index) {

chrome/browser/sessions/tab_restore_browsertest.cc

@@ -891,10 +891,17 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, GetRestoreTabType) {
   EXPECT_EQ(sessions::TabRestoreService::TAB, service->entries().front()->type);
 }
 
-IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreGroupedTab) {
-  base::test::ScopedFeatureList feature_override;
-  feature_override.InitAndEnableFeature(features::kTabGroups);
+class TabRestoreTestWithTabGroupsEnabled : public TabRestoreTest {
+ public:
+  TabRestoreTestWithTabGroupsEnabled() {
+    feature_list_.InitAndEnableFeature(features::kTabGroups);
+  }
 
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(TabRestoreTestWithTabGroupsEnabled, RestoreGroupedTab) {
   const int tab_count = AddSomeTabs(browser(), 1);
   ASSERT_LE(2, tab_count);
 
@@ -914,10 +921,8 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreGroupedTab) {
             browser()->tab_strip_model()->GetTabGroupForTab(grouped_tab_index));
 }
 
-IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindowWithGroupedTabs) {
-  base::test::ScopedFeatureList feature_override;
-  feature_override.InitAndEnableFeature(features::kTabGroups);
-
+IN_PROC_BROWSER_TEST_F(TabRestoreTestWithTabGroupsEnabled,
+                       RestoreWindowWithGroupedTabs) {
   ui_test_utils::NavigateToURLWithDisposition(
       browser(), GURL(chrome::kChromeUINewTabURL),
       WindowOpenDisposition::NEW_WINDOW,
@@ -962,7 +967,12 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindowWithGroupedTabs) {
 
 // Ensure tab groups aren't restored if |features::kTabGroups| is disabled.
 // Regression test for crbug.com/983962.
-IN_PROC_BROWSER_TEST_F(TabRestoreTest, GroupsNotRestoredWhenFeatureDisabled) {
+//
+// NOTE: This test is currently disabled because it fundamentally relies on
+// manipulating the FeatureList state mid-test, which is NOT safe and not
+// allowed by the FeatureList API.
+IN_PROC_BROWSER_TEST_F(TabRestoreTest,
+                       DISABLED_GroupsNotRestoredWhenFeatureDisabled) {
   auto feature_override = std::make_unique<base::test::ScopedFeatureList>();
   feature_override->InitAndEnableFeature(features::kTabGroups);
 

chrome/browser/subresource_filter/ruleset_browsertest.cc

@@ -186,23 +186,41 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
                                       1);
 }
 
-IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, LazyRulesetValidation) {
+class SubresourceFilterBrowserTestWithoutAdTagging
+    : public SubresourceFilterBrowserTest {
+ public:
+  SubresourceFilterBrowserTestWithoutAdTagging() {
+    feature_list_.InitAndDisableFeature(subresource_filter::kAdTagging);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTestWithoutAdTagging,
+                       LazyRulesetValidation) {
   // The ruleset shouldn't be validated until it's used, unless ad tagging is
   // enabled.
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndDisableFeature(subresource_filter::kAdTagging);
   SetRulesetToDisallowURLsWithPathSuffix("included_script.js");
   RulesetVerificationStatus dealer_status = GetRulesetVerification();
   EXPECT_EQ(RulesetVerificationStatus::kNotVerified, dealer_status);
 }
 
-IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+class SubresourceFilterBrowserTestWithAdTagging
+    : public SubresourceFilterBrowserTest {
+ public:
+  SubresourceFilterBrowserTestWithAdTagging() {
+    feature_list_.InitAndEnableFeature(subresource_filter::kAdTagging);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTestWithAdTagging,
                        AdsTaggingImmediateRulesetValidation) {
   // When Ads Tagging is enabled, the ruleset should be validated as soon as
   // it's published.
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(subresource_filter::kAdTagging);
-
   SetRulesetToDisallowURLsWithPathSuffix("included_script.js");
   RulesetVerificationStatus dealer_status = GetRulesetVerification();
   EXPECT_EQ(RulesetVerificationStatus::kIntact, dealer_status);

chrome/browser/subresource_filter/subresource_filter_browsertest.cc

@@ -724,11 +724,20 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
       tester, true /* expect_performance_measurements */);
 }
 
-IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+class SubresourceFilterBrowserTestWithoutAdTagging
+    : public SubresourceFilterBrowserTest {
+ public:
+  SubresourceFilterBrowserTestWithoutAdTagging() {
+    feature_list_.InitAndDisableFeature(kAdTagging);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+// This test only makes sense when AdTagging is disabled.
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTestWithoutAdTagging,
                        ExpectHistogramsNotRecordedWhenFilteringNotActivated) {
-  // This test only makes sense when AdTagging is disabled.
-  base::test::ScopedFeatureList scoped_tagging;
-  scoped_tagging.InitAndDisableFeature(kAdTagging);
   ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(
       "suffix-that-does-not-match-anything"));
   ResetConfigurationToEnableOnPhishingSites(true /* measure_performance */);

chrome/browser/subresource_filter/subresource_filter_devtools_browsertest.cc

@@ -153,14 +153,22 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterDevtoolsBrowserTest,
   console_observer.Wait();
 }
 
+class SubresourceFilterDevtoolsBrowserTestWithSitePerProcess
+    : public SubresourceFilterDevtoolsBrowserTest {
+ public:
+  SubresourceFilterDevtoolsBrowserTestWithSitePerProcess() {
+    feature_list_.InitAndEnableFeature(features::kSitePerProcess);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // See crbug.com/813197, where agent hosts from subframes could send messages to
 // disable ad blocking when they are detached (e.g. when the subframe goes
 // away).
-IN_PROC_BROWSER_TEST_F(SubresourceFilterDevtoolsBrowserTest,
+IN_PROC_BROWSER_TEST_F(SubresourceFilterDevtoolsBrowserTestWithSitePerProcess,
                        IsolatedSubframe_DoesNotSendAdBlockingMessages) {
-  base::test::ScopedFeatureList scoped_isolation;
-  scoped_isolation.InitAndEnableFeature(features::kSitePerProcess);
-
   ASSERT_NO_FATAL_FAILURE(
       SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
   ScopedDevtoolsOpener page_opener(web_contents());

chrome/browser/subresource_filter/subresource_filter_intercepting_browsertest.cc

@@ -176,14 +176,24 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterInterceptingBrowserTest,
   EXPECT_GE(timer.Elapsed(), delay);
 }
 
+class SubresourceFilterInterceptingBrowserTestConsiderRedirects
+    : public SubresourceFilterInterceptingBrowserTest {
+ public:
+  SubresourceFilterInterceptingBrowserTestConsiderRedirects() {
+    feature_list_.InitAndEnableFeature(
+        kSafeBrowsingSubresourceFilterConsiderRedirects);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Verify that the correct safebrowsing result is reported when there is a
 // redirect chain. With kSafeBrowsingSubresourceFilterConsiderRedirects, the
 // result with the highest priority should be returned.
-IN_PROC_BROWSER_TEST_F(SubresourceFilterInterceptingBrowserTest,
-                       SafeBrowsingNotificationsCheckBest) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      kSafeBrowsingSubresourceFilterConsiderRedirects);
+IN_PROC_BROWSER_TEST_F(
+    SubresourceFilterInterceptingBrowserTestConsiderRedirects,
+    SafeBrowsingNotificationsCheckBest) {
   ASSERT_NO_FATAL_FAILURE(
       SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
   GURL redirect_url(embedded_test_server()->GetURL(
@@ -194,14 +204,24 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterInterceptingBrowserTest,
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
 }
 
+class SubresourceFilterInterceptingBrowserTestDontConsiderRedirects
+    : public SubresourceFilterInterceptingBrowserTest {
+ public:
+  SubresourceFilterInterceptingBrowserTestDontConsiderRedirects() {
+    feature_list_.InitAndDisableFeature(
+        kSafeBrowsingSubresourceFilterConsiderRedirects);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // Verify that the correct safebrowsing result is reported when there is a
 // redirect chain. Without kSafeBrowsingSubresourceFilterConsiderRedirects, the
 // last result should be used.
-IN_PROC_BROWSER_TEST_F(SubresourceFilterInterceptingBrowserTest,
-                       SafeBrowsingNotificationsCheckLastResult) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(
-      kSafeBrowsingSubresourceFilterConsiderRedirects);
+IN_PROC_BROWSER_TEST_F(
+    SubresourceFilterInterceptingBrowserTestDontConsiderRedirects,
+    SafeBrowsingNotificationsCheckLastResult) {
   ASSERT_NO_FATAL_FAILURE(
       SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
   GURL redirect_url(embedded_test_server()->GetURL(

chrome/browser/subresource_filter/subresource_filter_popup_browsertest.cc

@@ -94,9 +94,7 @@ const char kSubresourceFilterActionsHistogram[] = "SubresourceFilter.Actions2";
 // Tests that subresource_filter interacts well with the abusive enforcement in
 // chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.
 class SubresourceFilterPopupBrowserTest
-    : public SubresourceFilterListInsertingBrowserTest,
-      public ::testing::WithParamInterface<
-          bool /* enable_adblock_on_abusive_sites */> {
+    : public SubresourceFilterListInsertingBrowserTest {
  public:
   void SetUpOnMainThread() override {
     SubresourceFilterBrowserTest::SetUpOnMainThread();
@@ -169,13 +167,24 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterPopupBrowserTest,
                                embedded_test_server()->GetURL("/title1.html"));
 }
 
-IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest,
+class SubresourceFilterPopupBrowserTestWithParam
+    : public SubresourceFilterPopupBrowserTest,
+      public ::testing::WithParamInterface<
+          bool /* enable_adblock_on_abusive_sites */> {
+ public:
+  SubresourceFilterPopupBrowserTestWithParam() {
+    const bool enable_adblock_on_abusive_sites = GetParam();
+    feature_list_.InitWithFeatureState(
+        subresource_filter::kFilterAdsOnAbusiveSites,
+        enable_adblock_on_abusive_sites);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTestWithParam,
                        BlockCreatingNewWindows) {
-  bool enable_adblock_on_abusive_sites = GetParam();
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatureState(
-      subresource_filter::kFilterAdsOnAbusiveSites,
-      enable_adblock_on_abusive_sites);
   base::HistogramTester tester;
   const char kWindowOpenPath[] = "/subresource_filter/window_open.html";
   GURL a_url(embedded_test_server()->GetURL("a.com", kWindowOpenPath));
@@ -203,6 +212,7 @@ IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest,
                                                    &opened_window));
   EXPECT_FALSE(opened_window);
 
+  const bool enable_adblock_on_abusive_sites = GetParam();
   EXPECT_EQ(enable_adblock_on_abusive_sites, AreDisallowedRequestsBlocked());
 
   // Navigate to |b_url|, which should successfully open the popup.
@@ -331,12 +341,8 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterPopupBrowserTest,
       web_contents(), {kActivationConsoleMessage}, {kAbusiveEnforceMessage});
 }
 
-IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest, BlockOpenURLFromTab) {
-  bool enable_adblock_on_abusive_sites = GetParam();
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatureState(
-      subresource_filter::kFilterAdsOnAbusiveSites,
-      enable_adblock_on_abusive_sites);
+IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTestWithParam,
+                       BlockOpenURLFromTab) {
   base::HistogramTester tester;
   const char kWindowOpenPath[] =
       "/subresource_filter/window_open_spoof_click.html";
@@ -356,6 +362,7 @@ IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest, BlockOpenURLFromTab) {
 
   EXPECT_TRUE(TabSpecificContentSettings::FromWebContents(web_contents)
                   ->IsContentBlocked(CONTENT_SETTINGS_TYPE_POPUPS));
+  const bool enable_adblock_on_abusive_sites = GetParam();
   EXPECT_EQ(enable_adblock_on_abusive_sites, AreDisallowedRequestsBlocked());
 
   // Navigate to |b_url|, which should successfully open the popup.
@@ -373,13 +380,8 @@ IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest, BlockOpenURLFromTab) {
   EXPECT_FALSE(AreDisallowedRequestsBlocked());
 }
 
-IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest,
+IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTestWithParam,
                        BlockOpenURLFromTabInIframe) {
-  bool enable_adblock_on_abusive_sites = GetParam();
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatureState(
-      subresource_filter::kFilterAdsOnAbusiveSites,
-      enable_adblock_on_abusive_sites);
   const char popup_path[] = "/subresource_filter/iframe_spoof_click_popup.html";
   GURL a_url(embedded_test_server()->GetURL("a.com", popup_path));
   ConfigureAsAbusiveAndBetterAds(
@@ -396,16 +398,12 @@ IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest,
   EXPECT_TRUE(sent_open);
   EXPECT_TRUE(TabSpecificContentSettings::FromWebContents(web_contents)
                   ->IsContentBlocked(CONTENT_SETTINGS_TYPE_POPUPS));
+  const bool enable_adblock_on_abusive_sites = GetParam();
   EXPECT_EQ(enable_adblock_on_abusive_sites, AreDisallowedRequestsBlocked());
 }
 
-IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest,
+IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTestWithParam,
                        TraditionalWindowOpen_NotBlocked) {
-  bool enable_adblock_on_abusive_sites = GetParam();
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatureState(
-      subresource_filter::kFilterAdsOnAbusiveSites,
-      enable_adblock_on_abusive_sites);
   GURL url(GetTestUrl("/title2.html"));
   ConfigureAsAbusiveAndBetterAds(
       url, SubresourceFilterLevel::ENFORCE /* abusive_level */,
@@ -422,11 +420,12 @@ IN_PROC_BROWSER_TEST_P(SubresourceFilterPopupBrowserTest,
       browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_FALSE(TabSpecificContentSettings::FromWebContents(web_contents)
                    ->IsContentBlocked(CONTENT_SETTINGS_TYPE_POPUPS));
+  const bool enable_adblock_on_abusive_sites = GetParam();
   EXPECT_EQ(enable_adblock_on_abusive_sites, AreDisallowedRequestsBlocked());
 }
 
 INSTANTIATE_TEST_SUITE_P(/* no prefix */,
-                         SubresourceFilterPopupBrowserTest,
+                         SubresourceFilterPopupBrowserTestWithParam,
                          ::testing::Values(false, true));
 
 }  // namespace subresource_filter