Change CanBePlacedInDefaultSiteInstance() param to SiteInfo.

This change converts the site_url param to a SiteInfo in preparation
for making a similar change to DoesSiteURLRequireDedicatedProcess().
This is part of an ongoing process to replace site URL usage in
method signatures with SiteInfo. This change also includes a few minor
cleanups related to default SiteInstances. This change does not
introduce any user visible behavior changes.

- Changed CanBePlacedInDefaultSiteInstance() to take a SiteInfo instead
  of site URL.
- Removed allow_default_site_url from GetSiteForURLInternal() and
  updated callers to handle default SiteInstance cases instead.
- Added SiteInfo::CreateForDefaultSiteInstance() and updated code to use
  that instead of doing a complicated path through ComputeSiteInfo().
- Added SiteInstanceImpl::SetSiteInfoToDefault() to make the conversion
  to a default SiteInstance a little more explicit.

Bug: 1085275
Change-Id: Ib65a41201902c48b13b82172304c8afe15b997fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2399016
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805466}

content/browser/browsing_instance.cc

@@ -108,15 +108,15 @@ bool BrowsingInstance::TrySettingDefaultSiteInstance(
   DCHECK(!site_instance->HasSite());
   const SiteInfo site_info = ComputeSiteInfoForURL(url);
   if (default_site_instance_ ||
-      !SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(
-          isolation_context_, url, site_info.site_url())) {
+      !SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(isolation_context_,
+                                                          url, site_info)) {
     return false;
   }
 
   // Note: |default_site_instance_| must be set before SetSite() call to
   // properly trigger default SiteInstance behavior inside that method.
   default_site_instance_ = site_instance;
-  site_instance->SetSite(SiteInstanceImpl::GetDefaultSiteURL());
+  site_instance->SetSiteInfoToDefault();
   site_url_set_.insert(site_info.site_url());
   return true;
 }
@@ -132,8 +132,8 @@ scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURLHelper(
   // Check to see if we can use the default SiteInstance for sites that don't
   // need to be isolated in their own process.
   if (allow_default_instance &&
-      SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(
-          isolation_context_, url, site_info.site_url())) {
+      SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(isolation_context_,
+                                                         url, site_info)) {
     DCHECK(!default_process_);
     scoped_refptr<SiteInstanceImpl> site_instance = default_site_instance_;
     if (!site_instance) {
@@ -148,7 +148,7 @@ scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURLHelper(
       // calls RegisterSiteInstance().
       default_site_instance_ = site_instance.get();
 
-      site_instance->SetSite(SiteInstanceImpl::GetDefaultSiteURL());
+      site_instance->SetSiteInfoToDefault();
     }
 
     // Add |site_url| to the set so we can keep track of all the sites the

content/browser/site_instance_impl.cc

@@ -42,6 +42,10 @@ GURL SchemeAndHostToSite(const std::string& scheme, const std::string& host) {
   return GURL(scheme + url::kStandardSchemeSeparator + host);
 }
 
+// Constant used to mark two call sites that must always agree on whether
+// the default SiteInstance is allowed.
+constexpr bool kCreateForURLAllowsDefaultSiteInstance = true;
+
 }  // namespace
 
 int32_t SiteInstanceImpl::next_site_instance_id_ = 1;
@@ -64,6 +68,13 @@ SiteInfo SiteInfo::CreateForErrorPage() {
                   false /* is_origin_keyed */);
 }
 
+// static
+SiteInfo SiteInfo::CreateForDefaultSiteInstance() {
+  return SiteInfo(SiteInstanceImpl::GetDefaultSiteURL(),
+                  SiteInstanceImpl::GetDefaultSiteURL(),
+                  false /* is_origin_keyed */);
+}
+
 SiteInfo::SiteInfo(const GURL& site_url,
                    const GURL& process_lock_url,
                    bool is_origin_keyed)
@@ -165,8 +176,8 @@ scoped_refptr<SiteInstanceImpl> SiteInstanceImpl::CreateForURL(
 
   // Note: The |allow_default_instance| value used here MUST match the value
   // used in DoesSiteForURLMatch().
-  return instance->GetSiteInstanceForURL(url,
-                                         true /* allow_default_instance */);
+  return instance->GetSiteInstanceForURL(
+      url, kCreateForURLAllowsDefaultSiteInstance);
 }
 
 // static
@@ -470,6 +481,14 @@ void SiteInstanceImpl::SetSite(const GURL& url) {
       url, /* allow_default_instance */ false));
 }
 
