Fix misc ScopedFeatureList usage (4/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 Safe Browsing, SSL, and Policy related
browser tests.

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: Ie1d4e9241c53e0e8fe4ec504508f3d51dc9c1d6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1850733
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704992}

chrome/browser/policy/policy_browsertest.cc

@@ -4831,7 +4831,11 @@ class NetworkTimePolicyTest : public PolicyTest {
   DISALLOW_COPY_AND_ASSIGN(NetworkTimePolicyTest);
 };
 
-IN_PROC_BROWSER_TEST_F(NetworkTimePolicyTest, NetworkTimeQueriesDisabled) {
+// TODO(https://crbug.com/1012853): This test is using ScopedFeatureList
+// incorrectly, and fixing it causes conflicts with PolicyTest's use of the
+// deprecated variations API.
+IN_PROC_BROWSER_TEST_F(NetworkTimePolicyTest,
+                       DISABLED_NetworkTimeQueriesDisabled) {
   // Set a policy to disable network time queries.
   PolicyMap policies;
   policies.Set(key::kBrowserNetworkTimeQueriesEnabled, POLICY_LEVEL_MANDATORY,
@@ -5713,13 +5717,21 @@ IN_PROC_BROWSER_TEST_F(SignedExchangePolicyTest, SignedExchangeEnabled) {
   ASSERT_TRUE(HadSignedExchangeInAcceptHeader(url));
 }
 
-IN_PROC_BROWSER_TEST_F(PolicyTest, CheckURLsInRealTime) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatures(
-      {safe_browsing::kRealTimeUrlLookupEnabled,
-       safe_browsing::kRealTimeUrlLookupFetchAllowlist},
-      {});
+class PolicyTestWithRealTimeUrlLookupFetchAllowList : public PolicyTest {
+ public:
+  PolicyTestWithRealTimeUrlLookupFetchAllowList() {
+    feature_list_.InitWithFeatures(
+        {safe_browsing::kRealTimeUrlLookupEnabled,
+         safe_browsing::kRealTimeUrlLookupFetchAllowlist},
+        {});
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
 
+IN_PROC_BROWSER_TEST_F(PolicyTestWithRealTimeUrlLookupFetchAllowList,
+                       CheckURLsInRealTime) {
   EXPECT_FALSE(safe_browsing::RealTimePolicyEngine::CanPerformFullURLLookup(
       browser()->profile()));
 

chrome/browser/safe_browsing/ad_redirect_trigger_browsertest.cc

@@ -32,12 +32,12 @@ namespace safe_browsing {
 class AdRedirectTriggerBrowserTest : public InProcessBrowserTest,
                                      public UrlListManager::Observer {
  public:
-  AdRedirectTriggerBrowserTest() = default;
+  AdRedirectTriggerBrowserTest() {
+    scoped_feature_list_.InitAndEnableFeature(kAdRedirectTriggerFeature);
+  }
 
   // InProcessBrowserTest:
   void SetUpOnMainThread() override {
-    scoped_feature_list_.InitAndEnableFeature(kAdRedirectTriggerFeature);
-
     host_resolver()->AddRule("*", "127.0.0.1");
     ASSERT_TRUE(embedded_test_server()->Start());
     current_browser_ = InProcessBrowserTest::browser();

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -1856,17 +1856,34 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
   EXPECT_FALSE(IsShowingInterstitial(contents));
 }
 
+INSTANTIATE_TEST_SUITE_P(
+    SafeBrowsingBlockingPageBrowserTestWithThreatTypeAndIsolationSetting,
+    SafeBrowsingBlockingPageBrowserTest,
+    testing::Combine(
+        testing::Values(SB_THREAT_TYPE_URL_MALWARE,  // Threat types
+                        SB_THREAT_TYPE_URL_PHISHING,
+                        SB_THREAT_TYPE_URL_UNWANTED),
+        testing::Bool()));  // If isolate all sites for testing.
+
+class SafeBrowsingBlockingPageBrowserTestWithCommittedSBInterstitials
+    : public SafeBrowsingBlockingPageBrowserTest {
+ public:
+  SafeBrowsingBlockingPageBrowserTestWithCommittedSBInterstitials() {
+    feature_list_.InitAndEnableFeature(kCommittedSBInterstitials);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 // TODO(crbug.com/916683): Once interstitial bindings are hooked with committed
 // interstitials, all other tests should run with committed interstitials
 // enabled. At that point this test will become redundant and should be removed.
 // Test that an main frame interstitial is displayed with committed
 // interstitials enabled.
-IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
-                       CommittedInterstitialShows) {
-  base::test::ScopedFeatureList feature_list;
-  std::vector<base::Feature> enable;
-  enable.push_back(kCommittedSBInterstitials);
-  feature_list.InitWithFeatures(enable, std::vector<base::Feature>());
+IN_PROC_BROWSER_TEST_P(
+    SafeBrowsingBlockingPageBrowserTestWithCommittedSBInterstitials,
+    CommittedInterstitialShows) {
   SetupWarningAndNavigate(browser());
   EXPECT_TRUE(IsShowingInterstitial(
       browser()->tab_strip_model()->GetActiveWebContents()));
@@ -1874,7 +1891,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 
 INSTANTIATE_TEST_SUITE_P(
     SafeBrowsingBlockingPageBrowserTestWithThreatTypeAndIsolationSetting,
-    SafeBrowsingBlockingPageBrowserTest,
+    SafeBrowsingBlockingPageBrowserTestWithCommittedSBInterstitials,
     testing::Combine(
         testing::Values(SB_THREAT_TYPE_URL_MALWARE,  // Threat types
                         SB_THREAT_TYPE_URL_PHISHING,

chrome/browser/ssl/chrome_expect_ct_reporter_browsertest.cc

@@ -29,6 +29,11 @@ namespace {
 class ExpectCTBrowserTest : public CertVerifierBrowserTest {
  public:
   ExpectCTBrowserTest() : CertVerifierBrowserTest() {
+    feature_list_.InitWithFeatures(
+        {network::features::kExpectCTReporting,
+         net::TransportSecurityState::kDynamicExpectCTFeature},
+        {});
+
     // Expect-CT reporting depends on actually enforcing Certificate
     // Transparency.
     SystemNetworkContextManager::SetEnableCertificateTransparencyForTesting(
@@ -107,6 +112,8 @@ class ExpectCTBrowserTest : public CertVerifierBrowserTest {
   void set_report_uri(const GURL& report_uri) { report_uri_ = report_uri; }
 
  private:
+  base::test::ScopedFeatureList feature_list_;
+
   std::unique_ptr<base::RunLoop> run_loop_;
   // The report-uri value to use in the Expect-CT header for requests handled by
   // ExpectCTHeaderRequestHandler.
@@ -118,12 +125,6 @@ class ExpectCTBrowserTest : public CertVerifierBrowserTest {
 // Tests that an Expect-CT reporter is properly set up and used for violations
 // of Expect-CT HTTP headers.
 IN_PROC_BROWSER_TEST_F(ExpectCTBrowserTest, TestDynamicExpectCTReporting) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatures(
-      {network::features::kExpectCTReporting,
-       net::TransportSecurityState::kDynamicExpectCTFeature},
-      {});
-
   net::EmbeddedTestServer report_server;
   report_server.RegisterRequestHandler(base::Bind(
       &ExpectCTBrowserTest::ReportRequestHandler, base::Unretained(this)));
@@ -161,12 +162,6 @@ IN_PROC_BROWSER_TEST_F(ExpectCTBrowserTest, TestDynamicExpectCTReporting) {
 // Tests that Expect-CT HTTP headers are processed correctly.
 IN_PROC_BROWSER_TEST_F(ExpectCTBrowserTest,
                        TestDynamicExpectCTHeaderProcessing) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatures(
-      {network::features::kExpectCTReporting,
-       net::TransportSecurityState::kDynamicExpectCTFeature},
-      {});
-
   net::EmbeddedTestServer test_server(net::EmbeddedTestServer::TYPE_HTTPS);
   test_server.RegisterRequestHandler(
       base::Bind(&ExpectCTBrowserTest::ExpectCTHeaderRequestHandler,

chrome/browser/ssl/connection_help_tab_helper_browsertest.cc

@@ -21,9 +21,9 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
-class ConnectionHelpTabHelperTest : public InProcessBrowserTest {
+class ConnectionHelpTabHelperTestBase : public InProcessBrowserTest {
  public:
-  ConnectionHelpTabHelperTest()
+  ConnectionHelpTabHelperTestBase()
       : https_server_(net::EmbeddedTestServer::TYPE_HTTPS),
         https_expired_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}
 
@@ -52,8 +52,29 @@ class ConnectionHelpTabHelperTest : public InProcessBrowserTest {
  private:
   net::EmbeddedTestServer https_server_;
   net::EmbeddedTestServer https_expired_server_;
-  base::test::ScopedFeatureList scoped_feature_list_;
-  DISALLOW_COPY_AND_ASSIGN(ConnectionHelpTabHelperTest);
+  DISALLOW_COPY_AND_ASSIGN(ConnectionHelpTabHelperTestBase);
+};
+
+class ConnectionHelpTabHelperTest : public ConnectionHelpTabHelperTestBase {
+ public:
+  ConnectionHelpTabHelperTest() {
+    feature_list_.InitAndEnableFeature(features::kBundledConnectionHelpFeature);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+class ConnectionHelpTabHelperTestWithFeatureDisabled
+    : public ConnectionHelpTabHelperTestBase {
+ public:
+  ConnectionHelpTabHelperTestWithFeatureDisabled() {
+    feature_list_.InitAndDisableFeature(
+        features::kBundledConnectionHelpFeature);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
 };
 
 // Tests that the chrome://connection-help redirect is not triggered (and
@@ -63,8 +84,6 @@ IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest,
                        InterstitialOnNonSupportURL) {
   const char kHistogramName[] = "SSL.CertificateErrorHelpCenterVisited";
   base::HistogramTester histograms;
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(features::kBundledConnectionHelpFeature);
 
   GURL expired_non_support_url = https_expired_server()->GetURL("/title2.html");
   GURL good_support_url = https_server()->GetURL("/title2.html");
@@ -84,8 +103,6 @@ IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest,
                        SupportURLWithNoInterstitial) {
   const char kHistogramName[] = "SSL.CertificateErrorHelpCenterVisited";
   base::HistogramTester histograms;
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(features::kBundledConnectionHelpFeature);
 
   GURL good_support_url = https_server()->GetURL("/title2.html");
   SetHelpCenterUrl(browser(), good_support_url);
@@ -105,8 +122,6 @@ IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest,
 IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest, InterstitialOnSupportURL) {
   const char kHistogramName[] = "SSL.CertificateErrorHelpCenterVisited";
   base::HistogramTester histograms;
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(features::kBundledConnectionHelpFeature);
 
   GURL expired_url = https_expired_server()->GetURL("/title2.html");
   SetHelpCenterUrl(browser(), expired_url);
@@ -126,12 +141,10 @@ IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest, InterstitialOnSupportURL) {
 
 // Tests that histogram logs correctly when an interstitial is triggered on the
 // support URL if the feature is disabled.
-IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest,
-                       InterstitialOnSupportURLWithFeatureDisabled) {
+IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTestWithFeatureDisabled,
+                       InterstitialOnSupportURL) {
   const char kHistogramName[] = "SSL.CertificateErrorHelpCenterVisited";
   base::HistogramTester histograms;
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndDisableFeature(features::kBundledConnectionHelpFeature);
 
   GURL expired_url = https_expired_server()->GetURL("/title2.html");
   SetHelpCenterUrl(browser(), expired_url);
@@ -166,9 +179,6 @@ IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest, NetworkErrorOnSupportURL) {
 // expanded.
 IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest,
                        CorrectlyExpandsCertErrorSection) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(features::kBundledConnectionHelpFeature);
-
   GURL expired_url = https_expired_server()->GetURL("/title2.html#-200");
   GURL::Replacements replacements;
   replacements.ClearRef();
@@ -198,9 +208,6 @@ IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest,
 // to an expired certificate, the clock section is automatically expanded.
 IN_PROC_BROWSER_TEST_F(ConnectionHelpTabHelperTest,
                        CorrectlyExpandsClockSection) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(features::kBundledConnectionHelpFeature);
-
   GURL expired_url = https_expired_server()->GetURL("/title2.html#-201");
   GURL::Replacements replacements;
   replacements.ClearRef();