Have the unload event execute in background on cross-site navigations

content/browser/frame_host/render_frame_host_manager_unittest.cc

Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index 92744e821c82838c24abb255e5af85e3e9101a59..3d2a13bf86649e4583d0c2af15d57ab933cf6061 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -169,10 +169,10 @@ class RenderFrameHostManagerTest
     // BeforeUnload finishes.
     ntp_rvh->SendShouldCloseACK(true);
 
-    // Assume SwapOutACK times out, so the dest_rvh proceeds and commits.
     dest_rvh->SendNavigate(101, kDestUrl);
+    ntp_rvh->OnSwappedOut(false);
 
-    EXPECT_TRUE(ntp_rvh->is_swapped_out());
+    EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT, ntp_rvh->rvh_state());
     return ntp_rvh;
   }
 
@@ -296,7 +296,7 @@ TEST_F(RenderFrameHostManagerTest, FilterMessagesWhileSwappedOut) {
 
   // The old renderer, being slow, now updates the title. It should be filtered
   // out and not take effect.
-  EXPECT_TRUE(ntp_rvh->is_swapped_out());
+  EXPECT_NE(RenderViewHostImpl::STATE_LIVE, ntp_rvh->rvh_state());

[nasko, 2014/01/27 19:09:51]

Shouldn't this be test for states where we filter IPCs?

[clamy, 2014/01/28 12:27:21]

Done.

   EXPECT_TRUE(ntp_rvh->OnMessageReceived(
       ViewHostMsg_UpdateTitle(rvh()->GetRoutingID(), 0, ntp_title, direction)));
   EXPECT_EQ(dest_title, contents()->GetTitle());
@@ -361,7 +361,8 @@ TEST_F(RenderFrameHostManagerTest, WhiteListDidActivateAcceleratedCompositing) {
 // widgets.
 TEST_F(RenderFrameHostManagerTest, GetRenderWidgetHostsReturnsActiveViews) {
   TestRenderViewHost* swapped_out_rvh = CreateSwappedOutRenderViewHost();
-  EXPECT_TRUE(swapped_out_rvh->is_swapped_out());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT,
+            swapped_out_rvh->rvh_state());
 
   scoped_ptr<RenderWidgetHostIterator> widgets(
       RenderWidgetHost::GetRenderWidgetHosts());
@@ -371,7 +372,8 @@ TEST_F(RenderFrameHostManagerTest, GetRenderWidgetHostsReturnsActiveViews) {
   RenderWidgetHost* widget = widgets->GetNextHost();
   EXPECT_FALSE(widgets->GetNextHost());
   RenderViewHost* rvh = RenderViewHost::From(widget);
-  EXPECT_FALSE(static_cast<RenderViewHostImpl*>(rvh)->is_swapped_out());
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE,
+            static_cast<RenderViewHostImpl*>(rvh)->rvh_state());
 }
 
 // Test if RenderViewHost::GetRenderWidgetHosts() returns a subset of
@@ -382,7 +384,8 @@ TEST_F(RenderFrameHostManagerTest, GetRenderWidgetHostsReturnsActiveViews) {
 TEST_F(RenderFrameHostManagerTest,
        GetRenderWidgetHostsWithinGetAllRenderWidgetHosts) {
   TestRenderViewHost* swapped_out_rvh = CreateSwappedOutRenderViewHost();
-  EXPECT_TRUE(swapped_out_rvh->is_swapped_out());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT,
+            swapped_out_rvh->rvh_state());
 
   scoped_ptr<RenderWidgetHostIterator> widgets(
       RenderWidgetHost::GetRenderWidgetHosts());
@@ -768,7 +771,7 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) {
   // CrossSiteResourceHandler::StartCrossSiteTransition triggers a
   // call of RenderFrameHostManager::SwapOutOldPage before
   // RenderFrameHostManager::DidNavigateMainFrame is called.
-  // The RVH is not swapped out until the commit.
+  // The RVH is swapped out after receiving the unload ack.
   manager->SwapOutOldPage();
   EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
       ViewMsg_SwapOut::ID));
