Allow process reuse on same-site nav only for BFCache

Previously we wanted to do both cross-site process reuse and same-site
process reuse for BFCache, but now we want to enable process-reuse on
same-site navigations only for BFCache experiment (at least for the
initial experiment).

This CL adds a case that handles BFCache process reuse separately from
ProactivelySwapBrowsingInstance, to ensure we will only do process-reuse
on same-site navigations if BFCache is enabled and
ProactivelySwapBrowsingInstance is not explicitly enabled (which is what
we will do on the experiment).

Also consolidates all three process-reuse cases into a more readable
flow.

Bug: 1122974
Change-Id: Iec6eb683293b9b384c74da238f3161ab149c0845
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2381223
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802898}

content/browser/back_forward_cache_browsertest.cc

@@ -7344,4 +7344,93 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   }
 }
 
+// We should try to reuse process on same-site renderer-initiated navigations.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       RendererInitiatedSameSiteNavigationReusesProcess) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_1(embedded_test_server()->GetURL("/title1.html"));
+  GURL url_2(embedded_test_server()->GetURL("/title2.html"));
+
+  // Navigate to title1.html.
+  EXPECT_TRUE(NavigateToURL(shell(), url_1));
+  scoped_refptr<SiteInstanceImpl> site_instance_1 =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+  // Navigate to title2.html. The navigation is document/renderer initiated.
+  EXPECT_TRUE(NavigateToURLFromRenderer(shell(), url_2));
+  scoped_refptr<SiteInstanceImpl> site_instance_2 =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+
+  // Check that title1.html and title2.html are in different BrowsingInstances
+  // but have the same renderer process.
+  EXPECT_FALSE(site_instance_1->IsRelatedSiteInstance(site_instance_2.get()));
+  EXPECT_EQ(site_instance_1->GetProcess(), site_instance_2->GetProcess());
+}
+
+// We should try to reuse process on same-site browser-initiated navigations.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       BrowserInitiatedSameSiteNavigationReusesProcess) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_1(embedded_test_server()->GetURL("/title1.html"));
+  GURL url_2(embedded_test_server()->GetURL("/title2.html"));
+
+  // 1) Navigate to title1.html.
+  EXPECT_TRUE(NavigateToURL(shell(), url_1));
+  scoped_refptr<SiteInstanceImpl> site_instance_1 =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+  // 2) Navigate to title2.html. The navigation is browser initiated.
+  EXPECT_TRUE(NavigateToURL(shell(), url_2));
+  scoped_refptr<SiteInstanceImpl> site_instance_2 =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+
+  // Check that title1.html and title2.html are in different BrowsingInstances
+  // but have the same renderer process.
+  EXPECT_FALSE(site_instance_1->IsRelatedSiteInstance(site_instance_2.get()));
+  EXPECT_EQ(site_instance_1->GetProcess(), site_instance_2->GetProcess());
+
+  // 3) Do a back navigation to title1.html.
+  web_contents()->GetController().GoBack();
+  EXPECT_TRUE(WaitForLoadStop(web_contents()));
+  EXPECT_EQ(web_contents()->GetLastCommittedURL(), url_1);
+  scoped_refptr<SiteInstanceImpl> site_instance_1_history_nav =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+
+  // We will reuse the SiteInstance and renderer process of |site_instance_1|.
+  EXPECT_EQ(site_instance_1_history_nav, site_instance_1);
+  EXPECT_EQ(site_instance_1_history_nav->GetProcess(),
+            site_instance_1->GetProcess());
+}
+
+// We should not try to reuse process on cross-site navigations.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       CrossSiteNavigationDoesNotReuseProcess) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL a1_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  GURL b_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  GURL a2_url(embedded_test_server()->GetURL("a.com", "/title2.html"));
+
+  // Navigate to A1.
+  EXPECT_TRUE(NavigateToURL(shell(), a1_url));
+  scoped_refptr<SiteInstanceImpl> a1_site_instance =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+  // Navigate to B. The navigation is browser initiated.
+  EXPECT_TRUE(NavigateToURL(shell(), b_url));
+  scoped_refptr<SiteInstanceImpl> b_site_instance =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+
+  // Check that A1 and B are in different BrowsingInstances and renderer
+  // processes.
+  EXPECT_FALSE(a1_site_instance->IsRelatedSiteInstance(b_site_instance.get()));
+  EXPECT_NE(a1_site_instance->GetProcess(), b_site_instance->GetProcess());
+
+  // Navigate to A2. The navigation is renderer-initiated.
+  EXPECT_TRUE(NavigateToURLFromRenderer(shell(), a2_url));
+  scoped_refptr<SiteInstanceImpl> a2_site_instance =
+      web_contents()->GetMainFrame()->GetSiteInstance();
+
+  // Check that B and A2 are in different BrowsingInstances and renderer
+  // processes.
+  EXPECT_FALSE(b_site_instance->IsRelatedSiteInstance(a2_site_instance.get()));
+  EXPECT_NE(b_site_instance->GetProcess(), a2_site_instance->GetProcess());
+}
+
 }  // namespace content