+void SiteInstanceImpl::SetSiteInfoToDefault() {
+  TRACE_EVENT1("navigation", "SiteInstanceImpl::SetSiteInfoToDefault",
+               "site id", id_);
+  DCHECK(!has_site_);
+  original_url_ = GetDefaultSiteURL();
+  SetSiteInfoInternal(SiteInfo::CreateForDefaultSiteInstance());
+}
+
 void SiteInstanceImpl::SetSiteInfoInternal(const SiteInfo& site_info) {
   // TODO(acolwell): Add logic to validate |site_url| and |lock_url| are valid.
   DCHECK(!has_site_);
@@ -718,12 +737,10 @@ bool SiteInstanceImpl::IsSameSiteWithURL(const GURL& url) {
     // prevent SiteInstances with no site URL from being used for URLs
     // that should be routed to the default SiteInstance.
     DCHECK_EQ(site_info_.site_url(), GetDefaultSiteURL());
-    return site_info_.site_url() ==
-               GetSiteForURLInternal(GetIsolationContext(), url,
-                                     true /* should_use_effective_urls */,
-                                     true /* allow_default_site_url */) &&
-           !browsing_instance_->HasSiteInstance(
-               ComputeSiteInfo(GetIsolationContext(), url));
+    auto site_info = ComputeSiteInfo(GetIsolationContext(), url);
+    return CanBePlacedInDefaultSiteInstance(GetIsolationContext(), url,
+                                            site_info) &&
+           !browsing_instance_->HasSiteInstance(site_info);
   }
 
   return SiteInstanceImpl::IsSameSite(GetIsolationContext(),
@@ -942,22 +959,13 @@ bool SiteInstanceImpl::IsSameSite(const IsolationContext& isolation_context,
 }
 
 bool SiteInstanceImpl::DoesSiteInfoForURLMatch(const GURL& url) {
-  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
-  bool is_origin_keyed = policy->ShouldOriginGetOptInIsolation(
-      GetIsolationContext(), url::Origin::Create(url));
-
-  // Note: The |allow_default_site_url| value used here MUST match the value
-  // used in CreateForURL().  This is why we can't use ComputeSiteInfo() or
-  // even DetermineProcessLockURL() here, which do not allow the default site
-  // URL.
-  return site_info_ ==
-         SiteInfo(GetSiteForURLInternal(GetIsolationContext(), url,
-                                        true /* should_use_effective_urls */,
-                                        true /* allow_default_site_url */),
-                  GetSiteForURLInternal(GetIsolationContext(), url,
-                                        false /* should_use_effective_urls */,
-                                        true /* allow_default_site_url */),
-                  is_origin_keyed);
+  auto site_info = ComputeSiteInfo(GetIsolationContext(), url);
+  if (kCreateForURLAllowsDefaultSiteInstance &&
+      CanBePlacedInDefaultSiteInstance(GetIsolationContext(), url, site_info)) {
+    site_info = SiteInfo::CreateForDefaultSiteInstance();
+  }
+
+  return site_info_ == site_info;
 }
 
 void SiteInstanceImpl::PreventOptInOriginIsolation(
@@ -1029,24 +1037,21 @@ GURL SiteInstanceImpl::DetermineProcessLockURL(
   // For the process lock URL, convert |url| to a site without resolving |url|
   // to an effective URL.
   return SiteInstanceImpl::GetSiteForURLInternal(
-      isolation_context, url, false /* should_use_effective_urls */,
-      false /* allow_default_site_url */);
+      isolation_context, url, false /* should_use_effective_urls */);
 }
 
 // static
 GURL SiteInstanceImpl::GetSiteForURL(const IsolationContext& isolation_context,
                                      const GURL& real_url) {
   return GetSiteForURLInternal(isolation_context, real_url,
-                               true /* should_use_effective_urls */,
-                               false /* allow_default_site_url */);
+                               true /* should_use_effective_urls */);
 }
 
 // static
 GURL SiteInstanceImpl::GetSiteForURLInternal(
     const IsolationContext& isolation_context,
     const GURL& real_url,
-    bool should_use_effective_urls,
-    bool allow_default_site_url) {
+    bool should_use_effective_urls) {
   // Explicitly group chrome-error: URLs based on their host component.
   // These URLs are special because we want to group them like other URLs
   // with a host even though they are considered "no access" and
@@ -1132,12 +1137,6 @@ GURL SiteInstanceImpl::GetSiteForURLInternal(
     }
   }
 
-  // We should never get here if we're origin_keyed, otherwise we would have
-  // returned after the GetMatchingIsolatedOrigin() call above.
-  if (allow_default_site_url &&
-      CanBePlacedInDefaultSiteInstance(isolation_context, real_url, site_url)) {
-    return GetDefaultSiteURL();
-  }
   return site_url;
 }
 
