Ignore memory threshold when password site isolation is overridden manually.

This CL fixes the undesirable behavior where manually enabling
password site isolation from chrome://flags didn't take effect if the
user's device was below the site isolation memory threshold.  The same
also affected strict origin isolation, which is also fixed.  (Strict
site isolation and IsolateOrigins are not affected because they map to
cmdline switches rather than base::Features.)

Bug: 1009828
Change-Id: Ibdfbca5ed1e2216e347e15a18c71ceaeafe2da03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845667
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704496}

chrome/browser/chrome_content_browser_client.cc

@@ -1851,30 +1851,7 @@ bool ChromeContentBrowserClient::ShouldEnableStrictSiteIsolation() {
 }
 
 bool ChromeContentBrowserClient::ShouldDisableSiteIsolation() {
-  // Using 1077 rather than 1024 because 1) it helps ensure that devices with
-  // exactly 1GB of RAM won't get included because of inaccuracies or off-by-one
-  // errors and 2) this is the bucket boundary in Memory.Stats.Win.TotalPhys2.
-  // See also https://crbug.com/844118.
-  constexpr int kDefaultMemoryThresholdMb = 1077;
-
-  // TODO(acolwell): Rename feature since it now affects more than just the
-  // site-per-process case.
-  if (base::FeatureList::IsEnabled(
-          features::kSitePerProcessOnlyForHighMemoryClients)) {
-    int memory_threshold_mb = base::GetFieldTrialParamByFeatureAsInt(
-        features::kSitePerProcessOnlyForHighMemoryClients,
-        features::kSitePerProcessOnlyForHighMemoryClientsParamName,
-        kDefaultMemoryThresholdMb);
-    return base::SysInfo::AmountOfPhysicalMemoryMB() <= memory_threshold_mb;
-  }
-
-#if defined(OS_ANDROID)
-  if (base::SysInfo::AmountOfPhysicalMemoryMB() <= kDefaultMemoryThresholdMb) {
-    return true;
-  }
-#endif
-
-  return false;
+  return SiteIsolationPolicy::ShouldDisableSiteIsolationDueToMemoryThreshold();
 }
 
 std::vector<std::string>

chrome/browser/chrome_content_browser_client_browsertest.cc

@@ -14,6 +14,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/system/sys_info.h"
+#include "base/test/mock_entropy_provider.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
@@ -22,6 +23,7 @@
 #include "chrome/browser/search/instant_service.h"
 #include "chrome/browser/search/instant_service_factory.h"
 #include "chrome/browser/search/search.h"
+#include "chrome/browser/site_isolation/site_isolation_policy.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/search/instant_test_base.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -35,6 +37,7 @@
 #include "components/network_session_configurator/common/network_switches.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/prefs/pref_service.h"
+#include "components/variations/variations_switches.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
@@ -503,6 +506,322 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessMemoryThresholdBrowserTest,
   }
 }
 
