Move all SiteURL and lock URL computations to SiteInfo.

This change moves all site URL and lock URL generation out of
SiteInstanceImpl and into SiteInfo. This allows all details related
to the creation of these URLs to be hidden from other classes. Tests
were updated to use the new methods. ProcessLock creation was modified
slightly to make it easier to hide threading concerns related to
ProcessLocks and their related SiteInfos. A test was also added to verify
that ProcessLocks created on different threads match even though the
underlying SiteInfos do no. This should help prevent this behavior from
accidentally regressing in the future. There should not be any
user visible behavior changes introduced by this patch. It is primarily
just moving code around and renaming things.

Bug: 1085275
Change-Id: I46794960f2bb669c2e6e2750e139dc25cd03ae84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2680068
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#853243}

content/browser/browsing_instance.cc

@@ -189,8 +189,8 @@ BrowsingInstance::~BrowsingInstance() {
 
 SiteInfo BrowsingInstance::ComputeSiteInfoForURL(
     const UrlInfo& url_info) const {
-  return SiteInstanceImpl::ComputeSiteInfo(isolation_context_, url_info,
-                                           cross_origin_isolated_info_);
+  return SiteInfo::Create(isolation_context_, url_info,
+                          cross_origin_isolated_info_);
 }
 
 }  // namespace content

content/browser/child_process_security_policy_impl.cc

@@ -177,6 +177,25 @@ ProcessLock ProcessLock::CreateAllowAnySite(
       SiteInfo(GURL(), GURL(), false, cross_origin_isolated_info));
 }
 
+// static
+ProcessLock ProcessLock::Create(
+    const IsolationContext& isolation_context,
+    const UrlInfo& url_info,
+    const CoopCoepCrossOriginIsolatedInfo& cross_origin_isolated_info) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI))
+    return ProcessLock(SiteInfo::Create(isolation_context, url_info,
+                                        cross_origin_isolated_info));
+
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // On the IO thread we need to use a special SiteInfo creation method because
+  // we cannot properly compute some SiteInfo fields on that thread.
+  // ProcessLocks must always match no matter which thread they were created on,
+  // but the SiteInfo objects used to create them may not always match.
+  return ProcessLock(SiteInfo::CreateOnIOThread(isolation_context, url_info,
+                                                cross_origin_isolated_info));
+}
+
 ProcessLock::ProcessLock(const SiteInfo& site_info) : site_info_(site_info) {}
 
 ProcessLock::ProcessLock() = default;