@@ -1145,7 +1144,7 @@ GURL SiteInstanceImpl::GetSiteForURLInternal(
 bool SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(
     const IsolationContext& isolation_context,
     const GURL& url,
-    const GURL& site_url) {
+    const SiteInfo& site_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (!base::FeatureList::IsEnabled(
@@ -1178,7 +1177,8 @@ bool SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(
 
   // Allow the default SiteInstance to be used for sites that don't need to be
   // isolated in their own process.
-  return !DoesSiteURLRequireDedicatedProcess(isolation_context, site_url);
+  return !DoesSiteURLRequireDedicatedProcess(isolation_context,
+                                             site_info.site_url());
 }
 
 // static

content/browser/site_instance_impl.h

@@ -51,6 +51,7 @@ class StoragePartitionImpl;
 class CONTENT_EXPORT SiteInfo {
  public:
   static SiteInfo CreateForErrorPage();
+  static SiteInfo CreateForDefaultSiteInstance();
 
   // The SiteInfo constructor should take in all values needed for comparing two
   // SiteInfos, to help ensure all creation sites are updated accordingly when
@@ -516,6 +517,10 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   // a dedicated process.
   void MaybeSetBrowsingInstanceDefaultProcess();
 
+  // Sets the SiteInfo and other fields so that this instance becomes a
+  // default SiteInstance.
+  void SetSiteInfoToDefault();
+
   // Sets |site_info_| with |site_info| and registers this object with
   // |browsing_instance_|. SetSite() calls this method to set the site and lock
   // for a user provided URL. This method should only be called by code that
@@ -558,12 +563,9 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   // |should_use_effective_urls| specifies whether to resolve |url| to an
   // effective URL (via ContentBrowserClient::GetEffectiveURL()) before
   // determining the site.
-  // |allow_default_site_url| specifies whether the default SiteInstance site
-  // URL is allowed to be returned.
   static GURL GetSiteForURLInternal(const IsolationContext& isolation_context,
                                     const GURL& url,
-                                    bool should_use_effective_urls,
-                                    bool allow_default_site_url);
+                                    bool should_use_effective_urls);
 
   // True if |url| resolves to an effective URL that is different from |url|.
   // See GetEffectiveURL().  This will be true for hosted apps as well as NTP
@@ -583,15 +585,15 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   // Returns true if |url| and its |site_url| can be placed inside a default
   // SiteInstance.
   //
-  // Note: |url| and |site_url| must be consistent with each other. In contexts
+  // Note: |url| and |site_info| must be consistent with each other. In contexts
   // where the caller only has |url| it can use
-  // SiteInstanceImpl::GetSiteForURL() to generate |site_url|. This call is
+  // SiteInstanceImpl::ComputeSiteInfo() to generate |site_info|. This call is
   // intentionally not set as a default value to encourage the caller to reuse
-  // a site url computation if they already have one.
+  // a SiteInfo computation if they already have one.
   static bool CanBePlacedInDefaultSiteInstance(
       const IsolationContext& isolation_context,
       const GURL& url,
-      const GURL& site_url);
+      const SiteInfo& site_info);
 
   // An object used to construct RenderProcessHosts.
   static const RenderProcessHostFactory* g_render_process_host_factory_;

content/browser/site_instance_impl_unittest.cc

@@ -628,13 +628,11 @@ TEST_F(SiteInstanceTest, ProcessLockDoesNotUseEffectiveURL) {
   // (foo.com).
   {
     GURL site_url = SiteInstanceImpl::GetSiteForURLInternal(
-        isolation_context, test_url, false /* use_effective_urls */,
-        false /* allow_default_site_url */);
+        isolation_context, test_url, false /* use_effective_urls */);
     EXPECT_EQ(nonapp_site_url, site_url);
 
     site_url = SiteInstanceImpl::GetSiteForURLInternal(
-        isolation_context, test_url, true /* use_effective_urls */,
-        false /* allow_default_site_url */);
+        isolation_context, test_url, true /* use_effective_urls */);
     EXPECT_EQ(app_url, site_url);
   }