Fix detection of RenderViewHosts pending deletion in CreateRenderViewHost.

content/browser/site_per_process_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/site_per_process_browsertest.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/command_line.h"
@@ skipping 257 matching lines @@
   DCHECK(source == web_contents_);
 
   std::string ascii_message = base::UTF16ToASCII(message);
   if (base::MatchPattern(ascii_message, filter_)) {
     message_ = ascii_message;
     message_loop_runner_->Quit();
   }
   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
 
 //
 // SitePerProcessBrowserTest
 //
 
 SitePerProcessBrowserTest::SitePerProcessBrowserTest() {
 };
 
 std::string SitePerProcessBrowserTest::DepictFrameTree(FrameTreeNode* node) {
@@ skipping 3076 matching lines @@
   TitleWatcher title_watcher(shell()->web_contents(), expected_title);
   EXPECT_TRUE(ExecuteScriptAndExtractBool(
       popup_root->child_at(0)->current_frame_host(),
       "window.domAutomationController.send("
       "    postToOpenerOfSibling('subframe2', 'msg', '*'));",
       &success));
   EXPECT_TRUE(success);
   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