@@ -1497,9 +1516,8 @@ CanCommitStatus ChildProcessSecurityPolicyImpl::CanCommitOriginAndUrl(
     // Check for special cases, like blob:null/ and data: URLs, where the
     // origin does not contain information to match against the process lock,
     // but using the whole URL can result in a process lock match.
-    const ProcessLock expected_process_lock =
-        SiteInstanceImpl::DetermineProcessLock(isolation_context, url_info,
-                                               cross_origin_isolated_info);
+    const auto expected_process_lock = ProcessLock::Create(
+        isolation_context, url_info, cross_origin_isolated_info);
     const ProcessLock& actual_process_lock = GetProcessLock(child_id);
     if (actual_process_lock == expected_process_lock)
       return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
@@ -1628,10 +1646,11 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(
             "[BI=%d]", browsing_instance_id.GetUnsafeValue());
         IsolationContext isolation_context(browsing_instance_id,
                                            browser_or_resource_context);
-        // NOTE: If we're on the IO thread, the call to DetermineProcessLock()
-        // below will return a ProcessLock with an (internally) identical
-        // site_url, one that does not use effective URLs. That's ok in this
-        // instance since we only ever look at the lock url.
+        // NOTE: If we're on the IO thread, the call to
+        // ProcessLock::Create() below will return a ProcessLock with
+        // an (internally) identical site_url, one that does not use effective
+        // URLs. That's ok in this instance since we only ever look at the lock
+        // url.
         //
         // Since we are dealing with a valid ProcessLock at this point, we know
         // the lock contains valid COOP/COEP information because that
@@ -1652,7 +1671,7 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(
         // their request's require COOP/COEP handling, to pass in their
         // COOP/COEP information so it can be used here instead of the values in
         // |actual_process_lock|.
-        expected_process_lock = SiteInstanceImpl::DetermineProcessLock(
+        expected_process_lock = ProcessLock::Create(
             isolation_context,
             UrlInfo(url, false /* origin_requests_isolation */),
             actual_process_lock.coop_coep_cross_origin_isolated_info());
@@ -1694,9 +1713,9 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(
             url::Origin expected_origin =
                 url::Origin::Create(expected_process_lock.lock_url());
             if (actual_process_lock.lock_url() ==
-                    SiteInstanceImpl::GetSiteForOrigin(expected_origin) ||
+                    SiteInfo::GetSiteForOrigin(expected_origin) ||
                 expected_process_lock.lock_url() ==
-                    SiteInstanceImpl::GetSiteForOrigin(actual_origin)) {
+                    SiteInfo::GetSiteForOrigin(actual_origin)) {
               failure_reason += "[origin vs site mismatch] ";
             }
           } else {
@@ -1735,9 +1754,9 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(
               return true;
           }
 
-          // See the DetermineProcessLock() call above regarding why we pass
-          // 'false' for |origin_requests_isolation| below.
-          SiteInfo site_info = SiteInstanceImpl::ComputeSiteInfo(
+          // See the ProcessLock::Create() call above regarding why we
+          // pass 'false' for |origin_requests_isolation| below.
+          SiteInfo site_info = SiteInfo::Create(
               isolation_context,
               UrlInfo(url, false /* origin_requests_isolation */),
               actual_process_lock.coop_coep_cross_origin_isolated_info());
@@ -1789,8 +1808,7 @@ void ChildProcessSecurityPolicyImpl::LockProcessForTesting(
     const IsolationContext& isolation_context,
     int child_id,
     const GURL& url) {
-  SiteInfo site_info =
-      SiteInstanceImpl::ComputeSiteInfoForTesting(isolation_context, url);
+  SiteInfo site_info = SiteInfo::CreateForTesting(isolation_context, url);
   LockProcess(isolation_context, child_id, ProcessLock(site_info));
 }
 
@@ -1892,7 +1910,7 @@ void ChildProcessSecurityPolicyImpl::AddIsolatedOrigins(
     // here, but *is* typically needed for making process model decisions. Be
     // very careful about using GetSiteForOrigin() elsewhere, and consider
     // whether you should be using GetSiteForURL() instead.
-    GURL key(SiteInstanceImpl::GetSiteForOrigin(origin_to_add));
+    GURL key(SiteInfo::GetSiteForOrigin(origin_to_add));
 
     // Isolated origins should apply only to future BrowsingInstances and
     // processes.  Save the first BrowsingInstance ID to which they should
@@ -2015,7 +2033,7 @@ bool ChildProcessSecurityPolicyImpl::IsIsolatedSiteFromSource(
     const url::Origin& origin,
     IsolatedOriginSource source) {
   base::AutoLock isolated_origins_lock(isolated_origins_lock_);
-  GURL site_url = SiteInstanceImpl::GetSiteForOrigin(origin);
+  GURL site_url = SiteInfo::GetSiteForOrigin(origin);
   auto it = isolated_origins_.find(site_url);
   if (it == isolated_origins_.end())
     return false;
@@ -2039,9 +2057,9 @@ bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
   // here, but *is* typically needed for making process model decisions. Be
   // very careful about using GetSiteForOrigin() elsewhere, and consider
   // whether you should be using GetSiteForURL() instead.
-  return GetMatchingIsolatedOrigin(
-      isolation_context, origin, origin_requests_isolation,
-      SiteInstanceImpl::GetSiteForOrigin(origin), result);
+  return GetMatchingIsolatedOrigin(isolation_context, origin,
+                                   origin_requests_isolation,
+                                   SiteInfo::GetSiteForOrigin(origin), result);
 }
 
 bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
@@ -2353,7 +2371,7 @@ bool ChildProcessSecurityPolicyImpl::UpdateOriginIsolationOptInListIfNecessary(
 
 void ChildProcessSecurityPolicyImpl::RemoveIsolatedOriginForTesting(
     const url::Origin& origin) {
-  GURL key(SiteInstanceImpl::GetSiteForOrigin(origin));
+  GURL key(SiteInfo::GetSiteForOrigin(origin));
   base::AutoLock isolated_origins_lock(isolated_origins_lock_);
   base::EraseIf(isolated_origins_[key],
                 [&origin](const IsolatedOriginEntry& entry) {

content/browser/child_process_security_policy_impl.h

@@ -80,6 +80,17 @@ class CONTENT_EXPORT ProcessLock {
   static ProcessLock CreateAllowAnySite(
       const CoopCoepCrossOriginIsolatedInfo& cross_origin_isolated_info);
 
+  // Create a lock for a specific UrlInfo and COOP/COEP information. This
+  // method can be called from both the UI and IO threads. Locks created with
+  // the same parameters must always be considered equal independent of what
+  // thread they are called on. Special care must be taken since SiteInfos
+  // created on different threads don't always have the same contents for
+  // all their fields (e.g. site_url field is thread dependent).
+  static ProcessLock Create(
+      const IsolationContext& isolation_context,
+      const UrlInfo& url_info,
+      const CoopCoepCrossOriginIsolatedInfo& cross_origin_isolated_info);
+
   ProcessLock();
   explicit ProcessLock(const SiteInfo& site_info);
   ProcessLock(const ProcessLock& rhs);

content/browser/child_process_security_policy_unittest.cc

@@ -136,7 +136,7 @@ class ChildProcessSecurityPolicyTest : public testing::Test {
                               const url::Origin& origin,
                               bool isolate_all_subdomains = false) {
     return std::pair<GURL, std::vector<IsolatedOriginEntry>>(
-        SiteInstanceImpl::GetSiteForOrigin(origin),
+        SiteInfo::GetSiteForOrigin(origin),
         {IsolatedOriginEntry(
             origin,
             BrowsingInstanceId::FromUnsafeValue(min_browsing_instance_id),
@@ -162,10 +162,10 @@ class ChildProcessSecurityPolicyTest : public testing::Test {
                               const url::Origin& origin2,
                               bool origin1_isolate_all_subdomains = false,
                               bool origin2_isolate_all_subdomains = false) {
-    EXPECT_EQ(SiteInstanceImpl::GetSiteForOrigin(origin1),
-              SiteInstanceImpl::GetSiteForOrigin(origin2));
+    EXPECT_EQ(SiteInfo::GetSiteForOrigin(origin1),
+              SiteInfo::GetSiteForOrigin(origin2));
     return std::pair<GURL, std::vector<IsolatedOriginEntry>>(
-        SiteInstanceImpl::GetSiteForOrigin(origin1),
+        SiteInfo::GetSiteForOrigin(origin1),
         {IsolatedOriginEntry(origin1,
                              SiteInstanceImpl::NextBrowsingInstanceId(),
                              nullptr, nullptr, origin1_isolate_all_subdomains,
@@ -193,7 +193,7 @@ class ChildProcessSecurityPolicyTest : public testing::Test {
   int GetIsolatedOriginEntryCount(const url::Origin& origin) {
     ChildProcessSecurityPolicyImpl* p =
         ChildProcessSecurityPolicyImpl::GetInstance();
-    GURL key(SiteInstanceImpl::GetSiteForOrigin(origin));
+    GURL key(SiteInfo::GetSiteForOrigin(origin));
     base::AutoLock isolated_origins_lock(p->isolated_origins_lock_);
     auto origins_for_key = p->isolated_origins_[key];
     return std::count_if(origins_for_key.begin(), origins_for_key.end(),
@@ -205,10 +205,9 @@ class ChildProcessSecurityPolicyTest : public testing::Test {
   void CheckGetSiteForURL(BrowserContext* context,
                           std::map<GURL, GURL> to_test) {
     for (const auto& entry : to_test) {
-      EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(
-                    IsolationContext(context),
-                    UrlInfo::CreateForTesting(entry.first)),
-                entry.second);
+      auto site_info =
+          SiteInfo::CreateForTesting(IsolationContext(context), entry.first);
+      EXPECT_EQ(site_info.site_url(), entry.second);
     }
   }
 
@@ -2642,4 +2641,70 @@ TEST_F(ChildProcessSecurityPolicyTest, WildcardDefaultPort) {
   EXPECT_THAT(p->GetIsolatedOrigins(), testing::IsEmpty());
 }
 
+TEST_F(ChildProcessSecurityPolicyTest, ProcessLockMatching) {
+  GURL nonapp_url("https://bar.com/");
+  GURL app_url("https://some.app.foo.com/");
+  GURL app_effective_url("https://app.com/");
+  EffectiveURLContentBrowserClient modified_client(
+      app_url, app_effective_url, /* requires_dedicated_process */ true);
+  ContentBrowserClient* original_client =
+      SetBrowserClientForTesting(&modified_client);
+
+  IsolationContext isolation_context(browser_context());
+  const auto coi_info = CoopCoepCrossOriginIsolatedInfo::CreateNonIsolated();
+
+  auto ui_nonapp_url_siteinfo = SiteInfo::Create(
+      isolation_context, UrlInfo::CreateForTesting(nonapp_url), coi_info);
+  auto ui_nonapp_url_lock = ProcessLock::Create(
+      isolation_context, UrlInfo::CreateForTesting(nonapp_url), coi_info);
+
+  auto ui_app_url_lock = ProcessLock::Create(
+      isolation_context, UrlInfo::CreateForTesting(app_url), coi_info);
+  auto ui_app_url_siteinfo = SiteInfo::Create(
+      isolation_context, UrlInfo::CreateForTesting(app_url), coi_info);
+
+  SiteInfo io_nonapp_url_siteinfo;
+  ProcessLock io_nonapp_url_lock;
+  SiteInfo io_app_url_siteinfo;
+  ProcessLock io_app_url_lock;
+
+  base::WaitableEvent io_locks_set_event;
+
+  // Post a task that will compute ProcessLocks for the same URLs in the
+  // IO thread.
+  GetIOThreadTaskRunner({})->PostTask(
+      FROM_HERE, base::BindLambdaForTesting([&]() {
+        io_nonapp_url_siteinfo = SiteInfo::CreateOnIOThread(
+            isolation_context, UrlInfo::CreateForTesting(nonapp_url), coi_info);
+        io_nonapp_url_lock = ProcessLock::Create(
+            isolation_context, UrlInfo::CreateForTesting(nonapp_url), coi_info);
+
+        io_app_url_siteinfo = SiteInfo::CreateOnIOThread(
+            isolation_context, UrlInfo::CreateForTesting(app_url), coi_info);
+        io_app_url_lock = ProcessLock::Create(
+            isolation_context, UrlInfo::CreateForTesting(app_url), coi_info);
+
+        // Tell the UI thread have computed the locks.
+        io_locks_set_event.Signal();
+      }));
+
+  io_locks_set_event.Wait();
+
+  // Expect URLs with effective URLs that match the original URL to have
+  // matching SiteInfos and matching ProcessLocks.
+  EXPECT_EQ(ui_nonapp_url_siteinfo, io_nonapp_url_siteinfo);
+  EXPECT_EQ(ui_nonapp_url_lock, io_nonapp_url_lock);
+
+  // Expect hosted app URLs where the effective URL does not match the original
+  // URL to have different SiteInfos but matching process locks. The SiteInfos,
+  // are expected to be different because the effective URL cannot be computed
+  // from the IO thread. This means the site_url fields will differ.
+  EXPECT_NE(ui_app_url_siteinfo, io_app_url_siteinfo);
+  EXPECT_NE(ui_app_url_siteinfo.site_url(), io_app_url_siteinfo.site_url());
+  EXPECT_EQ(ui_app_url_siteinfo.process_lock_url(),
+            io_app_url_siteinfo.process_lock_url());
+  EXPECT_EQ(ui_app_url_lock, io_app_url_lock);
+
+  SetBrowserClientForTesting(original_client);
+}
 }  // namespace content

content/browser/isolated_origin_browsertest.cc

@@ -1777,11 +1777,11 @@ IN_PROC_BROWSER_TEST_F(StrictOriginIsolationTest,
   // Calculate the expected SiteInfo for each URL.  Both |foo_url| and
   // |bar_url| should have a site URL of |app_url|, but the process locks
   // should be foo.com and bar.com.
-  SiteInfo foo_site_info = SiteInstanceImpl::ComputeSiteInfoForTesting(
+  SiteInfo foo_site_info = SiteInfo::CreateForTesting(
       web_contents()->GetSiteInstance()->GetIsolationContext(), foo_url);
   EXPECT_EQ(app_url, foo_site_info.site_url());
   EXPECT_EQ(foo_url.GetOrigin(), foo_site_info.process_lock_url());
-  SiteInfo bar_site_info = SiteInstanceImpl::ComputeSiteInfoForTesting(
+  SiteInfo bar_site_info = SiteInfo::CreateForTesting(
       web_contents()->GetSiteInstance()->GetIsolationContext(), bar_url);
   EXPECT_EQ(app_url, bar_site_info.site_url());
   EXPECT_EQ(bar_url.GetOrigin(), bar_site_info.process_lock_url());
@@ -2345,7 +2345,7 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginTest, ProcessLimit) {
                                                const GURL& url) {
     return RenderProcessHostImpl::IsSuitableHost(
         process, isolation_context,
-        SiteInstanceImpl::ComputeSiteInfoForTesting(isolation_context, url));
+        SiteInfo::CreateForTesting(isolation_context, url));
   };
   EXPECT_TRUE(is_suitable_host(foo_process, foo_url));
   EXPECT_FALSE(is_suitable_host(foo_process, isolated_foo_url));

content/browser/renderer_host/back_forward_cache_metrics.cc

@@ -233,10 +233,10 @@ void BackForwardCacheMetrics::MainFrameDidNavigateAwayFromDocument(
     //
     // [1]
     // https://chromium.googlesource.com/chromium/src/+/HEAD/docs/security/origin-vs-url.md#avoid-converting-urls-to-origins
-    GURL previous_site = SiteInstanceImpl::GetSiteForOrigin(
+    GURL previous_site = SiteInfo::GetSiteForOrigin(
         url::Origin::Create(details->previous_main_frame_url));
-    GURL new_site = SiteInstanceImpl::GetSiteForOrigin(
-        url::Origin::Create(navigation->GetURL()));
+    GURL new_site =
+        SiteInfo::GetSiteForOrigin(url::Origin::Create(navigation->GetURL()));
     if (previous_site == new_site) {
       page_store_result_->No(
           NotRestoredReason::kRenderFrameHostReused_SameSite);

content/browser/renderer_host/navigation_request.cc

@@ -2576,9 +2576,9 @@ void NavigationRequest::OnResponseStarted(
     // https://crbug.com/738634.
     SiteInstanceImpl* instance = render_frame_host_->GetSiteInstance();
     const IsolationContext& isolation_context = instance->GetIsolationContext();
-    auto site_info = SiteInstanceImpl::ComputeSiteInfo(
-        isolation_context, GetUrlInfo(),
-        instance->GetCoopCoepCrossOriginIsolatedInfo());
+    auto site_info =
+        SiteInfo::Create(isolation_context, GetUrlInfo(),
+                         instance->GetCoopCoepCrossOriginIsolatedInfo());
     if (!instance->HasSite() &&
         site_info.RequiresDedicatedProcess(isolation_context)) {
       instance->ConvertToDefaultOrSetSite(GetUrlInfo());
@@ -4568,9 +4568,8 @@ SiteInfo NavigationRequest::GetSiteInfoForCommonParamsURL(
     const CoopCoepCrossOriginIsolatedInfo& cross_origin_isolated_info) {
   // TODO(alexmos): Using |starting_site_instance_|'s IsolationContext may not
   // be correct for cross-BrowsingInstance redirects.
-  return SiteInstanceImpl::ComputeSiteInfo(
-      starting_site_instance_->GetIsolationContext(), GetUrlInfo(),
-      cross_origin_isolated_info);
+  return SiteInfo::Create(starting_site_instance_->GetIsolationContext(),
+                          GetUrlInfo(), cross_origin_isolated_info);
 }
 
 // TODO(zetamoo): Try to merge this function inside its callers.

content/browser/renderer_host/navigator_unittest.cc

@@ -1252,7 +1252,7 @@ TEST_F(NavigatorTest, SiteInstanceDescriptionConversion) {
     if (AreDefaultSiteInstancesEnabled()) {
       ASSERT_TRUE(related_instance_impl->IsDefaultSiteInstance());
     } else {
-      EXPECT_EQ(SiteInstanceImpl::ComputeSiteInfoForTesting(
+      EXPECT_EQ(SiteInfo::CreateForTesting(
                     current_instance->GetIsolationContext(), kUrlSameSiteAs2),
                 related_instance_impl->GetSiteInfo());
     }

content/browser/renderer_host/render_frame_host_impl.cc

@@ -8435,7 +8435,7 @@ void RenderFrameHostImpl::SetLastCommittedSiteInfo(const GURL& url) {
   SiteInfo site_info =
       url.is_empty()
           ? SiteInfo()
-          : SiteInstanceImpl::ComputeSiteInfo(
+          : SiteInfo::Create(
                 GetSiteInstance()->GetIsolationContext(),
                 UrlInfo(url, false /* origin_requests_isolation */),
                 GetSiteInstance()->GetCoopCoepCrossOriginIsolatedInfo());