Prevent a tab from being discarded if it shares its BrowsingInstance

This CL add a feature parameter to the Proactive Tab Discarding and Freezing experiment to prevent discarding tabs that share their browsing instance with another tab. This might be too agressive, in practice we might just want to consider tabs under the same SiteInstance but this is not an information that we can access yet. Check this page for more details about the SiteInstance/BrowsingInstance concepts: https://www.chromium.org/developers/design-documents/process-models#Implementation_Notes UKM privacy document has been updated here (pending approval): https://docs.google.com/a/google.com/document/d/1BNQ5nLOtPuwP7oxr9r-XKNKr5iObXEiA_69WXAvuYAo/edit?disco=AAAABzM-vE0 Bug: 775644, 862203 Change-Id: Iaf5107db1d68aa4a0155ecbae7283f487c23bb7c Reviewed-on: https://chromium-review.googlesource.com/1144188 Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org> Reviewed-by: Chris Hamilton <chrisha@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#576789}(cherry picked from commit 94d6d39) Reviewed-on: https://chromium-review.googlesource.com/1150261 Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#85} Cr-Branched-From: 271eaf5-refs/heads/master@{#576753}
nwjs · Jul 25, 2018 · 4dc421a · 4dc421a
1 parent 73635a1
commit 4dc421a
Show file tree

Hide file tree