@@ -796,9 +799,7 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) {
   EXPECT_NE(host3, host);
   EXPECT_NE(host3->GetProcess()->GetID(), host2_process_id);
 
-  // Navigations in the new RVH should be suspended, which is ok because the
-  // old RVH is not yet swapped out and can respond to a second beforeunload
-  // request.
+  // Navigations in the new RVH should be suspended.
   EXPECT_TRUE(static_cast<RenderViewHostImpl*>(
       host3->render_view_host())->are_navigations_suspended());
   EXPECT_EQ(host, manager->current_frame_host());
@@ -812,7 +813,6 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) {
   // CrossSiteResourceHandler::StartCrossSiteTransition triggers a
   // call of RenderFrameHostManager::SwapOutOldPage before
   // RenderFrameHostManager::DidNavigateMainFrame is called.
-  // The RVH is not swapped out until the commit.
   manager->SwapOutOldPage();
   EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
       ViewMsg_SwapOut::ID));
@@ -1025,14 +1025,12 @@ TEST_F(RenderFrameHostManagerTest, NavigateAfterMissingSwapOutACK) {
   contents()->ProceedWithCrossSiteNavigation();
   EXPECT_FALSE(rvh2->is_waiting_for_beforeunload_ack());
   rvh2->SwapOut();
-  EXPECT_TRUE(rvh2->is_waiting_for_unload_ack());
+  EXPECT_TRUE(rvh2->IsWaitingForUnloadACK());
 
-  // The back navigation commits.  We should proactively clear the
-  // is_waiting_for_unload_ack state to be safe.
+  // The back navigation commits.
   const NavigationEntry* entry1 = contents()->GetController().GetPendingEntry();
   rvh1->SendNavigate(entry1->GetPageID(), entry1->GetURL());
-  EXPECT_TRUE(rvh2->is_swapped_out());
-  EXPECT_FALSE(rvh2->is_waiting_for_unload_ack());

[nasko, 2014/01/27 19:09:51]

Why is this expectation dropped?

[clamy, 2014/01/28 12:27:21]

If the RVH is in STATE_PENDING_SWAP_OUT then IsWaitingForUnloadAck is true.
Since I dropped the boolean is_waiting_for_unload_ack_, IsWaitingForUnloadAck
returns true when the RVH is in STATE_WAITING_FOR_UNLOAD_ACK,
STATE_WAITING_FOR_CLOSE, STATE_PENDING_SWAP_OUT, STATE_PENDING_SHUTDOWN.

+  EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh2->rvh_state());
 
   // We should be able to navigate forward.
   contents()->GetController().GoForward();
@@ -1040,8 +1038,8 @@ TEST_F(RenderFrameHostManagerTest, NavigateAfterMissingSwapOutACK) {
   const NavigationEntry* entry2 = contents()->GetController().GetPendingEntry();
   rvh2->SendNavigate(entry2->GetPageID(), entry2->GetURL());
   EXPECT_EQ(rvh2, rvh());
-  EXPECT_FALSE(rvh2->is_swapped_out());
-  EXPECT_TRUE(rvh1->is_swapped_out());
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE, rvh2->rvh_state());
+  EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh1->rvh_state());

[nasko, 2014/01/27 19:09:51]

Why not add a call to OnSwapOut() and then check that we have moved rvh1 to
swapped out state? It will make the test match a bit closer what it used to
test.

[clamy, 2014/01/28 12:27:21]

Done.

 }
 
 // Test that we create swapped out RVHs for the opener chain when navigating an
@@ -1088,13 +1086,13 @@ TEST_F(RenderFrameHostManagerTest, CreateSwappedOutOpenerRVHs) {
   TestRenderViewHost* opener1_rvh = static_cast<TestRenderViewHost*>(
       opener1_manager->GetSwappedOutRenderViewHost(rvh2->GetSiteInstance()));
   EXPECT_TRUE(opener1_manager->IsRVHOnSwappedOutList(opener1_rvh));
-  EXPECT_TRUE(opener1_rvh->is_swapped_out());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT, opener1_rvh->rvh_state());
 
   // Ensure a swapped out RVH is created in the second opener tab.
   TestRenderViewHost* opener2_rvh = static_cast<TestRenderViewHost*>(
       opener2_manager->GetSwappedOutRenderViewHost(rvh2->GetSiteInstance()));
   EXPECT_TRUE(opener2_manager->IsRVHOnSwappedOutList(opener2_rvh));
-  EXPECT_TRUE(opener2_rvh->is_swapped_out());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT, opener2_rvh->rvh_state());
 
   // Navigate to a cross-BrowsingInstance URL.
   contents()->NavigateAndCommit(kChromeUrl);
