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 2e7ad3fca74c2fda31c0c9982198e0c0386094ac..90ef61fe956daf4a4d246f1bcead45024f05d690 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,68 @@ 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(WebContents* web_contents)
+      : WebContentsObserver(web_contents),
+        last_deleted_(NULL) {}
+
+  virtual void RenderViewDeleted(RenderViewHost* render_view_host) OVERRIDE {
+    last_deleted_ = render_view_host;
+  }
+
+  RenderViewHost* last_deleted() {
+    return last_deleted_;
+  }
+
+ private:
+  RenderViewHost* last_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 IpcMessageObserver : public WebContentsObserver {

[Charlie Reis, 2014/03/21 21:35:13]

This sounds like it's more general than just plugins and favicons.  Maybe
something like PluginFaviconMessageObserver?

[nasko, 2014/03/24 17:48:11]

Done.

+ public:
+  IpcMessageObserver(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(IpcMessageObserver);
+};
+
+
 }  // namespace
 
 class RenderFrameHostManagerTest
@@ -112,6 +175,20 @@ class RenderFrameHostManagerTest
     factory_.set_should_create_webui(should_create_webui);
   }
 
+  void StartCrossSiteTransition(TestWebContents* contents) {

[Charlie Reis, 2014/03/21 21:35:13]

Great, I'm a big fan of making most of the tests use this path instead of the
shortcut in OnSwappedOut.

Do we have any remaining tests for the OnSwappedOut path (i.e., without
OnCrossSiteResponse)?  If not, can we add one to make sure it's covered?

[nasko, 2014/03/24 17:48:11]

Does the NavigateWithEarlyReNavigation test case count? I've left it using the
old way, since getting this test right using OnCrossSiteResponse wasn't trivial.

[Charlie Reis, 2014/03/24 20:38:02]

Actually, that appears to be one of the few tests that doesn't hit the
SwapOutOldPage call in DidNavigateMainFrame.  I just locally changed
DidNavigateMainFrame to have NOTREACHED instead and ran all the
RenderFrameHostManagerTest.* tests, and that one didn't fail.

If you try the same thing with this CL in place, do we hit still hit the
NOTREACHED somewhere?

+    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,

[Charlie Reis, 2014/03/21 21:35:13]

Great, glad to see these sorts of checks.

[nasko, 2014/03/24 17:48:11]

Done.

+              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 +198,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(contents());

[Charlie Reis, 2014/03/21 21:35:13]

I'm not thrilled with keeping the pointer to a deleted object and comparing it
later, even if we don't dereference it.

Here's one alternative: have the RenderViewHostDeletedObserver take in old_rvh
and store the routing ID and process ID, plus a bool for whether it has been
deleted.  When the deletion occurs, flip the bool.  You can then check the bool
below.

That has the added advantage of not getting fooled if two RVHs get deleted in a
row.

[nasko, 2014/03/24 17:48:11]

Done.

     active_test_rvh()->SendNavigate(max_page_id + 1, url);
+
+    if (old_rvh != active_rvh() && old_rvh != rvh_observer.last_deleted())
+      EXPECT_TRUE(old_rvh->IsSwappedOut());
   }
 
   bool ShouldSwapProcesses(RenderFrameHostManager* manager,
@@ -222,7 +314,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 +330,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 +348,71 @@ 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);
-
+  {
+    IpcMessageObserver 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());
+  {
+    IpcMessageObserver 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 title. It should be filtered

[Charlie Reis, 2014/03/21 21:35:13]

s/title/favicon/

[nasko, 2014/03/24 17:48:11]

Done.

   // 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());
+  EXPECT_TRUE(ntp_rvh->IsSwappedOut());
+  {
+    IpcMessageObserver 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.
+  {
+    IpcMessageObserver observer(contents());
+    // TODO(nasko): The following line shouldn't be needed when we are properly
+    // swapping out RFH objects or move to proxy objects.
+    //ntp_rvh->main_render_frame_host()->set_swapped_out(true);
+    //EXPECT_TRUE(ntp_rvh->main_render_frame_host()->is_swapped_out());

[Charlie Reis, 2014/03/21 21:35:13]

Let's remove these lines until they're needed.

[nasko, 2014/03/24 17:48:11]

Done.

+    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
@@ -1029,7 +1139,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 +1150,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());