Try reusing old page's renderer process when we proactively swap BrowsingInstances

With ProactivelySwapBrowsingInstance, we will swap BrowsingInstances on
most cross-site navigations, which means there will be more
cross-process navigations on Android. This CL tries to reduce that by
trying to reuse the previous page's renderer process for the new page
when we do a proactive BrowsingInstance swap, if possible.

Note that the CL won't change cases where we are already forcing a
BrowsingInstance + process swap. Browser-initiated cross-site
navigations will cause BrowsingInstance + process swap no matter what,
and they will keep doing so even with this CL. Some renderer-initiated
cross-site navigations, however, do not trigger a BrowsingInstance swap
without us proactively swapping, so it is OK to reuse the process in
their case (because we would've done so anyways if we don't do a
proactive BrowsingInstance swap).

See https://docs.google.com/document/d/1b-uMmwzTlwGfpn8pjd59iA57RnBHVx4IOarWnxYYxQk/edit?usp=sharing
for more context (google-only)

Bug: 977562
Change-Id: I57d725aa1435de0ee54788de4bfe627161400725
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2043521
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751515}

content/browser/frame_host/render_frame_host_manager.cc

@@ -162,7 +162,7 @@ ShouldSwapBrowsingInstance ShouldProactivelySwapBrowsingInstance(
   }
 
   if (IsProactivelySwapBrowsingInstanceEnabled())
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ProactiveSwap;
 
   // If BackForwardCache is enabled, swap BrowsingInstances only when needed
   // for back-forward cache.
@@ -171,7 +171,7 @@ ShouldSwapBrowsingInstance ShouldProactivelySwapBrowsingInstance(
       current_rfh->frame_tree_node()->navigator()->GetController());
   if (controller->GetBackForwardCache().IsAllowed(
           current_rfh->GetLastCommittedURL())) {
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ProactiveSwap;
   } else {
     return ShouldSwapBrowsingInstance::kNo_NotNeededForBackForwardCache;
   }
@@ -1301,7 +1301,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
     bool should_swap = !destination_instance->IsRelatedSiteInstance(
         render_frame_host_->GetSiteInstance());
     if (should_swap) {
-      return ShouldSwapBrowsingInstance::kYes;
+      return ShouldSwapBrowsingInstance::kYes_ForceSwap;
     } else {
       return ShouldSwapBrowsingInstance::kNo_AlreadyHasMatchingBrowsingInstance;
     }
@@ -1319,7 +1319,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
     return ShouldSwapBrowsingInstance::kNo_RendererDebugURL;
 
   if (cross_origin_opener_policy_mismatch)
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ForceSwap;
 
   // Transitions across BrowserContexts should always require a
   // BrowsingInstance swap. For example, this can happen if an extension in a
@@ -1333,7 +1333,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
             render_frame_host_->GetSiteInstance()->GetBrowserContext());
   if (browser_context !=
       render_frame_host_->GetSiteInstance()->GetBrowserContext()) {
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ForceSwap;
   }
 
   // For security, we should transition between processes when one is a Web UI
@@ -1346,7 +1346,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
     // Here, data URLs are never allowed.
     if (!WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
             browser_context, destination_effective_url)) {
-      return ShouldSwapBrowsingInstance::kYes;
+      return ShouldSwapBrowsingInstance::kYes_ForceSwap;
     }
 
     // Force swap if the current WebUI type differs from the one for the
@@ -1355,13 +1355,13 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
             browser_context, current_effective_url) !=
         WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
             browser_context, destination_effective_url)) {
-      return ShouldSwapBrowsingInstance::kYes;
+      return ShouldSwapBrowsingInstance::kYes_ForceSwap;
     }
   } else {
     // Force a swap if it's a Web UI URL.
     if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
             browser_context, destination_effective_url)) {
-      return ShouldSwapBrowsingInstance::kYes;
+      return ShouldSwapBrowsingInstance::kYes_ForceSwap;
     }
   }
 
@@ -1371,7 +1371,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
   if (GetContentClient()->browser()->ShouldSwapBrowsingInstancesForNavigation(
           render_frame_host_->GetSiteInstance(), current_effective_url,
           destination_effective_url)) {
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ForceSwap;
   }
 
   // We can't switch a RenderView between view source and non-view source mode