@@ -1196,7 +1194,7 @@ TEST_F(RenderFrameHostManagerTest, EnableWebUIWithSwappedOutOpener) {
   TestRenderViewHost* opener1_rvh = static_cast<TestRenderViewHost*>(
       opener1_manager->GetSwappedOutRenderViewHost(rvh2->GetSiteInstance()));
   EXPECT_TRUE(opener1_manager->IsRVHOnSwappedOutList(opener1_rvh));
-  EXPECT_TRUE(opener1_rvh->is_swapped_out());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT, opener1_rvh->rvh_state());
 
   // Ensure the new RVH has WebUI bindings.
   EXPECT_TRUE(rvh2->GetEnabledBindings() & BINDINGS_POLICY_WEB_UI);
@@ -1342,4 +1340,224 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyClose) {
   EXPECT_EQ(host, manager->current_frame_host());
 }
 
+// This checks that the given RVH has been properly deleted.
+class RenderViewHostDestructionChecker : public WebContentsObserver {

[nasko, 2014/01/27 19:09:51]

nit: s/Checker/Observer/

[clamy, 2014/01/28 12:27:21]

Done.

+ public:
+  RenderViewHostDestructionChecker(RenderViewHost* render_view_host)
+      : WebContentsObserver(WebContents::FromRenderViewHost(render_view_host)),
+        render_view_host_(render_view_host),
+        rvh_deleted_(false) {}
+
+  bool rvh_deleted() { return rvh_deleted_; }
+
+  virtual void RenderViewDeleted(RenderViewHost* render_view_host) OVERRIDE {
+    if (render_view_host == render_view_host_)
+      rvh_deleted_ = true;
+  }
+
+ private:
+  RenderViewHost* render_view_host_;
+  bool rvh_deleted_;
+
+  DISALLOW_COPY_AND_ASSIGN(RenderViewHostDestructionChecker);
+};
+
+// Tests that the RenderViewHost is properly swapped out or deleted when the
+// SwapOutACK is received before the new page commits.
+TEST_F(RenderFrameHostManagerTest, SwapOutACKBeforeNewPageCommits) {
+  const GURL kUrl1("http://www.google.com/");
+  const GURL kUrl2("http://www.chromium.org/");
+
+  // Navigate to the first page.
+  contents()->NavigateAndCommit(kUrl1);
+  TestRenderViewHost* rvh1 = test_rvh();
+  RenderViewHostDestructionChecker destruction_checker1(rvh1);
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE, rvh1->rvh_state());
+
+  // Navigate to new site, simulating onbeforeunload approval.
+  controller().LoadURL(kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string());
+  base::TimeTicks now = base::TimeTicks::Now();
+  rvh1->OnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true, now, now));
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderViewHost* rvh2 =
+      static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost());
+
+  // Simulate rvh2's response, which leads to an unload request being sent to
+  // rvh1.
+  std::vector<GURL> url_chain1;
+  url_chain1.push_back(GURL());
+  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
+      rvh2,

[nasko, 2014/01/27 19:09:51]

nit: you can put multiple arguments on one line to save some vertical space

[clamy, 2014/01/28 12:27:21]

Done.

+      GlobalRequestID(0, 0),
+      false,
+      url_chain1,
+      Referrer(),
+      PAGE_TRANSITION_TYPED,
+      1,
+      false);
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
+            rvh1->rvh_state());
+
+  // Simulate the swap out ack.
+  rvh1->OnSwappedOut(false);
+  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_COMMIT, rvh1->rvh_state());
+
+  // The new page commits.
+  contents()->TestDidNavigate(rvh2, 1, kUrl2, PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(rvh2, rvh());
+  EXPECT_TRUE(contents()->GetPendingRenderViewHost() == NULL);
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE, rvh2->rvh_state());
+
+  // rvh1 should have been deleted.
+  EXPECT_TRUE(destruction_checker1.rvh_deleted());
+  rvh1 = NULL;
+
+  // Increment the number of active views in SiteInstanceImpl so that rvh2 is

[nasko, 2014/01/27 19:09:51]

The code following seems like a separate test case. Since this is unit tests and
it is cheap to start new test, might as well make it separate.

[clamy, 2014/01/28 12:27:21]

Done.

