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* 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