Fix misc ScopedFeatureList usage (1/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 media, geolocation, and permissions
related code.

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: I48856217d97c1b5d0acf44c4b36ebb138a3beee8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1850731Reviewed-by: Tim Volodine <timvolodine@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#705221}

chrome/browser/geolocation/geolocation_browsertest.cc

@@ -538,14 +538,23 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, NoLeakFromOffTheRecord) {
   ExpectPosition(fake_latitude(), fake_longitude());
 }
 
-IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, IFramesWithFreshPosition) {
-  // When permission delegation is enabled, there isn't a way to have a pending
-  // permission prompt when permission has already been granted in another frame
-  // on the same page. That means that this test isn't relevant and can be
-  // deleted after the feature is enabled by default.
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(features::kPermissionDelegation);
+// When permission delegation is enabled, there isn't a way to have a pending
+// permission prompt when permission has already been granted in another frame
+// on the same page. That means that once the feature is enabled by default,
+// tests which use this fixture are no longer relevant and can be deleted.
+class GeolocationBrowserTestWithNoPermissionDelegation
+    : public GeolocationBrowserTest {
+ public:
+  GeolocationBrowserTestWithNoPermissionDelegation() {
+    feature_list_.InitAndDisableFeature(features::kPermissionDelegation);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
 
+IN_PROC_BROWSER_TEST_F(GeolocationBrowserTestWithNoPermissionDelegation,
+                       IFramesWithFreshPosition) {
   set_html_for_tests("/geolocation/two_iframes.html");
   ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
   LoadIFrames();
@@ -599,12 +608,8 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, IFramesWithCachedPosition) {
   ExpectPosition(cached_position_latitude, cached_position_lognitude);
 }
 
-IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, CancelPermissionForFrame) {
-  // When permission delegation is removed, iframe requests are made for the top
-  // level frame. Navigating the iframe should not cancel the request. This
-  // test can be removed after the feature is enabled by default.
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(features::kPermissionDelegation);
+IN_PROC_BROWSER_TEST_F(GeolocationBrowserTestWithNoPermissionDelegation,
+                       CancelPermissionForFrame) {
   set_html_for_tests("/geolocation/two_iframes.html");
   ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
   LoadIFrames();

chrome/browser/media/media_engagement_autoplay_browsertest.cc

@@ -68,6 +68,13 @@ class MediaEngagementAutoplayBrowserTest
     http_server_.ServeFilesFromSourceDirectory(kMediaEngagementTestDataPath);
     http_server_origin2_.ServeFilesFromSourceDirectory(
         kMediaEngagementTestDataPath);
+
+    // Enable or disable MEI based on the test parameter.
+    if (GetParam()) {
+      scoped_feature_list_.InitWithFeatures(kFeatures, {});
+    } else {
+      scoped_feature_list_.InitWithFeatures({}, kFeatures);
+    }
   }
 
   ~MediaEngagementAutoplayBrowserTest() override = default;
@@ -82,13 +89,6 @@ class MediaEngagementAutoplayBrowserTest
     ASSERT_TRUE(http_server_.Start());
     ASSERT_TRUE(http_server_origin2_.Start());
 
-    // Enable or disable MEI based on the test parameter.
-    if (GetParam()) {
-      scoped_feature_list_.InitWithFeatures(kFeatures, {});
-    } else {
-      scoped_feature_list_.InitWithFeatures({}, kFeatures);
-    }
-
     InProcessBrowserTest::SetUp();
 
     // Clear any preloaded MEI data.
@@ -367,11 +367,19 @@ IN_PROC_BROWSER_TEST_P(MediaEngagementAutoplayBrowserTest, TopFrameNavigation) {
   ExpectAutoplayAllowedIfEnabled();
 }
 
-IN_PROC_BROWSER_TEST_P(MediaEngagementAutoplayBrowserTest,
-                       BypassAutoplayHighEngagement_HTTPSOnly) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(media::kMediaEngagementHTTPSOnly);
+class MediaEngagementAutoplayBrowserTestHttpsOnly
+    : public MediaEngagementAutoplayBrowserTest {
+ public:
+  MediaEngagementAutoplayBrowserTestHttpsOnly() {
+    feature_list_.InitAndEnableFeature(media::kMediaEngagementHTTPSOnly);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
 
+IN_PROC_BROWSER_TEST_P(MediaEngagementAutoplayBrowserTestHttpsOnly,
+                       BypassAutoplayHighEngagement) {
   SetScores(PrimaryOrigin(), 20, 20);
   LoadTestPage("engagement_autoplay_test.html");
   ExpectAutoplayDenied();
@@ -380,3 +388,7 @@ IN_PROC_BROWSER_TEST_P(MediaEngagementAutoplayBrowserTest,
 INSTANTIATE_TEST_SUITE_P(/* no prefix */,
                          MediaEngagementAutoplayBrowserTest,
                          testing::Bool());
+
+INSTANTIATE_TEST_SUITE_P(/* no prefix */,
+                         MediaEngagementAutoplayBrowserTestHttpsOnly,
+                         testing::Bool());

chrome/browser/media/unified_autoplay_browsertest.cc

@@ -60,11 +60,13 @@ class ChromeContentBrowserClientOverrideWebAppScope
 // conflict with "AutoplayBrowserTest" in extensions code.
 class UnifiedAutoplayBrowserTest : public InProcessBrowserTest {
  public:
+  UnifiedAutoplayBrowserTest() {
+    scoped_feature_list_.InitAndEnableFeature(media::kUnifiedAutoplay);
+  }
+
   ~UnifiedAutoplayBrowserTest() override = default;
 
   void SetUpOnMainThread() override {
-    scoped_feature_list_.InitAndEnableFeature(media::kUnifiedAutoplay);
-
     host_resolver()->AddRule("*", "127.0.0.1");
     ASSERT_TRUE(embedded_test_server()->Start());
 
@@ -417,13 +419,16 @@ IN_PROC_BROWSER_TEST_F(UnifiedAutoplayBrowserTest,
 
 class UnifiedAutoplaySettingBrowserTest : public UnifiedAutoplayBrowserTest {
  public:
+  UnifiedAutoplaySettingBrowserTest() {
+    scoped_feature_list_.InitWithFeatures(
+        {media::kAutoplayDisableSettings, media::kAutoplayWhitelistSettings},
+        {});
+  }
+
   ~UnifiedAutoplaySettingBrowserTest() override = default;
 
   void SetUpOnMainThread() override {
     UnifiedAutoplayBrowserTest::SetUpOnMainThread();
-    scoped_feature_list_.InitWithFeatures(
-        {media::kAutoplayDisableSettings, media::kAutoplayWhitelistSettings},
-        {});
   }
 
   bool AutoplayAllowed(const content::ToRenderFrameHost& adapter) {

chrome/browser/permissions/permission_delegation_browsertest.cc

@@ -21,11 +21,13 @@ class PermissionDelegationBrowserTest : public InProcessBrowserTest {
  public:
   PermissionDelegationBrowserTest()
       : geolocation_overrider_(
-            std::make_unique<device::ScopedGeolocationOverrider>(0, 0)) {}
+            std::make_unique<device::ScopedGeolocationOverrider>(0, 0)) {
+    scoped_feature_list_.InitAndEnableFeature(features::kPermissionDelegation);
+  }
+
   ~PermissionDelegationBrowserTest() override = default;
 
   void SetUpOnMainThread() override {
-    scoped_feature_list_.InitAndEnableFeature(features::kPermissionDelegation);
     PermissionRequestManager* manager =
         PermissionRequestManager::FromWebContents(GetWebContents());
     mock_permission_prompt_factory_.reset(

chrome/browser/permissions/permission_request_manager_browsertest.cc

@@ -52,15 +52,17 @@ const char kPermissionsKillSwitchTestGroup[] = "TestGroup";
 
 class PermissionRequestManagerBrowserTest : public InProcessBrowserTest {
  public:
-  PermissionRequestManagerBrowserTest() = default;
+  PermissionRequestManagerBrowserTest() {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kBlockRepeatedNotificationPermissionPrompts);
+  }
+
   ~PermissionRequestManagerBrowserTest() override = default;
 
   void SetUpOnMainThread() override {
     PermissionRequestManager* manager = GetPermissionRequestManager();
     mock_permission_prompt_factory_.reset(
         new MockPermissionPromptFactory(manager));
-    scoped_feature_list_.InitAndEnableFeature(
-        features::kBlockRepeatedNotificationPermissionPrompts);
 
     host_resolver()->AddRule("*", "127.0.0.1");
   }

content/browser/media/session/media_session_browsertest.cc

@@ -75,11 +75,11 @@ class MediaImageGetterHelper {
 // Integration tests for content::MediaSession that do not take into
 // consideration the implementation details contrary to
 // MediaSessionImplBrowserTest.
-class MediaSessionBrowserTest : public ContentBrowserTest {
+class MediaSessionBrowserTestBase : public ContentBrowserTest {
  public:
-  MediaSessionBrowserTest() {
+  MediaSessionBrowserTestBase() {
     embedded_test_server()->RegisterRequestMonitor(base::BindRepeating(
-        &MediaSessionBrowserTest::OnServerRequest, base::Unretained(this)));
+        &MediaSessionBrowserTestBase::OnServerRequest, base::Unretained(this)));
   }
 
   void SetUp() override {
@@ -91,14 +91,6 @@ class MediaSessionBrowserTest : public ContentBrowserTest {
     command_line->AppendSwitchASCII(
         switches::kAutoplayPolicy,
         switches::autoplay::kNoUserGestureRequiredPolicy);
-
-    scoped_feature_list_.InitAndEnableFeature(media::kInternalMediaSession);
-  }
-
-  void DisableInternalMediaSession() {
-    disabled_feature_list_.InitWithFeatures(
-        {}, {media::kInternalMediaSession,
-             media_session::features::kMediaSessionService});
   }
 
   void StartPlaybackAndWait(Shell* shell, const std::string& id) {
@@ -173,10 +165,6 @@ class MediaSessionBrowserTest : public ContentBrowserTest {
     return embedded_test_server()->GetURL(kMediaSessionTestImagePath);
   }
 
- protected:
-  base::test::ScopedFeatureList scoped_feature_list_;
-  base::test::ScopedFeatureList disabled_feature_list_;
-
  private:
   void OnServerRequest(const net::test_server::HttpRequest& request) {
     // Note this method is called on the EmbeddedTestServer's background thread.
@@ -190,31 +178,57 @@ class MediaSessionBrowserTest : public ContentBrowserTest {
   base::Lock visited_urls_lock_;
   std::set<GURL> visited_urls_;
 
-  DISALLOW_COPY_AND_ASSIGN(MediaSessionBrowserTest);
+  DISALLOW_COPY_AND_ASSIGN(MediaSessionBrowserTestBase);
 };
 
-// A MediaSessionBrowserTest with BackForwardCache enabled.
-class MediaSessionBrowserTestWithBackForwardCache
-    : public MediaSessionBrowserTest {
-  void SetUpOnMainThread() override {
-    host_resolver()->AddRule("*", "127.0.0.1");
-    MediaSessionBrowserTest::SetUpOnMainThread();
+class MediaSessionBrowserTest : public MediaSessionBrowserTestBase {
+ public:
+  MediaSessionBrowserTest() {
+    feature_list_.InitAndEnableFeature(media::kInternalMediaSession);
   }
 
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    scoped_feature_list_.InitWithFeaturesAndParameters(
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+class MediaSessionBrowserTestWithoutInternalMediaSession
+    : public MediaSessionBrowserTestBase {
+ public:
+  MediaSessionBrowserTestWithoutInternalMediaSession() {
+    disabled_feature_list_.InitWithFeatures(
+        {}, {media::kInternalMediaSession,
+             media_session::features::kMediaSessionService});
+  }
+
+ private:
+  base::test::ScopedFeatureList disabled_feature_list_;
+};
+
+// A MediaSessionBrowserTest with BackForwardCache enabled.
+class MediaSessionBrowserTestWithBackForwardCache
+    : public MediaSessionBrowserTestBase {
+ public:
+  MediaSessionBrowserTestWithBackForwardCache() {
+    feature_list_.InitWithFeaturesAndParameters(
         {{features::kBackForwardCache,
           {{"TimeToLiveInBackForwardCacheInSeconds", "3600"}}},
          {media::kInternalMediaSession, {}}},
         /*disabled_features=*/{});
   }
+
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    MediaSessionBrowserTestBase::SetUpOnMainThread();
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
 };
 
 }  // anonymous namespace
 
-IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest, MediaSessionNoOpWhenDisabled) {
-  DisableInternalMediaSession();
-
+IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTestWithoutInternalMediaSession,
+                       MediaSessionNoOpWhenDisabled) {
   EXPECT_TRUE(NavigateToURL(shell(),
                             GetTestUrl("media/session", "media-session.html")));