Add test for filtering of IPC messages when RenderFrameHost is swapped out.

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 d03aa2f0ced989d8a4d6e5a364f9b10643d647d3..f159f2ca43d77a686b9d07c165a50398b960b088 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
@@ -93,6 +94,82 @@ class BeforeUnloadFiredWebContentsDelegate : public WebContentsDelegate {
   DISALLOW_COPY_AND_ASSIGN(BeforeUnloadFiredWebContentsDelegate);
 };
 
+// This observer keeps track of the last deleted RenderViewHost to avoid
+// accessing it and causing use-after-free condition.
+class RenderViewHostDeletedObserver : public WebContentsObserver {
+ public:
+  RenderViewHostDeletedObserver(RenderViewHost* rvh)
+      : WebContentsObserver(WebContents::FromRenderViewHost(rvh)),
+        process_id_(rvh->GetProcess()->GetID()),
+        routing_id_(rvh->GetRoutingID()),
+        render_view_host_(rvh),
+        deleted_(false) {
+  }
+
+  virtual void RenderViewDeleted(RenderViewHost* render_view_host) OVERRIDE {
+    if (render_view_host->GetProcess()->GetID() == process_id_ &&
+        render_view_host->GetRoutingID() == routing_id_) {
+      // The expectation is that the pointers are exactly the same, but the
+      // pointers shouldn't be used, as the object is destroyed.

[Charlie Reis, 2014/03/24 23:24:43]

I don't understand this comment.  It says we shouldn't use the RVH pointers, but
then we use them on the next line.

If we're going to compare the pointers at all, then I don't think we need the
process ID and routing ID check.

At that point, we're essentially bringing RenderViewHostDestructionObserver up
from below and using that plus the render_view_host_ = NULL line so that we
don't have a dangling pointer afterward.

[nasko, 2014/03/25 18:30:22]

Ok, got back to not using pointers. The aim of this check was to enforce the
expectation that an RVH object with the same routing/process id pair is the
exact same object in memory.

+      EXPECT_EQ(render_view_host_, render_view_host);
+      render_view_host_ = NULL;
+      deleted_ = true;
+    }
+  }
+
+  bool deleted() {
+    return deleted_;
+  }
+
+ private:
+  int process_id_;
+  int routing_id_;
+  RenderViewHost* render_view_host_;
+  bool deleted_;
+
+  DISALLOW_COPY_AND_ASSIGN(RenderViewHostDeletedObserver);
+};
+
+
+// This observer is used to check whether IPC messages are being filtered for
+// swapped out RenderFrameHost objects. It observes the plugin crash and favicon
+// update events, which the FilterMessagesWhileSwappedOut test simulates being
+// sent. The test is successful if the event is not observed.
+// See http://crbug.com/351815
+class PluginFaviconMessageObserver : public WebContentsObserver {
+ public:
+  PluginFaviconMessageObserver(WebContents* web_contents)
+      : WebContentsObserver(web_contents),
+        plugin_crashed_(false),
+        favicon_received_(false) { }
+
+  virtual void PluginCrashed(const base::FilePath& plugin_path,
+                             base::ProcessId plugin_pid) OVERRIDE {
+    plugin_crashed_ = true;
+  }
+
+  virtual void DidUpdateFaviconURL(
+      int32 page_id,
+      const std::vector<FaviconURL>& candidates) OVERRIDE {
+    favicon_received_ = true;
+  }
+
+  bool plugin_crashed() {
+    return plugin_crashed_;
+  }
+
+  bool favicon_received() {
+    return favicon_received_;
+  }
+
+ private:
+  bool plugin_crashed_;
+  bool favicon_received_;
+
+  DISALLOW_COPY_AND_ASSIGN(PluginFaviconMessageObserver);
+};
+
+
 }  // namespace
 
 class RenderFrameHostManagerTest
@@ -112,6 +189,20 @@ class RenderFrameHostManagerTest
     factory_.set_should_create_webui(should_create_webui);
   }
 
