Allow the amount of physical memory to disable site isolation.

- Add ContentBrowserClient::ShouldDisableSiteIsolation() so embedders
  can disable Site Isolation.
- Move Chrome memory checks in ChromeContentBrowserClient from
  ShouldEnableStrictSiteIsolation() to ShouldDisableSiteIsolation() so
  they can also disable Site Isolation.
- Added tests that verify memory constraints now disable trial isolated
  origins

Bug: 899116
Change-Id: Ibe4cbb93486db7e70cadf9a306d33606a8b33510
Reviewed-on: https://chromium-review.googlesource.com/c/1347443
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611306}

chrome/browser/chrome_content_browser_client.cc

@@ -1779,18 +1779,30 @@ ChromeContentBrowserClient::GetOriginsRequiringDedicatedProcess() {
 }
 
 bool ChromeContentBrowserClient::ShouldEnableStrictSiteIsolation() {
+  return base::FeatureList::IsEnabled(features::kSitePerProcess);
+}
+
+bool ChromeContentBrowserClient::ShouldDisableSiteIsolation() {
+  constexpr int kDefaultMemoryThresholdMb = 1024;
+
+  // TODO(acolwell): Rename feature since it now affects more than just the
+  // site-per-process case.
   if (base::FeatureList::IsEnabled(
           features::kSitePerProcessOnlyForHighMemoryClients)) {
-    constexpr int kDefaultMemoryThresholdMb = 1024;
     int memory_threshold_mb = base::GetFieldTrialParamByFeatureAsInt(
         features::kSitePerProcessOnlyForHighMemoryClients,
         features::kSitePerProcessOnlyForHighMemoryClientsParamName,
         kDefaultMemoryThresholdMb);
-    if (base::SysInfo::AmountOfPhysicalMemoryMB() <= memory_threshold_mb)
-      return false;
+    return base::SysInfo::AmountOfPhysicalMemoryMB() <= memory_threshold_mb;
   }
 
-  return base::FeatureList::IsEnabled(features::kSitePerProcess);
+#if defined(OS_ANDROID)
+  if (base::SysInfo::AmountOfPhysicalMemoryMB() <= kDefaultMemoryThresholdMb) {
+    return true;
+  }
+#endif
+
+  return false;
 }
 
 bool ChromeContentBrowserClient::IsFileAccessAllowed(

chrome/browser/chrome_content_browser_client.h

@@ -180,6 +180,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   bool ShouldAssignSiteForURL(const GURL& url) override;
   std::vector<url::Origin> GetOriginsRequiringDedicatedProcess() override;
   bool ShouldEnableStrictSiteIsolation() override;
+  bool ShouldDisableSiteIsolation() override;
   bool IsFileAccessAllowed(const base::FilePath& path,
                            const base::FilePath& absolute_path,
                            const base::FilePath& profile_path) override;

chrome/browser/chrome_content_browser_client_browsertest.cc

@@ -4,10 +4,14 @@
 
 #include "chrome/browser/chrome_content_browser_client.h"
 
+#include <vector>
+
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/system/sys_info.h"
+#include "base/test/scoped_feature_list.h"
+#include "build/build_config.h"
 #include "chrome/browser/policy/cloud/policy_header_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/instant_service.h"
@@ -30,15 +34,19 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "google_apis/gaia/gaia_urls.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/http/http_status_code.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 
 namespace content {
 
@@ -188,6 +196,27 @@ class SitePerProcessMemoryThresholdBrowserTest : public InProcessBrowserTest {
     return false;
   }
 
+ protected:
+  // These are the origins we expect to be returned by
+  // content::SiteIsolationPolicy::GetIsolatedOrigins() even if
+  // ContentBrowserClient::ShouldDisableSiteIsolation() returns true.
+  const std::vector<url::Origin> kExpectedEmbedderOrigins = {
+#if !defined(OS_ANDROID)
+    url::Origin::Create(GaiaUrls::GetInstance()->gaia_url())
+#endif
+  };
+
+#if defined(OS_ANDROID)
+  // On Android we don't expect any trial origins because the 512MB
+  // physical memory used for testing is below the Android specific
+  // hardcoded 1024MB memory limit that disables site isolation.
+  const std::size_t kExpectedTrialOrigins = 0;
+#else
+  // All other platforms expect the single trial origin to be returned because
+  // they don't have the memory limit that disables site isolation.
+  const std::size_t kExpectedTrialOrigins = 1;
+#endif
+
  private:
   DISALLOW_COPY_AND_ASSIGN(SitePerProcessMemoryThresholdBrowserTest);
 };
@@ -241,7 +270,15 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessMemoryThresholdBrowserTest,
   base::test::ScopedFeatureList site_per_process;
   site_per_process.InitAndEnableFeature(features::kSitePerProcess);
 
+#if defined(OS_ANDROID)
+  // Expect false on Android because 512MB physical memory triggered by
+  // kEnableLowEndDeviceMode in SetUpCommandLine() is below the 1024MB Android
+  // specific memory limit which disbles site isolation for all sites.
+  EXPECT_FALSE(
+      content::SiteIsolationPolicy::UseDedicatedProcessesForAllSites());
+#else
   EXPECT_TRUE(content::SiteIsolationPolicy::UseDedicatedProcessesForAllSites());
+#endif
 }
 
 IN_PROC_BROWSER_TEST_F(SitePerProcessMemoryThresholdBrowserTest,
@@ -300,6 +337,93 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessMemoryThresholdBrowserTest,
       content::SiteIsolationPolicy::UseDedicatedProcessesForAllSites());
 }
 
