Allow origin-keyed agent clusters without process isolation.

This CL implements the behaviours of Origin-Keyed Agent Clusters for
same-process isolation. The same-process behaviour engages when
SiteIsolationPolicy::IsSiteIsolationDisabled() returns true, and is
assumed to be constant for the lifetime of the browser session. This
avoids (for now) the need to enforce consistent same-/different-process
behaviour for a BrowsingInstance, but this may change in future if
this mechanism is adapted for dynamic use gated on memory pressure
and/or process count.

Bug: 1148490
Change-Id: I1177a25b890aec43a8b2eedf7b2311fd161acd8a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2657930
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#857906}

components/site_isolation/site_isolation_policy_unittest.cc

@@ -979,14 +979,17 @@ TEST_F(OptInOriginIsolationPolicyTest, BelowThreshold) {
     return;
 
   // Define a memory threshold at 768MB.  This is above the 512MB of physical
-  // memory that this test simulates, so opt-in origin isolation should be
-  // disabled.
+  // memory that this test simulates, so process isolation for
+  // Origin-Agent-Cluster (OAC) should be disabled. But other aspects of OAC
+  // should still take effect.
   base::test::ScopedFeatureList memory_feature;
   memory_feature.InitAndEnableFeatureWithParameters(
       features::kSitePerProcessOnlyForHighMemoryClients,
       {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "768"}});
 
-  EXPECT_FALSE(content::SiteIsolationPolicy::IsOptInOriginIsolationEnabled());
+  EXPECT_FALSE(content::SiteIsolationPolicy::
+                   IsProcessIsolationForOriginAgentClusterEnabled());
+  EXPECT_TRUE(content::SiteIsolationPolicy::IsOriginAgentClusterEnabled());
 
   // Simulate a navigation to a URL that serves an Origin-Agent-Cluster header.
   // Since we're outside of content/, it's difficult to verify that internal
@@ -1012,6 +1015,12 @@ TEST_F(OptInOriginIsolationPolicyTest, BelowThreshold) {
   content::SiteInstance* site_instance =
       simulator->GetFinalRenderFrameHost()->GetSiteInstance();
   EXPECT_FALSE(site_instance->RequiresDedicatedProcess());
+  // Despite not getting process isolation, the origin will still get logical
+  // isolation in Blink, and should still be tracked by
+  // ChildProcessSecurityPolicy to ensure consistent OAC behavior for this
+  // origin within this BrowsingInstance.
+  EXPECT_TRUE(
+      ShouldOriginGetOptInIsolation(site_instance, url::Origin::Create(kUrl)));
 }
 
 // Counterpart to the test above, but verifies that opt-in origin isolation is
@@ -1028,7 +1037,9 @@ TEST_F(OptInOriginIsolationPolicyTest, AboveThreshold) {
       features::kSitePerProcessOnlyForHighMemoryClients,
       {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "128"}});
 
-  EXPECT_TRUE(content::SiteIsolationPolicy::IsOptInOriginIsolationEnabled());
+  EXPECT_TRUE(content::SiteIsolationPolicy::
+                  IsProcessIsolationForOriginAgentClusterEnabled());
+  EXPECT_TRUE(content::SiteIsolationPolicy::IsOriginAgentClusterEnabled());
 
   // Simulate a navigation to a URL that serves an Origin-Agent-Cluster header.
   // Verify that the resulting SiteInstance requires a dedicated process.  Note

content/browser/child_process_security_policy_impl.cc

@@ -1999,8 +1999,8 @@ bool ChildProcessSecurityPolicyImpl::IsIsolatedOrigin(
     const url::Origin& origin,
     bool origin_requests_isolation) {
   url::Origin unused_result;
-  return GetMatchingIsolatedOrigin(isolation_context, origin,
-                                   origin_requests_isolation, &unused_result);
+  return GetMatchingProcessIsolatedOrigin(
+      isolation_context, origin, origin_requests_isolation, &unused_result);
 }
 
 bool ChildProcessSecurityPolicyImpl::IsGloballyIsolatedOriginForTesting(
@@ -2055,7 +2055,7 @@ bool ChildProcessSecurityPolicyImpl::IsIsolatedSiteFromSource(
   return false;
 }
 
-bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
+bool ChildProcessSecurityPolicyImpl::GetMatchingProcessIsolatedOrigin(
     const IsolationContext& isolation_context,
     const url::Origin& origin,
     bool origin_requests_isolation,
@@ -2067,12 +2067,12 @@ 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,
-                                   SiteInfo::GetSiteForOrigin(origin), result);
+  return GetMatchingProcessIsolatedOrigin(
+      isolation_context, origin, origin_requests_isolation,
+      SiteInfo::GetSiteForOrigin(origin), result);
 }
 
-bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
+bool ChildProcessSecurityPolicyImpl::GetMatchingProcessIsolatedOrigin(
     const IsolationContext& isolation_context,
     const url::Origin& origin,
     bool origin_requests_isolation,
@@ -2100,7 +2100,19 @@ bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
     // false, or true with result set to |origin|. We give priority to origins
     // requesting opt-in isolation over command-line isolation, but don't check
     // for opt-in if we didn't get a valid BrowsingInstance id.
-    if (ShouldOriginGetOptInIsolation(isolation_context, origin,
+    // Note: This should only return a full origin if we are doing
+    // process-isolated Origin-keyed Agent Clusters, which will only be the case
+    // when site-isolation is enabled. Otherwise we put the origin into its
+    // corresponding site, even if Origin-keyed Agent Clusters will be enabled
+    // on the renderer side.
+    // TODO(wjmaclean,alexmos,acolwell): We should revisit this when we have
+    // SiteInstanceGroups, since at that point we can again return an origin
+    // here (and thus create a new SiteInstance) even when
+    // IsProcessIsolationForOriginAgentClusterEnabled() returns false; in that
+    // case a SiteInstanceGroup will allow a logical group of SiteInstances that
+    // live same-process.
+    if (SiteIsolationPolicy::IsProcessIsolationForOriginAgentClusterEnabled() &&
+        ShouldOriginGetOptInIsolation(isolation_context, origin,
                                       origin_requests_isolation)) {
       *result = origin;
       return true;

content/browser/child_process_security_policy_impl.h

@@ -364,10 +364,11 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // |isolation_context| is used to determine which origins are isolated in
   // this context.  For example, isolated origins that are dynamically added
   // will only affect future BrowsingInstances.
-  bool GetMatchingIsolatedOrigin(const IsolationContext& isolation_context,
-                                 const url::Origin& origin,
-                                 bool origin_requests_isolation,
-                                 url::Origin* result);
+  bool GetMatchingProcessIsolatedOrigin(
+      const IsolationContext& isolation_context,
+      const url::Origin& origin,
+      bool origin_requests_isolation,
+      url::Origin* result);
 
   // Removes any origin isolation opt-in entries associated with the
   // |browsing_instance_id| of the BrowsingInstance.
@@ -406,16 +407,17 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
       BrowserContext* browser_context,
       const url::Origin& origin);
 
-  // A version of GetMatchingIsolatedOrigin that takes in both the |origin| and
-  // the |site_url| that |origin| corresponds to.  |site_url| is the key by
-  // which |origin| will be looked up in |isolated_origins_| within
+  // A version of GetMatchingProcessIsolatedOrigin that takes in both the
+  // |origin| and the |site_url| that |origin| corresponds to.  |site_url| is
+  // the key by which |origin| will be looked up in |isolated_origins_| within
   // |isolation_context|; this function allows it to be passed in when it is
   // already known to avoid recomputing it internally.
-  bool GetMatchingIsolatedOrigin(const IsolationContext& isolation_context,
-                                 const url::Origin& origin,
-                                 bool origin_requests_isolation,
-                                 const GURL& site_url,
-                                 url::Origin* result);
+  bool GetMatchingProcessIsolatedOrigin(
+      const IsolationContext& isolation_context,
+      const url::Origin& origin,
+      bool origin_requests_isolation,
+      const GURL& site_url,
+      url::Origin* result);
 
   // Returns if |child_id| can read all of the |files|.
   bool CanReadAllFiles(int child_id, const std::vector<base::FilePath>& files);

content/browser/child_process_security_policy_unittest.cc

@@ -2615,9 +2615,9 @@ TEST_F(ChildProcessSecurityPolicyTest, WildcardDefaultPort) {
   // Requesting isolated_origin_with_port should return the same origin but with
   // the default port for the scheme.
   const bool kOriginRequestsIsolation = false;
-  EXPECT_TRUE(
-      p->GetMatchingIsolatedOrigin(isolation_context, isolated_origin_with_port,
-                                   kOriginRequestsIsolation, &lookup_origin));
+  EXPECT_TRUE(p->GetMatchingProcessIsolatedOrigin(
+      isolation_context, isolated_origin_with_port, kOriginRequestsIsolation,
+      &lookup_origin));
   EXPECT_EQ(url::DefaultPortForScheme(lookup_origin.scheme().data(),
                                       lookup_origin.scheme().length()),
             lookup_origin.port());
@@ -2628,9 +2628,9 @@ TEST_F(ChildProcessSecurityPolicyTest, WildcardDefaultPort) {
   // Similarly, looking up matching isolated origins for wildcard origins must
   // also return the default port for the origin's scheme, not the report of the
   // requested origin.
-  EXPECT_TRUE(p->GetMatchingIsolatedOrigin(isolation_context, wild_with_port,
-                                           kOriginRequestsIsolation,
-                                           &lookup_origin));
+  EXPECT_TRUE(p->GetMatchingProcessIsolatedOrigin(
+      isolation_context, wild_with_port, kOriginRequestsIsolation,
+      &lookup_origin));
   EXPECT_EQ(url::DefaultPortForScheme(lookup_origin.scheme().data(),
                                       lookup_origin.scheme().length()),
             lookup_origin.port());

content/browser/isolated_origin_browsertest.cc

@@ -243,6 +243,23 @@ class OriginIsolationOptInHeaderTest : public IsolatedOriginTestBase {
   DISALLOW_COPY_AND_ASSIGN(OriginIsolationOptInHeaderTest);
 };
 
+// As in OriginIsolationOptInHeaderTest, but with same-process origin isolation.
+class SameProcessOriginIsolationOptInHeaderTest
+    : public OriginIsolationOptInHeaderTest {
+ public:
+  SameProcessOriginIsolationOptInHeaderTest() = default;
+  ~SameProcessOriginIsolationOptInHeaderTest() override = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    OriginIsolationOptInHeaderTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kDisableSiteIsolation);
+    command_line->RemoveSwitch(switches::kSitePerProcess);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SameProcessOriginIsolationOptInHeaderTest);
+};
+
 // Used for a few tests that check non-HTTPS secure context behavior.
 class OriginIsolationOptInHttpServerHeaderTest : public IsolatedOriginTestBase {
  public:
@@ -488,6 +505,57 @@ IN_PROC_BROWSER_TEST_F(OriginIsolationOptInHeaderTest,
               1)));
 }
 
+// In this test the sub-origin is isolated because the header requests it. It
+// will have the same site instance as the main frame, and it will be in the
+// same process.
+IN_PROC_BROWSER_TEST_F(SameProcessOriginIsolationOptInHeaderTest,
+                       SimpleSubOriginIsolationTest) {
+  base::HistogramTester histograms;
+  SetHeaderValue("?1");
+  // Start off with an a(a) page, then navigate the subframe to an isolated sub
+  // origin.
+  GURL test_url(https_server()->GetURL("foo.com",
+                                       "/cross_site_iframe_factory.html?"
+                                       "foo.com(foo.com)"));
+  GURL isolated_suborigin_url(
+      https_server()->GetURL("isolated.foo.com", "/isolate_origin"));
+  GURL origin_url = url::Origin::Create(isolated_suborigin_url).GetURL();
+  EXPECT_FALSE(
+      SiteIsolationPolicy::IsProcessIsolationForOriginAgentClusterEnabled());
+  EXPECT_TRUE(NavigateToURL(shell(), test_url));
+  EXPECT_EQ(2u, shell()->web_contents()->GetAllFrames().size());
+
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+  FrameTreeNode* child_frame_node = root->child_at(0);
+  EXPECT_TRUE(
+      NavigateToURLFromRenderer(child_frame_node, isolated_suborigin_url));
+  EXPECT_EQ(root->current_frame_host()->GetSiteInstance(),
+            child_frame_node->current_frame_host()->GetSiteInstance());
+  EXPECT_FALSE(child_frame_node->current_frame_host()
+                   ->GetSiteInstance()
+                   ->RequiresDedicatedProcess());
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  EXPECT_TRUE(policy->ShouldOriginGetOptInIsolation(
+      root->current_frame_host()->GetSiteInstance()->GetIsolationContext(),
+      url::Origin::Create(isolated_suborigin_url),
+      false /* origin_requests_isolation */));
+  EXPECT_TRUE(policy->HasOriginEverRequestedOptInIsolation(
+      web_contents()->GetBrowserContext(),
+      url::Origin::Create(isolated_suborigin_url)));
+
+  EXPECT_THAT(
+      histograms.GetAllSamples("Navigation.OriginAgentCluster.Result"),
+      testing::ElementsAre(
+          base::Bucket(
+              static_cast<int>(NavigationRequest::OriginAgentClusterEndResult::
+                                   kNotRequestedAndNotOriginKeyed),
+              2),
+          base::Bucket(
+              static_cast<int>(NavigationRequest::OriginAgentClusterEndResult::
+                                   kRequestedAndOriginKeyed),
+              1)));
+}
+
 // In this test the sub-origin isn't isolated because no header is set. It will
 // have the same site instance as the main frame.
 IN_PROC_BROWSER_TEST_F(OriginIsolationOptInHeaderTest,

content/browser/renderer_host/navigation_request.cc

@@ -2152,6 +2152,28 @@ void NavigationRequest::CheckForIsolationOptIn(const GURL& url) {
   }
 }
 
+void NavigationRequest::AddSameProcessOriginAgentClusterOptInIfNecessary(
+    const IsolationContext& isolation_context,
+    const GURL& url) {
+  // If site isolation isn't disabled and OriginAgentCluster is allowed to use
+  // process isolation, then no need to add the opt-in here; it will be handled
+  // when the origin's SiteInstance is created.
+  if (SiteIsolationPolicy::IsProcessIsolationForOriginAgentClusterEnabled() ||
+      !IsOptInIsolationRequested()) {
+    return;
+  }
+
+  // Since site isolation is disabled, we can't rely on the newly created
+  // SiteInstance to add the origin as OAC, so we do it manually here.
+  url::Origin origin = url::Origin::Create(url);
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  if (policy->ShouldOriginGetOptInIsolation(isolation_context, origin,
+                                            true /* is_requested */)) {
+    policy->AddOptInIsolatedOriginForBrowsingInstance(isolation_context,
+                                                      origin);
+  }
+}
+
 bool NavigationRequest::HasCommittingOrigin(const url::Origin& origin) {
   // We are only interested in checking requests that have been assigned a
   // SiteInstance.
@@ -2171,17 +2193,11 @@ bool NavigationRequest::IsOptInIsolationRequested() {
   if (!response())
     return false;
 
-  // Do not attempt isolation if the environment prevents us from enabling site
-  // isolation (e.g., when we are under the memory threshold on Android).
-  if (!SiteIsolationPolicy::IsOptInOriginIsolationEnabled())
+  // Do not attempt isolation if the feature is not enabled.
+  if (!SiteIsolationPolicy::IsOriginAgentClusterEnabled())
     return false;
 
-  if (base::FeatureList::IsEnabled(features::kOriginIsolationHeader) &&
-      response_head_->parsed_headers->origin_agent_cluster) {
-    return true;
-  }
-
-  return false;
+  return response_head_->parsed_headers->origin_agent_cluster;
 }
 
 void NavigationRequest::DetermineOriginAgentClusterEndResult(
@@ -2640,7 +2656,18 @@ void NavigationRequest::OnResponseStarted(
         site_info.RequiresDedicatedProcess(isolation_context)) {
       instance->ConvertToDefaultOrSetSite(GetUrlInfo());
     }
-
+    // Now that we know the IsolationContext for the assigned SiteInstance, we
+    // opt the origin into OAC here if needed.
+    // TODO(wjmaclean): Remove this call/function when same-process
+    // OriginAgentCluster moves to SiteInstanceGroup, as then all OAC origins
+    // will get a SiteInstance (regardless of process isolation) and tracking
+    // will be handled by the existing pathway in
+    // SiteInstanceImpl::SetSiteInfoInternal().
+    AddSameProcessOriginAgentClusterOptInIfNecessary(isolation_context,
+                                                     GetURL());
+
+    // TODO(wjmaclean): Once this is all working, consider combining the
+    // following code into the function above.
     // If this navigation request didn't opt-in to origin isolation, we need
     // to check here in case the origin has previously requested isolation and
     // should be marked as opted-out in this SiteInstance. At this point we know

content/browser/renderer_host/navigation_request.h

@@ -847,6 +847,14 @@ class CONTENT_EXPORT NavigationRequest
   // Origin-Agent-Cluster header, and if so opts in the origin to be isolated.
   void CheckForIsolationOptIn(const GURL& url);
 
+  // Use to manually opt an origin into Origin-keyed Agent Cluster (OAC) in the
+  // event that process-isolation isn't being used for OAC.
+  // TODO(wjmaclean): When we switch to using separate SiteInstances even for
+  // same-process OAC, then this function can be removed.
+  void AddSameProcessOriginAgentClusterOptInIfNecessary(
+      const IsolationContext& isolation_context,
+      const GURL& url);
+
   // NavigationURLLoaderDelegate implementation.
   void OnRequestRedirected(
       const net::RedirectInfo& redirect_info,

content/browser/site_instance_impl.cc

@@ -445,7 +445,7 @@ GURL SiteInfo::GetSiteForURLInternal(const IsolationContext& isolation_context,
     // resolved prior to the isolated origin lookup.
     auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
     url::Origin isolated_origin;
-    if (policy->GetMatchingIsolatedOrigin(
+    if (policy->GetMatchingProcessIsolatedOrigin(
             isolation_context, origin, real_url_info.origin_requests_isolation,
             site_url, &isolated_origin)) {
       return isolated_origin.GetURL();
@@ -905,6 +905,12 @@ void SiteInstanceImpl::SetSiteInfoInternal(const SiteInfo& site_info) {
     ChildProcessSecurityPolicyImpl* policy =
         ChildProcessSecurityPolicyImpl::GetInstance();
     url::Origin site_origin(url::Origin::Create(site_info_.process_lock_url()));
+    // This is one of two places that origins can be marked as opted-in, the
+    // other is
+    // NavigationRequest::AddSameProcessOriginAgentClusterOptInIfNecessary().
+    // This site handles the case where OAC isolation gets a separate process.
+    // In future, when SiteInstance Groups are complete, this may revert to
+    // being the only call site.
     policy->AddOptInIsolatedOriginForBrowsingInstance(
         browsing_instance_->isolation_context(), site_origin);
   }
@@ -1404,10 +1410,10 @@ bool SiteInstanceImpl::IsSameSite(const IsolationContext& isolation_context,
   auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
   url::Origin src_isolated_origin;
   url::Origin dest_isolated_origin;
-  bool src_origin_is_isolated = policy->GetMatchingIsolatedOrigin(
+  bool src_origin_is_isolated = policy->GetMatchingProcessIsolatedOrigin(
       isolation_context, src_origin,
       real_src_url_info.origin_requests_isolation, &src_isolated_origin);
-  bool dest_origin_is_isolated = policy->GetMatchingIsolatedOrigin(
+  bool dest_origin_is_isolated = policy->GetMatchingProcessIsolatedOrigin(
       isolation_context, dest_origin,
       real_dest_url_info.origin_requests_isolation, &dest_isolated_origin);
   if (src_origin_is_isolated || dest_origin_is_isolated) {

content/public/browser/site_isolation_policy.cc

@@ -133,7 +133,7 @@ bool SiteIsolationPolicy::ArePreloadedIsolatedOriginsEnabled() {
 }
 
 // static
-bool SiteIsolationPolicy::IsOptInOriginIsolationEnabled() {
+bool SiteIsolationPolicy::IsProcessIsolationForOriginAgentClusterEnabled() {
   // If strict site isolation is in use (either by default on desktop or via a
   // user opt-in on Android), unconditionally enable opt-in origin isolation.
   if (UseDedicatedProcessesForAllSites())
@@ -144,7 +144,12 @@ bool SiteIsolationPolicy::IsOptInOriginIsolationEnabled() {
   if (IsSiteIsolationDisabled())
     return false;
 
-  return true;
+  return IsOriginAgentClusterEnabled();
+}
+
+// static
+bool SiteIsolationPolicy::IsOriginAgentClusterEnabled() {
+  return base::FeatureList::IsEnabled(features::kOriginIsolationHeader);
 }
 
 // static