@@ -1379,13 +1379,13 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
   // "view-source:http://foo.com/" and "http://foo.com/", Blink doesn't treat
   // it as a new navigation). So require a BrowsingInstance switch.
   if (current_is_view_source_mode != destination_is_view_source_mode)
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ForceSwap;
 
   // If we haven't used the current SiteInstance but the destination is a
   // view-source URL, we should force a BrowsingInstance swap so that we won't
   // reuse the current SiteInstance.
   if (!current_instance->HasSite() && destination_is_view_source_mode)
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ForceSwap;
 
   // If the target URL's origin was dynamically isolated, and the isolation
   // wouldn't apply in the current BrowsingInstance, see if this navigation can
@@ -1396,7 +1396,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
   // possible (e.g., when there are no existing script references).
   if (ShouldSwapBrowsingInstancesForDynamicIsolation(
           render_frame_host_.get(), destination_effective_url)) {
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ForceSwap;
   }
 
   // If this is a cross-site navigation, we may be able to force a
@@ -1416,7 +1416,7 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
       !is_for_isolated_error_page &&
       IsBrowsingInstanceSwapAllowedForPageTransition(
           transition, destination_effective_url)) {
-    return ShouldSwapBrowsingInstance::kYes;
+    return ShouldSwapBrowsingInstance::kYes_ForceSwap;
   }
 
   // Experimental mode to swap BrowsingInstances on most cross-site navigations
@@ -1484,33 +1484,37 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
                                          ? current_entry->IsViewSourceMode()
                                          : dest_is_view_source_mode;
 
-  ShouldSwapBrowsingInstance force_swap_result =
+  ShouldSwapBrowsingInstance should_swap_result =
       ShouldSwapBrowsingInstancesForNavigation(
           current_effective_url, current_is_view_source_mode, source_instance,
           static_cast<SiteInstanceImpl*>(current_instance), dest_instance,
           SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
           dest_is_view_source_mode, transition, is_failure, is_reload,
           cross_origin_opener_policy_mismatch, was_server_redirect);
-  bool force_swap = force_swap_result == ShouldSwapBrowsingInstance::kYes;
-  if (!force_swap) {
+  bool proactive_swap =
+      (should_swap_result == ShouldSwapBrowsingInstance::kYes_ProactiveSwap);
+  bool should_swap =
+      (should_swap_result == ShouldSwapBrowsingInstance::kYes_ForceSwap) ||
+      proactive_swap;
+  if (!should_swap) {
     render_frame_host_->set_browsing_instance_not_swapped_reason(
-        force_swap_result);
+        should_swap_result);
   }
   SiteInstanceDescriptor new_instance_descriptor = DetermineSiteInstanceForURL(
       dest_url, source_instance, current_instance, dest_instance, transition,
-      is_failure, dest_is_restore, dest_is_view_source_mode, force_swap,
+      is_failure, dest_is_restore, dest_is_view_source_mode, should_swap,
       was_server_redirect);
   DCHECK(new_instance_descriptor.relation != SiteInstanceRelation::UNRELATED ||
-         force_swap);
+         should_swap);
 
   scoped_refptr<SiteInstance> new_instance =
       ConvertToSiteInstance(new_instance_descriptor, candidate_instance);
 
-  // If |force_swap| is true, we must use a different SiteInstance than the
+  // If |should_swap| is true, we must use a different SiteInstance than the
   // current one. If we didn't, we would have two RenderFrameHosts in the same
   // SiteInstance and the same frame, breaking lookup of RenderFrameHosts by
   // SiteInstance.
-  if (force_swap)
+  if (should_swap)
     CHECK_NE(new_instance, current_instance);
 
   if (new_instance == current_instance) {
@@ -1545,6 +1549,14 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
     }
   }
 
+  // If we're doing a proactive BI swap, we should try to reuse the current
+  // SiteInstance's process for the new SiteInstance if possible.
+  if (IsProactivelySwapBrowsingInstanceWithProcessReuseEnabled() &&
+      proactive_swap && !current_instance->RequiresDedicatedProcess()) {
+    DCHECK(frame_tree_node_->IsMainFrame());
+    new_instance_impl->ReuseCurrentProcessIfPossible(
+        current_instance->GetProcess());
+  }
   return new_instance;
 }
 

content/browser/frame_host/should_swap_browsing_instance.h

