[bfcache] Don't immediately flush pages with related SiteInstances.

Rather than synchronously deleting all pages in the BackForwardCache
when there is a conflicting BrowsingInstance, call
EvictFramesInBrowsingInstance which asynchronously deletes only the
conflicting pages.

Calling evict is safer than immediately deleting frames, which can lead
to UAF bugs.

Change-Id: I4e453c3c1dacc5ed764cc344efbc26f24d7fe115
Bug: 993337
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1844964
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707310}

content/browser/back_forward_cache_browsertest.cc

@@ -371,6 +371,30 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
                 FROM_HERE);
 }
 
+// The BackForwardCache does not cache same-website navigations for now.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       DoesNotCacheSameWebsiteNavigations) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_a1(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  GURL url_a2(embedded_test_server()->GetURL("a.com", "/title2.html"));
+
+  // 1) Navigate to A1.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a1));
+  RenderFrameHostImpl* rfh_a1 = current_frame_host();
+  RenderFrameDeletedObserver delete_rfh_a1(rfh_a1);
+  int browsing_instance_id = rfh_a1->GetSiteInstance()->GetBrowsingInstanceId();
+
+  // 2) Navigate to A2.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a2));
+  RenderFrameHostImpl* rfh_a2 = current_frame_host();
+  // The BrowsingInstance shouldn't have changed.
+  EXPECT_EQ(browsing_instance_id,
+            rfh_a2->GetSiteInstance()->GetBrowsingInstanceId());
+  EXPECT_FALSE(rfh_a1->is_in_back_forward_cache());
+  // The main frame should have been reused for the navigation.
+  EXPECT_EQ(rfh_a1, rfh_a2);
+}
+
 // The current page can't enter the BackForwardCache if another page can script
 // it. This can happen when one document opens a popup using window.open() for
 // instance. It prevents the BackForwardCache from being used.
@@ -2360,12 +2384,31 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, CachePagesWithBeacon) {
 }
 
 // Regression test for https://crbug.com/993337.
