Fix ceftest failures with BackForwardCache enabled (see issue #2421)

When BackForwardCache is enabled the old RFH tree may be cached instead of being
immediately deleted as a result of main frame navigation. If a RFH is cached
then delivery of the CefFrameHandler::OnFrameDetached callback will be delayed
until the the RFH is ejected from the cache (possibly not occurring until the
browser is destroyed). This change in OnFrameDetached timing was causing
FrameHandlerTest.OrderSubCrossOrigin* to fail, and the inclusion of cached
frames in CefBrowserInfo::GetAllFrames was causing
FrameTest.NestedIframesDiffOrigin to fail.

BackForwardCache is currently being tested via field trials (see
https://crbug.com/1171298) and can be explicitly disabled using the
`--disable-back-forward-cache` or `--disable-features=BackForwardCache`
command-line flags.

libcef/browser/browser_contents_delegate.cc

@@ -243,6 +243,13 @@ void CefBrowserContentsDelegate::RenderFrameHostChanged(
   RenderFrameCreated(new_host);
 }
 
+void CefBrowserContentsDelegate::RenderFrameHostStateChanged(
+    content::RenderFrameHost* host,
+    content::RenderFrameHost::LifecycleState old_state,
+    content::RenderFrameHost::LifecycleState new_state) {
+  browser_info_->FrameHostStateChanged(host, old_state, new_state);
+}
+
 void CefBrowserContentsDelegate::RenderFrameDeleted(
     content::RenderFrameHost* render_frame_host) {
   const auto frame_id = CefFrameHostImpl::MakeFrameId(render_frame_host);

libcef/browser/browser_contents_delegate.h

@@ -108,6 +108,10 @@ class CefBrowserContentsDelegate : public content::WebContentsDelegate,
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
   void RenderFrameHostChanged(content::RenderFrameHost* old_host,
                               content::RenderFrameHost* new_host) override;
+  void RenderFrameHostStateChanged(
+      content::RenderFrameHost* host,
+      content::RenderFrameHost::LifecycleState old_state,
+      content::RenderFrameHost::LifecycleState new_state) override;
   void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
   void RenderViewReady() override;
   void RenderProcessGone(base::TerminationStatus status) override;

libcef/browser/browser_info.cc

@@ -153,6 +153,32 @@ void CefBrowserInfo::MaybeCreateFrame(content::RenderFrameHost* host,
   frame_info_set_.insert(base::WrapUnique(frame_info));
 }
 
+void CefBrowserInfo::FrameHostStateChanged(
+    content::RenderFrameHost* host,
+    content::RenderFrameHost::LifecycleState old_state,
+    content::RenderFrameHost::LifecycleState new_state) {
+  CEF_REQUIRE_UIT();
+
+  // We currently only care about BackForwardCache state changes.
+  bool added_to_bfcache =
+      new_state ==
+      content::RenderFrameHost::LifecycleState::kInBackForwardCache;
+  bool removed_from_bfcache =
+      old_state ==
+      content::RenderFrameHost::LifecycleState::kInBackForwardCache;
+  if (!added_to_bfcache && !removed_from_bfcache)
+    return;
+
+  base::AutoLock lock_scope(lock_);
+
+  const auto frame_id = CefFrameHostImpl::MakeFrameId(host);
+  auto it = frame_id_map_.find(frame_id);
+  DCHECK(it != frame_id_map_.end());
+  DCHECK((!it->second->is_in_bfcache_ && added_to_bfcache) ||
+         (it->second->is_in_bfcache_ && removed_from_bfcache));
+  it->second->is_in_bfcache_ = added_to_bfcache;
+}
+
 void CefBrowserInfo::RemoveFrame(content::RenderFrameHost* host) {
   CEF_REQUIRE_UIT();
 
@@ -190,8 +216,8 @@ void CefBrowserInfo::RemoveFrame(content::RenderFrameHost* host) {
     // Explicitly Detach everything but the current main frame.
     const auto& frame_info = *it2;
     if (frame_info->frame_ && !frame_info->IsCurrentMainFrame()) {
-      frame_info->frame_->Detach();
-      MaybeNotifyFrameDetached(browser_, frame_info->frame_);
+      if (frame_info->frame_->Detach())
+        MaybeNotifyFrameDetached(browser_, frame_info->frame_);
     }
 
     frame_info_set_.erase(it2);
@@ -314,8 +340,9 @@ CefBrowserInfo::FrameHostList CefBrowserInfo::GetAllFrames() const {
   base::AutoLock lock_scope(lock_);
   FrameHostList frames;
   for (const auto& info : frame_info_set_) {
-    if (info->frame_ && !info->is_speculative_)
+    if (info->frame_ && !info->is_speculative_ && !info->is_in_bfcache_) {
       frames.insert(info->frame_);
+    }
   }
   return frames;
 }
@@ -438,8 +465,8 @@ void CefBrowserInfo::SetMainFrame(CefRefPtr<CefBrowserHostBase> browser,
   CefRefPtr<CefFrameHostImpl> old_frame;
   if (main_frame_) {
     old_frame = main_frame_;
-    old_frame->Detach();
-    MaybeNotifyFrameDetached(browser, old_frame);
+    if (old_frame->Detach())
+      MaybeNotifyFrameDetached(browser, old_frame);
   }
 
   main_frame_ = frame;
@@ -520,8 +547,8 @@ void CefBrowserInfo::RemoveAllFrames(
   // Explicitly Detach everything but the current main frame.
   for (auto& info : frame_info_set_) {
     if (info->frame_ && !info->IsCurrentMainFrame()) {
-      info->frame_->Detach();
-      MaybeNotifyFrameDetached(old_browser, info->frame_);
+      if (info->frame_->Detach())
+        MaybeNotifyFrameDetached(old_browser, info->frame_);
     }
   }
 

libcef/browser/browser_info.h

@@ -19,10 +19,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/synchronization/lock.h"
 #include "base/values.h"
-
-namespace content {
-class RenderFrameHost;
-}
+#include "content/public/browser/render_frame_host.h"
 
 class CefBrowserHostBase;
 class CefFrameHandler;
@@ -62,6 +59,13 @@ class CefBrowserInfo : public base::RefCountedThreadSafe<CefBrowserInfo> {
   // true).
   void MaybeCreateFrame(content::RenderFrameHost* host, bool is_guest_view);
 
+  // Used to track state changes such as entering/exiting the BackForwardCache.
+  // Called from CefBrowserContentsDelegate::RenderFrameHostStateChanged.
+  void FrameHostStateChanged(
+      content::RenderFrameHost* host,
+      content::RenderFrameHost::LifecycleState old_state,
+      content::RenderFrameHost::LifecycleState new_state);
+
   // Remove the frame record for |host|. Called for the main frame when the
   // RenderView is destroyed, or for a sub-frame when the associated RenderFrame
   // is destroyed in the renderer process.
@@ -170,7 +174,7 @@ class CefBrowserInfo : public base::RefCountedThreadSafe<CefBrowserInfo> {
     ~FrameInfo();
 
     inline bool IsCurrentMainFrame() const {
-      return frame_ && is_main_frame_ && !is_speculative_;
+      return frame_ && is_main_frame_ && !is_speculative_ && !is_in_bfcache_;
     }
 
     content::RenderFrameHost* host_;
@@ -179,6 +183,7 @@ class CefBrowserInfo : public base::RefCountedThreadSafe<CefBrowserInfo> {
     bool is_guest_view_;
     bool is_main_frame_;
     bool is_speculative_;
+    bool is_in_bfcache_ = false;
     CefRefPtr<CefFrameHostImpl> frame_;
   };
 

libcef/browser/frame_host_impl.cc

@@ -427,18 +427,22 @@ content::RenderFrameHost* CefFrameHostImpl::GetRenderFrameHost() const {
   return render_frame_host_;
 }
 
-void CefFrameHostImpl::Detach() {
+bool CefFrameHostImpl::Detach() {
   CEF_REQUIRE_UIT();
 
+  // May be called multiple times (e.g. from CefBrowserInfo SetMainFrame and
+  // RemoveFrame).
+  bool first_detach = false;
+
   // Should not be called for temporary frames.
   DCHECK(!is_temporary());
 
   {
     base::AutoLock lock_scope(state_lock_);
-
-    // Should be called only once.
-    DCHECK(browser_info_);
-    browser_info_ = nullptr;
+    if (browser_info_) {
+      first_detach = true;
+      browser_info_ = nullptr;
+    }
   }
 
   // In case we never attached, clean up.
@@ -449,6 +453,8 @@ void CefFrameHostImpl::Detach() {
   render_frame_.reset();
   render_frame_host_ = nullptr;
   is_attached_ = false;
+
+  return first_detach;
 }
 
 // static

libcef/browser/frame_host_impl.h

@@ -115,8 +115,9 @@ class CefFrameHostImpl : public CefFrame, public cef::mojom::BrowserFrame {
 
   // Owned frame objects will be detached explicitly when the associated
   // RenderFrame is deleted. Temporary frame objects will be detached
-  // implicitly via CefBrowserInfo::browser() returning nullptr.
-  void Detach();
+  // implicitly via CefBrowserInfo::browser() returning nullptr. Returns true
+  // if this was the first call to Detach() for the frame.
+  bool Detach();
 
   // cef::mojom::BrowserFrame methods forwarded from CefBrowserFrame.
   void SendMessage(const std::string& name, base::Value arguments) override;