@@ -12,7 +12,7 @@ namespace content {
 // This enum is used for histograms and should not be renumbered.
 // TODO(crbug.com/1026101): Remove after the investigations are complete.
 enum class ShouldSwapBrowsingInstance {
-  kYes = 0,
+  kYes_ForceSwap = 0,
   kNo_ProactiveSwapDisabled = 1,
   kNo_NotMainFrame = 2,
   kNo_HasRelatedActiveContents = 3,
@@ -24,8 +24,9 @@ enum class ShouldSwapBrowsingInstance {
   kNo_AlreadyHasMatchingBrowsingInstance = 9,
   kNo_RendererDebugURL = 10,
   kNo_NotNeededForBackForwardCache = 11,
+  kYes_ProactiveSwap = 12,
 
-  kMaxValue = kNo_NotNeededForBackForwardCache
+  kMaxValue = kYes_ProactiveSwap
 };
 
 }  // namespace content

content/browser/site_instance_impl.cc

@@ -293,33 +293,63 @@ RenderProcessHost* SiteInstanceImpl::GetProcess() {
       process_reuse_policy_ = ProcessReusePolicy::DEFAULT;
     }
 
-    process_ = RenderProcessHostImpl::GetProcessHostForSiteInstance(this);
-
-    CHECK(process_);
-    process_->AddObserver(this);
-
-    MaybeSetBrowsingInstanceDefaultProcess();
-
-    // If we are using process-per-site, we need to register this process
-    // for the current site so that we can find it again.  (If no site is set
-    // at this time, we will register it in SetSite().)
-    if (process_reuse_policy_ == ProcessReusePolicy::PROCESS_PER_SITE &&
-        has_site_) {
-      RenderProcessHostImpl::RegisterSoleProcessHostForSite(process_, this);
-    }
-
-    TRACE_EVENT2("navigation", "SiteInstanceImpl::GetProcess",
-                 "site id", id_, "process id", process_->GetID());
-    GetContentClient()->browser()->SiteInstanceGotProcess(this);
-
-    if (has_site_)
-      LockToOriginIfNeeded();
+    SetProcessInternal(
+        RenderProcessHostImpl::GetProcessHostForSiteInstance(this));
   }
   DCHECK(process_);
 
   return process_;
 }
 
+void SiteInstanceImpl::ReuseCurrentProcessIfPossible(
+    RenderProcessHost* current_process) {
+  if (IsGuest() || HasProcess() || RequiresDedicatedProcess())
+    return;
+  // We should not reuse the current process if the destination uses
+  // process-per-site. Note that this includes the case where the process for
+  // the site is not there yet (so we're going to create a new process).
+  // Note also that this does not apply for the reverse case: if the current
+  // process is used for a process-per-site site, it is ok to reuse this for the
+  // new page (regardless of the site).
+  if (HasSite() && RenderProcessHost::ShouldUseProcessPerSite(
+                       browsing_instance_->GetBrowserContext(), GetSiteURL()))
+    return;
+  if (!current_process->MayReuseHost() ||
+      !RenderProcessHostImpl::IsSuitableHost(
+          current_process, GetIsolationContext(), GetSiteURL(), lock_url(),
+          IsGuest()))
+    return;
+  SetProcessInternal(current_process);
+}
+
+void SiteInstanceImpl::SetProcessInternal(RenderProcessHost* process) {
+  //  It is never safe to change |process_| without going through
+  //  RenderProcessHostDestroyed first to set it to null. Otherwise, same-site
+  //  frames will end up in different processes and everything will get
+  //  confused.
+  CHECK(!process_);
+  CHECK(process);
+  process_ = process;
+  process_->AddObserver(this);
+
+  MaybeSetBrowsingInstanceDefaultProcess();
+
+  // If we are using process-per-site, we need to register this process
+  // for the current site so that we can find it again.  (If no site is set
+  // at this time, we will register it in SetSite().)
+  if (process_reuse_policy_ == ProcessReusePolicy::PROCESS_PER_SITE &&
+      has_site_) {
+    RenderProcessHostImpl::RegisterSoleProcessHostForSite(process_, this);
+  }
+
+  TRACE_EVENT2("navigation", "SiteInstanceImpl::SetProcessInternal", "site id",
+               id_, "process id", process_->GetID());
+  GetContentClient()->browser()->SiteInstanceGotProcess(this);
+
+  if (has_site_)
+    LockToOriginIfNeeded();
+}
+
 bool SiteInstanceImpl::CanAssociateWithSpareProcess() {
   return can_associate_with_spare_process_;
 }

content/browser/site_instance_impl.h

@@ -140,6 +140,10 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
     return process_reuse_policy_;
   }
 