+//
+// A note about sharing BrowsingInstances and the BackForwardCache:
+//
+// We should never keep around more than one main frame that belongs to the same
+// BrowsingInstance. When swapping two pages, when one is stored in the
+// back-forward cache or one is restored from it, the current code expects the
+// two to live in different BrowsingInstances.
+//
+// History navigation can recreate a page with the same BrowsingInstance as the
+// one stored in the back-forward cache. This case must to be handled. When it
+// happens, the back-forward cache page is evicted.
+//
+// Since cache eviction is asynchronous, it's is possible for two main frames
+// belonging to the same BrowsingInstance to be alive for a brief period of time
+// (the new page being navigated to, and a page in the cache, until it is
+// destroyed asynchronously via eviction).
+//
+// The test below tests that the brief period of time where two main frames are
+// alive in the same BrowsingInstance does not cause anything to blow up.
 IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
                        NavigateToTwoPagesOnSameSite) {
   ASSERT_TRUE(embedded_test_server()->Start());
   GURL url_a1(embedded_test_server()->GetURL("a.com", "/title1.html"));
   GURL url_a2(embedded_test_server()->GetURL("a.com", "/title2.html"));
-  GURL url_b1(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  GURL url_b3(embedded_test_server()->GetURL("b.com", "/title1.html"));
 
   // 1) Navigate to A1.
   EXPECT_TRUE(NavigateToURL(shell(), url_a1));
@@ -2375,21 +2418,159 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   RenderFrameHostImpl* rfh_a2 = current_frame_host();
   RenderFrameDeletedObserver delete_rfh_a2(current_frame_host());
 
-  // 3) Navigate to B1.
-  EXPECT_TRUE(NavigateToURL(shell(), url_b1));
+  // 3) Navigate to B3.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b3));
   EXPECT_TRUE(rfh_a2->is_in_back_forward_cache());
-  RenderFrameHostImpl* rfh_b1 = current_frame_host();
+  RenderFrameHostImpl* rfh_b3 = current_frame_host();
 
   // 4) Do a history navigation back to A1.
   web_contents()->GetController().GoToIndex(0);
   EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
-  EXPECT_TRUE(rfh_b1->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_b3->is_in_back_forward_cache());
+
+  // Note that the frame for A1 gets created before A2 is deleted from the
+  // cache, so there will be a brief period where two the main frames (A1 and
+  // A2) are alive in the same BrowsingInstance/SiteInstance, at the same time.
+  // That is the scenario this test is covering. This used to cause a CHECK,
+  // because the two main frames shared a single RenderViewHost (no longer the
+  // case after https://crrev.com/c/1833616).
+
+  // A2 should be evicted from the cache and asynchronously deleted, due to the
+  // cache size limit (B3 took its place in the cache).
+  delete_rfh_a2.WaitUntilDeleted();
+}
+
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       NavigateToTwoPagesOnSameSiteWithSubframes) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  // This test covers the same scenario as NavigateToTwoPagesOnSameSite, except
+  // the pages contain subframes:
+  // A1(B) -> A2(B(C)) -> D3 -> A1(B)
+  //
+  // The subframes shouldn't make a difference, so the expected behavior is the
+  // same as NavigateToTwoPagesOnSameSite.
+  GURL url_a1(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b)"));
+  GURL url_a2(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b(c))"));
+  GURL url_d3(embedded_test_server()->GetURL("d.com", "/title1.html"));
+
+  // 1) Navigate to A1(B).
+  EXPECT_TRUE(NavigateToURL(shell(), url_a1));
+
+  // 2) Navigate to A2(B(C)).
+  EXPECT_TRUE(NavigateToURL(shell(), url_a2));
+  RenderFrameHostImpl* rfh_a2 = current_frame_host();
+  RenderFrameDeletedObserver delete_rfh_a2(current_frame_host());
+
+  // 3) Navigate to D3.
+  EXPECT_TRUE(NavigateToURL(shell(), url_d3));
+  EXPECT_TRUE(rfh_a2->is_in_back_forward_cache());
+  RenderFrameHostImpl* rfh_d3 = current_frame_host();
+
+  // 4) Do a history navigation back to A1(B).
+  web_contents()->GetController().GoToIndex(0);
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
 
