[bfcache] Don't reuse RenderViewHosts belonging to cached pages.

After a page enters the BackForwardCache, a user may navigate to a new
page on the same site as the cached page (See
BackForwardCacheBrowserTest.NavigateToTwoPagesOnSameSite).

In the above case, it's important not to reuse the RenderViewHost(s)
owned by the frames in the BackForwardCache, because this could lead to
two main frames (the one in the cache, and the one the user just
navigated to), existing at the same time for a single RenderViewHost
(not allowed).

To solve this, we can simply stop reusing RenderViewHosts that belong
to a page when it enters the BackForwardCache, and begin reusing them
again when it leaves the BackForwardCache.

This approach also allows us to remove a work-around in
navigation_controller_impl.cc that was aggressively deleting cached
frames.

Note: RenderViewHost is owned by RenderFrameHost, this CL changes
nothing WRT object lifetimes.

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

content/browser/back_forward_cache_browsertest.cc

@@ -88,6 +88,10 @@ class BackForwardCacheBrowserTest : public ContentBrowserTest {
     return web_contents()->GetFrameTree()->root()->render_manager();
   }
 
+  std::string DepictFrameTree(FrameTreeNode* node) {
+    return visualizer_.DepictFrameTree(node);
+  }
+
   void ExpectOutcome(BackForwardCacheMetrics::HistoryNavigationOutcome outcome,
                      base::Location location) {
     base::HistogramBase::Sample sample = base::HistogramBase::Sample(outcome);
@@ -168,6 +172,7 @@ class BackForwardCacheBrowserTest : public ContentBrowserTest {
 
   base::test::ScopedFeatureList feature_list_;
 
+  FrameTreeVisualizer visualizer_;
   base::HistogramTester histogram_tester_;
   std::vector<base::Bucket> expected_outcomes_;
   std::vector<base::Bucket> expected_disabled_reasons_;
@@ -932,6 +937,7 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   EXPECT_EQ(2u, render_frame_host_manager()->GetProxyCount());
   RenderFrameHostImpl* rfh_a = current_frame_host();
   RenderFrameDeletedObserver delete_observer_rfh_a(rfh_a);
+  std::string frame_tree_a = DepictFrameTree(rfh_a->frame_tree_node());
 
   // 2. Navigate from a cacheable page to an uncacheable page (A->B).
   EXPECT_TRUE(NavigateToURL(shell(), url_b));
@@ -958,6 +964,10 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   delete_observer_rfh_b.WaitUntilDeleted();
   EXPECT_EQ(2u, render_frame_host_manager()->GetProxyCount());
 
+  // Page A should still have the correct frame tree.
+  EXPECT_EQ(frame_tree_a,
+            DepictFrameTree(current_frame_host()->frame_tree_node()));
+
   // 4. Navigate from a cacheable page to a cacheable page (A->C).
   EXPECT_TRUE(NavigateToURL(shell(), url_c));
   EXPECT_EQ(3u, render_frame_host_manager()->GetProxyCount());
@@ -977,6 +987,10 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
   EXPECT_EQ(2u, render_frame_host_manager()->GetProxyCount());
 
+  // Page A should still have the correct frame tree.
+  EXPECT_EQ(frame_tree_a,
+            DepictFrameTree(current_frame_host()->frame_tree_node()));
+
   // Page C should be in the cache.
   EXPECT_FALSE(delete_observer_rfh_c.deleted());
   EXPECT_TRUE(rfh_c->is_in_back_forward_cache());

content/browser/frame_host/back_forward_cache_impl.cc

@@ -147,10 +147,13 @@ BackForwardCacheTestDelegate* g_bfcache_disabled_test_observer = nullptr;
 
 }  // namespace
 
-BackForwardCacheImpl::Entry::Entry(std::unique_ptr<RenderFrameHostImpl> rfh,
-                                   RenderFrameProxyHostMap proxies)
-    : render_frame_host(std::move(rfh)), proxy_hosts(std::move(proxies)) {}
-
+BackForwardCacheImpl::Entry::Entry(
+    std::unique_ptr<RenderFrameHostImpl> rfh,
+    RenderFrameProxyHostMap proxies,
+    std::set<RenderViewHostImpl*> render_view_hosts)
+    : render_frame_host(std::move(rfh)),
+      proxy_hosts(std::move(proxies)),
+      render_view_hosts(std::move(render_view_hosts)) {}
 BackForwardCacheImpl::Entry::~Entry() {}
 
 std::string BackForwardCacheImpl::CanStoreDocumentResult::ToString() {

content/browser/frame_host/back_forward_cache_impl.h

@@ -26,6 +26,7 @@ namespace content {
 
 class RenderFrameHostImpl;
 class RenderFrameProxyHost;
+class RenderViewHostImpl;
 
 // BackForwardCache:
 //
@@ -41,7 +42,8 @@ class CONTENT_EXPORT BackForwardCacheImpl : public BackForwardCache {
                            std::unique_ptr<RenderFrameProxyHost>>;
 
     Entry(std::unique_ptr<RenderFrameHostImpl> rfh,
-          RenderFrameProxyHostMap proxy_hosts);
+          RenderFrameProxyHostMap proxy_hosts,
+          std::set<RenderViewHostImpl*> render_view_hosts);
     ~Entry();
 
     // The main document being stored.
@@ -53,6 +55,17 @@ class CONTENT_EXPORT BackForwardCacheImpl : public BackForwardCache {
     // cached.
     RenderFrameProxyHostMap proxy_hosts;
 
+    // RenderViewHosts belonging to the main frame, and its proxies (if any).
+    //
+    // While RenderViewHostImpl(s) are in the BackForwardCache, they aren't
+    // reused for pages outside the cache. This prevents us from having two main
+    // frames, (one in the cache, one live), associated with a single
+    // RenderViewHost.
+    //
+    // Keeping these here also prevents RenderFrameHostManager code from
+    // unwittingly iterating over RenderViewHostImpls that are in the cache.
+    std::set<RenderViewHostImpl*> render_view_hosts;
+
     DISALLOW_COPY_AND_ASSIGN(Entry);
   };
 

content/browser/frame_host/frame_tree.cc

@@ -14,6 +14,7 @@
 #include "base/callback.h"
 #include "base/lazy_instance.h"
 #include "base/memory/ptr_util.h"
+#include "base/stl_util.h"
 #include "base/unguessable_token.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
@@ -380,7 +381,7 @@ scoped_refptr<RenderViewHostImpl> FrameTree::CreateRenderViewHost(
       static_cast<RenderViewHostImpl*>(RenderViewHostFactory::Create(
           site_instance, render_view_delegate_, render_widget_delegate_,
           routing_id, main_frame_routing_id, widget_routing_id, swapped_out));
-  render_view_host_map_[site_instance->GetId()] = rvh;
+  RegisterRenderViewHost(rvh);
   return base::WrapRefCounted(rvh);
 }
 
@@ -393,7 +394,13 @@ scoped_refptr<RenderViewHostImpl> FrameTree::GetRenderViewHost(
   return base::WrapRefCounted(it->second);
 }
 
-void FrameTree::RenderViewHostDeleted(RenderViewHost* rvh) {
+void FrameTree::RegisterRenderViewHost(RenderViewHostImpl* rvh) {
+  CHECK(
+      !base::Contains(render_view_host_map_, rvh->GetSiteInstance()->GetId()));
+  render_view_host_map_[rvh->GetSiteInstance()->GetId()] = rvh;
+}
+
+void FrameTree::UnregisterRenderViewHost(RenderViewHostImpl* rvh) {
   auto it = render_view_host_map_.find(rvh->GetSiteInstance()->GetId());
   CHECK(it != render_view_host_map_.end());
   CHECK_EQ(it->second, rvh);

content/browser/frame_host/frame_tree.h

@@ -222,11 +222,24 @@ class CONTENT_EXPORT FrameTree {
   scoped_refptr<RenderViewHostImpl> GetRenderViewHost(
       SiteInstance* site_instance);
 
-  // The FrameTree maintains a list of existing RenderViewHostImpl so that
-  // FrameTree::CreateRenderViewHost() can return them directly instead of
-  // creating a new one. Calling this function removes it from the list when the
-  // |render_view_host| is deleted.
-  void RenderViewHostDeleted(RenderViewHost* render_view_host);
+  // Registers a RenderViewHost so that it can be reused by other frames
+  // belonging to the same SiteInstance.
+  //
+  // This method does not take ownership of|rvh|.
+  //
+  // NOTE: This method CHECK fails if a RenderViewHost is already registered for
+  // |rvh|'s SiteInstance.
+  //
+  // ALSO NOTE: After calling RegisterRenderViewHost, UnregisterRenderViewHost
+  // *must* be called for |rvh| when it is destroyed or put into the
+  // BackForwardCache, to prevent FrameTree::CreateRenderViewHost from trying to
+  // reuse it.
+  void RegisterRenderViewHost(RenderViewHostImpl* rvh);
+
+  // Unregisters the RenderViewHostImpl that's available for reuse for a
+  // particular SiteInstance. NOTE: This method CHECK fails if it is called for
+  // a |render_view_host| that is not currently set for reuse.
+  void UnregisterRenderViewHost(RenderViewHostImpl* render_view_host);
 
   // This is called when the frame is about to be removed and started to run
   // unload handlers.

content/browser/frame_host/render_frame_host_manager.cc

@@ -507,8 +507,12 @@ void RenderFrameHostManager::SwapOutOldFrame(
     TRACE_EVENT1("navigation", "BackForwardCache_MaybeStorePage", "can_store",
                  can_store.ToString());
     if (can_store) {
+      std::set<RenderViewHostImpl*> old_render_view_hosts;
+
       // Prepare the main frame.
       back_forward_cache.Freeze(old_render_frame_host.get());
+      old_render_view_hosts.insert(static_cast<RenderViewHostImpl*>(
+          old_render_frame_host->GetRenderViewHost()));
 
       // Prepare the proxies.
       RenderFrameProxyHostMap old_proxy_hosts;
@@ -517,8 +521,10 @@ void RenderFrameHostManager::SwapOutOldFrame(
         // This avoids including the proxy created when starting a
         // new cross-process, cross-BrowsingInstance navigation, as well as any
         // restored proxies which are also in a different BrowsingInstance.
-        if (instance->IsRelatedSiteInstance(it.second->GetSiteInstance()))
+        if (instance->IsRelatedSiteInstance(it.second->GetSiteInstance())) {
+          old_render_view_hosts.insert(it.second->GetRenderViewHost());
           old_proxy_hosts[it.first] = std::move(it.second);
+        }
       }
       // Remove the previously extracted proxies from the
       // RenderFrameHostManager, which also remove their respective
@@ -526,8 +532,13 @@ void RenderFrameHostManager::SwapOutOldFrame(
       for (auto& it : old_proxy_hosts)
         DeleteRenderFrameProxyHost(it.second->GetSiteInstance());
 
+      // Ensures RenderViewHosts are not reused while they are in the cache.
+      for (RenderViewHostImpl* rvh : old_render_view_hosts)
+        rvh->EnterBackForwardCache();
+
       auto entry = std::make_unique<BackForwardCacheImpl::Entry>(
-          std::move(old_render_frame_host), std::move(old_proxy_hosts));
+          std::move(old_render_frame_host), std::move(old_proxy_hosts),
+          std::move(old_render_view_hosts));
       back_forward_cache.StoreEntry(std::move(entry));
       return;
     }
@@ -2494,9 +2505,9 @@ void RenderFrameHostManager::CommitPending(
   // If a document is being restored from the BackForwardCache, restore all
   // cached state now.
   if (pending_bfcache_entry) {
-    RenderFrameProxyHostMap pending_proxy_hosts =
+    RenderFrameProxyHostMap proxy_hosts_to_restore =
         std::move(pending_bfcache_entry->proxy_hosts);
-    for (auto& proxy : pending_proxy_hosts) {
+    for (auto& proxy : proxy_hosts_to_restore) {
       // We only cache pages when swapping BrowsingInstance, so we should never
       // be reusing SiteInstances.
       CHECK(!base::Contains(proxy_hosts_,
@@ -2505,6 +2516,11 @@ void RenderFrameHostManager::CommitPending(
           ->AddObserver(this);
       proxy_hosts_.insert(std::move(proxy));
     }
+
+    std::set<RenderViewHostImpl*> render_view_hosts_to_restore =
+        std::move(pending_bfcache_entry->render_view_hosts);
+    for (RenderViewHostImpl* rvh : render_view_hosts_to_restore)
+      rvh->LeaveBackForwardCache();
   }
 
   // For top-level frames, the RenderWidget{Host} will not be destroyed when the

content/browser/renderer_host/render_view_host_impl.cc

@@ -224,10 +224,6 @@ RenderViewHostImpl::RenderViewHostImpl(
       is_swapped_out_(swapped_out),
       routing_id_(routing_id),
       main_frame_routing_id_(main_frame_routing_id),
-      is_waiting_for_close_ack_(false),
-      sudden_termination_allowed_(false),
-      updating_web_preferences_(false),
-      has_notified_about_creation_(false),
       visual_properties_manager_(this) {
   DCHECK(instance_.get());
   CHECK(delegate_);  // http://crbug.com/82827
@@ -287,8 +283,12 @@ RenderViewHostImpl::~RenderViewHostImpl() {
 
   // This can be called inside the FrameTree destructor. When the delegate is
   // the InterstialPageImpl, the |frame_tree| is set to null before deleting it.
-  if (FrameTree* frame_tree = GetDelegate()->GetFrameTree())
-    frame_tree->RenderViewHostDeleted(this);
+  if (FrameTree* frame_tree = GetDelegate()->GetFrameTree()) {
+    // If |this| is in the BackForwardCache, then it was already removed from
+    // the FrameTree at the time it entered the BackForwardCache.
+    if (!is_in_back_forward_cache_)
+      frame_tree->UnregisterRenderViewHost(this);
+  }
 }
 
 RenderViewHostDelegate* RenderViewHostImpl::GetDelegate() {
@@ -405,6 +405,20 @@ void RenderViewHostImpl::SetMainFrameRoutingId(int routing_id) {
   GetWidget()->UpdatePriority();
 }
 
+void RenderViewHostImpl::EnterBackForwardCache() {
+  FrameTree* frame_tree = GetDelegate()->GetFrameTree();
+  frame_tree->UnregisterRenderViewHost(this);
+  is_in_back_forward_cache_ = true;
+}
+
+void RenderViewHostImpl::LeaveBackForwardCache() {
+  FrameTree* frame_tree = GetDelegate()->GetFrameTree();
+  // At this point, the frames |this| RenderViewHostImpl belongs to are
+  // guaranteed to be committed, so it should be reused going forward.
+  frame_tree->RegisterRenderViewHost(this);
+  is_in_back_forward_cache_ = false;
+}
+
 bool RenderViewHostImpl::IsRenderViewLive() {
   return GetProcess()->IsInitializedAndNotDead() &&
          GetWidget()->renderer_initialized();

content/browser/renderer_host/render_view_host_impl.h

@@ -207,6 +207,15 @@ class CONTENT_EXPORT RenderViewHostImpl
   // view is not considered active.
   void SetMainFrameRoutingId(int routing_id);
 
+  // Called when the RenderFrameHostImpls/RenderFrameProxyHosts that own this
+  // RenderViewHost enter the BackForwardCache.
+  void EnterBackForwardCache();
+
+  // Called when the RenderFrameHostImpls/RenderFrameProxyHosts that own this
+  // RenderViewHost leave the BackForwardCache. This occurs immediately before a
+  // restored document is committed.
+  void LeaveBackForwardCache();
+
   // Called during frame eviction to return all SurfaceIds in the frame tree.
   // Marks all views in the frame tree as evicted.
   std::vector<viz::SurfaceId> CollectSurfaceIdsForEviction();
@@ -329,10 +338,10 @@ class CONTENT_EXPORT RenderViewHostImpl
   // Set to true when waiting for a ViewHostMsg_ClosePageACK.
   // TODO(creis): Move to RenderFrameHost and RenderWidgetHost.
   // See http://crbug.com/418265.
-  bool is_waiting_for_close_ack_;
+  bool is_waiting_for_close_ack_ = false;
 
   // True if the render view can be shut down suddenly.
-  bool sudden_termination_allowed_;
+  bool sudden_termination_allowed_ = false;
 
   // This is updated every time UpdateWebkitPreferences is called. That method
   // is in turn called when any of the settings change that the WebPreferences
@@ -348,14 +357,17 @@ class CONTENT_EXPORT RenderViewHostImpl
   // This monitors input changes so they can be reflected to the interaction MQ.
   std::unique_ptr<InputDeviceChangeObserver> input_device_change_observer_;
 
-  bool updating_web_preferences_;
+  bool updating_web_preferences_ = false;
 
   // This tracks whether this RenderViewHost has notified observers about its
   // creation with RenderViewCreated.  RenderViewHosts may transition from
   // active (with a RenderFrameHost for the main frame) to inactive state and
   // then back to active, and for the latter transition, this avoids firing
   // duplicate RenderViewCreated events.
-  bool has_notified_about_creation_;
+  bool has_notified_about_creation_ = false;
+
+  // BackForwardCache:
+  bool is_in_back_forward_cache_ = false;
 
   // Used to send Page and Widget visual properties to Renderers.
   VisualPropertiesManager visual_properties_manager_;