Showing 8 changed files with 132 additions and 9 deletions.
diff --git a/chrome/browser/resource_coordinator/decision_details.cc b/chrome/browser/resource_coordinator/decision_details.cc
@@ -34,6 +34,7 @@ const char* kDecisionFailureReasonStrings[] = {
     "Tab is currently visible",
     "Tab is currently using DevTools",
     "Tab is currently capturing a window or screen",
+    "Tab is sharing its BrowsingInstance with another tab",
 };
 static_assert(base::size(kDecisionFailureReasonStrings) ==
                   static_cast<size_t>(DecisionFailureReason::MAX),
@@ -130,6 +131,9 @@ void PopulateFailureReason(
     case DecisionFailureReason::LIVE_STATE_DESKTOP_CAPTURE:
       ukm->SetFailureLiveStateDesktopCapture(1);
       break;
+    case DecisionFailureReason::LIVE_STATE_SHARING_BROWSING_INSTANCE:
+      ukm->SetFailureLiveStateSharingBrowsingInstance(1);
+      break;
     case DecisionFailureReason::MAX:
       break;
   }

diff --git a/chrome/browser/resource_coordinator/decision_details.h b/chrome/browser/resource_coordinator/decision_details.h
@@ -73,6 +73,9 @@ enum class DecisionFailureReason : int32_t {
   // The tab is opted out of the intervention as it's currently capturing a
   // window or screen.
   LIVE_STATE_DESKTOP_CAPTURE,
+  // This tab is sharing its BrowsingInstance with another tab, and so could
+  // want to communicate with it.
+  LIVE_STATE_SHARING_BROWSING_INSTANCE,
   // This must remain last.
   MAX,
 };

diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc b/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
@@ -883,28 +883,38 @@ void TabLifecycleUnitSource::TabLifecycleUnit::CheckIfTabIsUsedInBackground(
   // This ensures that the decision details lists all possible reasons that the
   // transition can be denied.
 
+  auto* web_contents = GetWebContents();
+
   // Do not freeze tabs that are casting/mirroring/playing audio.
   IsMediaTabImpl(decision_details);
 
+  if (GetStaticProactiveTabFreezeAndDiscardParams()
+          .should_protect_tabs_sharing_browsing_instance) {
+    if (web_contents->GetSiteInstance()->GetRelatedActiveContentsCount() > 1U) {
+      decision_details->AddReason(
+          DecisionFailureReason::LIVE_STATE_SHARING_BROWSING_INSTANCE);
+    }
+  }
+
   // Consult the local database to see if this tab could try to communicate with
   // the user while in background (don't check for the visibility here as
   // there's already a check for that above). Only do this for proactive
   // interventions.
   if (intervention_type == InterventionType::kProactive) {
-    CheckIfTabCanCommunicateWithUserWhileInBackground(GetWebContents(),
+    CheckIfTabCanCommunicateWithUserWhileInBackground(web_contents,
                                                       decision_details);
   }
 
   // Do not freeze/discard a tab that has active WebUSB connections.
-  if (auto* usb_tab_helper = UsbTabHelper::FromWebContents(GetWebContents())) {
+  if (auto* usb_tab_helper = UsbTabHelper::FromWebContents(web_contents)) {
     if (usb_tab_helper->IsDeviceConnected()) {
       decision_details->AddReason(
           DecisionFailureReason::LIVE_STATE_USING_WEB_USB);
     }
   }
 
   // Do not freeze tabs that are currently using DevTools.
-  if (DevToolsWindow::GetInstanceForInspectedWebContents(GetWebContents())) {
+  if (DevToolsWindow::GetInstanceForInspectedWebContents(web_contents)) {
     decision_details->AddReason(
         DecisionFailureReason::LIVE_STATE_DEVTOOLS_OPEN);
   }

diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit_unittest.cc b/chrome/browser/resource_coordinator/tab_lifecycle_unit_unittest.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/resource_coordinator/tab_lifecycle_unit.h"
 
 #include <memory>
+#include <string>
 
 #include "base/macros.h"
 #include "base/observer_list.h"
@@ -31,6 +32,7 @@
 #include "chrome/browser/usb/usb_tab_helper.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/referrer.h"
 #include "content/public/test/web_contents_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -648,6 +650,51 @@ TEST_F(TabLifecycleUnitTest, NotifiedOfWebContentsVisibilityChanges) {
   tab_lifecycle_unit.RemoveObserver(&observer);
 }
 
+TEST_F(TabLifecycleUnitTest, CannotFreezeOrDiscardIfSharingBrowsingInstance) {
+  TabLifecycleUnit tab_lifecycle_unit(GetSource(), &observers_,
+                                      usage_clock_.get(), web_contents_,
+                                      tab_strip_model_.get());
+  TabLoadTracker::Get()->TransitionStateForTesting(web_contents_,
+                                                   LoadingState::LOADED);
+
+  // Creates a second WebContents that use the same SiteInstance.
+  auto* site_instance = web_contents_->GetSiteInstance();
+  std::unique_ptr<content::WebContents> contents =
+      content::WebContentsTester::CreateTestWebContents(browser_context(),
+                                                        site_instance);
+  // Navigate this second WebContents to another URL, after this these 2
+  // WebContents will use separate SiteInstances owned by the same
+  // BrowsingInstance.
+  contents->GetController().LoadURL(GURL("http://another-url.com/"),
+                                    content::Referrer(),
+                                    ui::PAGE_TRANSITION_LINK, std::string());
+  EXPECT_NE(web_contents_->GetSiteInstance(), contents->GetSiteInstance());
+  EXPECT_EQ(2U,
+            web_contents_->GetSiteInstance()->GetRelatedActiveContentsCount());
+
+  DecisionDetails decision_details;
+  EXPECT_TRUE(tab_lifecycle_unit.CanFreeze(&decision_details));
+  EXPECT_TRUE(decision_details.IsPositive());
+
+  {
+    GetMutableStaticProactiveTabFreezeAndDiscardParamsForTesting()
+        ->should_protect_tabs_sharing_browsing_instance = true;
+    decision_details.Clear();
+    EXPECT_FALSE(tab_lifecycle_unit.CanFreeze(&decision_details));
+    EXPECT_FALSE(decision_details.IsPositive());
+    EXPECT_EQ(DecisionFailureReason::LIVE_STATE_SHARING_BROWSING_INSTANCE,
+              decision_details.FailureReason());
+
+    decision_details.Clear();
+
+    EXPECT_FALSE(tab_lifecycle_unit.CanDiscard(DiscardReason::kProactive,
+                                               &decision_details));
+    EXPECT_FALSE(decision_details.IsPositive());
+    EXPECT_EQ(DecisionFailureReason::LIVE_STATE_SHARING_BROWSING_INSTANCE,
+              decision_details.FailureReason());
+  }
+}
+
 TEST_F(TabLifecycleUnitTest, CannotDiscardIfEnterpriseOptOutUsed) {
   TabLifecycleUnit tab_lifecycle_unit(GetSource(), &observers_,
                                       usage_clock_.get(), web_contents_,

diff --git a/chrome/browser/resource_coordinator/tab_manager_features.cc b/chrome/browser/resource_coordinator/tab_manager_features.cc
@@ -89,6 +89,15 @@ const char kProactiveTabFreezeAndDiscard_ShouldProactivelyDiscardParam[] =
     "ShouldProactivelyDiscard";
 const char kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeParam[] =
     "ShouldPeriodicallyUnfreeze";
+
+// NOTE: This parameter is disabled by default and shouldn't be enabled until
+// the privacy review for the UKM associated with it has been approved, see
+//  https://docs.google.com/a/google.com/document/d/1BNQ5nLOtPuwP7oxr9r-XKNKr5iObXEiA_69WXAvuYAo/edit?disco=AAAABzM-vE0
+//
+// TODO(sebmarchand): Remove this comment once the UKM has been approved.
+const char
+    kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceParam
+        [] = "ShouldProtectTabsSharingBrowsingInstance";
 const char kProactiveTabFreezeAndDiscard_LowLoadedTabCountParam[] =
     "LowLoadedTabCount";
 const char kProactiveTabFreezeAndDiscard_ModerateLoadedTabsPerGbRamParam[] =
@@ -137,6 +146,16 @@ const bool kProactiveTabFreezeAndDiscard_ShouldProactivelyDiscardDefault =
     false;
 const bool kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeDefault =
     false;
+
+// NOTE: This parameter is disabled by default and shouldn't be enabled until
+// the privacy review for the UKM associated with it has been approved, see
+//  https://docs.google.com/a/google.com/document/d/1BNQ5nLOtPuwP7oxr9r-XKNKr5iObXEiA_69WXAvuYAo/edit?disco=AAAABzM-vE0
+//
+// TODO(sebmarchand): Remove this comment once the UKM has been approved.
+const bool
+    kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceDefault =
+        false;
+
 // 50% of people cap out at 4 tabs, so for them proactive discarding won't even
 // be invoked. See Tabs.MaxTabsInADay.
 // TODO(chrisha): This should eventually be informed by the number of tabs
@@ -231,6 +250,12 @@ ProactiveTabFreezeAndDiscardParams GetProactiveTabFreezeAndDiscardParams(
       kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeParam,
       kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeDefault);
 
+  params.should_protect_tabs_sharing_browsing_instance =
+      base::GetFieldTrialParamByFeatureAsBool(
+          features::kProactiveTabFreezeAndDiscard,
+          kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceParam,
+          kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceDefault);
+
   params.low_loaded_tab_count = base::GetFieldTrialParamByFeatureAsInt(
       features::kProactiveTabFreezeAndDiscard,
       kProactiveTabFreezeAndDiscard_LowLoadedTabCountParam,
@@ -294,6 +319,12 @@ GetStaticProactiveTabFreezeAndDiscardParams() {
   return *params;
 }
 
+ProactiveTabFreezeAndDiscardParams*
+GetMutableStaticProactiveTabFreezeAndDiscardParamsForTesting() {
+  return const_cast<ProactiveTabFreezeAndDiscardParams*>(
+      &GetStaticProactiveTabFreezeAndDiscardParams());
+}
+
 base::TimeDelta GetTabLoadTimeout(const base::TimeDelta& default_timeout) {
   int timeout_in_ms = base::GetFieldTrialParamByFeatureAsInt(
       features::kCustomizedTabLoadTimeout, kTabLoadTimeoutInMsParameterName,

diff --git a/chrome/browser/resource_coordinator/tab_manager_features.h b/chrome/browser/resource_coordinator/tab_manager_features.h
@@ -29,9 +29,14 @@ extern const char kProactiveTabFreezeAndDiscardFeatureName[];
 
 // Variations parameter names related to proactive discarding.
 // See ProactiveTabFreezeAndDiscardsParams for details.
+//
+// TODO(sebmarchand): Use the base::FeatureParam API here.
 extern const char kProactiveTabFreezeAndDiscard_ShouldProactivelyDiscardParam[];
 extern const char
     kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeParam[];
+extern const char
+    kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceParam
+        [];
 extern const char kProactiveTabFreezeAndDiscard_LowLoadedTabCountParam[];
 extern const char
     kProactiveTabFreezeAndDiscard_ModerateLoadedTabsPerGbRamParam[];
@@ -66,6 +71,8 @@ extern const char kInfiniteSessionRestore_MinSiteEngagementToRestore[];
 extern const bool kProactiveTabFreezeAndDiscard_ShouldProactivelyDiscardDefault;
 extern const bool
     kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeDefault;
+extern const bool
+    kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceDefault;
 extern const uint32_t kProactiveTabFreezeAndDiscard_LowLoadedTabCountDefault;
 extern const uint32_t
     kProactiveTabFreezeAndDiscard_ModerateLoadedTabsPerGbRamDefault;
@@ -152,6 +159,9 @@ struct ProactiveTabFreezeAndDiscardParams {
   bool should_proactively_discard;
   // Whether frozen tabs should periodically be unfrozen to update their state.
   bool should_periodically_unfreeze;
+  // Whether tabs should be protected from freezing/discarding if they share
+  // their BrowsingInstance with another tab.
+  bool should_protect_tabs_sharing_browsing_instance;
   // Tab count (inclusive) beyond which the state transitions to MODERATE.
   // Intended to cover the majority of simple workflows and be small enough that
   // it is very unlikely that memory pressure will be encountered with this many
@@ -256,6 +266,8 @@ ProactiveTabFreezeAndDiscardParams GetProactiveTabFreezeAndDiscardParams(
 // all the classes that need one.
 const ProactiveTabFreezeAndDiscardParams&
 GetStaticProactiveTabFreezeAndDiscardParams();
+ProactiveTabFreezeAndDiscardParams*
+GetMutableStaticProactiveTabFreezeAndDiscardParamsForTesting();
 
 base::TimeDelta GetTabLoadTimeout(const base::TimeDelta& default_timeout);
 

diff --git a/chrome/browser/resource_coordinator/tab_manager_features_unittest.cc b/chrome/browser/resource_coordinator/tab_manager_features_unittest.cc
@@ -50,6 +50,7 @@ class TabManagerFeaturesTest : public testing::Test {
   void ExpectProactiveTabFreezeAndDiscardParams(
       bool should_proactively_discard,
       bool should_periodically_unfreeze,
+      bool should_protect_tabs_sharing_browsing_instance,
       int low_loaded_tab_count,
       int moderate_loaded_tab_count,
       int high_loaded_tab_count,
@@ -66,6 +67,8 @@ class TabManagerFeaturesTest : public testing::Test {
     EXPECT_EQ(should_proactively_discard, params.should_proactively_discard);
     EXPECT_EQ(should_periodically_unfreeze,
               params.should_periodically_unfreeze);
+    EXPECT_EQ(should_protect_tabs_sharing_browsing_instance,
+              params.should_protect_tabs_sharing_browsing_instance);
     EXPECT_EQ(low_loaded_tab_count, params.low_loaded_tab_count);
     EXPECT_EQ(moderate_loaded_tab_count, params.moderate_loaded_tab_count);
 
@@ -131,6 +134,7 @@ class TabManagerFeaturesTest : public testing::Test {
     ExpectProactiveTabFreezeAndDiscardParams(
         kProactiveTabFreezeAndDiscard_ShouldProactivelyDiscardDefault,
         kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeDefault,
+        kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceDefault,
         kProactiveTabFreezeAndDiscard_LowLoadedTabCountDefault,
         kProactiveTabFreezeAndDiscard_ModerateLoadedTabsPerGbRamDefault *
             memory_in_gb,
@@ -187,6 +191,9 @@ TEST_F(TabManagerFeaturesTest,
   SetParam(kProactiveTabFreezeAndDiscard_ShouldProactivelyDiscardParam, "blah");
   SetParam(kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeParam,
            "blah");
+  SetParam(
+      kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceParam,
+      "bleh");
   SetParam(kProactiveTabFreezeAndDiscard_LowLoadedTabCountParam, "ab");
   SetParam(kProactiveTabFreezeAndDiscard_ModerateLoadedTabsPerGbRamParam,
            "27.8");
@@ -205,6 +212,9 @@ TEST_F(TabManagerFeaturesTest, GetProactiveTabFreezeAndDiscardParams) {
   SetParam(kProactiveTabFreezeAndDiscard_ShouldProactivelyDiscardParam, "true");
   SetParam(kProactiveTabFreezeAndDiscard_ShouldPeriodicallyUnfreezeParam,
            "true");
+  SetParam(
+      kProactiveTabFreezeAndDiscard_ShouldProtectTabsSharingBrowsingInstanceParam,
+      "true");
   SetParam(kProactiveTabFreezeAndDiscard_LowLoadedTabCountParam, "7");
   SetParam(kProactiveTabFreezeAndDiscard_ModerateLoadedTabsPerGbRamParam, "4");
   SetParam(kProactiveTabFreezeAndDiscard_HighLoadedTabCountParam, "42");
@@ -222,17 +232,17 @@ TEST_F(TabManagerFeaturesTest, GetProactiveTabFreezeAndDiscardParams) {
   // |moderate_tab_count_per_gb_ram|) < |low_loaded_tab_count|).
   int memory_in_gb_low = 1;
   ExpectProactiveTabFreezeAndDiscardParams(
-      true, true, 7, 7, 42, memory_in_gb_low, base::TimeDelta::FromSeconds(60),
-      base::TimeDelta::FromSeconds(120), base::TimeDelta::FromSeconds(247),
-      base::TimeDelta::FromSeconds(10), base::TimeDelta::FromSeconds(20),
-      base::TimeDelta::FromSeconds(30));
+      true, true, true, 7, 7, 42, memory_in_gb_low,
+      base::TimeDelta::FromSeconds(60), base::TimeDelta::FromSeconds(120),
+      base::TimeDelta::FromSeconds(247), base::TimeDelta::FromSeconds(10),
+      base::TimeDelta::FromSeconds(20), base::TimeDelta::FromSeconds(30));
 
   // Should snap |moderate_loaded_tab_count| to |high_loaded_tab_count|, when
   // the amount of physical memory is so high that (|memory_in_gb| *
   // |moderate_tab_count_per_gb_ram|) > |high_loaded_tab_count|).
   int memory_in_gb_high = 100;
   ExpectProactiveTabFreezeAndDiscardParams(
-      true, true, 7, 42, 42, memory_in_gb_high,
+      true, true, true, 7, 42, 42, memory_in_gb_high,
       base::TimeDelta::FromSeconds(60), base::TimeDelta::FromSeconds(120),
       base::TimeDelta::FromSeconds(247), base::TimeDelta::FromSeconds(10),
       base::TimeDelta::FromSeconds(20), base::TimeDelta::FromSeconds(30));
@@ -241,7 +251,7 @@ TEST_F(TabManagerFeaturesTest, GetProactiveTabFreezeAndDiscardParams) {
   // in the interval [low_loaded_tab_count, high_loaded_tab_count].
   int memory_in_gb_normal = 4;
   ExpectProactiveTabFreezeAndDiscardParams(
-      true, true, 7, 16, 42, memory_in_gb_normal,
+      true, true, true, 7, 16, 42, memory_in_gb_normal,
       base::TimeDelta::FromSeconds(60), base::TimeDelta::FromSeconds(120),
       base::TimeDelta::FromSeconds(247), base::TimeDelta::FromSeconds(10),
       base::TimeDelta::FromSeconds(20), base::TimeDelta::FromSeconds(30));

diff --git a/tools/metrics/ukm/ukm.xml b/tools/metrics/ukm/ukm.xml
@@ -3688,6 +3688,12 @@ be describing additional metrics about the same event.
       currently playing audio.
     </summary>
   </metric>
+  <metric name="FailureLiveStateSharingBrowsingInstance">
+    <summary>
+      Boolean indicating that the intervention was disallowed because the tab is
+      sharing its BrowsingInstance with at least one other tab.
+    </summary>
+  </metric>
   <metric name="FailureLiveStateUsingWebSockets">
     <summary>
       Boolean indicating that the intervention was disallowed because the tab is