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 plugi
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 u
nasko
2014/03/24 17:48:11
Does the NavigateWithEarlyReNavigation test case c
Charlie Reis
2014/03/24 20:38:02
Actually, that appears to be one of the few tests
|
+ 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 del
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()); |