+  void StartCrossSiteTransition(TestWebContents* contents) {
+    std::vector<GURL> url_chain;
+    url_chain.push_back(GURL());
+    contents->GetRenderManagerForTesting()->OnCrossSiteResponse(
+        contents->GetRenderManagerForTesting()->pending_frame_host(),
+        GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
+        url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
+    EXPECT_TRUE(contents->cross_navigation_pending());
+    RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
+        contents->GetRenderViewHost());
+    EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
+              rvh->rvh_state());
+  }
+
   void NavigateActiveAndCommit(const GURL& url) {
     // Note: we navigate the active RenderViewHost because previous navigations
     // won't have committed yet, so NavigateAndCommit does the wrong thing
@@ -121,19 +212,34 @@ class RenderFrameHostManagerTest
 
     // Simulate the BeforeUnload_ACK that is received from the current renderer
     // for a cross-site navigation.
-    if (old_rvh != active_rvh())
+    if (old_rvh != active_rvh()) {
       old_rvh->SendBeforeUnloadACK(true);
+      EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, old_rvh->rvh_state());
+    }
 
     // Commit the navigation with a new page ID.
     int32 max_page_id = contents()->GetMaxPageIDForSiteInstance(
         active_rvh()->GetSiteInstance());
 
+    // Simulate the response comming from the pending renderer.
+    if (old_rvh != active_rvh())
+      StartCrossSiteTransition(contents());
+
     // Simulate the SwapOut_ACK that fires if you commit a cross-site
     // navigation.
-    if (old_rvh != active_rvh())
+    if (old_rvh != active_rvh()) {
       old_rvh->OnSwappedOut(false);
+      EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_COMMIT,
+                old_rvh->rvh_state());
+    }
 
+    // Use an observer to avoid accessing a deleted renderer later on when the
+    // state is being checked.
+    RenderViewHostDeletedObserver rvh_observer(old_rvh);
     active_test_rvh()->SendNavigate(max_page_id + 1, url);