+// Helper class to run tests with password-triggered site isolation initialized
+// via a regular field trial and *not* via a command-line override.  It
+// creates a new field trial (with 100% probability of being in the group), and
+// initializes the test class's ScopedFeatureList using it.  Two derived
+// classes below control are used to initialize the feature to either enabled
+// or disabled state.
+class PasswordSiteIsolationFieldTrialTest
+    : public SitePerProcessMemoryThresholdBrowserTest {
+ public:
+  explicit PasswordSiteIsolationFieldTrialTest(bool should_enable)
+      : field_trial_list_(std::make_unique<base::MockEntropyProvider>()) {
+    const std::string kTrialName = "PasswordSiteIsolation";
+    const std::string kGroupName = "FooGroup";  // unused
+    scoped_refptr<base::FieldTrial> trial =
+        base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);
+
+    std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    feature_list->RegisterFieldTrialOverride(
+        features::kSiteIsolationForPasswordSites.name,
+        should_enable
+            ? base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE
+            : base::FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE,
+        trial.get());
+
+    feature_list_.InitWithFeatureList(std::move(feature_list));
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // This test creates and tests its own field trial group, so it needs to
+    // disable the field trial testing config, which might define an
+    // incompatible trial name/group.
+    command_line->AppendSwitch(
+        variations::switches::kDisableFieldTrialTestingConfig);
+    SitePerProcessMemoryThresholdBrowserTest::SetUpCommandLine(command_line);
+  }
+
+ protected:
+  base::test::ScopedFeatureList feature_list_;
+  base::FieldTrialList field_trial_list_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PasswordSiteIsolationFieldTrialTest);
+};
+
+class EnabledPasswordSiteIsolationFieldTrialTest
+    : public PasswordSiteIsolationFieldTrialTest {
+ public:
+  EnabledPasswordSiteIsolationFieldTrialTest()
+      : PasswordSiteIsolationFieldTrialTest(true /* should_enable */) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EnabledPasswordSiteIsolationFieldTrialTest);
+};
+
+class DisabledPasswordSiteIsolationFieldTrialTest
+    : public PasswordSiteIsolationFieldTrialTest {
+ public:
+  DisabledPasswordSiteIsolationFieldTrialTest()
+      : PasswordSiteIsolationFieldTrialTest(false /* should_enable */) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DisabledPasswordSiteIsolationFieldTrialTest);
+};
+
+IN_PROC_BROWSER_TEST_F(EnabledPasswordSiteIsolationFieldTrialTest,
+                       BelowThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  // If no memory threshold is defined, password site isolation should be
+  // enabled.
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  // Define a memory threshold at 768MB.  Since this is above the 512MB of
+  // physical memory that this test simulates, password site isolation should
+  // now be disabled.
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "768"}});
+
+  EXPECT_FALSE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  // Simulate enabling password site isolation from command line.  (Note that
+  // InitAndEnableFeature uses ScopedFeatureList::InitFromCommandLine
+  // internally, and that triggering the feature via chrome://flags follows the
+  // same override path as well.)
+  base::test::ScopedFeatureList password_site_isolation_feature;
+  password_site_isolation_feature.InitAndEnableFeature(
+      features::kSiteIsolationForPasswordSites);
+
+  // This should override the memory threshold and enable password site
+  // isolation.
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+}
+
+IN_PROC_BROWSER_TEST_F(EnabledPasswordSiteIsolationFieldTrialTest,
+                       AboveThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  // If no memory threshold is defined, password site isolation should be
+  // enabled.
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  // Define a memory threshold at 128MB.  Since this is below the 512MB of
+  // physical memory that this test simulates, password site isolation should
+  // still be enabled.
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "128"}});
+
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  // Simulate disabling password site isolation from command line.  (Note that
+  // InitAndEnableFeature uses ScopedFeatureList::InitFromCommandLine
+  // internally, and that triggering the feature via chrome://flags follows the
+  // same override path as well.)  This should take precedence over the regular
+  // field trial behavior.
+  base::test::ScopedFeatureList password_site_isolation_feature;
+  password_site_isolation_feature.InitAndDisableFeature(
+      features::kSiteIsolationForPasswordSites);
+  EXPECT_FALSE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+}
+
+// This test verifies that when password-triggered site isolation is disabled
+// via field trials but force-enabled via command line, it takes effect even
+// when below the memory threshold.  See https://crbug.com/1009828.
+IN_PROC_BROWSER_TEST_F(DisabledPasswordSiteIsolationFieldTrialTest,
+                       CommandLineOverride_BelowThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  // Password site isolation should be disabled at this point.
+  EXPECT_FALSE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  // Simulate enabling password site isolation from command line.  (Note that
+  // InitAndEnableFeature uses ScopedFeatureList::InitFromCommandLine
+  // internally, and that triggering the feature via chrome://flags follows the
+  // same override path as well.)
+  base::test::ScopedFeatureList password_site_isolation_feature;
+  password_site_isolation_feature.InitAndEnableFeature(
+      features::kSiteIsolationForPasswordSites);
+
+  // If no memory threshold is defined, password site isolation should be
+  // enabled.
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  // Define a memory threshold at 768MB.  This is above the 512MB of physical
+  // memory that this test simulates, but password site isolation should still
+  // be enabled, because the test has simulated the user manually overriding
+  // this feature via command line.
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "768"}});
+
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+}
+
+// Similar to the test above, but with device memory being above memory
+// threshold.
+IN_PROC_BROWSER_TEST_F(DisabledPasswordSiteIsolationFieldTrialTest,
+                       CommandLineOverride_AboveThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  EXPECT_FALSE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  base::test::ScopedFeatureList password_site_isolation_feature;
+  password_site_isolation_feature.InitAndEnableFeature(
+      features::kSiteIsolationForPasswordSites);
+
+  // If no memory threshold is defined, password site isolation should be
+  // enabled.
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "128"}});
+
+  EXPECT_TRUE(::SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled());
+}
+
+// Helper class to run tests with strict origin isolation initialized via
+// a regular field trial and *not* via a command-line override.  It creates a
+// new field trial (with 100% probability of being in the group), and
+// initializes the test class's ScopedFeatureList using it.  Two derived
+// classes below control are used to initialize the feature to either enabled
+// or disabled state.
+class StrictOriginIsolationFieldTrialTest
+    : public SitePerProcessMemoryThresholdBrowserTest {
+ public:
+  explicit StrictOriginIsolationFieldTrialTest(bool should_enable)
+      : field_trial_list_(std::make_unique<base::MockEntropyProvider>()) {
+    const std::string kTrialName = "StrictOriginIsolation";
+    const std::string kGroupName = "FooGroup";  // unused
+    scoped_refptr<base::FieldTrial> trial =
+        base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);
+
+    std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    feature_list->RegisterFieldTrialOverride(
+        features::kStrictOriginIsolation.name,
+        should_enable
+            ? base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE
+            : base::FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE,
+        trial.get());
+
+    feature_list_.InitWithFeatureList(std::move(feature_list));
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // This test creates and tests its own field trial group, so it needs to
+    // disable the field trial testing config, which might define an
+    // incompatible trial name/group.
+    command_line->AppendSwitch(
+        variations::switches::kDisableFieldTrialTestingConfig);
+    SitePerProcessMemoryThresholdBrowserTest::SetUpCommandLine(command_line);
+  }
+
+ protected:
+  base::test::ScopedFeatureList feature_list_;
+  base::FieldTrialList field_trial_list_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(StrictOriginIsolationFieldTrialTest);
+};
+
+class EnabledStrictOriginIsolationFieldTrialTest
+    : public StrictOriginIsolationFieldTrialTest {
+ public:
+  EnabledStrictOriginIsolationFieldTrialTest()
+      : StrictOriginIsolationFieldTrialTest(true /* should_enable */) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EnabledStrictOriginIsolationFieldTrialTest);
+};
+
+class DisabledStrictOriginIsolationFieldTrialTest
+    : public StrictOriginIsolationFieldTrialTest {
+ public:
+  DisabledStrictOriginIsolationFieldTrialTest()
+      : StrictOriginIsolationFieldTrialTest(false /* should_enable */) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DisabledStrictOriginIsolationFieldTrialTest);
+};
+
+// Check that when strict origin isolation is enabled via a field trial, and
+// the device is above the memory threshold, disabling it via the command line
+// takes precedence.
+IN_PROC_BROWSER_TEST_F(EnabledStrictOriginIsolationFieldTrialTest,
+                       DisabledViaCommandLineOverride) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  // If no memory threshold is defined, strict origin isolation should be
+  // enabled.
+  EXPECT_TRUE(SiteIsolationPolicy::IsStrictOriginIsolationEnabled());
+
+  // Define a memory threshold at 128MB.  Since this is below the 512MB of
+  // physical memory that this test simulates, strict origin isolation should
+  // still be enabled.
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "128"}});
+  EXPECT_TRUE(SiteIsolationPolicy::IsStrictOriginIsolationEnabled());
+
+  // Simulate disabling strict origin isolation from command line.  (Note that
+  // InitAndEnableFeature uses ScopedFeatureList::InitFromCommandLine
+  // internally, and that disabling the feature via chrome://flags follows the
+  // same override path as well.)
+  base::test::ScopedFeatureList strict_origin_isolation_feature;
+  strict_origin_isolation_feature.InitAndDisableFeature(
+      features::kStrictOriginIsolation);
+  EXPECT_FALSE(SiteIsolationPolicy::IsStrictOriginIsolationEnabled());
+}
+
+// This test verifies that when strict origin isolation is disabled
+// via field trials but force-enabled via command line, it takes effect even
+// when below the memory threshold.  See https://crbug.com/1009828.
+IN_PROC_BROWSER_TEST_F(DisabledStrictOriginIsolationFieldTrialTest,
+                       EnabledViaCommandLineOverride_BelowThreshold) {
+  if (ShouldSkipBecauseOfConflictingCommandLineSwitches())
+    return;
+
+  // Strict origin isolation should be disabled at this point.
+  EXPECT_FALSE(content::SiteIsolationPolicy::IsStrictOriginIsolationEnabled());
+
+  // Simulate enabling strict origin isolation from command line.  (Note that
+  // InitAndEnableFeature uses ScopedFeatureList::InitFromCommandLine
+  // internally, and that triggering the feature via chrome://flags follows the
+  // same override path as well.)
+  base::test::ScopedFeatureList strict_origin_isolation_feature;
+  strict_origin_isolation_feature.InitAndEnableFeature(
+      features::kStrictOriginIsolation);
+
+  // If no memory threshold is defined, strict origin isolation should be
+  // enabled.
+  EXPECT_TRUE(SiteIsolationPolicy::IsStrictOriginIsolationEnabled());
+
+  // Define a memory threshold at 768MB.  This is above the 512MB of physical
+  // memory that this test simulates, but strict origin isolation should still
+  // be enabled, because the test has simulated the user manually overriding
+  // this feature via command line.
+  base::test::ScopedFeatureList memory_feature;
+  memory_feature.InitAndEnableFeatureWithParameters(
+      features::kSitePerProcessOnlyForHighMemoryClients,
+      {{features::kSitePerProcessOnlyForHighMemoryClientsParamName, "768"}});
+
+  EXPECT_TRUE(SiteIsolationPolicy::IsStrictOriginIsolationEnabled());
+}
+
 // Helper class to test window creation from NTP.
 class OpenWindowFromNTPBrowserTest : public InProcessBrowserTest,
                                      public InstantTestBase {

chrome/browser/site_isolation/site_isolation_policy.cc

@@ -16,9 +16,23 @@
 
 // static
 bool SiteIsolationPolicy::IsIsolationForPasswordSitesEnabled() {
-  // Ignore attempts to add new isolated origins when site isolation is turned
-  // off, for example via a command-line switch, or via a content/ embedder
-  // that turns site isolation off for low-memory devices.
+  // If the user has explicitly enabled site isolation for password sites from
+  // chrome://flags or from the command line, honor this regardless of policies
+  // that may disable site isolation.  In particular, this means that the
+  // chrome://flags switch for this feature takes precedence over any memory
+  // threshold restrictions and over a switch for disabling site isolation.
+  if (base::FeatureList::GetInstance()->IsFeatureOverriddenFromCommandLine(
+          features::kSiteIsolationForPasswordSites.name,
+          base::FeatureList::OVERRIDE_ENABLE_FEATURE)) {
+    return true;
+  }
+
+  // Don't isolate anything when site isolation is turned off by the user or
+  // policy. This includes things like the switches::kDisableSiteIsolation
+  // command-line switch, the corresponding "Disable site isolation" entry in
+  // chrome://flags, enterprise policy controlled via
+  // switches::kDisableSiteIsolationForPolicy, and memory threshold checks in
+  // ShouldDisableSiteIsolationDueToMemoryThreshold().
   if (!content::SiteIsolationPolicy::AreDynamicIsolatedOriginsEnabled())
     return false;
 
@@ -42,6 +56,34 @@ bool SiteIsolationPolicy::IsEnterprisePolicyApplicable() {
 #endif
 }
 
+// static
+bool SiteIsolationPolicy::ShouldDisableSiteIsolationDueToMemoryThreshold() {
+  // Using 1077 rather than 1024 because 1) it helps ensure that devices with
+  // exactly 1GB of RAM won't get included because of inaccuracies or off-by-one
+  // errors and 2) this is the bucket boundary in Memory.Stats.Win.TotalPhys2.
+  // See also https://crbug.com/844118.
+  constexpr int kDefaultMemoryThresholdMb = 1077;
+
+  // TODO(acolwell): Rename feature since it now affects more than just the
+  // site-per-process case.
+  if (base::FeatureList::IsEnabled(
+          features::kSitePerProcessOnlyForHighMemoryClients)) {
+    int memory_threshold_mb = base::GetFieldTrialParamByFeatureAsInt(
+        features::kSitePerProcessOnlyForHighMemoryClients,
+        features::kSitePerProcessOnlyForHighMemoryClientsParamName,
+        kDefaultMemoryThresholdMb);
+    return base::SysInfo::AmountOfPhysicalMemoryMB() <= memory_threshold_mb;
+  }
+
+#if defined(OS_ANDROID)
+  if (base::SysInfo::AmountOfPhysicalMemoryMB() <= kDefaultMemoryThresholdMb) {
+    return true;
+  }
+#endif
+
+  return false;
+}
+
 // static
 void SiteIsolationPolicy::ApplyPersistedIsolatedOrigins(Profile* profile) {
   // If the user turned off password-triggered isolation, don't apply any

chrome/browser/site_isolation/site_isolation_policy.h

@@ -31,6 +31,15 @@ class SiteIsolationPolicy {
   // have been loaded.
   static void ApplyPersistedIsolatedOrigins(Profile* profile);
 
+  // Determines whether Site Isolation should be disabled because the device
+  // does not have the minimum required amount of memory.
+  //
+  // TODO(alexmos): Currently, the memory threshold is shared for all site
+  // isolation modes, including strict site isolation and password site
+  // isolation.  In the future, some site isolation modes may require their own
+  // memory threshold.
+  static bool ShouldDisableSiteIsolationDueToMemoryThreshold();
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(SiteIsolationPolicy);
 };

content/public/browser/site_isolation_policy.cc

@@ -45,6 +45,8 @@ bool IsSiteIsolationDisabled() {
   }
 #endif
 
+  // Check with the embedder.  In particular, chrome/ uses this to disable site
+  // isolation when below a memory threshold.
   return GetContentClient() &&
          GetContentClient()->browser()->ShouldDisableSiteIsolation();
 }
@@ -90,6 +92,16 @@ bool SiteIsolationPolicy::AreIsolatedOriginsEnabled() {
 
 // static
 bool SiteIsolationPolicy::IsStrictOriginIsolationEnabled() {
+  // If the feature is explicitly enabled by the user (e.g., from
+  // chrome://flags), honor this regardless of checks to disable site isolation
+  // below.  This means this takes precedence over memory thresholds or
+  // switches to disable site isolation.
+  if (base::FeatureList::GetInstance()->IsFeatureOverriddenFromCommandLine(
+          features::kStrictOriginIsolation.name,
+          base::FeatureList::OVERRIDE_ENABLE_FEATURE)) {
+    return true;
+  }
+
   // TODO(wjmaclean): Figure out what should happen when this feature is
   // combined with --isolate-origins.
   if (IsSiteIsolationDisabled())