Lock SchemeRegistry on first use.

This change locks the SchemeRegistry on first use, which required
refactoring url_util.cc to differentiate between SchemeRegistry use for
adding Schemes, and SchemeRegistry use for using Schemes.

Tests can now only modify schemes after initialization by calling
url::UnlockForTests(), which creates a scoped object that resets the
schemes, so tests can't mistakenly leave global scheme state modified.

This change required changes to browser startup to decouple scheme
registration from ContentMain, found here:
https://chromium-review.googlesource.com/c/chromium/src/+/1945926

Doc: https://docs.google.com/document/d/1kDKqBaq-b6EbUm0F4ea7ARoksUcj1KUx3qxFuSXEwM4/edit

Bug: 783819
Change-Id: Idcac37ccae297735176eba20b50ced3f9e63f9db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1901591
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738544}

android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java

@@ -83,6 +83,10 @@ private void startChromiumWithClient(TestAwContentsClient contentsClient) {
     public void testStartup() throws Throwable {
         TestWebServer webServer = TestWebServer.start();
         try {
+            // This must come before usages of the AwCookieManager as this call initializes the
+            // scheme registry that the AwCookieManager uses (indirectly).
+            startChromium();
+
             String path = "/cookie_test.html";
             String url = webServer.setResponse(path, CommonResources.ABOUT_HTML, null);
 
@@ -97,7 +101,6 @@ public void testStartup() throws Throwable {
 
             cookieManager.setCookie(url, "count=41");
 
-            startChromium();
             mActivityTestRule.loadUrlSync(
                     mAwContents, mContentsClient.getOnPageFinishedHelper(), url);
             mActivityTestRule.executeJavaScriptAndWaitForResult(mAwContents, mContentsClient,

chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc

@@ -16,7 +16,6 @@
 #include "storage/browser/file_system/external_mount_points.h"
 #include "storage/browser/file_system/file_system_url.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "url/url_util.h"
 
 #define FPL(x) FILE_PATH_LITERAL(x)
 
@@ -108,8 +107,6 @@ TEST(ChromeOSFileSystemBackendTest, GetRootDirectories) {
 }
 
 TEST(ChromeOSFileSystemBackendTest, AccessPermissions) {
-  url::AddStandardScheme(extensions::kExtensionScheme, url::SCHEME_WITH_HOST);
-
   scoped_refptr<storage::ExternalMountPoints> mount_points(
       storage::ExternalMountPoints::CreateRefCounted());
   scoped_refptr<storage::ExternalMountPoints> system_mount_points(

chrome/browser/prerender/isolated/isolated_prerender_service_workers_observer_unittest.cc

@@ -10,20 +10,19 @@
 #include "url/gurl.h"
 #include "url/origin.h"
 
-namespace {
-const GURL kTestURL("https://test.com/path?foo=bar");
-const GURL kOtherURL("https://other.com/path?what=ever");
-const url::Origin kTestOrigin(url::Origin::Create(kTestURL));
-const url::Origin kOtherOrigin(url::Origin::Create(kOtherURL));
-}  // namespace
-
 TEST(IsolatedPrerenderServiceWorkersObserverTest, NotReady) {
+  const url::Origin kTestOrigin(
+      url::Origin::Create(GURL("https://test.com/path?foo=bar")));
   IsolatedPrerenderServiceWorkersObserver observer(nullptr);
   EXPECT_EQ(base::nullopt,
             observer.IsServiceWorkerRegisteredForOrigin(kTestOrigin));
 }
 
 TEST(IsolatedPrerenderServiceWorkersObserverTest, UsageInfoCallback) {
+  const url::Origin kTestOrigin(
+      url::Origin::Create(GURL("https://test.com/path?foo=bar")));
+  const url::Origin kOtherOrigin(
+      url::Origin::Create(GURL("https://other.com/path?what=ever")));
   IsolatedPrerenderServiceWorkersObserver observer(nullptr);
 
   content::StorageUsageInfo info{kTestOrigin, /*total_size_bytes=*/0,
@@ -37,6 +36,8 @@ TEST(IsolatedPrerenderServiceWorkersObserverTest, UsageInfoCallback) {
 }
 
 TEST(IsolatedPrerenderServiceWorkersObserverTest, OnRegistration) {
+  const GURL kTestURL("https://test.com/path?foo=bar");
+  const url::Origin kTestOrigin(url::Origin::Create(kTestURL));
   IsolatedPrerenderServiceWorkersObserver observer(nullptr);
   observer.CallOnHasUsageInfoForTesting({});
   EXPECT_EQ(base::Optional<bool>(false),

chrome/browser/ui/app_list/search/search_result_ranker/app_launch_event_logger_unittest.cc

@@ -23,7 +23,6 @@
 #include "services/metrics/public/mojom/ukm_interface.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
-#include "url/url_util.h"
 
 namespace app_list {
 
@@ -133,7 +132,6 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeChrome) {
 
   GURL url(std::string("chrome-extension://") + kGmailChromeApp + "/");
 
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
   test_ukm_recorder_.SetIsWebstoreExtensionCallback(
       base::BindRepeating(&TestIsWebstoreExtension));
 

chrome/test/base/chrome_unit_test_suite.cc

@@ -108,6 +108,10 @@ void ChromeUnitTestSuite::Initialize() {
       testing::UnitTest::GetInstance()->listeners();
   listeners.Append(new ChromeUnitTestSuiteInitializer);
 
+  {
+    ChromeContentClient content_client;
+    RegisterContentSchemes(&content_client);
+  }
   InitializeProviders();
   RegisterInProcessThreads();
 
@@ -127,11 +131,6 @@ void ChromeUnitTestSuite::Shutdown() {
 }
 
 void ChromeUnitTestSuite::InitializeProviders() {
-  {
-    ChromeContentClient content_client;
-    RegisterContentSchemes(&content_client);
-  }
-
   chrome::RegisterPathProvider();
   content::RegisterPathProvider();
   ui::RegisterPathProvider();

components/test/components_test_suite.cc

@@ -55,8 +55,13 @@ class ComponentsTestSuite : public base::TestSuite {
 
     mojo::core::Init();
 
-    // Before registering any schemes, clear GURL's internal state.
-    url::ResetForTests();
+    // These schemes need to be added globally to pass tests of
+    // autocomplete_input_unittest.cc and content_settings_pattern*
+    // TODO(https://crbug.com/1047702): Move this scheme initialization into the
+    //    individual tests that need these schemes.
+    url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
+    url::AddStandardScheme("chrome-search", url::SCHEME_WITH_HOST);
+    url::AddStandardScheme("chrome-distiller", url::SCHEME_WITH_HOST);
 
 #if !defined(OS_IOS)
     gl::GLSurfaceTestSupport::InitializeOneOff();
@@ -68,6 +73,10 @@ class ComponentsTestSuite : public base::TestSuite {
       content::ContentClient content_client;
       content::ContentTestSuiteBase::RegisterContentSchemes(&content_client);
     }
+#else
+    url::AddStandardScheme("chrome", url::SCHEME_WITH_HOST);
+    url::AddStandardScheme("devtools", url::SCHEME_WITH_HOST);
+
 #endif
 
     ui::RegisterPathProvider();
@@ -87,14 +96,6 @@ class ComponentsTestSuite : public base::TestSuite {
         pak_path.AppendASCII("components_tests_resources.pak"),
         ui::SCALE_FACTOR_NONE);
 
-    // These schemes need to be added globally to pass tests of
-    // autocomplete_input_unittest.cc and content_settings_pattern*
-    url::AddStandardScheme("chrome", url::SCHEME_WITH_HOST);
-    url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
-    url::AddStandardScheme("devtools", url::SCHEME_WITH_HOST);
-    url::AddStandardScheme("chrome-search", url::SCHEME_WITH_HOST);
-    url::AddStandardScheme("chrome-distiller", url::SCHEME_WITH_HOST);
-
     ContentSettingsPattern::SetNonWildcardDomainNonPortSchemes(
         kNonWildcardDomainNonPortSchemes,
         base::size(kNonWildcardDomainNonPortSchemes));

content/browser/navigation_browsertest.cc

@@ -72,6 +72,7 @@
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
 #include "url/gurl.h"
+#include "url/url_util.h"
 
 namespace content {
 
@@ -3082,11 +3083,9 @@ class NavigationUrlRewriteBrowserTest : public NavigationBaseBrowserTest {
     }
   };
 
-  void SetUp() override {
+  NavigationUrlRewriteBrowserTest() {
     url::AddStandardScheme(kNoAccessScheme, url::SCHEME_WITH_HOST);
     url::AddNoAccessScheme(kNoAccessScheme);
-
-    NavigationBaseBrowserTest::SetUp();
   }
 
   void SetUpOnMainThread() override {
@@ -3110,6 +3109,7 @@ class NavigationUrlRewriteBrowserTest : public NavigationBaseBrowserTest {
  private:
   std::unique_ptr<BrowserClient> browser_client_;
   ContentBrowserClient* old_browser_client_;
+  url::ScopedSchemeRegistryForTests scoped_registry_;
 };
 
 // TODO(1021779): Figure out why this fails on the kitkat-dbg builder

content/browser/service_worker/service_worker_provider_host_unittest.cc

@@ -36,6 +36,7 @@
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/service_worker/service_worker_object.mojom.h"
 #include "third_party/blink/public/mojom/service_worker/service_worker_registration.mojom.h"
+#include "url/url_util.h"
 
 namespace content {
 
@@ -107,13 +108,13 @@ class ServiceWorkerProviderHostTest : public testing::Test {
   ServiceWorkerProviderHostTest()
       : task_environment_(BrowserTaskEnvironment::IO_MAINLOOP) {
     SetContentClient(&test_content_client_);
+    ReRegisterContentSchemesForTests();
   }
   ~ServiceWorkerProviderHostTest() override {}
 
   void SetUp() override {
     old_content_browser_client_ =
         SetBrowserClientForTesting(&test_content_browser_client_);
-    ResetSchemesAndOriginsWhitelist();
     mojo::core::SetDefaultProcessErrorCallback(base::BindRepeating(
         &ServiceWorkerProviderHostTest::OnMojoError, base::Unretained(this)));
 
@@ -143,8 +144,6 @@ class ServiceWorkerProviderHostTest : public testing::Test {
     registration3_ = nullptr;
     helper_.reset();
     SetBrowserClientForTesting(old_content_browser_client_);
-    // Reset cached security schemes so we don't affect other tests.
-    ResetSchemesAndOriginsWhitelist();
     mojo::core::SetDefaultProcessErrorCallback(
         mojo::core::ProcessErrorCallback());
   }
@@ -342,6 +341,8 @@ class ServiceWorkerProviderHostTest : public testing::Test {
     return container_host;
   }
 
+  url::ScopedSchemeRegistryForTests scoped_registry_;
+
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerProviderHostTest);
 };
 

content/browser/site_instance_impl_unittest.cc

@@ -114,13 +114,12 @@ class SiteInstanceTestBrowserClient : public TestContentBrowserClient {
 
 class SiteInstanceTest : public testing::Test {
  public:
-  SiteInstanceTest() : old_browser_client_(nullptr) {}
+  SiteInstanceTest() : old_browser_client_(nullptr) {
+    url::AddStandardScheme(kPrivilegedScheme, url::SCHEME_WITH_HOST);
+  }
 
   void SetUp() override {
     old_browser_client_ = SetBrowserClientForTesting(&browser_client_);
-    url::AddStandardScheme(kPrivilegedScheme, url::SCHEME_WITH_HOST);
-    url::AddStandardScheme(kChromeUIScheme, url::SCHEME_WITH_HOST);
-
     RenderProcessHostImpl::set_render_process_host_factory_for_testing(
         &rph_factory_);
   }
@@ -142,8 +141,6 @@ class SiteInstanceTest : public testing::Test {
     // scheduled for deletion. Here, call DrainMessageLoop() again so the
     // AppCacheDatabase actually gets deleted.
     DrainMessageLoop();
-
-    ResetSchemesAndOriginsWhitelist();
   }
 
   void set_privileged_process_id(int process_id) {
@@ -178,6 +175,8 @@ class SiteInstanceTest : public testing::Test {
   SiteInstanceTestBrowserClient browser_client_;
   ContentBrowserClient* old_browser_client_;
   MockRenderProcessHostFactory rph_factory_;
+
+  url::ScopedSchemeRegistryForTests scoped_registry_;
 };
 
 // Test to ensure no memory leaks for SiteInstance objects.

content/browser/webauth/authenticator_impl_unittest.cc

@@ -400,7 +400,10 @@ class AuthenticatorTestBase : public content::RenderViewHostTestHarness {
 
 class AuthenticatorImplTest : public AuthenticatorTestBase {
  protected:
-  ~AuthenticatorImplTest() override {}
+  AuthenticatorImplTest() {
+    url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
+  }
+  ~AuthenticatorImplTest() override = default;
 
   void TearDown() override {
     // The |RenderFrameHost| must outlive |AuthenticatorImpl|.
@@ -541,6 +544,9 @@ class AuthenticatorImplTest : public AuthenticatorTestBase {
   base::Optional<base::test::ScopedFeatureList> scoped_feature_list_;
   scoped_refptr<::testing::NiceMock<device::MockBluetoothAdapter>>
       mock_adapter_;
+
+ private:
+  url::ScopedSchemeRegistryForTests scoped_registry_;
 };
 
 // Verify behavior for various combinations of origins and RP IDs.
@@ -947,7 +953,6 @@ TEST_F(AuthenticatorImplTest, CryptotokenBypass) {
   auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
       base::Time::Now(), base::TimeTicks::Now());
   auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
 
   {
     OverrideLastCommittedOrigin(main_rfh(),
@@ -1002,7 +1007,6 @@ TEST_F(AuthenticatorImplTest, CryptoTokenU2fOnly) {
   auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
       base::Time::Now(), base::TimeTicks::Now());
   auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
 
   // TODO(martinkr): VirtualFidoDeviceFactory does not offer devices that
   // support both U2F and CTAP yet; we should test those.
@@ -1037,7 +1041,6 @@ TEST_F(AuthenticatorImplTest, CryptotokenUsbOnly) {
   SimulateNavigation(GURL(kTestOrigin1));
   auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
       base::Time::Now(), base::TimeTicks::Now());
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
   auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
   SetTransports(device::GetAllTransportProtocols());
   auto bluetooth_values = SetUpMockBluetooth();
@@ -1089,7 +1092,6 @@ TEST_F(AuthenticatorImplTest, AttestationPermitted) {
   auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
       base::Time::Now(), base::TimeTicks::Now());
   auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
 
   // TODO(martinkr): VirtualFidoDeviceFactory does not offer devices that
   // support both U2F and CTAP yet; we should test those.
@@ -1692,7 +1694,6 @@ class OverrideRPIDAuthenticatorTest : public AuthenticatorImplTest {
 TEST_F(OverrideRPIDAuthenticatorTest, ChromeExtensions) {
   // Test that credentials can be created and used from an extension origin when
   // permitted by the delegate.
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
   constexpr char kExtensionId[] = "abcdefg";
   const std::string extension_origin =
       std::string("chrome-extension://") + kExtensionId;
@@ -2518,7 +2519,6 @@ TEST_F(AuthenticatorContentBrowserClientTest,
   auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
       base::Time::Now(), base::TimeTicks::Now());
   auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
   OverrideLastCommittedOrigin(main_rfh(),
                               url::Origin::Create(GURL(kCryptotokenOrigin)));
 
@@ -3686,7 +3686,6 @@ TEST_F(InternalUVAuthenticatorImplTest, MakeCredentialCryptotoken) {
   auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
       base::Time::Now(), base::TimeTicks::Now());
   auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
   OverrideLastCommittedOrigin(main_rfh(),
                               url::Origin::Create(GURL(kCryptotokenOrigin)));
 
@@ -3760,7 +3759,6 @@ TEST_F(InternalUVAuthenticatorImplTest, GetAssertion) {
 TEST_F(InternalUVAuthenticatorImplTest, GetAssertionCryptotoken) {
   mojo::Remote<blink::mojom::Authenticator> authenticator =
       ConnectToAuthenticator();
-  url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST);
   OverrideLastCommittedOrigin(main_rfh(),
                               url::Origin::Create(GURL(kCryptotokenOrigin)));
   ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectRegistration(

content/common/url_schemes.cc

@@ -50,7 +50,7 @@ std::vector<std::string>& GetMutableServiceWorkerSchemes() {
 }  // namespace
 
 void RegisterContentSchemes() {
-  // On Android, schemes may have been registered already.
+  // On Android and in tests, schemes may have been registered already.
   if (g_registered_url_schemes)
     return;
   g_registered_url_schemes = true;
@@ -98,6 +98,13 @@ void RegisterContentSchemes() {
     url::EnableNonStandardSchemesForAndroidWebView();
 #endif
 
+  // Prevent future modification of the scheme lists. This is to prevent
+  // accidental creation of data races in the program. Add*Scheme aren't
+  // threadsafe so must be called when GURL isn't used on any other thread. This
+  // is really easy to mess up, so we say that all calls to Add*Scheme in Chrome
+  // must be inside this function.
+  url::LockSchemeRegistries();
+
   // Combine the default savable schemes with the additional ones given.
   GetMutableSavableSchemes().assign(std::begin(kDefaultSavableSchemes),
                                     std::end(kDefaultSavableSchemes));
@@ -109,6 +116,7 @@ void RegisterContentSchemes() {
 }
 
 void ReRegisterContentSchemesForTests() {
+  url::ClearSchemesForTests();
   g_registered_url_schemes = false;
   RegisterContentSchemes();
 }

content/common/url_schemes.h

@@ -13,8 +13,9 @@
 namespace content {
 
 // Called near the beginning of startup to register URL schemes that should be
-// parsed as "standard" or "referrer" with the src/url/ library. The embedder
-// can add additional schemes by overriding the AddAdditionalSchemes method.
+// parsed as "standard" or "referrer" with the src/url/ library, then locks the
+// sets of schemes down. The embedder can add additional schemes by
+// overriding the ContentClient::AddAdditionalSchemes method.
 CONTENT_EXPORT void RegisterContentSchemes();
 
 // Re-initializes schemes for tests.