+
+    if (old_rvh != active_rvh() && !rvh_observer.deleted())
+      EXPECT_TRUE(old_rvh->IsSwappedOut());
   }
 
   bool ShouldSwapProcesses(RenderFrameHostManager* manager,
@@ -222,7 +328,7 @@ TEST_F(RenderFrameHostManagerTest, NewTabPageProcesses) {
   ASSERT_TRUE(dest_rvh2);
 
   ntp_rvh2->SendBeforeUnloadACK(true);
-  ntp_rvh2->OnSwappedOut(false);
+  StartCrossSiteTransition(contents2.get());
   dest_rvh2->SendNavigate(101, kDestUrl);
 
   // The two RVH's should be different in every way.
@@ -238,9 +344,9 @@ TEST_F(RenderFrameHostManagerTest, NewTabPageProcesses) {
   contents2->GetController().LoadURL(
       kChromeUrl, Referrer(), PAGE_TRANSITION_LINK, std::string());
   dest_rvh2->SendBeforeUnloadACK(true);
-  dest_rvh2->OnSwappedOut(false);
+  StartCrossSiteTransition(contents2.get());
   static_cast<TestRenderViewHost*>(contents2->GetRenderManagerForTesting()->
-     pending_render_view_host())->SendNavigate(102, kChromeUrl);
+      pending_render_view_host())->SendNavigate(102, kChromeUrl);
 
   EXPECT_NE(active_rvh()->GetSiteInstance(),
             contents2->GetRenderViewHost()->GetSiteInstance());
@@ -256,53 +362,69 @@ TEST_F(RenderFrameHostManagerTest, NewTabPageProcesses) {
 TEST_F(RenderFrameHostManagerTest, FilterMessagesWhileSwappedOut) {
   const GURL kChromeURL("chrome://foo");
   const GURL kDestUrl("http://www.google.com/");
+  std::vector<FaviconURL> icons;
 
   // Navigate our first tab to a chrome url and then to the destination.
   NavigateActiveAndCommit(kChromeURL);
   TestRenderViewHost* ntp_rvh = static_cast<TestRenderViewHost*>(
       contents()->GetRenderManagerForTesting()->current_host());
 
-  // Send an update title message and make sure it works.
+  // Send an update favicon message and make sure it works.
   const base::string16 ntp_title = base::ASCIIToUTF16("NTP Title");
-  blink::WebTextDirection direction = blink::WebTextDirectionLeftToRight;
-  EXPECT_TRUE(ntp_rvh->OnMessageReceived(
-      ViewHostMsg_UpdateTitle(rvh()->GetRoutingID(), 0, ntp_title, direction)));
-  EXPECT_EQ(ntp_title, contents()->GetTitle());
-
-  // Navigate to a cross-site URL.
-  contents()->GetController().LoadURL(
-      kDestUrl, Referrer(), PAGE_TRANSITION_LINK, std::string());
-  EXPECT_TRUE(contents()->cross_navigation_pending());
-  TestRenderViewHost* dest_rvh = static_cast<TestRenderViewHost*>(
-      contents()->GetRenderManagerForTesting()->pending_render_view_host());
-  ASSERT_TRUE(dest_rvh);
-  EXPECT_NE(ntp_rvh, dest_rvh);
-
+  {
+    PluginFaviconMessageObserver observer(contents());
+    EXPECT_TRUE(ntp_rvh->OnMessageReceived(
+                    ViewHostMsg_UpdateFaviconURL(
+                        rvh()->GetRoutingID(), 0, icons)));
+    EXPECT_TRUE(observer.favicon_received());
+  }
   // Create one more view in the same SiteInstance where dest_rvh2
   // exists so that it doesn't get deleted on navigation to another
   // site.
   static_cast<SiteInstanceImpl*>(ntp_rvh->GetSiteInstance())->
       increment_active_view_count();
 
-  // BeforeUnload finishes.
-  ntp_rvh->SendBeforeUnloadACK(true);
 
-  // Assume SwapOutACK times out, so the dest_rvh proceeds and commits.
-  dest_rvh->SendNavigate(101, kDestUrl);
+  // Navigate to a cross-site URL.
+  NavigateActiveAndCommit(kDestUrl);
+  TestRenderViewHost* dest_rvh = static_cast<TestRenderViewHost*>(
+      contents()->GetRenderViewHost());
+  ASSERT_TRUE(dest_rvh);
+  EXPECT_NE(ntp_rvh, dest_rvh);
 
-  // The new RVH should be able to update its title.
+  // The new RVH should be able to update its favicon.
   const base::string16 dest_title = base::ASCIIToUTF16("Google");
-  EXPECT_TRUE(dest_rvh->OnMessageReceived(
-      ViewHostMsg_UpdateTitle(rvh()->GetRoutingID(), 101, dest_title,
-                              direction)));
-  EXPECT_EQ(dest_title, contents()->GetTitle());
-
-  // The old renderer, being slow, now updates the title. It should be filtered
-  // out and not take effect.
-  EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, ntp_rvh->rvh_state());
-  EXPECT_TRUE(ntp_rvh->OnMessageReceived(
-      ViewHostMsg_UpdateTitle(rvh()->GetRoutingID(), 0, ntp_title, direction)));
-  EXPECT_EQ(dest_title, contents()->GetTitle());
+  {
+    PluginFaviconMessageObserver observer(contents());
+    EXPECT_TRUE(
+        dest_rvh->OnMessageReceived(
+            ViewHostMsg_UpdateFaviconURL(rvh()->GetRoutingID(), 101, icons)));
+    EXPECT_TRUE(observer.favicon_received());
+  }
+
+  // The old renderer, being slow, now updates the favicon. It should be
+  // filtered out and not take effect.
+  EXPECT_TRUE(ntp_rvh->IsSwappedOut());
+  {
+    PluginFaviconMessageObserver observer(contents());
+    EXPECT_TRUE(
+        ntp_rvh->OnMessageReceived(
+            ViewHostMsg_UpdateFaviconURL(rvh()->GetRoutingID(), 101, icons)));
+    EXPECT_FALSE(observer.favicon_received());
+  }
+
+  // The same logic should apply to RenderFrameHosts as well and routing through
+  // swapped out RFH shouldn't be allowed. Use a PluginCrashObserver to check
+  // if the IPC message is allowed through or not.
+  {
+    PluginFaviconMessageObserver observer(contents());
+    // TODO(nasko): Check that the RFH is in swapped out when the state moves
+    // from RVH to RFH.
+    EXPECT_TRUE(ntp_rvh->main_render_frame_host()->OnMessageReceived(
+                    FrameHostMsg_PluginCrashed(
+                        main_rfh()->GetRoutingID(), base::FilePath(), 0)));
+    EXPECT_FALSE(observer.plugin_crashed());
+  }
 
   // We cannot filter out synchronous IPC messages, because the renderer would
   // be left waiting for a reply.  We pick RunBeforeUnloadConfirm as an example
@@ -813,9 +935,11 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) {
 
   // CrossSiteResourceHandler::StartCrossSiteTransition triggers a
   // call of RenderFrameHostManager::SwapOutOldPage before
-  // RenderFrameHostManager::DidNavigateFrame is called.
+  // RenderFrameHostManager::DidNavigateFrame is called. Since the previous
+  // navigation has already cause the renderer to start swapping out, there
+  // will be no more SwapOut messages being sent.
   manager->SwapOutOldPage();
-  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
+  EXPECT_FALSE(test_process_host->sink().GetUniqueMessageMatching(
       ViewMsg_SwapOut::ID));
   test_host->OnSwappedOut(false);
 
@@ -1029,7 +1153,7 @@ TEST_F(RenderFrameHostManagerTest, NavigateAfterMissingSwapOutACK) {
   EXPECT_TRUE(rvh2->is_waiting_for_beforeunload_ack());
   contents()->ProceedWithCrossSiteNavigation();
   EXPECT_FALSE(rvh2->is_waiting_for_beforeunload_ack());
-  rvh2->SwapOut();
+  StartCrossSiteTransition(contents());
   EXPECT_TRUE(rvh2->IsWaitingForUnloadACK());
 
   // The back navigation commits.
@@ -1040,6 +1164,7 @@ TEST_F(RenderFrameHostManagerTest, NavigateAfterMissingSwapOutACK) {
   // We should be able to navigate forward.
   contents()->GetController().GoForward();
   contents()->ProceedWithCrossSiteNavigation();
+  StartCrossSiteTransition(contents());
   const NavigationEntry* entry2 = contents()->GetController().GetPendingEntry();
   rvh2->SendNavigate(entry2->GetPageID(), entry2->GetURL());
   EXPECT_EQ(rvh2, rvh());
@@ -1347,28 +1472,6 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyClose) {
   EXPECT_EQ(host, manager->current_frame_host());
 }
 
-// This checks that the given RVH has been properly deleted.
-class RenderViewHostDestructionObserver : public WebContentsObserver {
- public:
-  RenderViewHostDestructionObserver(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(RenderViewHostDestructionObserver);
-};
-
 // Tests that the RenderViewHost is properly deleted when the SwapOutACK is
 // received before the new page commits.
 TEST_F(RenderFrameHostManagerTest,
@@ -1379,7 +1482,7 @@ TEST_F(RenderFrameHostManagerTest,
   // Navigate to the first page.
   contents()->NavigateAndCommit(kUrl1);
   TestRenderViewHost* rvh1 = test_rvh();
-  RenderViewHostDestructionObserver destruction_observer(rvh1);
+  RenderViewHostDeletedObserver rvh_deleted_observer(rvh1);
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh1->rvh_state());
 
   // Navigate to new site, simulating onbeforeunload approval.
@@ -1415,7 +1518,7 @@ TEST_F(RenderFrameHostManagerTest,
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh2->rvh_state());
 
   // rvh1 should have been deleted.
-  EXPECT_TRUE(destruction_observer.rvh_deleted());
+  EXPECT_TRUE(rvh_deleted_observer.deleted());
   rvh1 = NULL;
 }
 
@@ -1429,7 +1532,7 @@ TEST_F(RenderFrameHostManagerTest,
   // Navigate to the first page.
   contents()->NavigateAndCommit(kUrl1);
   TestRenderViewHost* rvh1 = test_rvh();
-  RenderViewHostDestructionObserver destruction_observer(rvh1);
+  RenderViewHostDeletedObserver rvh_deleted_observer(rvh1);
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh1->rvh_state());
 
   // Increment the number of active views in SiteInstanceImpl so that rvh2 is
@@ -1470,7 +1573,7 @@ TEST_F(RenderFrameHostManagerTest,
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh2->rvh_state());
 
   // rvh1 should be swapped out.
-  EXPECT_FALSE(destruction_observer.rvh_deleted());
+  EXPECT_FALSE(rvh_deleted_observer.deleted());
   EXPECT_TRUE(rvh1->IsSwappedOut());
 }
 
@@ -1484,7 +1587,7 @@ TEST_F(RenderFrameHostManagerTest,
   // Navigate to the first page.
   contents()->NavigateAndCommit(kUrl1);
   TestRenderViewHost* rvh1 = test_rvh();
-  RenderViewHostDestructionObserver destruction_observer(rvh1);
+  RenderViewHostDeletedObserver rvh_deleted_observer(rvh1);
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh1->rvh_state());
 
   // Navigate to new site, simulating onbeforeunload approval.
@@ -1520,7 +1623,7 @@ TEST_F(RenderFrameHostManagerTest,
   rvh1->OnSwappedOut(false);
 
   // rvh1 should have been deleted.
-  EXPECT_TRUE(destruction_observer.rvh_deleted());
+  EXPECT_TRUE(rvh_deleted_observer.deleted());
   rvh1 = NULL;
 }
 
@@ -1534,7 +1637,7 @@ TEST_F(RenderFrameHostManagerTest,
   // Navigate to the first page.
   contents()->NavigateAndCommit(kUrl1);
   TestRenderViewHost* rvh1 = test_rvh();
-  RenderViewHostDestructionObserver destruction_observer(rvh1);
+  RenderViewHostDeletedObserver rvh_deleted_observer(rvh1);
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh1->rvh_state());
 
   // Increment the number of active views in SiteInstanceImpl so that rvh1 is
@@ -1575,7 +1678,58 @@ TEST_F(RenderFrameHostManagerTest,
   rvh1->OnSwappedOut(false);
 
   // rvh1 should be swapped out.
-  EXPECT_FALSE(destruction_observer.rvh_deleted());
+  EXPECT_FALSE(rvh_deleted_observer.deleted());
+  EXPECT_TRUE(rvh1->IsSwappedOut());
+}
+
+// Test that the RenderViewHost is properly swapped out if navigation in the

[Charlie Reis, 2014/03/24 23:24:43]

a navigation

[nasko, 2014/03/25 18:30:22]

Done.

+// new renderer commits before sendint the SwapOut message to the old renderer.

[Charlie Reis, 2014/03/24 23:24:43]

sending

[nasko, 2014/03/25 18:30:22]

Done.

+// This simulates a cross-site navigation to a synchronously committing URL

[Charlie Reis, 2014/03/24 23:24:43]

a synchronously committing URL (e.g., a data URL)

[nasko, 2014/03/25 18:30:22]

Done.

+// and ensures it works properly.
+TEST_F(RenderFrameHostManagerTest,
+       CommitNewNavigationBeforeSendingSwapOut) {
+  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();
+  RenderViewHostDeletedObserver rvh_deleted_observer(rvh1);
+  EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh1->rvh_state());
+
+  // Increment the number of active views in SiteInstanceImpl so that rvh1 is
+  // not deleted on swap out.
+  static_cast<SiteInstanceImpl*>(
+      rvh1->GetSiteInstance())->increment_active_view_count();
+
+  // Navigate to new site, simulating onbeforeunload approval.
+  controller().LoadURL(kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string());
+  base::TimeTicks now = base::TimeTicks::Now();
+  main_test_rfh()->OnMessageReceived(
+      FrameHostMsg_BeforeUnload_ACK(0, true, now, now));
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderViewHost* rvh2 =
+      static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost());
+
+  // 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_DEFAULT, rvh2->rvh_state());
+  EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh1->rvh_state());
+
+  // Simulate the effect of calling SwapOut in OnCrossSiteResponse.

[Charlie Reis, 2014/03/24 23:24:43]

I thought we were testing the case without OnCrossSiteResponse.  Won't
DidNavigateFrame call SwapOut for us?

[nasko, 2014/03/25 18:30:22]

Done.

+  rvh1->SwapOut();
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT,
+            rvh1->rvh_state());
+
+  // Simulate the swap out ack.
+  rvh1->OnSwappedOut(false);
+
+  // rvh1 should be swapped out.
+  EXPECT_FALSE(rvh_deleted_observer.deleted());
   EXPECT_TRUE(rvh1->IsSwappedOut());
 }