-  // As a result, rfh_a2 is deleted. The history navigation tried to restore an
-  // entry using the same BrowsingInstance. They both can't live together.
+  // D3 takes A2(B(C))'s place in the cache.
+  EXPECT_TRUE(rfh_d3->is_in_back_forward_cache());
   delete_rfh_a2.WaitUntilDeleted();
 }
 
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       ConflictingBrowsingInstances) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_a1(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  GURL url_a2(embedded_test_server()->GetURL("a.com", "/title2.html"));
+  GURL url_b3(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+  // 1) Navigate to A1.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a1));
+
+  // 2) Navigate to A2.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a2));
+  RenderFrameHostImpl* rfh_a2 = current_frame_host();
+  RenderFrameDeletedObserver delete_rfh_a2(current_frame_host());
+
+  // 3) Navigate to B3.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b3));
+  EXPECT_TRUE(rfh_a2->is_in_back_forward_cache());
+  RenderFrameHostImpl* rfh_b3 = current_frame_host();
+  // Make B3 ineligible for caching, so that navigating doesn't evict A2
+  // due to the cache size limit.
+  content::BackForwardCache::DisableForRenderFrameHost(
+      rfh_b3, "BackForwardCacheBrowserTest");
+
+  // 4) Do a history navigation back to A1.  At this point, A1 is going to have
+  // the same BrowsingInstance as A2. This should cause A2 to get
+  // evicted from the BackForwardCache due to its conflicting BrowsingInstance.
+  web_contents()->GetController().GoToIndex(0);
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_EQ(current_frame_host()->GetLastCommittedURL(), url_a1);
+  delete_rfh_a2.WaitUntilDeleted();
+}
+
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       CanCacheMultiplesPagesOnSameDomain) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_a1(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  GURL url_b2(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  GURL url_a3(embedded_test_server()->GetURL("a.com", "/title2.html"));
+  GURL url_b4(embedded_test_server()->GetURL("b.com", "/title2.html"));
+
+  // Increase the cache size so we're able to store multiple pages for the same
+  // site in the cache at once.
+  web_contents()
+      ->GetController()
+      .GetBackForwardCache()
+      .set_cache_size_limit_for_testing(3);
+
+  // 1) Navigate to A1.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a1));
+  RenderFrameHostImpl* rfh_a1 = current_frame_host();
+
+  // 2) Navigate to B2.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b2));
+  RenderFrameHostImpl* rfh_b2 = current_frame_host();
+  EXPECT_TRUE(rfh_a1->is_in_back_forward_cache());
+
+  // 3) Navigate to A3.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a3));
+  RenderFrameHostImpl* rfh_a3 = current_frame_host();
+  EXPECT_TRUE(rfh_a1->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_b2->is_in_back_forward_cache());
+  // A1 and A3 shouldn't be treated as the same site instance.
+  EXPECT_NE(rfh_a1->GetSiteInstance(), rfh_a3->GetSiteInstance());
+
+  // 4) Navigate to B4.
+  // Make sure we can store A1 and A3 in the cache at the same time.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b4));
+  RenderFrameHostImpl* rfh_b4 = current_frame_host();
+  EXPECT_TRUE(rfh_a1->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_b2->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_a3->is_in_back_forward_cache());
+
+  // 5) Go back to A3.
+  // Make sure we can restore A3, while A1 remains in the cache.
+  web_contents()->GetController().GoBack();
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_TRUE(rfh_a1->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_b2->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_b4->is_in_back_forward_cache());
+  EXPECT_EQ(rfh_a3, current_frame_host());
+  // B2 and B4 shouldn't be treated as the same site instance.
+  EXPECT_NE(rfh_b2->GetSiteInstance(), rfh_b4->GetSiteInstance());
+
+  // 6) Do a history navigation back to A1.
+  // Make sure we can restore A1, while coming from A3.
+  web_contents()->GetController().GoToIndex(0);
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_TRUE(rfh_b2->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_b4->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_a3->is_in_back_forward_cache());
+  EXPECT_EQ(rfh_a1, current_frame_host());
+}
+
 class GeolocationBackForwardCacheBrowserTest
     : public BackForwardCacheBrowserTest {
  protected:

content/browser/frame_host/back_forward_cache_impl.cc

@@ -222,6 +222,8 @@ std::string BackForwardCacheImpl::CanStoreDocumentResult::ToString() {
       return "No: granted media stream access";
     case Reason::kSchedulerTrackedFeatureUsed:
       return "No: scheduler tracked feature is used";
+    case Reason::kConflictingBrowsingInstance:
+      return "No: conflicting BrowsingInstance";
   }
 }
 
@@ -464,6 +466,17 @@ void BackForwardCacheImpl::Flush() {
   entries_.clear();
 }
 
+void BackForwardCacheImpl::EvictFramesInRelatedSiteInstances(
+    SiteInstance* site_instance) {
+  for (std::unique_ptr<Entry>& entry : entries_) {
+    if (entry->render_frame_host->GetSiteInstance()->IsRelatedSiteInstance(
+            site_instance))
+      entry->render_frame_host->EvictFromBackForwardCacheWithReason(
+          BackForwardCacheMetrics::NotRestoredReason::
+              kConflictingBrowsingInstance);
+  }
+}
+
 void BackForwardCacheImpl::PostTaskToDestroyEvictedFrames() {
   base::PostTask(FROM_HERE, {BrowserThread::UI},
                  base::BindOnce(&BackForwardCacheImpl::DestroyEvictedFrames,

content/browser/frame_host/back_forward_cache_impl.h

@@ -27,6 +27,7 @@ namespace content {
 class RenderFrameHostImpl;
 class RenderFrameProxyHost;
 class RenderViewHostImpl;
+class SiteInstance;
 
 // BackForwardCache:
 //
@@ -131,6 +132,10 @@ class CONTENT_EXPORT BackForwardCacheImpl : public BackForwardCache {
   // Remove all entries from the BackForwardCache.
   void Flush();
 
+  // Evict all cached pages in the same BrowsingInstance as
+  // |site_instance|.
+  void EvictFramesInRelatedSiteInstances(SiteInstance* site_instance);
+
   // Posts a task to destroy all frames in the BackForwardCache that have been
   // marked as evicted.
   void PostTaskToDestroyEvictedFrames();

content/browser/frame_host/back_forward_cache_metrics.h

@@ -58,7 +58,8 @@ class BackForwardCacheMetrics
     kDialog = 17,
     kGrantedMediaStreamAccess = 18,
     kSchedulerTrackedFeatureUsed = 19,
-    kMaxValue = kSchedulerTrackedFeatureUsed,
+    kConflictingBrowsingInstance = 20,
+    kMaxValue = kConflictingBrowsingInstance,
   };
 
   // Please keep in sync with BackForwardCacheHistoryNavigationOutcome in

content/browser/frame_host/navigation_controller_impl.cc

@@ -2558,18 +2558,30 @@ void NavigationControllerImpl::NavigateToExistingPendingEntry(
     return;
   }
 
-  // By design, a page in the BackForwardCache is alone in its BrowsingInstance.
-  // History navigation might try to reuse a specific SiteInstance, already used
-  // by a page in the cache. This must not happen. It would fail creating the
-  // RenderFrame, because only one main document can live there. For this
-  // reason, the BackForwardCache is flushed.
-  // TODO(arthursonzogni): Flushing the entire cache is a bit overkill, this can
-  // be refined to only delete the page (if any) using the same
-  // BrowsingInstance.
+  // History navigation might try to reuse a specific BrowsingInstance, already
+  // used by a page in the cache. To avoid having two different main frames that
+  // live in the same BrowsingInstance, evict the all pages with this
+  // BrowsingInstance from the cache.
+  //
+  // For example, take the following scenario:
+  //
+  // A1 = Some page on a.com
+  // A2 = Some other page on a.com
+  // B3 = An uncacheable page on b.com
+  //
+  // Then the following navigations occur:
+  // A1->A2->B3->A1
+  // On the navigation from B3 to A1, A2 will remain in the cache (B3 doesn't
+  // take its place) and A1 will be created in the same BrowsingInstance (and
+  // SiteInstance), as A2.
+  //
+  // If we didn't do anything, both A1 and A2 would remain alive in the same
+  // BrowsingInstance/SiteInstance, which is unsupported by
+  // RenderFrameHostManager::CommitPending(). To avoid this conundrum, we evict
+  // A2 from the cache.
   if (pending_entry_->site_instance()) {
-    SiteInstance* current = root->current_frame_host()->GetSiteInstance();
-    if (!current->IsRelatedSiteInstance(pending_entry_->site_instance()))
-      back_forward_cache_.Flush();
+    back_forward_cache_.EvictFramesInRelatedSiteInstances(
+        pending_entry_->site_instance());
   }
 
   // If we were navigating to a slow-to-commit page, and the user performs

tools/metrics/histograms/enums.xml

@@ -4766,6 +4766,7 @@ Unknown properties are collapsed to zero. -->
   <int value="17" label="Dialog"/>
   <int value="18" label="Granted media stream access"/>
   <int value="19" label="Scheduler tracked feature used"/>
+  <int value="20" label="Conflicting BrowsingInstance"/>
 </enum>
 
 <enum name="BackForwardNavigationType">