+  // not deleted on swap out.
+  static_cast<SiteInstanceImpl*>(
+      rvh2->GetSiteInstance())->increment_active_view_count();
+  RenderViewHostDestructionChecker destruction_checker2(rvh2);
+
+  // Navigate to new site, simulating onbeforeunload approval.
+  controller().LoadURL(kUrl1, Referrer(), PAGE_TRANSITION_LINK, std::string());
+  now = base::TimeTicks::Now();
+  rvh2->OnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true, now, now));
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderViewHost* rvh3 =
+      static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost());
+
+  // Simulate rvh3's response, which leads to an unload request being sent to
+  // rvh2.
+  std::vector<GURL> url_chain2;
+  url_chain2.push_back(GURL());
+  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
+      rvh3,
+      GlobalRequestID(0, 0),
+      false,
+      url_chain2,
+      Referrer(),
+      PAGE_TRANSITION_TYPED,
+      1,
+      false);
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
+            rvh2->rvh_state());
+
+  // Simulate the swap out ack.
+  rvh2->OnSwappedOut(false);
+  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_COMMIT, rvh2->rvh_state());
+
+  // The new page commits.
+  contents()->TestDidNavigate(rvh3, 1, kUrl2, PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(rvh3, rvh());
+  EXPECT_TRUE(contents()->GetPendingRenderViewHost() == NULL);
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE, rvh3->rvh_state());
+
+  // rvh2 should be swapped out.
+  EXPECT_FALSE(destruction_checker2.rvh_deleted());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT, rvh2->rvh_state());
+}
+
+// Tests that the RenderViewHost is properly swapped out or deleted when the new
+// page commits befoer the swap out ack is received.
+TEST_F(RenderFrameHostManagerTest, NewPageCommitsBeforeSwapOutACK) {
+  const GURL kUrl1("http://www.google.com/");
+  const GURL kUrl2("http://www.chromium.org/");
+
+  // Navigate to the first page.
+  contents()->NavigateAndCommit(kUrl1);
+  TestRenderViewHost* rvh1 = test_rvh();
+  RenderViewHostDestructionChecker destruction_checker1(rvh1);
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE, rvh1->rvh_state());
+
+  // Navigate to new site, simulating onbeforeunload approval.
+  controller().LoadURL(kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string());
+  base::TimeTicks now = base::TimeTicks::Now();
+  rvh1->OnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true, now, now));
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderViewHost* rvh2 =
+      static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost());
+
+  // Simulate rvh2's response, which leads to an unload request being sent to
+  // rvh1.
+  std::vector<GURL> url_chain1;
+  url_chain1.push_back(GURL());
+  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
+      rvh2,
+      GlobalRequestID(0, 0),
+      false,
+      url_chain1,
+      Referrer(),
+      PAGE_TRANSITION_TYPED,
+      1,
+      false);
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
+            rvh1->rvh_state());
+
+  // The new page commits.
+  contents()->TestDidNavigate(rvh2, 1, kUrl2, PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(rvh2, rvh());
+  EXPECT_TRUE(contents()->GetPendingRenderViewHost() == NULL);
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE, rvh2->rvh_state());
+  EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SHUTDOWN, rvh1->rvh_state());
+
+  // Simulate the swap out ack.
+  rvh1->OnSwappedOut(false);
+
+  // rvh1 should have been deleted.
+  EXPECT_TRUE(destruction_checker1.rvh_deleted());
+  rvh1 = NULL;
+
+  // Increment the number of active views in SiteInstanceImpl so that rvh2 is
+  // not deleted on swap out.
+  static_cast<SiteInstanceImpl*>(
+      rvh2->GetSiteInstance())->increment_active_view_count();
+  RenderViewHostDestructionChecker destruction_checker2(rvh2);
+
+  // Navigate to new site, simulating onbeforeunload approval.
+  controller().LoadURL(kUrl1, Referrer(), PAGE_TRANSITION_LINK, std::string());
+  now = base::TimeTicks::Now();
+  rvh2->OnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true, now, now));
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderViewHost* rvh3 =
+      static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost());
+
+  // Simulate rvh3's response, which leads to an unload request being sent to
+  // rvh2.
+  std::vector<GURL> url_chain2;
+  url_chain2.push_back(GURL());
+  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
+      rvh3,
+      GlobalRequestID(0, 0),
+      false,
+      url_chain2,
+      Referrer(),
+      PAGE_TRANSITION_TYPED,
+      1,
+      false);
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
+            rvh2->rvh_state());
+
+  // The new page commits.
+  contents()->TestDidNavigate(rvh3, 1, kUrl2, PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(rvh3, rvh());
+  EXPECT_TRUE(contents()->GetPendingRenderViewHost() == NULL);
+  EXPECT_EQ(RenderViewHostImpl::STATE_LIVE, rvh3->rvh_state());
+  EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh2->rvh_state());
+
+  // Simulate the swap out ack.
+  rvh2->OnSwappedOut(false);
+
+  // rvh2 should be swapped out.
+  EXPECT_FALSE(destruction_checker2.rvh_deleted());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT, rvh2->rvh_state());
+}
+
 }  // namespace content