+  // Checks if |current_process| can be reused for this SiteInstance, and
+  // sets |process_| to |current_process| if so.
+  void ReuseCurrentProcessIfPossible(RenderProcessHost* current_process);
+
   // Whether the SiteInstance is created for a service worker. If this flag
   // is true, when a new process is created for this SiteInstance or a randomly
   // chosen existing process is reused because of the process limit, the process
@@ -377,6 +381,12 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   // lock directly without any "url to site URL" transformation.
   void SetSiteAndLockInternal(const GURL& site_url, const GURL& lock_url);
 
+  // Helper method to set the process of this SiteInstance, only in cases
+  // where it is safe. It is not generally safe to change the process of a
+  // SiteInstance, unless the RenderProcessHost itself is entirely destroyed and
+  // a new one later replaces it.
+  void SetProcessInternal(RenderProcessHost* process);
+
   // Returns the site for the given URL, which includes only the scheme and
   // registered domain.  Returns an empty GURL if the URL has no host.
   // |should_use_effective_urls| specifies whether to resolve |url| to an

content/common/content_navigation_policy.cc

@@ -39,9 +39,36 @@ bool IsBackForwardCacheEnabled() {
   return base::FeatureList::IsEnabled(features::kBackForwardCache);
 }
 
+const char kProactivelySwapBrowsingInstanceLevelParameterName[] = "level";
+
+constexpr base::FeatureParam<ProactivelySwapBrowsingInstanceLevel>::Option
+    proactively_swap_browsing_instance_levels[] = {
+        {ProactivelySwapBrowsingInstanceLevel::kDisabled, "Disabled"},
+        {ProactivelySwapBrowsingInstanceLevel::kCrossSiteSwapProcess,
+         "CrossSiteSwapProcess"},
+        {ProactivelySwapBrowsingInstanceLevel::kCrossSiteReuseProcess,
+         "CrossSiteReuseProcess"}};
+const base::FeatureParam<ProactivelySwapBrowsingInstanceLevel>
+    proactively_swap_browsing_instance_level{
+        &features::kProactivelySwapBrowsingInstance,
+        kProactivelySwapBrowsingInstanceLevelParameterName,
+        ProactivelySwapBrowsingInstanceLevel::kDisabled,
+        &proactively_swap_browsing_instance_levels};
+
+ProactivelySwapBrowsingInstanceLevel GetProactivelySwapBrowsingInstanceLevel() {
+  if (base::FeatureList::IsEnabled(features::kProactivelySwapBrowsingInstance))
+    return proactively_swap_browsing_instance_level.Get();
+  return ProactivelySwapBrowsingInstanceLevel::kDisabled;
+}
+
 bool IsProactivelySwapBrowsingInstanceEnabled() {
-  return base::FeatureList::IsEnabled(
-      features::kProactivelySwapBrowsingInstance);
+  return GetProactivelySwapBrowsingInstanceLevel() >=
+         ProactivelySwapBrowsingInstanceLevel::kCrossSiteSwapProcess;
+}
+
+bool IsProactivelySwapBrowsingInstanceWithProcessReuseEnabled() {
+  return GetProactivelySwapBrowsingInstanceLevel() >=
+         ProactivelySwapBrowsingInstanceLevel::kCrossSiteReuseProcess;
 }
 
 bool IsRenderDocumentEnabled() {

content/common/content_navigation_policy.h

@@ -11,7 +11,25 @@ namespace content {
 
 CONTENT_EXPORT bool IsBackForwardCacheEnabled();
 CONTENT_EXPORT bool DeviceHasEnoughMemoryForBackForwardCache();
+
+// Levels of ProactivelySwapBrowsingInstance support.
+// These are additive; features enabled at lower levels remain enabled at all
+// higher levels.
+enum class ProactivelySwapBrowsingInstanceLevel {
+  kDisabled = 0,
+  // Swap BrowsingInstance and renderer process on cross-site navigations.
+  kCrossSiteSwapProcess = 1,
+  // Swap BrowsingInstance on cross-site navigations, but try to reuse the
+  // current renderer process if possible.
+  kCrossSiteReuseProcess = 2,
+  // TODO(rakina): Add another level for BrowsingInstance swap on same-site
+  // navigations with process reuse.
+};
 CONTENT_EXPORT bool IsProactivelySwapBrowsingInstanceEnabled();
+CONTENT_EXPORT bool IsProactivelySwapBrowsingInstanceWithProcessReuseEnabled();
+CONTENT_EXPORT extern const char
+    kProactivelySwapBrowsingInstanceLevelParameterName[];
+
 CONTENT_EXPORT bool IsRenderDocumentEnabled();
 
 }  // namespace content