Update RenderFrameHost::IsCurrent()

Update the implementation of RenderFrameHost::IsCurrent() to also
check recursively for current RenderFrameHost and all parents if it is
the current RenderFrameHost in its FrameTreeNode. More specifically,
changing the meaning of IsCurrent from "current in the FrameTreeNode"
to "current in their FrameTreeNodes for its RFH and its parents".

The major problem with the current implementation is that during
a navigation, new RenderFrameHost replaces the old one in its
FrameTreeNode but the children are still the current
ones within their FrameTreeNode as we keep the RFH alive for sometime
after commit.

In particular, after a navigation from A(B) to C, only A.IsCurrent()
becomes false. B.IsCurrent() still remains true. With the current
update now both A.IsCurrent() and B.IsCurrent() would return false
which is the expected behaviour.

BUG=1058984

Change-Id: I3c22e3e512021c8ca2e8ccc7eed66075700bb632
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2060374
Commit-Queue: Sreeja Kamishetty <sreejakshetty@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@{#749439}

content/browser/back_forward_cache_browsertest.cc

@@ -5550,4 +5550,28 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, PageshowMetrics) {
               ElementsAre(base::Bucket(0, 1), base::Bucket(1, 1)));
 }
 
+// Navigate from A(B) to C and check IsCurrent status for RenderFrameHost A
+// and B before and after entering back-forward cache.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, CheckIsCurrent) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_a(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b)"));
+  GURL url_c(embedded_test_server()->GetURL("c.com", "/title1.html"));
+
+  // 1) Navigate to A(B).
+  EXPECT_TRUE(NavigateToURL(shell(), url_a));
+  RenderFrameHostImpl* rfh_a = current_frame_host();
+  RenderFrameHostImpl* rfh_b = rfh_a->child_at(0)->current_frame_host();
+
+  EXPECT_TRUE(rfh_a->IsCurrent());
+  EXPECT_TRUE(rfh_b->IsCurrent());
+
+  // 2) Navigate to C.
+  EXPECT_TRUE(NavigateToURL(shell(), url_c));
+  EXPECT_TRUE(rfh_a->is_in_back_forward_cache());
+  EXPECT_TRUE(rfh_b->is_in_back_forward_cache());
+
+  EXPECT_FALSE(rfh_a->IsCurrent());
+  EXPECT_FALSE(rfh_b->IsCurrent());
+}
 }  // namespace content

content/browser/frame_host/render_frame_host_impl.cc

@@ -6189,7 +6189,19 @@ bool RenderFrameHostImpl::IsRenderFrameLive() {
 }
 
 bool RenderFrameHostImpl::IsCurrent() {
-  return this == frame_tree_node_->current_frame_host();
+  RenderFrameHostImpl* rfh = this;
+  // Check this RenderFrameHost and all its ancestors to see if they are the
+  // current ones in their respective FrameTreeNodes.
+  // It is important to check for all ancestors as when navigation commits a new
+  // RenderFrameHost may replace one of the parents, swapping out the old with
+  // its entire subtree but |this| will still be a current one in its
+  // FrameTreeNode.
+  while (rfh) {
+    if (rfh->frame_tree_node()->current_frame_host() != rfh)
+      return false;
+    rfh = rfh->GetParent();
+  }
+  return true;
 }
 
 size_t RenderFrameHostImpl::GetProxyCount() {

content/browser/frame_host/render_frame_host_impl.h

@@ -1506,6 +1506,8 @@ class CONTENT_EXPORT RenderFrameHostImpl
                            TimerNotRestartedBySecondDialog);
   FRIEND_TEST_ALL_PREFIXES(RenderFrameHostImplBrowserTest,
                            ComputeSiteForCookiesParentNavigatedAway);
+  FRIEND_TEST_ALL_PREFIXES(RenderFrameHostImplBrowserTest,
+                           CheckIsCurrentBeforeAndAfterUnload);
   FRIEND_TEST_ALL_PREFIXES(RenderFrameHostManagerTest,
                            CreateRenderViewAfterProcessKillAndClosedProxy);
   FRIEND_TEST_ALL_PREFIXES(RenderFrameHostManagerTest, DontSelectInvalidFiles);

content/browser/frame_host/render_frame_host_impl_browsertest.cc

@@ -4,6 +4,7 @@
 
 #include "content/browser/frame_host/render_frame_host_impl.h"
 
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ -190,6 +191,10 @@ class RenderFrameHostImplBrowserTest : public ContentBrowserTest {
   }
   net::EmbeddedTestServer* https_server() { return &https_server_; }
 
+  WebContentsImpl* web_contents() const {
+    return static_cast<WebContentsImpl*>(shell()->web_contents());
+  }
+
  private:
   base::test::ScopedFeatureList feature_list_;
   net::EmbeddedTestServer https_server_;
@@ -3567,6 +3572,55 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest, WebUiReloadAfterCrash) {
             EvalJs(main_document, "document.querySelector('h3').textContent"));
 }
 
