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

content/browser/frame_host/render_frame_host_manager_unittest.cc

 // Copyright 2013 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 "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"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_manager.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/frame_messages.h"
 #include "content/common/view_messages.h"
@@ skipping 71 matching lines @@
   virtual void BeforeUnloadFired(WebContents* web_contents,
                                  bool proceed,
                                  bool* proceed_to_fire_unload) OVERRIDE {
     *proceed_to_fire_unload = proceed;
   }
 
  private:
   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
     : public RenderViewHostImplTestHarness {
  public:
   virtual void SetUp() OVERRIDE {
     RenderViewHostImplTestHarness::SetUp();
     WebUIControllerFactory::RegisterFactory(&factory_);
   }
 
   virtual void TearDown() OVERRIDE {
     RenderViewHostImplTestHarness::TearDown();
     WebUIControllerFactory::UnregisterFactoryForTesting(&factory_);
   }
 
   void set_should_create_webui(bool should_create_webui) {
     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
     // for us.
     controller().LoadURL(url, Referrer(), PAGE_TRANSITION_LINK, std::string());
     TestRenderViewHost* old_rvh = test_rvh();
 
     // 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,
                            const NavigationEntryImpl* current_entry,
                            const NavigationEntryImpl* new_entry) const {
     return manager->ShouldSwapBrowsingInstancesForNavigation(current_entry,
                                                              new_entry);
   }
 
   // Creates a test RenderViewHost that's swapped out.
@@ skipping 68 matching lines @@
   // The second one is the opposite, creating a cross-site transition and
   // requiring a beforeunload ack.
   contents2->GetController().LoadURL(
       kDestUrl, Referrer(), PAGE_TRANSITION_LINK, std::string());
   EXPECT_TRUE(contents2->cross_navigation_pending());
   TestRenderViewHost* dest_rvh2 = static_cast<TestRenderViewHost*>(
       contents2->GetRenderManagerForTesting()->pending_render_view_host());
   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.
   EXPECT_NE(active_rvh()->GetProcess(), dest_rvh2->GetProcess());
   EXPECT_NE(active_rvh()->GetSiteInstance(), dest_rvh2->GetSiteInstance());
   EXPECT_FALSE(active_rvh()->GetSiteInstance()->IsRelatedSiteInstance(
                    dest_rvh2->GetSiteInstance()));
 
   // Navigate both to the new tab page, and verify that they share a
   // RenderProcessHost (not a SiteInstance).
   NavigateActiveAndCommit(kChromeUrl);
 
   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());
   EXPECT_EQ(active_rvh()->GetSiteInstance()->GetProcess(),
             contents2->GetRenderViewHost()->GetSiteInstance()->GetProcess());
 }
 
 // Ensure that the browser ignores most IPC messages that arrive from a
 // RenderViewHost that has been swapped out.  We do not want to take
 // action on requests from a non-active renderer.  The main exception is
 // for synchronous messages, which cannot be ignored without leaving the
 // renderer in a stuck state.  See http://crbug.com/93427.
 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
   // that can run easily within a unit test, and that needs to receive a reply
   // without showing an actual dialog.
   MockRenderProcessHost* ntp_process_host =
       static_cast<MockRenderProcessHost*>(ntp_rvh->GetProcess());
   ntp_process_host->sink().ClearMessages();
   const base::string16 msg = base::ASCIIToUTF16("Message");
   bool result = false;
@@ skipping 706 matching lines @@
   static_cast<SiteInstanceImpl*>(rvh2->GetSiteInstance())->
       increment_active_view_count();
 
   // Now go back, but suppose the SwapOut_ACK isn't received.  This shouldn't
   // happen, but we have seen it when going back quickly across many entries
   // (http://crbug.com/93427).
   contents()->GetController().GoBack();
   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.
   const NavigationEntry* entry1 = contents()->GetController().GetPendingEntry();
   rvh1->SendNavigate(entry1->GetPageID(), entry1->GetURL());
   EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh2->rvh_state());
 
   // 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());
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh2->rvh_state());
   EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh1->rvh_state());
   rvh1->OnSwappedOut(false);
   EXPECT_TRUE(rvh1->IsSwappedOut());
 }
 
 // Test that we create swapped out RVHs for the opener chain when navigating an
@@ skipping 520 matching lines @@
 
   // Simulate the swap out ack.
   rvh1->OnSwappedOut(false);
 
   // rvh1 should be swapped out.
   EXPECT_FALSE(destruction_observer.rvh_deleted());
   EXPECT_TRUE(rvh1->IsSwappedOut());
 }
 
 }  // namespace content