content/browser/frame_host/render_frame_host_manager.cc

@@ -1604,27 +1604,44 @@ 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.
-  // It might not be possible to reuse the process in some cases, including when
-  // the current SiteInstance needs a dedicated process (unless this is a
-  // same-site navigation).
   *did_same_site_proactive_browsing_instance_swap =
       (should_swap_result ==
        ShouldSwapBrowsingInstance::kYes_SameSiteProactiveSwap);
+  bool reuse_current_process_if_possible = false;
+  // With proactive BrowsingInstance swap, we should try to reuse the current
+  // SiteInstance's process. This avoids swapping processes too many times,
+  // which might cause performance regressions.
+  // Note: process reuse might not be possible in some cases, e.g. for
+  // cross-site navigations when the current SiteInstance needs a dedicated
+  // process.
+
+  // Process-reuse cases include:
+  // 1) When ProactivelySwapBrowsingInstance with process-reuse is explicitly
+  // enabled. In this case, we will try to reuse process on both cross-site and
+  // same-site navigations.
   if (IsProactivelySwapBrowsingInstanceWithProcessReuseEnabled() &&
       proactive_swap &&
       (!current_instance->RequiresDedicatedProcess() ||
        *did_same_site_proactive_browsing_instance_swap)) {
-    DCHECK(frame_tree_node_->IsMainFrame());
-    new_instance_impl->ReuseCurrentProcessIfPossible(
-        current_instance->GetProcess());
+    reuse_current_process_if_possible = true;
+  }
+
+  // 2) When BackForwardCache is enabled on same-site navigations.
+  // Note 1: When BackForwardCache is disabled, we typically reuse processes on
+  // same-site navigations. This follows that behavior.
+  // Note 2: This doesn't cover cross-site navigations. Cross-site process-reuse
+  // is being experimented independently and is covered in path #1 above.
+  // See crbug.com/1122974 for further details.
+  if (IsSameSiteBackForwardCacheEnabled() &&
+      *did_same_site_proactive_browsing_instance_swap) {
+    reuse_current_process_if_possible = true;
   }
 
-  // If this is a same-site history navigation with different BrowsingInstances,
-  // the original navigation might have done a proactive BrowsingInstance swap,
-  // which means we should try to reuse the current process (because we did too
-  // on the original navigation).
+  // 3) When we're doing a same-site history navigation with different
+  // BrowsingInstances. We typically do not swap BrowsingInstances on same-site
+  // navigations. This might indicate that the original navigation did a
+  // proactive BrowsingInstance swap (and process-reuse) before, so we should
+  // try to reuse the current process.
   bool is_history_navigation = !!dest_instance;
   bool swapped_browsing_instance =
       !new_instance->IsRelatedSiteInstance(current_instance);
@@ -1636,10 +1653,15 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
       IsCurrentlySameSite(
           static_cast<RenderFrameHostImpl*>(render_frame_host_.get()),
           dest_url)) {
+    reuse_current_process_if_possible = true;
+  }
+
+  if (reuse_current_process_if_possible) {
     DCHECK(frame_tree_node_->IsMainFrame());
     new_instance_impl->ReuseCurrentProcessIfPossible(
         current_instance->GetProcess());
   }
+
   return new_instance;
 }