+// Start with A(B), navigate A to C. By emulating a slow unload handler B, check
+// the status of IsCurrent for subframes of A i.e., B before and after
+// navigating to C.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
+                       CheckIsCurrentBeforeAndAfterUnload) {
+  IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
+  std::string onunload_script = "window.onunload = function(){}";
+  GURL url_ab(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b)"));
+  GURL url_c(embedded_test_server()->GetURL("c.com", "/title1.html"));
+
+  // 1) Navigate to a page with an iframe.
+  EXPECT_TRUE(NavigateToURL(shell(), url_ab));
+  RenderFrameHostImpl* rfh_a = web_contents()->GetMainFrame();
+  RenderFrameHostImpl* rfh_b = rfh_a->child_at(0)->current_frame_host();
+  RenderFrameDeletedObserver delete_rfh_b(rfh_b);
+  EXPECT_EQ(RenderFrameHostImpl::UnloadState::NotRun, rfh_b->unload_state_);
+
+  // 2) Set an arbitrarily long timeout to ensure the subframe unload timer
+  // doesn't fire before we call OnDetach(). Act as if there was a slow unload
+  // handler on rfh_b. The non navigating frames are waiting for
+  // FrameHostMsg_Detach.
+  rfh_b->SetSubframeUnloadTimeoutForTesting(base::TimeDelta::FromSeconds(30));
+  auto detach_filter = base::MakeRefCounted<DropMessageFilter>(
+      FrameMsgStart, FrameHostMsg_Detach::ID);
+  rfh_b->GetProcess()->AddFilter(detach_filter.get());
+  EXPECT_TRUE(ExecuteScript(rfh_b->frame_tree_node(), onunload_script));
+
+  // 3) Check the IsCurrent state of rfh_a, rfh_b before navigating to C.
+  EXPECT_TRUE(rfh_a->IsCurrent());
+  EXPECT_TRUE(rfh_b->IsCurrent());
+
+  // 4) Navigate rfh_a to C.
+  EXPECT_TRUE(NavigateToURL(shell(), url_c));
+  RenderFrameHostImpl* rfh_c = web_contents()->GetMainFrame();
+  EXPECT_EQ(RenderFrameHostImpl::UnloadState::InProgress, rfh_b->unload_state_);
+
+  // 5) Check the IsCurrent state of rfh_a, rfh_b and rfh_c after navigating to
+  // C.
+  EXPECT_FALSE(rfh_a->IsCurrent());
+  EXPECT_FALSE(rfh_b->IsCurrent());
+  EXPECT_TRUE(rfh_c->IsCurrent());
+
+  // 6) Run detach on rfh_b to delete its frame.
+  EXPECT_FALSE(delete_rfh_b.deleted());
+  rfh_b->OnDetach();
+  EXPECT_TRUE(delete_rfh_b.deleted());
+}
+
 namespace {
 
 // Collects the committed IPAddressSpaces, and makes them available for

content/public/browser/render_frame_host.h

@@ -298,14 +298,21 @@ class CONTENT_EXPORT RenderFrameHost : public IPC::Listener,
   // and still has a connection.  This is valid for all frames.
   virtual bool IsRenderFrameLive() = 0;
 
-  // Returns true if this is the currently-visible RenderFrameHost for our frame
-  // tree node. During process transfer, a RenderFrameHost may be created that
-  // is not current. After process transfer, the old RenderFrameHost becomes
-  // non-current until it is deleted (which may not happen until its unload
-  // handler runs).
+  // Returns true if this RenderFrameHost is currently in the frame tree for its
+  // page. Specifically, this is when the RenderFrameHost and all of its
+  // ancestors are the current RenderFrameHost in their respective
+  // FrameTreeNodes.
   //
-  // Changes to the IsCurrent() state of a RenderFrameHost may be observed via
-  // WebContentsObserver::RenderFrameHostChanged().
+  // For instance, during a navigation, if a new RenderFrameHost replaces this
+  // RenderFrameHost, IsCurrent() becomes false for this frame and its
+  // children even if the children haven't been replaced.
+  //
+  // After a RenderFrameHost has been replaced in its frame, it will either:
+  //  1) Enter the BackForwardCache.
+  //  2) Start running unload handlers and will be deleted after this ("pending
+  //  deletion").
+  // In both cases, IsCurrent() becomes false for this frame and all its
+  // children.
   virtual bool IsCurrent() = 0;
 
   // Get the number of proxies to this frame, in all processes. Exposed for

content/test/web_contents_observer_sanity_checker.cc

@@ -123,11 +123,15 @@ void WebContentsObserverSanityChecker::RenderFrameHostChanged(
     RenderFrameHost* new_host) {
   CHECK(new_host);
   CHECK_NE(new_host, old_host);
+  CHECK(GetRoutingPair(old_host) != GetRoutingPair(new_host));
 
   if (old_host) {
     EnsureStableParentValue(old_host);
     CHECK_EQ(old_host->GetParent(), new_host->GetParent());
     GlobalRoutingID routing_pair = GetRoutingPair(old_host);
+    // If the navigation requires a new RFH, IsCurrent on old host should be
+    // false.
+    CHECK(!old_host->IsCurrent());
     bool old_did_exist = !!current_hosts_.erase(routing_pair);
     if (!old_did_exist) {
       CHECK(false)
@@ -136,6 +140,7 @@ void WebContentsObserverSanityChecker::RenderFrameHostChanged(
     }
   }
 
+  CHECK(new_host->IsCurrent());
   EnsureStableParentValue(new_host);
   if (new_host->GetParent()) {
     AssertRenderFrameExists(new_host->GetParent());
@@ -226,6 +231,9 @@ void WebContentsObserverSanityChecker::DidFinishNavigation(
   CHECK(!navigation_handle->HasCommitted() ||
         navigation_handle->GetRenderFrameHost() != nullptr);
 
+  CHECK(!navigation_handle->HasCommitted() ||
+        navigation_handle->GetRenderFrameHost()->IsCurrent());
+
   // If ReadyToCommitNavigation was dispatched, verify that the
   // |navigation_handle| has the same RenderFrameHost at this time as the one
   // returned at ReadyToCommitNavigation.