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..c1d588d66e5d973a48508d8a08bea4391dd5c37d 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,43 @@ 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* rfhi = root->current_frame_host();

[nasko, 2015/09/18 19:06:17]

nit: Either rfh or rvhi, for consistency. IMHO, the i isn't really adding much.

[alexmos, 2015/09/18 22:37:05]

Done.

+  RenderViewHostImpl* rvh = rfhi->render_view_host();
+
+  // Install a BrowserMessageFilter to drop SwapOut ACK messages in A's
+  // process.
+  scoped_refptr<SwapoutACKMessageFilter> filter = new SwapoutACKMessageFilter();
+  rfhi->GetProcess()->AddFilter(filter.get());
+
+  // Navigate to B.  The SwapOut completion should be delayed.
+  GURL b_url(embedded_test_server()->GetURL("b.com", "/title2.html"));
+  NavigateToURL(shell(), b_url);
+
+  // Navigate back to A.  Since there was no SwapOut ACK for A->B, this should
+  // happen while first page's RenderFrameHost and RenderViewHost are pending
+  // deletion.
+  EXPECT_TRUE(root->render_manager()->IsPendingDeletion(rfhi));

[nasko, 2015/09/18 19:06:17]

nit: I would put those expectations right after the NavigateToURL above, as they
are valid right after that and are an outcome of that navigation, regardless of
whether we navigate next or not.

[alexmos, 2015/09/18 22:37:05]

Done.

+  EXPECT_TRUE(rvh->is_pending_deletion());
+  NavigateToURL(shell(), a_url);

[nasko, 2015/09/18 19:06:17]

nit: I wonder if we should make this navigation async, such that we don't wait
for a commit. Sending the swapped out ack after this commit is ... interesting :
). I don't feel strongly though, so your call.

[alexmos, 2015/09/18 22:37:05]

Done. 

+
+  // Check that the RenderViewHost wasn't reused.
+  EXPECT_NE(rvh, shell()->web_contents()->GetRenderViewHost());
+
+  // Simulate that the dropped SwapOut ACK message arrives now on the original
+  // RenderFrameHost.
+  rfhi->OnSwappedOut();

[nasko, 2015/09/18 19:06:17]

For going the extra mile, should we put a RenderFrameDeleted observer to ensure
it gets deleted?
Also, I wonder if there is any difference of calling the method directly vs
making up a FrameHostMsg_SwapOutACK and calling OnMessageReceived.

[alexmos, 2015/09/18 22:37:05]

Done.
Seems like they would do the same thing, as OnMessageReceived would call
OnSwapOutACK(), which only calls OnSwappedOut.  I tried to use OnMessageReceived
but that caused WebContentsObserverSanityChecker to complain.

+}
+
 }  // namespace content