From 2add7d44778f7be638639aeb26a3b14f0b222e09 Mon Sep 17 00:00:00 2001 From: Michael Thiessen Date: Wed, 5 Feb 2020 13:49:38 +0000 Subject: [PATCH] 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 Reviewed-by: Yaron Friedman Reviewed-by: Gauthier Ambard Reviewed-by: Daniel Cheng Commit-Queue: Michael Thiessen Cr-Commit-Position: refs/heads/master@{#738544} --- .../test/CookieManagerStartupTest.java | 5 +- .../fileapi/file_system_backend_unittest.cc | 3 - ...ender_service_workers_observer_unittest.cc | 15 +- .../app_launch_event_logger_unittest.cc | 2 - chrome/test/base/chrome_unit_test_suite.cc | 9 +- components/test/components_test_suite.cc | 21 +-- content/browser/navigation_browsertest.cc | 6 +- .../service_worker_provider_host_unittest.cc | 7 +- .../browser/site_instance_impl_unittest.cc | 11 +- .../webauth/authenticator_impl_unittest.cc | 16 +-- content/common/url_schemes.cc | 10 +- content/common/url_schemes.h | 5 +- content/public/test/browser_test_base.cc | 2 - .../public/test/content_test_suite_base.cc | 4 + content/public/test/content_test_suite_base.h | 4 + content/public/test/test_launcher.cc | 14 ++ content/public/test/test_utils.cc | 6 - content/public/test/test_utils.h | 3 - fuchsia/engine/BUILD.gn | 1 + .../browser/content_directory_browsertest.cc | 10 ++ .../drive/drive_api_url_generator_unittest.cc | 5 +- .../browser/u2f/u2f_controller_unittest.mm | 1 + .../browser/u2f/u2f_tab_helper_unittest.mm | 1 + ios/chrome/test/ios_chrome_unit_test_suite.mm | 3 +- ios/web/init/web_main_runner.mm | 2 +- .../navigation_manager_impl_unittest.mm | 3 + ios/web/navigation/url_schemes.mm | 5 +- ..._based_navigation_manager_impl_unittest.mm | 1 + ios/web/public/navigation/url_schemes.h | 4 +- ios/web/test/web_test_suite.mm | 2 +- net/base/network_isolation_key_unittest.cc | 1 + net/cookies/site_for_cookies_unittest.cc | 1 + net/url_request/url_request_unittest.cc | 3 +- .../restricted_cookie_manager_unittest.cc | 3 +- .../controller/tests/run_all_tests.cc | 2 - .../blink/renderer/core/core_initializer.cc | 4 - .../blink/renderer/core/dom/document_test.cc | 5 +- .../renderer/core/page/plugin_data_test.cc | 1 + .../testing/blink_fuzzer_test_support.cc | 3 - .../blink/renderer/platform/weborigin/kurl.cc | 6 - .../blink/renderer/platform/weborigin/kurl.h | 4 - .../platform/weborigin/scheme_registry.cc | 6 - .../platform/weborigin/scheme_registry.h | 2 - .../weborigin/security_origin_test.cc | 3 +- .../weborigin/security_policy_test.cc | 3 +- url/origin_unittest.cc | 8 +- url/scheme_host_port_unittest.cc | 7 +- url/url_util.cc | 132 +++++++++++++----- url/url_util.h | 21 ++- url/url_util_unittest.cc | 20 +-- 50 files changed, 254 insertions(+), 162 deletions(-) diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java index 666c8a0fbd2f4e..ec198ee95f6ee0 100644 --- a/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java +++ b/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, diff --git a/chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc b/chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc index e2c590c13284c4..391058be7a78d0 100644 --- a/chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc +++ b/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 mount_points( storage::ExternalMountPoints::CreateRefCounted()); scoped_refptr system_mount_points( diff --git a/chrome/browser/prerender/isolated/isolated_prerender_service_workers_observer_unittest.cc b/chrome/browser/prerender/isolated/isolated_prerender_service_workers_observer_unittest.cc index 75978d2ee6b3b3..c3ef4390ce9798 100644 --- a/chrome/browser/prerender/isolated/isolated_prerender_service_workers_observer_unittest.cc +++ b/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(false), diff --git a/chrome/browser/ui/app_list/search/search_result_ranker/app_launch_event_logger_unittest.cc b/chrome/browser/ui/app_list/search/search_result_ranker/app_launch_event_logger_unittest.cc index 42d9be6641f37b..96bf647af37512 100644 --- a/chrome/browser/ui/app_list/search/search_result_ranker/app_launch_event_logger_unittest.cc +++ b/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)); diff --git a/chrome/test/base/chrome_unit_test_suite.cc b/chrome/test/base/chrome_unit_test_suite.cc index 7a555e8b9038e1..b2da00257f0dbe 100644 --- a/chrome/test/base/chrome_unit_test_suite.cc +++ b/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(); diff --git a/components/test/components_test_suite.cc b/components/test/components_test_suite.cc index 7b5396d2d92f2e..aff36df970771d 100644 --- a/components/test/components_test_suite.cc +++ b/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)); diff --git a/content/browser/navigation_browsertest.cc b/content/browser/navigation_browsertest.cc index 30e5a32115421a..63c297928ad405 100644 --- a/content/browser/navigation_browsertest.cc +++ b/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 browser_client_; ContentBrowserClient* old_browser_client_; + url::ScopedSchemeRegistryForTests scoped_registry_; }; // TODO(1021779): Figure out why this fails on the kitkat-dbg builder diff --git a/content/browser/service_worker/service_worker_provider_host_unittest.cc b/content/browser/service_worker/service_worker_provider_host_unittest.cc index dc1b834aa3345d..eabf4bd7207822 100644 --- a/content/browser/service_worker/service_worker_provider_host_unittest.cc +++ b/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); }; diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc index 79577a5d6dce81..ff1cde4dbacf60 100644 --- a/content/browser/site_instance_impl_unittest.cc +++ b/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. diff --git a/content/browser/webauth/authenticator_impl_unittest.cc b/content/browser/webauth/authenticator_impl_unittest.cc index 751377094d1e37..9d53195182d078 100644 --- a/content/browser/webauth/authenticator_impl_unittest.cc +++ b/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 scoped_feature_list_; scoped_refptr<::testing::NiceMock> 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::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::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::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::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::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::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 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( diff --git a/content/common/url_schemes.cc b/content/common/url_schemes.cc index ea162dfaf42e96..686c7157e5db35 100644 --- a/content/common/url_schemes.cc +++ b/content/common/url_schemes.cc @@ -50,7 +50,7 @@ std::vector& 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(); } diff --git a/content/common/url_schemes.h b/content/common/url_schemes.h index 4c8f417a80c1b6..3038f9d25798f3 100644 --- a/content/common/url_schemes.h +++ b/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. diff --git a/content/public/test/browser_test_base.cc b/content/public/test/browser_test_base.cc index 167b627d84a099..b3c63b5712ac33 100644 --- a/content/public/test/browser_test_base.cc +++ b/content/public/test/browser_test_base.cc @@ -80,7 +80,6 @@ #include "components/discardable_memory/service/discardable_shared_memory_manager.h" // nogncheck #include "content/app/mojo/mojo_init.h" #include "content/app/service_manager_environment.h" -#include "content/common/url_schemes.h" #include "content/public/app/content_main_delegate.h" #include "content/public/common/content_paths.h" #include "testing/android/native_test/native_browser_test_support.h" @@ -411,7 +410,6 @@ void BrowserTestBase::SetUp() { SetRendererClientForTesting(delegate->CreateContentRendererClient()); content::RegisterPathProvider(); - content::RegisterContentSchemes(); ui::RegisterPathProvider(); delegate->PreSandboxStartup(); diff --git a/content/public/test/content_test_suite_base.cc b/content/public/test/content_test_suite_base.cc index 5c68215c882b76..f6c7ac2054b5b1 100644 --- a/content/public/test/content_test_suite_base.cc +++ b/content/public/test/content_test_suite_base.cc @@ -81,6 +81,10 @@ void ContentTestSuiteBase::RegisterContentSchemes( SetContentClient(nullptr); } +void ContentTestSuiteBase::ReRegisterContentSchemes() { + content::ReRegisterContentSchemesForTests(); +} + void ContentTestSuiteBase::RegisterInProcessThreads() { UtilityProcessHost::RegisterUtilityMainThreadFactory( CreateInProcessUtilityThread); diff --git a/content/public/test/content_test_suite_base.h b/content/public/test/content_test_suite_base.h index 2700f32d9a1c31..3bd085170bbc1f 100644 --- a/content/public/test/content_test_suite_base.h +++ b/content/public/test/content_test_suite_base.h @@ -21,6 +21,10 @@ class ContentTestSuiteBase : public base::TestSuite { // registered temporarily so that it can provide additional schemes. static void RegisterContentSchemes(ContentClient* content_client); + // Re-initializes content's schemes even if schemes have already been + // registered. + static void ReRegisterContentSchemes(); + // Registers renderer/utility/gpu processes to run in-thread. static void RegisterInProcessThreads(); diff --git a/content/public/test/test_launcher.cc b/content/public/test/test_launcher.cc index 819e358293e228..656a50d6f7b829 100644 --- a/content/public/test/test_launcher.cc +++ b/content/public/test/test_launcher.cc @@ -35,8 +35,10 @@ #include "base/test/test_timeouts.h" #include "base/time/time.h" #include "build/build_config.h" +#include "content/common/url_schemes.h" #include "content/public/app/content_main.h" #include "content/public/app/content_main_delegate.h" +#include "content/public/common/content_client.h" #include "content/public/common/content_switches.h" #include "content/public/common/sandbox_init.h" #include "content/public/test/browser_test.h" @@ -274,6 +276,13 @@ void AppendCommandLineSwitches() { } // namespace +class ContentClientCreator { + public: + static void Create(ContentMainDelegate* delegate) { + SetContentClient(delegate->CreateContentClient()); + } +}; + std::string TestLauncherDelegate::GetUserDataDirectoryCommandLineSwitch() { return std::string(); } @@ -306,6 +315,11 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate, // browser test target and is not created by the |launcher_delegate|. std::unique_ptr content_main_delegate( launcher_delegate->CreateContentMainDelegate()); + ContentClientCreator::Create(content_main_delegate.get()); + // Many tests use GURL during setup, so we need to register schemes early in + // test launching. + RegisterContentSchemes(); + // ContentMain is not run on Android in the test process, and is run via // java for child processes. ContentMainParams params(content_main_delegate.get()); diff --git a/content/public/test/test_utils.cc b/content/public/test/test_utils.cc index 0276a17c943884..56d5dccfcc8e5d 100644 --- a/content/public/test/test_utils.cc +++ b/content/public/test/test_utils.cc @@ -25,7 +25,6 @@ #include "build/build_config.h" #include "content/browser/frame_host/render_frame_host_delegate.h" #include "content/browser/frame_host/render_frame_host_impl.h" -#include "content/common/url_schemes.h" #include "content/public/browser/browser_child_process_host_iterator.h" #include "content/public/browser/browser_plugin_guest_delegate.h" #include "content/public/browser/browser_task_traits.h" @@ -211,11 +210,6 @@ void IsolateAllSitesForTesting(base::CommandLine* command_line) { command_line->AppendSwitch(switches::kSitePerProcess); } -void ResetSchemesAndOriginsWhitelist() { - url::ResetForTests(); - ReRegisterContentSchemesForTests(); -} - GURL GetWebUIURL(const std::string& host) { return GURL(GetWebUIURLString(host)); } diff --git a/content/public/test/test_utils.h b/content/public/test/test_utils.h index c3b89a2ce80d6f..51fef174628a65 100644 --- a/content/public/test/test_utils.h +++ b/content/public/test/test_utils.h @@ -101,9 +101,6 @@ bool AreDefaultSiteInstancesEnabled(); // the test; the flag will be read on the first real navigation. void IsolateAllSitesForTesting(base::CommandLine* command_line); -// Resets the internal secure schemes/origins whitelist. -void ResetSchemesAndOriginsWhitelist(); - // Returns a GURL constructed from the WebUI scheme and the given host. GURL GetWebUIURL(const std::string& host); diff --git a/fuchsia/engine/BUILD.gn b/fuchsia/engine/BUILD.gn index f175c885a9ec11..ada71f1bbc270f 100644 --- a/fuchsia/engine/BUILD.gn +++ b/fuchsia/engine/BUILD.gn @@ -262,6 +262,7 @@ test("web_engine_browsertests") { ":web_engine_core", "//base/test:test_support", "//content/public/browser", + "//content/test:test_support", "//fuchsia/base", "//fuchsia/base:test_support", "//net:test_support", diff --git a/fuchsia/engine/browser/content_directory_browsertest.cc b/fuchsia/engine/browser/content_directory_browsertest.cc index da956ba3cff0fa..7124362766448b 100644 --- a/fuchsia/engine/browser/content_directory_browsertest.cc +++ b/fuchsia/engine/browser/content_directory_browsertest.cc @@ -14,11 +14,13 @@ #include "base/fuchsia/fuchsia_logging.h" #include "base/path_service.h" #include "base/threading/thread_restrictions.h" +#include "content/public/test/content_test_suite_base.h" #include "fuchsia/base/frame_test_util.h" #include "fuchsia/base/test_navigation_listener.h" #include "fuchsia/engine/browser/content_directory_loader_factory.h" #include "fuchsia/engine/switches.h" #include "testing/gtest/include/gtest/gtest.h" +#include "url/url_util.h" namespace { @@ -65,6 +67,11 @@ class ContentDirectoryTest : public cr_fuchsia::WebEngineBrowserTest { base::CommandLine::ForCurrentProcess()->AppendSwitch( switches::kContentDirectories); + // Scheme initialization for the WebEngineContentClient depends on the above + // command line modification, which won't have been present when the schemes + // were initially registered. + content::ContentTestSuiteBase::ReRegisterContentSchemes(); + cr_fuchsia::WebEngineBrowserTest::SetUp(); } @@ -103,6 +110,9 @@ class ContentDirectoryTest : public cr_fuchsia::WebEngineBrowserTest { cr_fuchsia::TestNavigationListener navigation_listener_; + private: + url::ScopedSchemeRegistryForTests scoped_registry_; + DISALLOW_COPY_AND_ASSIGN(ContentDirectoryTest); }; diff --git a/google_apis/drive/drive_api_url_generator_unittest.cc b/google_apis/drive/drive_api_url_generator_unittest.cc index 2629141a72b247..af4fa3bca09aa8 100644 --- a/google_apis/drive/drive_api_url_generator_unittest.cc +++ b/google_apis/drive/drive_api_url_generator_unittest.cc @@ -29,10 +29,11 @@ class DriveApiUrlGeneratorTest : public testing::Test { url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST); } - ~DriveApiUrlGeneratorTest() override { url::ResetForTests(); } - protected: DriveApiUrlGenerator url_generator_; + + private: + url::ScopedSchemeRegistryForTests scoped_registry_; }; // Make sure the hard-coded urls are returned. diff --git a/ios/chrome/browser/u2f/u2f_controller_unittest.mm b/ios/chrome/browser/u2f/u2f_controller_unittest.mm index 7ad1cb2769ea7a..cd3428a142c6f2 100644 --- a/ios/chrome/browser/u2f/u2f_controller_unittest.mm +++ b/ios/chrome/browser/u2f/u2f_controller_unittest.mm @@ -62,6 +62,7 @@ } U2FController* _U2FController; + url::ScopedSchemeRegistryForTests scoped_registry_; }; TEST_F(U2FControllerTest, XCallbackFromRequestURLWithCorrectFlowTest) { diff --git a/ios/chrome/browser/u2f/u2f_tab_helper_unittest.mm b/ios/chrome/browser/u2f/u2f_tab_helper_unittest.mm index 2961913ce3f056..b32f0ccf4f4a56 100644 --- a/ios/chrome/browser/u2f/u2f_tab_helper_unittest.mm +++ b/ios/chrome/browser/u2f/u2f_tab_helper_unittest.mm @@ -78,6 +78,7 @@ } web::TestWebState web_state_; + url::ScopedSchemeRegistryForTests scoped_registry_; }; // Tests that IsU2FUrl returns true only if U2F url param is true. diff --git a/ios/chrome/test/ios_chrome_unit_test_suite.mm b/ios/chrome/test/ios_chrome_unit_test_suite.mm index 01be6f19a28964..35c9fe3487c5c5 100644 --- a/ios/chrome/test/ios_chrome_unit_test_suite.mm +++ b/ios/chrome/test/ios_chrome_unit_test_suite.mm @@ -67,6 +67,8 @@ void OnTestEnd(const testing::TestInfo& test_info) override { IOSChromeUnitTestSuite::~IOSChromeUnitTestSuite() {} void IOSChromeUnitTestSuite::Initialize() { + url::AddStandardScheme(kChromeUIScheme, url::SCHEME_WITH_HOST); + // Add an additional listener to do the extra initialization for unit tests. // It will be started before the base class listeners and ended after the // base class listeners. @@ -87,6 +89,5 @@ void OnTestEnd(const testing::TestInfo& test_info) override { ios::RegisterPathProvider(); ui::RegisterPathProvider(); - url::AddStandardScheme(kChromeUIScheme, url::SCHEME_WITH_HOST); ContentSettingsPattern::SetNonWildcardDomainNonPortSchemes(nullptr, 0); } diff --git a/ios/web/init/web_main_runner.mm b/ios/web/init/web_main_runner.mm index 4139b692091017..fb8358c5411b51 100644 --- a/ios/web/init/web_main_runner.mm +++ b/ios/web/init/web_main_runner.mm @@ -61,7 +61,7 @@ int Initialize(WebMainParams params) override { if (!GetWebClient()) SetWebClient(&empty_web_client_); - RegisterWebSchemes(true); + RegisterWebSchemes(); ui::RegisterPathProvider(); CHECK(base::i18n::InitializeICU()); diff --git a/ios/web/navigation/navigation_manager_impl_unittest.mm b/ios/web/navigation/navigation_manager_impl_unittest.mm index 071c9c3848dfa0..e6478e7a2e925b 100644 --- a/ios/web/navigation/navigation_manager_impl_unittest.mm +++ b/ios/web/navigation/navigation_manager_impl_unittest.mm @@ -153,6 +153,9 @@ void SimulateReturningPendingItemFromDelegate(web::NavigationItemImpl* item) { TestWebState web_state_; MockNavigationManagerDelegate delegate_; std::unique_ptr manager_; + + private: + url::ScopedSchemeRegistryForTests scoped_registry_; }; // Tests state of an empty navigation manager. diff --git a/ios/web/navigation/url_schemes.mm b/ios/web/navigation/url_schemes.mm index e2dbcf0759d8a6..767ec854a0851d 100644 --- a/ios/web/navigation/url_schemes.mm +++ b/ios/web/navigation/url_schemes.mm @@ -16,7 +16,7 @@ namespace web { -void RegisterWebSchemes(bool lock_schemes) { +void RegisterWebSchemes() { web::WebClient::Schemes schemes; GetWebClient()->AddAdditionalSchemes(&schemes); for (const auto& scheme : schemes.standard_schemes) @@ -30,8 +30,7 @@ void RegisterWebSchemes(bool lock_schemes) { // 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. - if (lock_schemes) - url::LockSchemeRegistries(); + url::LockSchemeRegistries(); } } // namespace web diff --git a/ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm b/ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm index ce604440cc973f..4efba3838f0e3e 100644 --- a/ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm +++ b/ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm @@ -129,6 +129,7 @@ void RemoveWebView() override { private: TestBrowserState browser_state_; + url::ScopedSchemeRegistryForTests scoped_registry_; }; // Tests that GetItemAtIndex() on an empty manager will sync navigation items to diff --git a/ios/web/public/navigation/url_schemes.h b/ios/web/public/navigation/url_schemes.h index 1c09251a8d4a59..44a41ec7482c81 100644 --- a/ios/web/public/navigation/url_schemes.h +++ b/ios/web/public/navigation/url_schemes.h @@ -15,10 +15,10 @@ namespace web { // // Called near the beginning of startup to register URL schemes that should be // parsed as "standard" or "secure" with the src/url/ library. The set of -// schemes is locked if |lock_schemes| is true. +// schemes is then locked, disallowing further modification. // The embedder can add additional schemes by overriding the // WebClient::AddAdditionalSchemes method. -void RegisterWebSchemes(bool lock_schemes); +void RegisterWebSchemes(); } // namespace web diff --git a/ios/web/test/web_test_suite.mm b/ios/web/test/web_test_suite.mm index 45dd1f2b6279c2..d1b82a8803e597 100644 --- a/ios/web/test/web_test_suite.mm +++ b/ios/web/test/web_test_suite.mm @@ -34,7 +34,7 @@ void WebTestSuite::Initialize() { base::TestSuite::Initialize(); - RegisterWebSchemes(false); + RegisterWebSchemes(); // Force unittests to run using en-US so if testing string output will work // regardless of the system language. diff --git a/net/base/network_isolation_key_unittest.cc b/net/base/network_isolation_key_unittest.cc index 43b3f19cde7bd4..842822efc35e76 100644 --- a/net/base/network_isolation_key_unittest.cc +++ b/net/base/network_isolation_key_unittest.cc @@ -500,6 +500,7 @@ TEST(NetworkIsolationKeyTest, UseRegistrableDomainWithNonStandardScheme) { // Have to register the scheme, or url::Origin::Create() will return an opaque // origin. + url::ScopedSchemeRegistryForTests scoped_registry; url::AddStandardScheme("foo", url::SCHEME_WITH_HOST); url::Origin origin = url::Origin::Create(GURL("foo://a.foo.com")); diff --git a/net/cookies/site_for_cookies_unittest.cc b/net/cookies/site_for_cookies_unittest.cc index 30ef061679e23b..3583f1a744ef74 100644 --- a/net/cookies/site_for_cookies_unittest.cc +++ b/net/cookies/site_for_cookies_unittest.cc @@ -99,6 +99,7 @@ TEST(SiteForCookiesTest, File) { } TEST(SiteForCookiesTest, Extension) { + url::ScopedSchemeRegistryForTests scoped_registry; url::AddStandardScheme("chrome-extension", url::SCHEME_WITH_HOST); std::vector equivalent = {GURL("chrome-extension://abc/"), GURL("chrome-extension://abc/foo.txt"), diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc index c57203b4431d18..1141b5a3343359 100644 --- a/net/url_request/url_request_unittest.cc +++ b/net/url_request/url_request_unittest.cc @@ -2397,6 +2397,7 @@ TEST_F(URLRequestTest, SettingSameSiteCookies) { // Tests special chrome:// scheme that is supposed to always attach SameSite // cookies if the requested site is secure. TEST_F(URLRequestTest, SameSiteCookiesSpecialScheme) { + url::ScopedSchemeRegistryForTests scoped_registry; url::AddStandardScheme("chrome", url::SchemeType::SCHEME_WITH_HOST); EmbeddedTestServer https_test_server(EmbeddedTestServer::TYPE_HTTPS); @@ -2485,8 +2486,6 @@ TEST_F(URLRequestTest, SameSiteCookiesSpecialScheme) { d.data_received().find("StrictSameSiteCookie")); EXPECT_EQ(std::string::npos, d.data_received().find("LaxSameSiteCookie")); } - - url::ResetForTests(); } // Tests that __Secure- cookies can't be set on non-secure origins. diff --git a/services/network/restricted_cookie_manager_unittest.cc b/services/network/restricted_cookie_manager_unittest.cc index 8a3ed31ad6d1c1..e83a45c682a168 100644 --- a/services/network/restricted_cookie_manager_unittest.cc +++ b/services/network/restricted_cookie_manager_unittest.cc @@ -773,6 +773,7 @@ TEST_P(RestrictedCookieManagerTest, CookiesEnabledFor) { // Test that special chrome:// scheme always attaches SameSite cookies when the // requested origin is secure. TEST_P(RestrictedCookieManagerTest, SameSiteCookiesSpecialScheme) { + url::ScopedSchemeRegistryForTests scoped_registry; cookie_settings_.set_secure_origin_cookies_allowed_schemes({"chrome"}); url::AddStandardScheme("chrome", url::SchemeType::SCHEME_WITH_HOST); @@ -836,8 +837,6 @@ TEST_P(RestrictedCookieManagerTest, SameSiteCookiesSpecialScheme) { cookies = sync_service_->GetAllForUrl(http_url, chrome_url, http_origin, std::move(options)); EXPECT_THAT(cookies, testing::SizeIs(0)); - - url::ResetForTests(); } namespace { diff --git a/third_party/blink/renderer/controller/tests/run_all_tests.cc b/third_party/blink/renderer/controller/tests/run_all_tests.cc index 7e8bb565ae421b..115d3138e5f6d1 100644 --- a/third_party/blink/renderer/controller/tests/run_all_tests.cc +++ b/third_party/blink/renderer/controller/tests/run_all_tests.cc @@ -37,7 +37,6 @@ #include "content/public/test/blink_test_environment.h" #include "third_party/blink/renderer/bindings/core/v8/v8_gc_controller.h" #include "third_party/blink/renderer/platform/heap/thread_state.h" -#include "third_party/blink/renderer/platform/weborigin/scheme_registry.h" #include "v8/include/v8.h" namespace { @@ -51,7 +50,6 @@ class BlinkUnitTestSuite : public base::TestSuite { base::TestSuite::Initialize(); content::SetUpBlinkTestEnvironment(); - blink::SchemeRegistry::Initialize(); } void Shutdown() override { // Tickle EndOfTaskRunner which among other things will flush the queue diff --git a/third_party/blink/renderer/core/core_initializer.cc b/third_party/blink/renderer/core/core_initializer.cc index 82b8f94210f80c..cb5ad15a22edf8 100644 --- a/third_party/blink/renderer/core/core_initializer.cc +++ b/third_party/blink/renderer/core/core_initializer.cc @@ -59,8 +59,6 @@ #include "third_party/blink/renderer/platform/font_family_names.h" #include "third_party/blink/renderer/platform/loader/fetch/fetch_initiator_type_names.h" #include "third_party/blink/renderer/platform/network/http_names.h" -#include "third_party/blink/renderer/platform/weborigin/kurl.h" -#include "third_party/blink/renderer/platform/weborigin/scheme_registry.h" #include "third_party/blink/renderer/platform/weborigin/security_policy.h" #include "third_party/blink/renderer/platform/wtf/allocator/partitions.h" #include "third_party/blink/renderer/platform/wtf/text/atomic_string_table.h" @@ -136,8 +134,6 @@ void CoreInitializer::Initialize() { style_change_extra_data::Init(); - KURL::Initialize(); - SchemeRegistry::Initialize(); SecurityPolicy::Init(); RegisterEventFactory(); diff --git a/third_party/blink/renderer/core/dom/document_test.cc b/third_party/blink/renderer/core/dom/document_test.cc index 57958368a5a7a3..b95154dc59e83d 100644 --- a/third_party/blink/renderer/core/dom/document_test.cc +++ b/third_party/blink/renderer/core/dom/document_test.cc @@ -74,6 +74,7 @@ #include "third_party/blink/renderer/platform/weborigin/scheme_registry.h" #include "third_party/blink/renderer/platform/weborigin/security_origin.h" #include "third_party/blink/renderer/platform/wtf/text/string_builder.h" +#include "url/url_util.h" namespace blink { @@ -630,8 +631,8 @@ TEST_F(DocumentTest, EnforceSandboxFlags) { // A unique origin does not bypass secure context checks unless it // is also potentially trustworthy. - url::AddStandardScheme("very-special-scheme", - url::SchemeType::SCHEME_WITH_HOST); + url::ScopedSchemeRegistryForTests scoped_registry; + url::AddStandardScheme("very-special-scheme", url::SCHEME_WITH_HOST); SchemeRegistry::RegisterURLSchemeBypassingSecureContextCheck( "very-special-scheme"); NavigateTo(KURL("very-special-scheme://example.test"), "", "sandbox"); diff --git a/third_party/blink/renderer/core/page/plugin_data_test.cc b/third_party/blink/renderer/core/page/plugin_data_test.cc index e522955ef65a8c..9114eac609a42c 100644 --- a/third_party/blink/renderer/core/page/plugin_data_test.cc +++ b/third_party/blink/renderer/core/page/plugin_data_test.cc @@ -37,6 +37,7 @@ class MockPluginRegistry : public mojom::blink::PluginRegistry { TEST(PluginDataTest, NonStandardUrlSchemeRequestsPluginsWithUniqueOrigin) { ScopedTestingPlatformSupport support; // Create a scheme that's local but nonstandard, as in bug 862282. + url::ScopedSchemeRegistryForTests scoped_registry; url::AddLocalScheme("nonstandard-862282"); SchemeRegistry::RegisterURLSchemeAsLocal("nonstandard-862282"); diff --git a/third_party/blink/renderer/platform/testing/blink_fuzzer_test_support.cc b/third_party/blink/renderer/platform/testing/blink_fuzzer_test_support.cc index a01f3006487433..1a8a8a1a5f03d4 100644 --- a/third_party/blink/renderer/platform/testing/blink_fuzzer_test_support.cc +++ b/third_party/blink/renderer/platform/testing/blink_fuzzer_test_support.cc @@ -10,7 +10,6 @@ #include "base/test/test_timeouts.h" #include "content/public/test/blink_test_environment.h" #include "third_party/blink/renderer/platform/heap/thread_state.h" -#include "third_party/blink/renderer/platform/weborigin/scheme_registry.h" namespace blink { @@ -30,8 +29,6 @@ BlinkFuzzerTestSupport::BlinkFuzzerTestSupport(int argc, char** argv) { TestTimeouts::Initialize(); content::SetUpBlinkTestEnvironment(); - - blink::SchemeRegistry::Initialize(); } BlinkFuzzerTestSupport::~BlinkFuzzerTestSupport() { diff --git a/third_party/blink/renderer/platform/weborigin/kurl.cc b/third_party/blink/renderer/platform/weborigin/kurl.cc index 4ab44fc6c7d9db..2853faa348f178 100644 --- a/third_party/blink/renderer/platform/weborigin/kurl.cc +++ b/third_party/blink/renderer/platform/weborigin/kurl.cc @@ -125,12 +125,6 @@ bool IsValidProtocol(const String& protocol) { return true; } -void KURL::Initialize() { - // This must be called before we create other threads to - // avoid racy static local initialization. - BlankURL(); -} - String KURL::StrippedForUseAsReferrer() const { if (!ProtocolIsInHTTPFamily()) return String(); diff --git a/third_party/blink/renderer/platform/weborigin/kurl.h b/third_party/blink/renderer/platform/weborigin/kurl.h index 6500b29d8920eb..8ddb31a682ba76 100644 --- a/third_party/blink/renderer/platform/weborigin/kurl.h +++ b/third_party/blink/renderer/platform/weborigin/kurl.h @@ -77,10 +77,6 @@ class PLATFORM_EXPORT KURL { USING_FAST_MALLOC(KURL); public: - // This must be called during initialization (before we create - // other threads). - static void Initialize(); - KURL(); KURL(const KURL&); diff --git a/third_party/blink/renderer/platform/weborigin/scheme_registry.cc b/third_party/blink/renderer/platform/weborigin/scheme_registry.cc index 3903a94f5dfb37..5e53886a855eb6 100644 --- a/third_party/blink/renderer/platform/weborigin/scheme_registry.cc +++ b/third_party/blink/renderer/platform/weborigin/scheme_registry.cc @@ -112,12 +112,6 @@ URLSchemesRegistry& GetMutableURLSchemesRegistry() { } // namespace -// Must be called before we create other threads to avoid racy static local -// initialization. -void SchemeRegistry::Initialize() { - GetURLSchemesRegistry(); -} - void SchemeRegistry::RegisterURLSchemeAsLocal(const String& scheme) { DCHECK_EQ(scheme, scheme.LowerASCII()); GetMutableURLSchemesRegistry().local_schemes.insert(scheme); diff --git a/third_party/blink/renderer/platform/weborigin/scheme_registry.h b/third_party/blink/renderer/platform/weborigin/scheme_registry.h index 4ef1bdfd07f74b..3a6f7734f19d9a 100644 --- a/third_party/blink/renderer/platform/weborigin/scheme_registry.h +++ b/third_party/blink/renderer/platform/weborigin/scheme_registry.h @@ -49,8 +49,6 @@ class PLATFORM_EXPORT SchemeRegistry { STATIC_ONLY(SchemeRegistry); public: - static void Initialize(); - static void RegisterURLSchemeAsLocal(const String&); static bool ShouldTreatURLSchemeAsLocal(const String&); diff --git a/third_party/blink/renderer/platform/weborigin/security_origin_test.cc b/third_party/blink/renderer/platform/weborigin/security_origin_test.cc index 1a9f00f8867f60..879eef2c67e274 100644 --- a/third_party/blink/renderer/platform/weborigin/security_origin_test.cc +++ b/third_party/blink/renderer/platform/weborigin/security_origin_test.cc @@ -662,6 +662,7 @@ TEST_F(SecurityOriginTest, CanonicalizeHost) { } TEST_F(SecurityOriginTest, UrlOriginConversions) { + url::ScopedSchemeRegistryForTests scoped_registry; url::AddLocalScheme("nonstandard-but-local"); SchemeRegistry::RegisterURLSchemeAsLocal("nonstandard-but-local"); struct TestCases { @@ -884,6 +885,7 @@ TEST_F(SecurityOriginTest, NonStandardScheme) { } TEST_F(SecurityOriginTest, NonStandardSchemeWithAndroidWebViewHack) { + url::ScopedSchemeRegistryForTests scoped_registry; url::EnableNonStandardSchemesForAndroidWebView(); scoped_refptr origin = SecurityOrigin::CreateFromString("cow://"); @@ -891,7 +893,6 @@ TEST_F(SecurityOriginTest, NonStandardSchemeWithAndroidWebViewHack) { EXPECT_EQ("cow", origin->Protocol()); EXPECT_EQ("", origin->Host()); EXPECT_EQ(0, origin->Port()); - url::ResetForTests(); } TEST_F(SecurityOriginTest, OpaqueIsolatedCopy) { diff --git a/third_party/blink/renderer/platform/weborigin/security_policy_test.cc b/third_party/blink/renderer/platform/weborigin/security_policy_test.cc index befd36dd8b15d8..4ba9986724ca81 100644 --- a/third_party/blink/renderer/platform/weborigin/security_policy_test.cc +++ b/third_party/blink/renderer/platform/weborigin/security_policy_test.cc @@ -569,9 +569,10 @@ TEST_F(SecurityPolicyAccessTest, IsOriginAccessAllowedPriority) { // Test that referrers for custom hierarchical (standard) schemes are correctly // handled by the new policy. (For instance, this covers android-app://.) TEST(SecurityPolicyTest, ReferrerForCustomScheme) { + url::ScopedSchemeRegistryForTests scoped_registry; const char kCustomStandardScheme[] = "my-new-scheme"; - SchemeRegistry::RegisterURLSchemeAsAllowedForReferrer(kCustomStandardScheme); url::AddStandardScheme(kCustomStandardScheme, url::SCHEME_WITH_HOST); + SchemeRegistry::RegisterURLSchemeAsAllowedForReferrer(kCustomStandardScheme); String kFullReferrer = "my-new-scheme://com.foo.me/this-should-be-truncated"; String kTruncatedReferrer = "my-new-scheme://com.foo.me/"; diff --git a/url/origin_unittest.cc b/url/origin_unittest.cc index 3384f323ad702e..3cb24cff140155 100644 --- a/url/origin_unittest.cc +++ b/url/origin_unittest.cc @@ -55,7 +55,6 @@ class OriginTest : public ::testing::Test { AddStandardScheme("standard-but-noaccess", SchemeType::SCHEME_WITH_HOST); AddNoAccessScheme("standard-but-noaccess"); } - void TearDown() override { url::ResetForTests(); } ::testing::AssertionResult DoEqualityComparisons(const url::Origin& a, const url::Origin& b, @@ -105,6 +104,9 @@ class OriginTest : public ::testing::Test { return Origin::UnsafelyCreateOpaqueOriginWithoutNormalization( precursor_scheme, precursor_host, precursor_port, nonce); } + + private: + ScopedSchemeRegistryForTests scoped_registry_; }; TEST_F(OriginTest, OpaqueOriginComparison) { @@ -656,6 +658,7 @@ TEST_F(OriginTest, NonStandardScheme) { Origin origin = Origin::Create(GURL("cow://")); EXPECT_TRUE(origin.opaque()); } + TEST_F(OriginTest, NonStandardSchemeWithAndroidWebViewHack) { EnableNonStandardSchemesForAndroidWebView(); Origin origin = Origin::Create(GURL("cow://")); @@ -663,10 +666,10 @@ TEST_F(OriginTest, NonStandardSchemeWithAndroidWebViewHack) { EXPECT_EQ("cow", origin.scheme()); EXPECT_EQ("", origin.host()); EXPECT_EQ(0, origin.port()); - ResetForTests(); } TEST_F(OriginTest, CanBeDerivedFrom) { + AddStandardScheme("new-standard", SchemeType::SCHEME_WITH_HOST); Origin opaque_unique_origin = Origin(); Origin regular_origin = Origin::Create(GURL("https://a.com/")); @@ -684,7 +687,6 @@ TEST_F(OriginTest, CanBeDerivedFrom) { non_standard_scheme_origin.DeriveNewOpaqueOrigin(); // Also, add new standard scheme that is local to the test. - AddStandardScheme("new-standard", SchemeType::SCHEME_WITH_HOST); Origin new_standard_origin = Origin::Create(GURL("new-standard://host/")); Origin new_standard_opaque_precursor_origin = new_standard_origin.DeriveNewOpaqueOrigin(); diff --git a/url/scheme_host_port_unittest.cc b/url/scheme_host_port_unittest.cc index bd7919292bb277..2ebf7e78c9b6bb 100644 --- a/url/scheme_host_port_unittest.cc +++ b/url/scheme_host_port_unittest.cc @@ -16,12 +16,11 @@ namespace { class SchemeHostPortTest : public testing::Test { public: SchemeHostPortTest() = default; - ~SchemeHostPortTest() override { - // Reset any added schemes. - url::ResetForTests(); - } + ~SchemeHostPortTest() override = default; private: + url::ScopedSchemeRegistryForTests scoped_registry_; + DISALLOW_COPY_AND_ASSIGN(SchemeHostPortTest); }; diff --git a/url/url_util.cc b/url/url_util.cc index 1716f8ab4dfb71..58d976cd615c89 100644 --- a/url/url_util.cc +++ b/url/url_util.cc @@ -6,6 +6,7 @@ #include #include +#include #include "base/logging.h" #include "base/no_destructor.h" @@ -95,11 +96,26 @@ struct SchemeRegistry { bool allow_non_standard_schemes = false; }; -SchemeRegistry* GetSchemeRegistry() { +// See the LockSchemeRegistries declaration in the header. +bool scheme_registries_locked = false; + +// Ensure that the schemes aren't modified after first use. +static std::atomic g_scheme_registries_used{false}; + +// Gets the scheme registry without locking the schemes. This should *only* be +// used for adding schemes to the registry. +SchemeRegistry* GetSchemeRegistryWithoutLocking() { static base::NoDestructor registry; return registry.get(); } +const SchemeRegistry& GetSchemeRegistry() { +#if DCHECK_IS_ON() + g_scheme_registries_used.store(true); +#endif + return *GetSchemeRegistryWithoutLocking(); +} + // Pass this enum through for methods which would like to know if whitespace // removal is necessary. enum WhitespaceRemovalPolicy { @@ -107,9 +123,6 @@ enum WhitespaceRemovalPolicy { DO_NOT_REMOVE_WHITESPACE, }; -// See the LockSchemeRegistries declaration in the header. -bool scheme_registries_locked = false; - // This template converts a given character type to the corresponding // StringPiece type. template struct CharToStringPiece { @@ -159,7 +172,7 @@ bool DoIsInSchemes(const CHAR* spec, template bool DoIsStandard(const CHAR* spec, const Component& scheme, SchemeType* type) { return DoIsInSchemes(spec, scheme, type, - GetSchemeRegistry()->standard_schemes); + GetSchemeRegistry().standard_schemes); } @@ -443,7 +456,16 @@ bool DoReplaceComponents(const char* spec, return ReplacePathURL(spec, parsed, replacements, output, out_parsed); } -void DoAddScheme(const char* new_scheme, std::vector* schemes) { +void DoSchemeModificationPreamble() { + // If this assert triggers, it means you've called Add*Scheme after + // the SchemeRegistry has been used. + // + // This normally means you're trying to set up a new scheme too late or using + // the SchemeRegistry too early in your application's init process. Make sure + // that you haven't added any static GURL initializers in tests. + DCHECK(!g_scheme_registries_used.load()) + << "Trying to add a scheme after the lists have been used."; + // If this assert triggers, it means you've called Add*Scheme after // LockSchemeRegistries has been called (see the header file for // LockSchemeRegistries for more). @@ -453,105 +475,145 @@ void DoAddScheme(const char* new_scheme, std::vector* schemes) { // and calls LockSchemeRegistries, and add your new scheme there. DCHECK(!scheme_registries_locked) << "Trying to add a scheme after the lists have been locked."; +} +void DoAddScheme(const char* new_scheme, std::vector* schemes) { + DoSchemeModificationPreamble(); DCHECK(schemes); DCHECK(strlen(new_scheme) > 0); DCHECK_EQ(base::ToLowerASCII(new_scheme), new_scheme); + DCHECK(std::find(schemes->begin(), schemes->end(), new_scheme) == + schemes->end()); schemes->push_back(new_scheme); } void DoAddSchemeWithType(const char* new_scheme, SchemeType type, std::vector* schemes) { - // See DoAddScheme above. - DCHECK(!scheme_registries_locked) - << "Trying to add a scheme after the lists have been locked."; - + DoSchemeModificationPreamble(); DCHECK(schemes); DCHECK(strlen(new_scheme) > 0); DCHECK_EQ(base::ToLowerASCII(new_scheme), new_scheme); + DCHECK(std::find_if(schemes->begin(), schemes->end(), + [&new_scheme](const SchemeWithType& scheme) { + return scheme.scheme == new_scheme; + }) == schemes->end()); schemes->push_back({new_scheme, type}); } } // namespace -void ResetForTests() { - *GetSchemeRegistry() = SchemeRegistry(); -} +void ClearSchemesForTests() { + DCHECK(!g_scheme_registries_used.load()) + << "Schemes already used " + << "(use ScopedSchemeRegistryForTests to relax for tests)."; + DCHECK(!scheme_registries_locked) + << "Schemes already locked " + << "(use ScopedSchemeRegistryForTests to relax for tests)."; + *GetSchemeRegistryWithoutLocking() = SchemeRegistry(); +} + +class ScopedSchemeRegistryInternal { + public: + ScopedSchemeRegistryInternal() + : registry_(std::make_unique( + *GetSchemeRegistryWithoutLocking())) { + g_scheme_registries_used.store(false); + scheme_registries_locked = false; + } + ~ScopedSchemeRegistryInternal() { + *GetSchemeRegistryWithoutLocking() = *registry_; + g_scheme_registries_used.store(true); + scheme_registries_locked = true; + } + + private: + std::unique_ptr registry_; +}; + +ScopedSchemeRegistryForTests::ScopedSchemeRegistryForTests() + : internal_(std::make_unique()) {} + +ScopedSchemeRegistryForTests::~ScopedSchemeRegistryForTests() = default; void EnableNonStandardSchemesForAndroidWebView() { - // See DoAddScheme above. - DCHECK(!scheme_registries_locked) - << "Trying to modify schemes after the lists have been locked."; - GetSchemeRegistry()->allow_non_standard_schemes = true; + DoSchemeModificationPreamble(); + GetSchemeRegistryWithoutLocking()->allow_non_standard_schemes = true; } bool AllowNonStandardSchemesForAndroidWebView() { - return GetSchemeRegistry()->allow_non_standard_schemes; + return GetSchemeRegistry().allow_non_standard_schemes; } void AddStandardScheme(const char* new_scheme, SchemeType type) { - DoAddSchemeWithType(new_scheme, type, &GetSchemeRegistry()->standard_schemes); + DoAddSchemeWithType(new_scheme, type, + &GetSchemeRegistryWithoutLocking()->standard_schemes); } void AddReferrerScheme(const char* new_scheme, SchemeType type) { - DoAddSchemeWithType(new_scheme, type, &GetSchemeRegistry()->referrer_schemes); + DoAddSchemeWithType(new_scheme, type, + &GetSchemeRegistryWithoutLocking()->referrer_schemes); } void AddSecureScheme(const char* new_scheme) { - DoAddScheme(new_scheme, &GetSchemeRegistry()->secure_schemes); + DoAddScheme(new_scheme, &GetSchemeRegistryWithoutLocking()->secure_schemes); } const std::vector& GetSecureSchemes() { - return GetSchemeRegistry()->secure_schemes; + return GetSchemeRegistry().secure_schemes; } void AddLocalScheme(const char* new_scheme) { - DoAddScheme(new_scheme, &GetSchemeRegistry()->local_schemes); + DoAddScheme(new_scheme, &GetSchemeRegistryWithoutLocking()->local_schemes); } const std::vector& GetLocalSchemes() { - return GetSchemeRegistry()->local_schemes; + return GetSchemeRegistry().local_schemes; } void AddNoAccessScheme(const char* new_scheme) { - DoAddScheme(new_scheme, &GetSchemeRegistry()->no_access_schemes); + DoAddScheme(new_scheme, + &GetSchemeRegistryWithoutLocking()->no_access_schemes); } const std::vector& GetNoAccessSchemes() { - return GetSchemeRegistry()->no_access_schemes; + return GetSchemeRegistry().no_access_schemes; } void AddCorsEnabledScheme(const char* new_scheme) { - DoAddScheme(new_scheme, &GetSchemeRegistry()->cors_enabled_schemes); + DoAddScheme(new_scheme, + &GetSchemeRegistryWithoutLocking()->cors_enabled_schemes); } const std::vector& GetCorsEnabledSchemes() { - return GetSchemeRegistry()->cors_enabled_schemes; + return GetSchemeRegistry().cors_enabled_schemes; } void AddWebStorageScheme(const char* new_scheme) { - DoAddScheme(new_scheme, &GetSchemeRegistry()->web_storage_schemes); + DoAddScheme(new_scheme, + &GetSchemeRegistryWithoutLocking()->web_storage_schemes); } const std::vector& GetWebStorageSchemes() { - return GetSchemeRegistry()->web_storage_schemes; + return GetSchemeRegistry().web_storage_schemes; } void AddCSPBypassingScheme(const char* new_scheme) { - DoAddScheme(new_scheme, &GetSchemeRegistry()->csp_bypassing_schemes); + DoAddScheme(new_scheme, + &GetSchemeRegistryWithoutLocking()->csp_bypassing_schemes); } const std::vector& GetCSPBypassingSchemes() { - return GetSchemeRegistry()->csp_bypassing_schemes; + return GetSchemeRegistry().csp_bypassing_schemes; } void AddEmptyDocumentScheme(const char* new_scheme) { - DoAddScheme(new_scheme, &GetSchemeRegistry()->empty_document_schemes); + DoAddScheme(new_scheme, + &GetSchemeRegistryWithoutLocking()->empty_document_schemes); } const std::vector& GetEmptyDocumentSchemes() { - return GetSchemeRegistry()->empty_document_schemes; + return GetSchemeRegistry().empty_document_schemes; } void LockSchemeRegistries() { @@ -583,7 +645,7 @@ bool IsStandard(const base::char16* spec, const Component& scheme) { bool IsReferrerScheme(const char* spec, const Component& scheme) { SchemeType unused_scheme_type; return DoIsInSchemes(spec, scheme, &unused_scheme_type, - GetSchemeRegistry()->referrer_schemes); + GetSchemeRegistry().referrer_schemes); } bool FindAndCompareScheme(const char* str, diff --git a/url/url_util.h b/url/url_util.h index 676c06876e841b..d4f5e1798ddca8 100644 --- a/url/url_util.h +++ b/url/url_util.h @@ -5,6 +5,7 @@ #ifndef URL_URL_UTIL_H_ #define URL_URL_UTIL_H_ +#include #include #include @@ -19,8 +20,22 @@ namespace url { // Init ------------------------------------------------------------------------ -// Resets all custom schemes to the default values. Not thread-safe. -COMPONENT_EXPORT(URL) void ResetForTests(); +// Used for tests that need to reset schemes. Note that this can only be used +// in conjunction with ScopedSchemeRegistryForTests. +COMPONENT_EXPORT(URL) void ClearSchemesForTests(); + +class ScopedSchemeRegistryInternal; + +// Stores the SchemeRegistry upon creation, allowing tests to modify a copy of +// it, and restores the original SchemeRegistry when deleted. +class COMPONENT_EXPORT(URL) ScopedSchemeRegistryForTests { + public: + ScopedSchemeRegistryForTests(); + ~ScopedSchemeRegistryForTests(); + + private: + std::unique_ptr internal_; +}; // Schemes --------------------------------------------------------------------- @@ -39,7 +54,7 @@ COMPONENT_EXPORT(URL) bool AllowNonStandardSchemesForAndroidWebView(); // The following Add*Scheme method are not threadsafe and can not be called // concurrently with any other url_util function. They will assert if the lists -// of schemes have been locked (see LockSchemeRegistries). +// of schemes have been locked (see LockSchemeRegistries), or used. // Adds an application-defined scheme to the internal list of "standard-format" // URL schemes. A standard-format scheme adheres to what RFC 3986 calls "generic diff --git a/url/url_util_unittest.cc b/url/url_util_unittest.cc index aed5fa899ac8ee..ea4cd82aa7a37e 100644 --- a/url/url_util_unittest.cc +++ b/url/url_util_unittest.cc @@ -17,12 +17,11 @@ namespace url { class URLUtilTest : public testing::Test { public: URLUtilTest() = default; - ~URLUtilTest() override { - // Reset any added schemes. - ResetForTests(); - } + ~URLUtilTest() override = default; private: + ScopedSchemeRegistryForTests scoped_registry_; + DISALLOW_COPY_AND_ASSIGN(URLUtilTest); }; @@ -92,21 +91,24 @@ TEST_F(URLUtilTest, IsReferrerScheme) { } TEST_F(URLUtilTest, AddReferrerScheme) { - const char kFooScheme[] = "foo"; + static const char kFooScheme[] = "foo"; EXPECT_FALSE(IsReferrerScheme(kFooScheme, Component(0, strlen(kFooScheme)))); + url::ScopedSchemeRegistryForTests scoped_registry; AddReferrerScheme(kFooScheme, url::SCHEME_WITH_HOST); EXPECT_TRUE(IsReferrerScheme(kFooScheme, Component(0, strlen(kFooScheme)))); } TEST_F(URLUtilTest, ShutdownCleansUpSchemes) { - const char kFooScheme[] = "foo"; + static const char kFooScheme[] = "foo"; EXPECT_FALSE(IsReferrerScheme(kFooScheme, Component(0, strlen(kFooScheme)))); - AddReferrerScheme(kFooScheme, url::SCHEME_WITH_HOST); - EXPECT_TRUE(IsReferrerScheme(kFooScheme, Component(0, strlen(kFooScheme)))); + { + url::ScopedSchemeRegistryForTests scoped_registry; + AddReferrerScheme(kFooScheme, url::SCHEME_WITH_HOST); + EXPECT_TRUE(IsReferrerScheme(kFooScheme, Component(0, strlen(kFooScheme)))); + } - ResetForTests(); EXPECT_FALSE(IsReferrerScheme(kFooScheme, Component(0, strlen(kFooScheme)))); }