+IN_PROC_BROWSER_TEST_F(SitePerProcessMemoryThresholdBrowserTest,
+                       TrialIsolatedOrigins_HighThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  // 512MB of physical memory that the test simulates is below the 768MB
+  // threshold.
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "768"}});
+
+  const url::Origin trial_origin = url::Origin::Create(GURL("http://foo.com/"));
+  base::test::ScopedFeatureList isolated_origins_feature;
+  isolated_origins_feature.InitAndEnableFeatureWithParameters(
+      features::kIsolateOrigins, {{features::kIsolateOriginsFieldTrialParamName,
+                                   trial_origin.Serialize()}});
+
+  std::vector<url::Origin> isolated_origins =
+      content::SiteIsolationPolicy::GetIsolatedOrigins();
+  EXPECT_EQ(kExpectedTrialOrigins, isolated_origins.size());
+
+  // Verify that the expected embedder origins are present even though site
+  // isolation has been disabled and the trial origins should not be present.
+  EXPECT_THAT(kExpectedEmbedderOrigins,
+              ::testing::IsSubsetOf(isolated_origins));
+
+  // Verify that the trial origin is not present.
+  EXPECT_THAT(isolated_origins,
+              ::testing::Not(::testing::Contains(trial_origin)));
+}
+
+IN_PROC_BROWSER_TEST_F(SitePerProcessMemoryThresholdBrowserTest,
+                       TrialIsolatedOrigins_LowThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  // 512MB of physical memory that the test simulates is above the 128MB
+  // threshold.
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "128"}});
+
+  const url::Origin trial_origin = url::Origin::Create(GURL("http://foo.com/"));
+  base::test::ScopedFeatureList isolated_origins_feature;
+  isolated_origins_feature.InitAndEnableFeatureWithParameters(
+      features::kIsolateOrigins, {{features::kIsolateOriginsFieldTrialParamName,
+                                   trial_origin.Serialize()}});
+
+  std::vector<url::Origin> isolated_origins =
+      content::SiteIsolationPolicy::GetIsolatedOrigins();
+  EXPECT_EQ(1u + kExpectedEmbedderOrigins.size(), isolated_origins.size());
+  EXPECT_THAT(kExpectedEmbedderOrigins,
+              ::testing::IsSubsetOf(isolated_origins));
+
+  // Verify that the trial origin is present.
+  EXPECT_THAT(isolated_origins, ::testing::Contains(trial_origin));
+}
+
+IN_PROC_BROWSER_TEST_F(SitePerProcessMemoryThresholdBrowserTest,
+                       TrialIsolatedOrigins_NoThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  const url::Origin trial_origin = url::Origin::Create(GURL("http://foo.com/"));
+  base::test::ScopedFeatureList isolated_origins_feature;
+  isolated_origins_feature.InitAndEnableFeatureWithParameters(
+      features::kIsolateOrigins, {{features::kIsolateOriginsFieldTrialParamName,
+                                   trial_origin.Serialize()}});
+
+  std::vector<url::Origin> isolated_origins =
+      content::SiteIsolationPolicy::GetIsolatedOrigins();
+  EXPECT_EQ(kExpectedTrialOrigins + kExpectedEmbedderOrigins.size(),
+            isolated_origins.size());
+  EXPECT_THAT(kExpectedEmbedderOrigins,
+              ::testing::IsSubsetOf(isolated_origins));
+
+  if (kExpectedTrialOrigins > 0) {
+    // Verify that the trial origin is present.
+    EXPECT_THAT(isolated_origins, ::testing::Contains(trial_origin));
+  } else {
+    EXPECT_THAT(isolated_origins,
+                ::testing::Not(::testing::Contains(trial_origin)));
+  }
+}
+
 class PolicyHeaderServiceBrowserTest : public InProcessBrowserTest {
  public:
   PolicyHeaderServiceBrowserTest() = default;

content/public/browser/content_browser_client.cc

@@ -242,6 +242,10 @@ bool ContentBrowserClient::ShouldEnableStrictSiteIsolation() {
 #endif
 }
 
+bool ContentBrowserClient::ShouldDisableSiteIsolation() {
+  return false;
+}
+
 bool ContentBrowserClient::IsFileAccessAllowed(
     const base::FilePath& path,
     const base::FilePath& absolute_path,

content/public/browser/content_browser_client.h

@@ -464,6 +464,12 @@ class CONTENT_EXPORT ContentBrowserClient {
   // See also https://crbug.com/825369
   virtual bool ShouldEnableStrictSiteIsolation();
 
+  // Allows the embedder to programmatically control whether Site Isolation
+  // should be disabled.
+  //
+  // Note that for correctness, the same value should be consistently returned.
+  virtual bool ShouldDisableSiteIsolation();
+
   // Indicates whether a file path should be accessible via file URL given a
   // request from a browser context which lives within |profile_path|.
   virtual bool IsFileAccessAllowed(const base::FilePath& path,

content/public/browser/site_isolation_policy.cc

@@ -27,6 +27,20 @@
 
 namespace content {
 
+namespace {
+
+bool IsSiteIsolationDisabled() {
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableSiteIsolation)) {
+    return true;
+  }
+
+  return GetContentClient() &&
+         GetContentClient()->browser()->ShouldDisableSiteIsolation();
+}
+
+}  // namespace
+
 // static
 bool SiteIsolationPolicy::UseDedicatedProcessesForAllSites() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
@@ -34,10 +48,8 @@ bool SiteIsolationPolicy::UseDedicatedProcessesForAllSites() {
     return true;
   }
 
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableSiteIsolation)) {
+  if (IsSiteIsolationDisabled())
     return false;
-  }
 
   // The switches above needs to be checked first, because if the
   // ContentBrowserClient consults a base::Feature, then it will activate the
@@ -72,10 +84,8 @@ bool SiteIsolationPolicy::AreIsolatedOriginsEnabled() {
     return true;
   }
 
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableSiteIsolation)) {
+  if (IsSiteIsolationDisabled())
     return false;
-  }
 
   // The feature needs to be checked last, because checking the feature
   // activates the field trial and assigns the client either to a control or an
@@ -114,10 +124,8 @@ SiteIsolationPolicy::GetIsolatedOriginsFromEnvironment() {
 
   // --isolate-origins (both command-line flag and enterprise policy) trumps
   // the opt-out flag.
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableSiteIsolation)) {
+  if (IsSiteIsolationDisabled())
     return origins;
-  }
 
   // The feature needs to be checked last, because checking the feature
   // activates the field trial and assigns the client either to a control or an