From 91e32b14000f146ec1815f268cc0fee2123e4f8e Mon Sep 17 00:00:00 2001 From: Aaron Colwell Date: Wed, 17 Feb 2021 01:40:56 +0000 Subject: [PATCH] Remove SiteInstance::GetSiteForURL(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing SiteInstance::GetSiteForURL() and migrate existing usage to alternatives like GetSiteURL() on the SiteInstance or SiteInfo based checks. This method was only used for testing and no production code paths are affected. Bug: 1085275 Change-Id: I9be7cb73508de32851b981316c63e10c3cdf0b16 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2680274 Reviewed-by: Giovanni Ortuño Urquidi Reviewed-by: Nasko Oskov Commit-Queue: Aaron Colwell Cr-Commit-Position: refs/heads/master@{#854574} --- ...rome_content_browser_client_browsertest.cc | 13 +++- .../ui/extensions/hosted_app_browsertest.cc | 34 +++++---- .../browser/isolated_origin_browsertest.cc | 7 +- .../navigation_controller_impl_browsertest.cc | 8 ++- .../renderer_host/navigator_unittest.cc | 72 ++++++++++--------- .../render_frame_host_manager_browsertest.cc | 26 +++---- content/browser/site_instance_impl.cc | 21 ------ .../browser/site_instance_impl_unittest.cc | 46 ++++++------ .../webui/web_ui_navigation_browsertest.cc | 50 +++++++------ content/public/browser/site_instance.h | 6 -- content/public/test/test_utils.cc | 5 +- 11 files changed, 148 insertions(+), 140 deletions(-) diff --git a/chrome/browser/chrome_content_browser_client_browsertest.cc b/chrome/browser/chrome_content_browser_client_browsertest.cc index 90a71a2be1af61..d1ca2b929ebec7 100644 --- a/chrome/browser/chrome_content_browser_client_browsertest.cc +++ b/chrome/browser/chrome_content_browser_client_browsertest.cc @@ -149,11 +149,16 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginNTPBrowserTest, scoped_refptr site_instance = content::SiteInstance::CreateForURL(context, isolated_url); EXPECT_TRUE(site_instance->RequiresDedicatedProcess()); + // Verify the isolated origin does not receive an NTP site URL scheme. + EXPECT_FALSE( + site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme)); // The site URL for the NTP URL should resolve to a chrome-search:// URL via // GetEffectiveURL(), even if the NTP URL matches an isolated origin. - GURL site_url(content::SiteInstance::GetSiteForURL(context, ntp_url)); - EXPECT_TRUE(site_url.SchemeIs(chrome::kChromeSearchScheme)); + scoped_refptr ntp_site_instance = + content::SiteInstance::CreateForURL(context, ntp_url); + EXPECT_TRUE( + ntp_site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme)); // Navigate to the NTP URL and verify that the resulting process is marked as // an Instant process. @@ -164,12 +169,16 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginNTPBrowserTest, InstantServiceFactory::GetForProfile(browser()->profile()); EXPECT_TRUE(instant_service->IsInstantProcess( contents->GetMainFrame()->GetProcess()->GetID())); + EXPECT_EQ(contents->GetMainFrame()->GetSiteInstance()->GetSiteURL(), + ntp_site_instance->GetSiteURL()); // Navigating to a non-NTP URL on ntp.com should not result in an Instant // process. ui_test_utils::NavigateToURL(browser(), isolated_url); EXPECT_FALSE(instant_service->IsInstantProcess( contents->GetMainFrame()->GetProcess()->GetID())); + EXPECT_EQ(contents->GetMainFrame()->GetSiteInstance()->GetSiteURL(), + site_instance->GetSiteURL()); } // Helper class to test window creation from NTP. diff --git a/chrome/browser/ui/extensions/hosted_app_browsertest.cc b/chrome/browser/ui/extensions/hosted_app_browsertest.cc index 5d63dcd1c2ffd0..e44a0e6036e46f 100644 --- a/chrome/browser/ui/extensions/hosted_app_browsertest.cc +++ b/chrome/browser/ui/extensions/hosted_app_browsertest.cc @@ -850,6 +850,12 @@ class HostedAppProcessModelTest : public HostedOrWebAppTest { return subframe; } + GURL GetSiteForURL(content::BrowserContext* browser_context, + const GURL& url) { + return content::SiteInstance::CreateForURL(browser_context, url) + ->GetSiteURL(); + } + protected: bool should_swap_for_cross_site_; @@ -897,22 +903,22 @@ IN_PROC_BROWSER_TEST_P(HostedAppProcessModelTest, IframesInsideHostedApp) { RenderFrameHost* cross_site = find_frame("CrossSite"); // Sanity-check sites of all relevant frames to verify test setup. - GURL app_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), app->GetLastCommittedURL()); + GURL app_site = + GetSiteForURL(app_browser_->profile(), app->GetLastCommittedURL()); EXPECT_EQ(extensions::kExtensionScheme, app_site.scheme()); - GURL same_dir_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), same_dir->GetLastCommittedURL()); + GURL same_dir_site = + GetSiteForURL(app_browser_->profile(), same_dir->GetLastCommittedURL()); EXPECT_EQ(extensions::kExtensionScheme, same_dir_site.scheme()); EXPECT_EQ(same_dir_site, app_site); - GURL diff_dir_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), diff_dir->GetLastCommittedURL()); + GURL diff_dir_site = + GetSiteForURL(app_browser_->profile(), diff_dir->GetLastCommittedURL()); EXPECT_NE(extensions::kExtensionScheme, diff_dir_site.scheme()); EXPECT_NE(diff_dir_site, app_site); - GURL same_site_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), same_site->GetLastCommittedURL()); + GURL same_site_site = + GetSiteForURL(app_browser_->profile(), same_site->GetLastCommittedURL()); EXPECT_NE(extensions::kExtensionScheme, same_site_site.scheme()); EXPECT_NE(same_site_site, app_site); EXPECT_EQ(same_site_site, diff_dir_site); @@ -928,15 +934,15 @@ IN_PROC_BROWSER_TEST_P(HostedAppProcessModelTest, IframesInsideHostedApp) { // content/public via SiteInfo. For now, this verification will be done // implicitly by comparing SiteInstances and then actual processes further // below. - GURL isolated_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), isolated->GetLastCommittedURL()); + GURL isolated_site = + GetSiteForURL(app_browser_->profile(), isolated->GetLastCommittedURL()); EXPECT_EQ(extensions::kExtensionScheme, isolated_site.scheme()); EXPECT_EQ(isolated_site, app_site); EXPECT_NE(isolated->GetSiteInstance(), app->GetSiteInstance()); EXPECT_NE(isolated_site, diff_dir_site); - GURL cross_site_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), cross_site->GetLastCommittedURL()); + GURL cross_site_site = + GetSiteForURL(app_browser_->profile(), cross_site->GetLastCommittedURL()); EXPECT_NE(cross_site_site, app_site); EXPECT_NE(cross_site_site, same_site_site); @@ -1341,8 +1347,8 @@ IN_PROC_BROWSER_TEST_P(HostedAppIsolatedOriginTest, RenderFrameHost* app = web_contents->GetMainFrame(); EXPECT_EQ(extensions::kExtensionScheme, app->GetSiteInstance()->GetSiteURL().scheme()); - GURL app_site = content::SiteInstance::GetSiteForURL( - app_browser_->profile(), app->GetLastCommittedURL()); + GURL app_site = + GetSiteForURL(app_browser_->profile(), app->GetLastCommittedURL()); EXPECT_EQ(extensions::kExtensionScheme, app_site.scheme()); EXPECT_TRUE(process_map_->Contains(app->GetProcess()->GetID())); diff --git a/content/browser/isolated_origin_browsertest.cc b/content/browser/isolated_origin_browsertest.cc index 56db17a2b70bac..b48949c3f0d15f 100644 --- a/content/browser/isolated_origin_browsertest.cc +++ b/content/browser/isolated_origin_browsertest.cc @@ -3122,9 +3122,10 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginTest, SubframeErrorPages) { if (!SiteIsolationPolicy::IsErrorPageIsolationEnabled( /*in_main_frame=*/false)) { EXPECT_EQ( - SiteInstance::GetSiteForURL(web_contents()->GetBrowserContext(), - regular_url), - child2->current_frame_host()->GetSiteInstance()->GetSiteURL()); + SiteInfo::CreateForTesting( + IsolationContext(web_contents()->GetBrowserContext()), + regular_url), + child2->current_frame_host()->GetSiteInstance()->GetSiteInfo()); } } else { EXPECT_EQ(root->current_frame_host()->GetSiteInstance(), diff --git a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc index a7f79b3627706e..241c2a83f3db6e 100644 --- a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc +++ b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc @@ -11531,9 +11531,11 @@ IN_PROC_BROWSER_TEST_P(NavigationControllerBrowserTest, if (AreAllSitesIsolatedForTesting()) { EXPECT_NE(initial_site_instance, root->current_frame_host()->GetSiteInstance()); - EXPECT_EQ(SiteInstance::GetSiteForURL( - shell()->web_contents()->GetBrowserContext(), url3), - root->current_frame_host()->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ( + SiteInfo::CreateForTesting( + IsolationContext(shell()->web_contents()->GetBrowserContext()), + url3), + root->current_frame_host()->GetSiteInstance()->GetSiteInfo()); EXPECT_EQ(NAVIGATION_TYPE_NEW_ENTRY, capturer.navigation_type()); } else { EXPECT_EQ(initial_site_instance, diff --git a/content/browser/renderer_host/navigator_unittest.cc b/content/browser/renderer_host/navigator_unittest.cc index a35b6fc720ca18..e7b3da8a5fe885 100644 --- a/content/browser/renderer_host/navigator_unittest.cc +++ b/content/browser/renderer_host/navigator_unittest.cc @@ -75,13 +75,19 @@ class NavigatorTest : public RenderViewHostImplTestHarness { node->render_manager()->speculative_render_frame_host_.get()); } - scoped_refptr ConvertToSiteInstance( + scoped_refptr ConvertToSiteInstance( RenderFrameHostManager* rfhm, const SiteInstanceDescriptor& descriptor, SiteInstance* candidate_instance) { - return rfhm->ConvertToSiteInstance( - descriptor, static_cast(candidate_instance), - false /* is_speculative */); + return static_cast( + rfhm->ConvertToSiteInstance( + descriptor, static_cast(candidate_instance), + false /* is_speculative */) + .get()); + } + + SiteInfo CreateExpectedSiteInfo(const GURL& url) { + return SiteInfo::CreateForTesting(IsolationContext(browser_context()), url); } }; @@ -122,8 +128,8 @@ TEST_F(NavigatorTest, SimpleBrowserInitiatedNavigationFromNonLiveRenderer) { if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(main_test_rfh()->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl), - main_test_rfh()->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrl), + main_test_rfh()->GetSiteInstance()->GetSiteInfo()); } EXPECT_EQ(kUrl, contents()->GetLastCommittedURL()); @@ -190,8 +196,8 @@ TEST_F(NavigatorTest, SimpleRendererInitiatedSameSiteNavigation) { if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(main_test_rfh()->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl2), - main_test_rfh()->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrl2), + main_test_rfh()->GetSiteInstance()->GetSiteInfo()); } EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL()); EXPECT_FALSE(GetSpeculativeRenderFrameHost(node)); @@ -658,9 +664,9 @@ TEST_F(NavigatorTest, RedirectCrossSite) { TEST_F(NavigatorTest, BrowserInitiatedNavigationCancel) { const GURL kUrl0("http://www.wikipedia.org/"); const GURL kUrl1("http://www.chromium.org/"); - const GURL kUrl1_site = SiteInstance::GetSiteForURL(browser_context(), kUrl1); + const auto kUrl1SiteInfo = CreateExpectedSiteInfo(kUrl1); const GURL kUrl2("http://www.google.com/"); - const GURL kUrl2_site = SiteInstance::GetSiteForURL(browser_context(), kUrl2); + const auto kUrl2SiteInfo = CreateExpectedSiteInfo(kUrl2); // Initialization. contents()->NavigateAndCommit(kUrl0); @@ -686,7 +692,7 @@ TEST_F(NavigatorTest, BrowserInitiatedNavigationCancel) { if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(speculative_rfh->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(kUrl1_site, speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(kUrl1SiteInfo, speculative_rfh->GetSiteInstance()->GetSiteInfo()); } // Request navigation to the 2nd URL; the NavigationRequest must have been @@ -726,7 +732,7 @@ TEST_F(NavigatorTest, BrowserInitiatedNavigationCancel) { if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(main_test_rfh()->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(kUrl2SiteInfo, main_test_rfh()->GetSiteInstance()->GetSiteInfo()); } EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL()); @@ -1019,8 +1025,8 @@ TEST_F(NavigatorTest, SpeculativeRendererWorksBaseCase) { if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(speculative_rfh->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl), - speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrl), + speculative_rfh->GetSiteInstance()->GetSiteInfo()); } navigation->ReadyToCommit(); @@ -1059,8 +1065,8 @@ TEST_F(NavigatorTest, SpeculativeRendererDiscardedAfterRedirectToAnotherSite) { if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(speculative_rfh->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl), - speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrl), + speculative_rfh->GetSiteInstance()->GetSiteInfo()); } // It then redirects to yet another site. @@ -1103,8 +1109,8 @@ TEST_F(NavigatorTest, SpeculativeRendererDiscardedAfterRedirectToAnotherSite) { // the SiteInstance stayed the same. EXPECT_FALSE(rfh_deleted_observer.deleted()); } else { - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrlRedirect), - speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrlRedirect), + speculative_rfh->GetSiteInstance()->GetSiteInfo()); EXPECT_NE(site_instance_id, redirect_site_instance_id); // Verify the old speculative RenderFrameHost was deleted because @@ -1230,7 +1236,7 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) { // 4) Convert a descriptor of a related instance with a site different from // the current one. GURL kUrlSameSiteAs2("http://www.b.com/foo"); - scoped_refptr related_instance; + scoped_refptr related_instance; { SiteInstanceDescriptor descriptor( UrlInfo::CreateForTesting(kUrlSameSiteAs2), @@ -1246,15 +1252,12 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) { EXPECT_NE(current_instance, related_instance.get()); EXPECT_NE(unrelated_instance.get(), related_instance.get()); - auto* related_instance_impl = - static_cast(related_instance.get()); - if (AreDefaultSiteInstancesEnabled()) { - ASSERT_TRUE(related_instance_impl->IsDefaultSiteInstance()); + ASSERT_TRUE(related_instance->IsDefaultSiteInstance()); } else { EXPECT_EQ(SiteInfo::CreateForTesting( current_instance->GetIsolationContext(), kUrlSameSiteAs2), - related_instance_impl->GetSiteInfo()); + related_instance->GetSiteInfo()); } } @@ -1265,7 +1268,7 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) { UrlInfo::CreateForTesting(kUrlSameSiteAs1), SiteInstanceRelation::UNRELATED, CoopCoepCrossOriginIsolatedInfo::CreateNonIsolated()); - scoped_refptr converted_instance_1 = + scoped_refptr converted_instance_1 = ConvertToSiteInstance(rfhm, descriptor, nullptr); // Should return a new instance, unrelated to the current one, set to the // provided site URL. @@ -1273,12 +1276,12 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) { current_instance->IsRelatedSiteInstance(converted_instance_1.get())); EXPECT_NE(current_instance, converted_instance_1.get()); EXPECT_NE(unrelated_instance.get(), converted_instance_1.get()); - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrlSameSiteAs1), - converted_instance_1->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrlSameSiteAs1), + converted_instance_1->GetSiteInfo()); // Does the same but this time using unrelated_instance as a candidate, // which has a different site. - scoped_refptr converted_instance_2 = + scoped_refptr converted_instance_2 = ConvertToSiteInstance(rfhm, descriptor, unrelated_instance.get()); // Should return yet another new instance, unrelated to the current one, set // to the same site URL. @@ -1287,8 +1290,8 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) { EXPECT_NE(current_instance, converted_instance_2.get()); EXPECT_NE(unrelated_instance.get(), converted_instance_2.get()); EXPECT_NE(converted_instance_1.get(), converted_instance_2.get()); - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrlSameSiteAs1), - converted_instance_2->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrlSameSiteAs1), + converted_instance_1->GetSiteInfo()); // Converts once more but with |converted_instance_1| as a candidate. scoped_refptr converted_instance_3 = @@ -1305,7 +1308,7 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) { UrlInfo::CreateForTesting(kUrlSameSiteAs2), SiteInstanceRelation::UNRELATED, CoopCoepCrossOriginIsolatedInfo::CreateNonIsolated()); - scoped_refptr converted_instance_1 = + scoped_refptr converted_instance_1 = ConvertToSiteInstance(rfhm, descriptor, related_instance.get()); // Should return a new instance, unrelated to the current, set to the // provided site URL. @@ -1315,11 +1318,10 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) { EXPECT_NE(unrelated_instance.get(), converted_instance_1.get()); if (AreDefaultSiteInstancesEnabled()) { - EXPECT_TRUE(static_cast(converted_instance_1.get()) - ->IsDefaultSiteInstance()); + EXPECT_TRUE(converted_instance_1->IsDefaultSiteInstance()); } else { - EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrlSameSiteAs2), - converted_instance_1->GetSiteURL()); + EXPECT_EQ(CreateExpectedSiteInfo(kUrlSameSiteAs2), + converted_instance_1->GetSiteInfo()); } scoped_refptr converted_instance_2 = diff --git a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc index 5bf544c11baf6c..cd17f9661545be 100644 --- a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc +++ b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc @@ -2014,10 +2014,12 @@ IN_PROC_BROWSER_TEST_P( const GURL kCrossSiteURL = embedded_test_server()->GetURL("c.com", "/title1.html"); - const GURL kOriginalSiteURL = SiteInstance::GetSiteForURL( - shell()->web_contents()->GetBrowserContext(), kOriginalURL); - const GURL kRedirectSiteURL = SiteInstance::GetSiteForURL( - shell()->web_contents()->GetBrowserContext(), kRedirectURL); + IsolationContext isolation_context( + shell()->web_contents()->GetBrowserContext()); + const auto kOriginalSiteInfo = + SiteInfo::CreateForTesting(isolation_context, kOriginalURL); + const auto kRedirectSiteInfo = + SiteInfo::CreateForTesting(isolation_context, kRedirectURL); // First navigate to the initial URL. shell()->LoadURL(kOriginalURL); @@ -2072,13 +2074,13 @@ IN_PROC_BROWSER_TEST_P( CHECK(speculative_rfh); EXPECT_TRUE(speculative_rfh->is_loading()); if (AreAllSitesIsolatedForTesting()) { - EXPECT_EQ(kRedirectSiteURL, - speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(kRedirectSiteInfo, + speculative_rfh->GetSiteInstance()->GetSiteInfo()); } else if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(speculative_rfh->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(kOriginalSiteURL, - speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(kOriginalSiteInfo, + speculative_rfh->GetSiteInstance()->GetSiteInfo()); } int site_instance_id = speculative_rfh->GetSiteInstance()->GetId(); @@ -2098,8 +2100,8 @@ IN_PROC_BROWSER_TEST_P( if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(speculative_rfh->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(kRedirectSiteURL, - speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(kRedirectSiteInfo, + speculative_rfh->GetSiteInstance()->GetSiteInfo()); if (AreAllSitesIsolatedForTesting()) EXPECT_EQ(site_instance_id, speculative_rfh->GetSiteInstance()->GetId()); } @@ -2120,8 +2122,8 @@ IN_PROC_BROWSER_TEST_P( if (AreDefaultSiteInstancesEnabled()) { EXPECT_TRUE(speculative_rfh->GetSiteInstance()->IsDefaultSiteInstance()); } else { - EXPECT_EQ(kOriginalSiteURL, - speculative_rfh->GetSiteInstance()->GetSiteURL()); + EXPECT_EQ(kOriginalSiteInfo, + speculative_rfh->GetSiteInstance()->GetSiteInfo()); } if (AreAllSitesIsolatedForTesting()) EXPECT_NE(site_instance_id, speculative_rfh->GetSiteInstance()->GetId()); diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc index cef08cbe4bf82a..211c1a56b28037 100644 --- a/content/browser/site_instance_impl.cc +++ b/content/browser/site_instance_impl.cc @@ -1443,27 +1443,6 @@ void SiteInstanceImpl::PreventOptInOriginIsolation( true /* is_global_walk_or_frame_removal */); } -// static -GURL SiteInstance::GetSiteForURL(BrowserContext* browser_context, - const GURL& url) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - DCHECK(browser_context); - - // By default, GetSiteForURL will resolve |url| to an effective URL - // before computing its site. - // - // TODO(alexmos): Callers inside content/ should already be using the - // internal SiteInstanceImpl version and providing a proper IsolationContext. - // For callers outside content/, plumb the applicable IsolationContext here, - // where needed. Eventually, GetSiteForURL should always require an - // IsolationContext to be passed in, and this implementation should just - // become SiteInstanceImpl::GetSiteForURL. - return SiteInfo::Create(IsolationContext(browser_context), - UrlInfo(url, false /* origin_requests_isolation */), - CoopCoepCrossOriginIsolatedInfo::CreateNonIsolated()) - .site_url(); -} - // static bool SiteInstanceImpl::CanBePlacedInDefaultSiteInstance( const IsolationContext& isolation_context, diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc index 8656f61ac1e58c..e0d6dcaede2a8d 100644 --- a/content/browser/site_instance_impl_unittest.cc +++ b/content/browser/site_instance_impl_unittest.cc @@ -191,6 +191,10 @@ class SiteInstanceTest : public testing::Test { BrowserContext* context() { return &context_; } + GURL GetSiteForURL(const GURL& url) { + return GetSiteInfoForURL(url).site_url(); + } + SiteInfo GetSiteInfoForURL(const std::string& url) { return SiteInfo::CreateForTesting(IsolationContext(&context_), GURL(url)); } @@ -535,51 +539,51 @@ TEST_F(SiteInstanceTest, GetSiteForURL) { // Pages are irrelevant. GURL test_url = GURL("http://www.google.com/index.html"); - GURL site_url = SiteInstance::GetSiteForURL(&context, test_url); + GURL site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://google.com"), site_url); EXPECT_EQ("http", site_url.scheme()); EXPECT_EQ("google.com", site_url.host()); // Ports are irrelevant. test_url = GURL("https://www.google.com:8080"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("https://google.com"), site_url); // Punycode is canonicalized. test_url = GURL("http://☃snowperson☃.net:333/"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://xn--snowperson-di0gka.net"), site_url); // Username and password are stripped out. test_url = GURL("ftp://username:password@ftp.chromium.org/files/README"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("ftp://chromium.org"), site_url); // Literal IP addresses of any flavor are okay. test_url = GURL("http://127.0.0.1/a.html"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://127.0.0.1"), site_url); EXPECT_EQ("127.0.0.1", site_url.host()); test_url = GURL("http://2130706433/a.html"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://127.0.0.1"), site_url); EXPECT_EQ("127.0.0.1", site_url.host()); test_url = GURL("http://[::1]:2/page.html"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://[::1]"), site_url); EXPECT_EQ("[::1]", site_url.host()); // Hostnames without TLDs are okay. test_url = GURL("http://foo/a.html"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://foo"), site_url); EXPECT_EQ("foo", site_url.host()); // File URLs should include the scheme. test_url = GURL("file:///C:/Downloads/"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("file:"), site_url); EXPECT_EQ("file", site_url.scheme()); EXPECT_FALSE(site_url.has_host()); @@ -588,26 +592,26 @@ TEST_F(SiteInstanceTest, GetSiteForURL) { // maps *all* file://... URLs into "file://" origin) such file URLs still need // to map into "file:" site URL. See also https://crbug.com/776160. test_url = GURL("file://server/path"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("file:"), site_url); EXPECT_EQ("file", site_url.scheme()); EXPECT_FALSE(site_url.has_host()); // Data URLs should include the whole URL, except for the hash. test_url = GURL("data:text/html,foo"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(test_url, site_url); EXPECT_EQ("data", site_url.scheme()); EXPECT_FALSE(site_url.has_host()); test_url = GURL("data:text/html,foo#bar"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_FALSE(site_url.has_ref()); EXPECT_NE(test_url, site_url); EXPECT_TRUE(site_url.EqualsIgnoringRef(test_url)); // Javascript URLs should include the scheme. test_url = GURL("javascript:foo();"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("javascript:"), site_url); EXPECT_EQ("javascript", site_url.scheme()); EXPECT_FALSE(site_url.has_host()); @@ -616,12 +620,12 @@ TEST_F(SiteInstanceTest, GetSiteForURL) { test_url = GURL( "blob:https://www.ftp.chromium.org/" "4d4ff040-6d61-4446-86d3-13ca07ec9ab9"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("https://chromium.org"), site_url); // Blob URLs with file origin also extract the site from the origin. test_url = GURL("blob:file:///1029e5a4-2983-4b90-a585-ed217563acfeb"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("file:"), site_url); EXPECT_EQ("file", site_url.scheme()); EXPECT_FALSE(site_url.has_host()); @@ -629,10 +633,10 @@ TEST_F(SiteInstanceTest, GetSiteForURL) { // Blob URLs created from a unique origin use the full URL as the site URL, // except for the hash. test_url = GURL("blob:null/1029e5a4-2983-4b90-a585-ed217563acfeb"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(test_url, site_url); test_url = GURL("blob:null/1029e5a4-2983-4b90-a585-ed217563acfeb#foo"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_FALSE(site_url.has_ref()); EXPECT_NE(test_url, site_url); EXPECT_TRUE(site_url.EqualsIgnoringRef(test_url)); @@ -641,26 +645,26 @@ TEST_F(SiteInstanceTest, GetSiteForURL) { test_url = GURL( "blob:http://www.example.appspot.com:44/" "4d4ff040-6d61-4446-86d3-13ca07ec9ab9"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://example.appspot.com"), site_url); // The site of filesystem URLs is determined by the inner URL. test_url = GURL("filesystem:http://www.google.com/foo/bar.html?foo#bar"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(GURL("http://google.com"), site_url); // Error page URLs. auto error_site_info = SiteInfo::CreateForErrorPage( CoopCoepCrossOriginIsolatedInfo::CreateNonIsolated()); test_url = GURL(kUnreachableWebDataURL); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(error_site_info.site_url(), site_url); // Verify that other URLs that use the chrome-error scheme also map // to the error page SiteInfo. These type of URLs should not appear in the // codebase, but the mapping is intended to cover the whole scheme. test_url = GURL("chrome-error://someerror"); - site_url = SiteInstance::GetSiteForURL(&context, test_url); + site_url = GetSiteForURL(test_url); EXPECT_EQ(error_site_info.site_url(), site_url); DrainMessageLoop(); diff --git a/content/browser/webui/web_ui_navigation_browsertest.cc b/content/browser/webui/web_ui_navigation_browsertest.cc index e13a8f7c4199c1..aaa05b211e2cda 100644 --- a/content/browser/webui/web_ui_navigation_browsertest.cc +++ b/content/browser/webui/web_ui_navigation_browsertest.cc @@ -930,21 +930,25 @@ IN_PROC_BROWSER_TEST_F(WebUINavigationBrowserTest, IN_PROC_BROWSER_TEST_F(WebUINavigationBrowserTest, WebUIOriginsRequireDedicatedProcess) { - GURL chrome_url(GetWebUIURL("web-ui/title1.html")); - GURL expected_site_url(GetWebUIURL("web-ui")); - // chrome:// URLs should require a dedicated process. WebContents* web_contents = shell()->web_contents(); BrowserContext* browser_context = web_contents->GetBrowserContext(); - EXPECT_TRUE(DoesURLRequireDedicatedProcess(IsolationContext(browser_context), - chrome_url)); + IsolationContext isolation_context(browser_context); + + GURL chrome_url(GetWebUIURL("web-ui/title1.html")); + auto expected_site_info = + SiteInfo::CreateForTesting(isolation_context, chrome_url); + + EXPECT_TRUE(DoesURLRequireDedicatedProcess(isolation_context, chrome_url)); // Navigate to a WebUI page. EXPECT_TRUE(NavigateToURL(shell(), chrome_url)); // Verify that the "hostname" is also part of the site URL. - GURL site_url = web_contents->GetMainFrame()->GetSiteInstance()->GetSiteURL(); - EXPECT_EQ(expected_site_url, site_url); + auto site_info = static_cast( + web_contents->GetMainFrame()->GetSiteInstance()) + ->GetSiteInfo(); + EXPECT_EQ(expected_site_info, site_info); // Ask the page to create a blob URL and return back the blob URL. const char* kScript = R"( @@ -959,33 +963,37 @@ IN_PROC_BROWSER_TEST_F(WebUINavigationBrowserTest, // Verify that the blob also requires a dedicated process and that it would // use the same site url as the original page. - EXPECT_TRUE(DoesURLRequireDedicatedProcess(IsolationContext(browser_context), - blob_url)); - EXPECT_EQ(expected_site_url, - SiteInstance::GetSiteForURL(browser_context, blob_url)); + EXPECT_TRUE(DoesURLRequireDedicatedProcess(isolation_context, blob_url)); + EXPECT_EQ(expected_site_info, + SiteInfo::CreateForTesting(isolation_context, blob_url)); } // Verify chrome-untrusted:// uses a dedicated process. IN_PROC_BROWSER_TEST_F(WebUINavigationBrowserTest, UntrustedWebUIOriginsRequireDedicatedProcess) { + // chrome-untrusted:// URLs should require a dedicated process. + WebContents* web_contents = shell()->web_contents(); + BrowserContext* browser_context = web_contents->GetBrowserContext(); + IsolationContext isolation_context(browser_context); + // Add a DataSource which disallows iframes by default. untrusted_factory().add_web_ui_config( std::make_unique("test-host")); GURL chrome_untrusted_url(GetChromeUntrustedUIURL("test-host/title1.html")); - GURL expected_site_url(GetChromeUntrustedUIURL("test-host")); + auto expected_site_info = SiteInfo::CreateForTesting( + isolation_context, GetChromeUntrustedUIURL("test-host")); - // chrome-untrusted:// URLs should require a dedicated process. - WebContents* web_contents = shell()->web_contents(); - BrowserContext* browser_context = web_contents->GetBrowserContext(); - EXPECT_TRUE(DoesURLRequireDedicatedProcess(IsolationContext(browser_context), - chrome_untrusted_url)); + EXPECT_TRUE( + DoesURLRequireDedicatedProcess(isolation_context, chrome_untrusted_url)); // Navigate to a chrome-untrusted:// page. EXPECT_TRUE(NavigateToURL(shell(), chrome_untrusted_url)); // Verify that the "hostname" is also part of the site URL. - GURL site_url = web_contents->GetMainFrame()->GetSiteInstance()->GetSiteURL(); - EXPECT_EQ(expected_site_url, site_url); + auto site_info = static_cast( + web_contents->GetMainFrame()->GetSiteInstance()) + ->GetSiteInfo(); + EXPECT_EQ(expected_site_info, site_info); // Ask the page to create a blob URL and return back the blob URL. const char* kScript = R"( @@ -1002,8 +1010,8 @@ IN_PROC_BROWSER_TEST_F(WebUINavigationBrowserTest, // use the same site url as the original page. EXPECT_TRUE(DoesURLRequireDedicatedProcess(IsolationContext(browser_context), blob_url)); - EXPECT_EQ(expected_site_url, - SiteInstance::GetSiteForURL(browser_context, blob_url)); + EXPECT_EQ(expected_site_info, + SiteInfo::CreateForTesting(isolation_context, blob_url)); } // Verify that navigating back/forward between WebUI and an error page for a diff --git a/content/public/browser/site_instance.h b/content/public/browser/site_instance.h index a721f2fbab6e62..25502c2f733b63 100644 --- a/content/public/browser/site_instance.h +++ b/content/public/browser/site_instance.h @@ -206,12 +206,6 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted { // chrome-native:// leave the site unassigned. static bool ShouldAssignSiteForURL(const GURL& url); - // Returns the site for the given URL, which includes only the scheme and - // registered domain. Returns an empty GURL if the URL has no host. Prior to - // determining the site, |url| is resolved to an effective URL via - // ContentBrowserClient::GetEffectiveURL(). - static GURL GetSiteForURL(BrowserContext* context, const GURL& url); - // Starts requiring a dedicated process for |url|'s site. On platforms where // strict site isolation is disabled, this may be used as a runtime signal // that a certain site should become process-isolated, because its security diff --git a/content/public/test/test_utils.cc b/content/public/test/test_utils.cc index 4f9708188e463b..2445e5d0de941c 100644 --- a/content/public/test/test_utils.cc +++ b/content/public/test/test_utils.cc @@ -548,8 +548,9 @@ bool EffectiveURLContentBrowserClient::DoesSiteRequireDedicatedProcess( return false; for (const auto& pair : urls_to_modify_) { - if (SiteInstance::GetSiteForURL(browser_context, pair.first) == - effective_site_url) + auto site_info = SiteInfo::CreateForTesting( + IsolationContext(browser_context), pair.first); + if (site_info.site_url() == effective_site_url) return true; } return false;