Fix detection of RenderViewHosts pending deletion in CreateRenderViewHost.

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index d500ce60e40f4ba222745b220de50392867d9365..8724d31c4823519afc930b0aaf860b2fca2ff346 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -275,6 +275,23 @@ bool ConsoleObserverDelegate::AddMessageToConsole(
   return false;
 }
 
+// A BrowserMessageFilter that drops SwapOut ACK messages.
+class SwapoutACKMessageFilter : public BrowserMessageFilter {
+ public:
+  SwapoutACKMessageFilter() : BrowserMessageFilter(FrameMsgStart) {}
+
+ protected:
+  ~SwapoutACKMessageFilter() override {}
+
+ private:
+  // BrowserMessageFilter:
+  bool OnMessageReceived(const IPC::Message& message) override {
+    return message.type() == FrameHostMsg_SwapOut_ACK::ID;
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(SwapoutACKMessageFilter);
+};
+
 }  // namespace
 
 //
@@ -3371,4 +3388,52 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
 }
 
+// Test for https://crbug.com/515302.  Perform two navigations, A->B->A, and
+// delay the SwapOut ACK from the A->B navigation, so that the second B->A
+// navigation is initiated before the first page receives the SwapOut ACK.
+// Ensure that the RVH(A) that's pending deletion is not reused in that case.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       RenderViewHostPendingDeletionIsNotReused) {
+  GURL a_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  NavigateToURL(shell(), a_url);
+
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  RenderFrameHostImpl* rfh = root->current_frame_host();
+  RenderViewHostImpl* rvh = rfh->render_view_host();
+  RenderFrameDeletedObserver deleted_observer(rfh);
+
+  // Install a BrowserMessageFilter to drop SwapOut ACK messages in A's
+  // process.
+  scoped_refptr<SwapoutACKMessageFilter> filter = new SwapoutACKMessageFilter();
+  rfh->GetProcess()->AddFilter(filter.get());
+
+  // Navigate to B.  This must wait for DidCommitProvisionalLoad, as opposed to
+  // DidStopLoading, since otherwise the SwapOut timer might call OnSwappedOut
+  // and destroy |rvh| before its pending deletion status is checked.
+  GURL b_url(embedded_test_server()->GetURL("b.com", "/title2.html"));
+  TestFrameNavigationObserver observer(root);
+  NavigationController::LoadURLParams params(b_url);
+  params.transition_type = ui::PAGE_TRANSITION_LINK;
+  root->navigator()->GetController()->LoadURLWithParams(params);
+  observer.Wait();
+
+  // Since the SwapOut ACK for A->B is dropped, the first page's
+  // RenderFrameHost and RenderViewHost should be pending deletion after the
+  // last navigation.
+  EXPECT_TRUE(root->render_manager()->IsPendingDeletion(rfh));
+  EXPECT_TRUE(rvh->is_pending_deletion());
+
+  // Start a navigation back to A and check that the RenderViewHost
+  // wasn't reused.
+  shell()->LoadURL(a_url);

[nasko, 2015/09/18 23:29:14]

This navigation doesn't wait for anything. Is this ok? Also, its implementation
is almost equivalent to the code on 3417-3419. Why not use it there too?

[alexmos, 2015/09/21 16:25:24]

The interesting part happens when creating the pending RFH and figuring out
which RVH to use and that's covered by the RVH check below.  But since the
original bug manifested as a renderer crash on this navigation, I do think it's
a good idea to make sure it succeeds, so I added the wait as the last step.

Incidentally, after a fresh look, I found a problem with the RVH check: I was
comparing the current RVH (for B) vs. the pending one.  That's fixed now, and
re-verified that it fails without my fix -- please take a quick look.
Good point!  Done.

+  EXPECT_NE(rvh, shell()->web_contents()->GetRenderViewHost());
+
+  // Simulate that the dropped SwapOut ACK message arrives now on the original
+  // RenderFrameHost, which should now get deleted.
+  rfh->OnSwappedOut();
+  EXPECT_TRUE(deleted_observer.deleted());
+}
+
 }  // namespace content