From 5ac402d1ed5d323a9ea9223c2acf213b5dfc9aaa Mon Sep 17 00:00:00 2001 From: alexmos Date: Thu, 9 Jul 2015 00:51:10 -0700 Subject: [PATCH] Move browser-to-renderer opener plumbing to frame routing IDs. This CL allows a new RenderView to set its main frame's opener to any frame, rather than a top-level frame. I.e., it is now possible to use window.opener to refer to a cross-process subframe. Summary of changes: * The ViewMsg_New IPC now passes the opener's frame ID rather than view ID. RenderView::Initialize resolves this into the opener's WebFrame. * The flow of opener routing IDs from RFHM::InitRenderView to RenderViewHostImpl::CreateRenderView (which sends the IPC) is moved from view to frame IDs. * The passing of opener routing IDs from CreateOpenerProxies, etc., into RFHM::InitRenderView is removed. Instead, InitRenderView now looks up the proper opener frame routing ID itself. This is because (1) the previous flow would complicate implementing opener updates, which would allow subframes to have openers and FrameTrees to have more than one opener to traverse, and (2) previous plumbing missed some cases (e.g., creating swapped-out RVs via CreateRenderFrameProxy or CreateProxiesForSiteInstance), and fixing this would add more complexity. * There are paths where the opener chain is intentionally suppressed from a new RenderView, for example for : see WCI::CreateSwappedOutRenderView and EnsureOpenerProxiesExist. This is now supported via an explicit flag which is passed into InitRenderView rather than passing around MSG_ROUTING_NONE for the opener's routing ID. BUG=225940, 304341 Review URL: https://codereview.chromium.org/1202593002 Cr-Commit-Position: refs/heads/master@{#337995} --- content/browser/frame_host/frame_tree.cc | 4 +- .../frame_host/render_frame_host_manager.cc | 110 ++++++++++----------- .../browser/frame_host/render_frame_host_manager.h | 45 ++++----- .../render_frame_host_manager_unittest.cc | 6 +- .../browser/renderer_host/render_view_host_impl.cc | 4 +- .../browser/renderer_host/render_view_host_impl.h | 2 +- content/browser/site_per_process_browsertest.cc | 37 +++++-- content/browser/web_contents/web_contents_impl.cc | 14 ++- content/browser/web_contents/web_contents_impl.h | 2 +- .../web_contents/web_contents_impl_unittest.cc | 23 +++-- content/common/view_messages.h | 6 +- content/public/test/render_view_test.cc | 3 +- content/public/test/test_renderer_host.h | 11 +-- content/renderer/render_view_impl.cc | 62 ++++++++++-- content/test/data/post_message.html | 6 ++ content/test/test_render_view_host.cc | 10 +- content/test/test_render_view_host.h | 12 +-- content/test/test_web_contents.cc | 11 +-- content/test/test_web_contents.h | 2 +- 19 files changed, 218 insertions(+), 152 deletions(-) diff --git a/content/browser/frame_host/frame_tree.cc b/content/browser/frame_host/frame_tree.cc index 680766435dbd..2bc147dc27d4 100644 --- a/content/browser/frame_host/frame_tree.cc +++ b/content/browser/frame_host/frame_tree.cc @@ -231,8 +231,8 @@ void FrameTree::CreateProxiesForSiteInstance( root()->render_manager()->CreateRenderFrameProxy(site_instance); } else { root()->render_manager()->CreateRenderFrame( - site_instance, nullptr, MSG_ROUTING_NONE, - CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN, nullptr); + site_instance, nullptr, CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN, + nullptr); } } else { root()->render_manager()->EnsureRenderViewInitialized(render_view_host, diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc index 19d68be77813..d12415dd7c93 100644 --- a/content/browser/frame_host/render_frame_host_manager.cc +++ b/content/browser/frame_host/render_frame_host_manager.cc @@ -277,10 +277,8 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate( dest_render_frame_host->SetUpMojoIfNeeded(); // Recreate the opener chain. - int opener_route_id = - CreateOpenerProxiesIfNeeded(dest_render_frame_host->GetSiteInstance()); + CreateOpenerProxiesIfNeeded(dest_render_frame_host->GetSiteInstance()); if (!InitRenderView(dest_render_frame_host->render_view_host(), - opener_route_id, MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame())) return NULL; @@ -873,10 +871,9 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation( // created or has crashed), initialize it. if (!navigation_rfh->IsRenderFrameLive()) { // Recreate the opener chain. - int opener_route_id = - CreateOpenerProxiesIfNeeded(navigation_rfh->GetSiteInstance()); - if (!InitRenderView(navigation_rfh->render_view_host(), opener_route_id, - MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame())) { + CreateOpenerProxiesIfNeeded(navigation_rfh->GetSiteInstance()); + if (!InitRenderView(navigation_rfh->render_view_host(), MSG_ROUTING_NONE, + frame_tree_node_->IsMainFrame())) { return nullptr; } @@ -1409,23 +1406,21 @@ void RenderFrameHostManager::CreatePendingRenderFrameHost( if (!new_instance->GetProcess()->Init()) return; - int opener_route_id = CreateProxiesForNewRenderFrameHost( - old_instance, new_instance, &create_render_frame_flags); + CreateProxiesForNewRenderFrameHost(old_instance, new_instance, + &create_render_frame_flags); // Create a non-swapped-out RFH with the given opener. - pending_render_frame_host_ = - CreateRenderFrame(new_instance, pending_web_ui(), opener_route_id, - create_render_frame_flags, nullptr); + pending_render_frame_host_ = CreateRenderFrame( + new_instance, pending_web_ui(), create_render_frame_flags, nullptr); } -int RenderFrameHostManager::CreateProxiesForNewRenderFrameHost( +void RenderFrameHostManager::CreateProxiesForNewRenderFrameHost( SiteInstance* old_instance, SiteInstance* new_instance, int* create_render_frame_flags) { - int opener_route_id = MSG_ROUTING_NONE; // Only create opener proxies if they are in the same BrowsingInstance. if (new_instance->IsRelatedSiteInstance(old_instance)) - opener_route_id = CreateOpenerProxiesIfNeeded(new_instance); + CreateOpenerProxiesIfNeeded(new_instance); if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kSitePerProcess)) { // Ensure that the frame tree has RenderFrameProxyHosts for the new @@ -1443,7 +1438,6 @@ int RenderFrameHostManager::CreateProxiesForNewRenderFrameHost( new_instance) *create_render_frame_flags |= CREATE_RF_NEEDS_RENDER_WIDGET_HOST; } - return opener_route_id; } scoped_ptr RenderFrameHostManager::CreateRenderFrameHost( @@ -1501,8 +1495,8 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost( return false; int create_render_frame_flags = 0; - int opener_route_id = CreateProxiesForNewRenderFrameHost( - old_instance, new_instance, &create_render_frame_flags); + CreateProxiesForNewRenderFrameHost(old_instance, new_instance, + &create_render_frame_flags); if (frame_tree_node_->IsMainFrame()) create_render_frame_flags |= CREATE_RF_FOR_MAIN_FRAME_NAVIGATION; @@ -1510,7 +1504,7 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost( create_render_frame_flags |= CREATE_RF_HIDDEN; speculative_render_frame_host_ = CreateRenderFrame(new_instance, speculative_web_ui_.get(), - opener_route_id, create_render_frame_flags, nullptr); + create_render_frame_flags, nullptr); if (!speculative_render_frame_host_) { speculative_web_ui_.reset(); @@ -1522,7 +1516,6 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost( scoped_ptr RenderFrameHostManager::CreateRenderFrame( SiteInstance* instance, WebUIImpl* web_ui, - int opener_route_id, int flags, int* view_routing_id_ptr) { bool swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT); @@ -1584,9 +1577,8 @@ scoped_ptr RenderFrameHostManager::CreateRenderFrame( proxy->TakeFrameHostOwnership(new_render_frame_host.Pass()); } - success = - InitRenderView(render_view_host, opener_route_id, proxy_routing_id, - !!(flags & CREATE_RF_FOR_MAIN_FRAME_NAVIGATION)); + success = InitRenderView(render_view_host, proxy_routing_id, + !!(flags & CREATE_RF_FOR_MAIN_FRAME_NAVIGATION)); if (success) { // Remember that InitRenderView also created the RenderFrameProxy. if (swapped_out) @@ -1671,8 +1663,7 @@ int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) { if (RenderFrameHostManager::IsSwappedOutStateForbidden() && frame_tree_node_->IsMainFrame()) { - InitRenderView( - render_view_host, MSG_ROUTING_NONE, proxy->GetRoutingID(), true); + InitRenderView(render_view_host, proxy->GetRoutingID(), true); proxy->set_render_frame_proxy_created(true); } else { proxy->InitRenderFrameProxy(); @@ -1697,7 +1688,7 @@ void RenderFrameHostManager::EnsureRenderViewInitialized( return; // Recreate the opener chain. - int opener_route_id = CreateOpenerProxiesIfNeeded(instance); + CreateOpenerProxiesIfNeeded(instance); // If the proxy in |instance| doesn't exist, this RenderView is not swapped // out and shouldn't be reinitialized here. @@ -1705,8 +1696,7 @@ void RenderFrameHostManager::EnsureRenderViewInitialized( if (!proxy) return; - InitRenderView(render_view_host, opener_route_id, proxy->GetRoutingID(), - false); + InitRenderView(render_view_host, proxy->GetRoutingID(), false); proxy->set_render_frame_proxy_created(true); } @@ -1741,7 +1731,6 @@ void RenderFrameHostManager::SetRWHViewForInnerContents( bool RenderFrameHostManager::InitRenderView( RenderViewHostImpl* render_view_host, - int opener_route_id, int proxy_routing_id, bool for_main_frame_navigation) { // Ensure the renderer process is initialized before creating the @@ -1775,12 +1764,12 @@ bool RenderFrameHostManager::InitRenderView( } } + int opener_frame_routing_id = + GetOpenerRoutingID(render_view_host->GetSiteInstance()); + return delegate_->CreateRenderViewForRenderManager( - render_view_host, - opener_route_id, - proxy_routing_id, - frame_tree_node_->current_replication_state(), - for_main_frame_navigation); + render_view_host, opener_frame_routing_id, proxy_routing_id, + frame_tree_node_->current_replication_state(), for_main_frame_navigation); } bool RenderFrameHostManager::InitRenderFrame( @@ -1836,10 +1825,17 @@ int RenderFrameHostManager::GetRoutingIdForSiteInstance( if (render_frame_host_->GetSiteInstance() == site_instance) return render_frame_host_->GetRoutingID(); - RenderFrameProxyHostMap::iterator iter = - proxy_hosts_.find(site_instance->GetId()); - if (iter != proxy_hosts_.end()) - return iter->second->GetRoutingID(); + // If there is a matching pending RFH, only return it if swapped out is + // allowed, since otherwise there should be a proxy that should be used + // instead. + if (pending_render_frame_host_ && + pending_render_frame_host_->GetSiteInstance() == site_instance && + !RenderFrameHostManager::IsSwappedOutStateForbidden()) + return pending_render_frame_host_->GetRoutingID(); + + RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(site_instance); + if (proxy) + return proxy->GetRoutingID(); return MSG_ROUTING_NONE; } @@ -2282,19 +2278,17 @@ void RenderFrameHostManager::DeleteRenderFrameProxyHost( } } -int RenderFrameHostManager::CreateOpenerProxiesIfNeeded( +void RenderFrameHostManager::CreateOpenerProxiesIfNeeded( SiteInstance* instance) { FrameTreeNode* opener = frame_tree_node_->opener(); if (!opener) - return MSG_ROUTING_NONE; + return; // Create proxies for the opener chain. - return opener->render_manager()->CreateOpenerProxies(instance); + opener->render_manager()->CreateOpenerProxies(instance); } -int RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) { - int opener_route_id = MSG_ROUTING_NONE; - +void RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) { // If this tab has an opener, recursively create proxies for the nodes on its // frame tree. // TODO(alexmos): Once we allow frame openers to be updated (which can happen @@ -2304,39 +2298,43 @@ int RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) { // be traversed). FrameTreeNode* opener = frame_tree_node_->opener(); if (opener) - opener_route_id = opener->render_manager()->CreateOpenerProxies(instance); + opener->render_manager()->CreateOpenerProxies(instance); // If any of the RenderViews (current, pending, or swapped out) for this - // FrameTree has the same SiteInstance, use it. If such a RenderView exists, - // that implies that proxies for all nodes in the tree should also exist - // (when in site-per-process mode), so it is safe to exit early. + // FrameTree has the same SiteInstance, then we can return early, since + // proxies for other nodes in the tree should also exist (when in + // site-per-process mode). FrameTree* frame_tree = frame_tree_node_->frame_tree(); RenderViewHostImpl* rvh = frame_tree->GetRenderViewHost(instance); if (rvh && rvh->IsRenderViewLive()) - return rvh->GetRoutingID(); + return; - int render_view_routing_id = MSG_ROUTING_NONE; if (RenderFrameHostManager::IsSwappedOutStateForbidden()) { // Ensure that all the nodes in the opener's frame tree have // RenderFrameProxyHosts for the new SiteInstance. frame_tree->CreateProxiesForSiteInstance(nullptr, instance); - rvh = frame_tree->GetRenderViewHost(instance); - render_view_routing_id = rvh->GetRoutingID(); } else if (rvh && !rvh->IsRenderViewLive()) { EnsureRenderViewInitialized(rvh, instance); - render_view_routing_id = rvh->GetRoutingID(); } else { // Create a swapped out RenderView in the given SiteInstance if none exists, // setting its opener to the given route_id. Since the opener can point to // a subframe, do this on the root frame of the opener's frame tree. // Return the new view's route_id. frame_tree->root()->render_manager()-> - CreateRenderFrame(instance, nullptr, opener_route_id, + CreateRenderFrame(instance, nullptr, CREATE_RF_FOR_MAIN_FRAME_NAVIGATION | CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN, - &render_view_routing_id); + nullptr); } - return render_view_routing_id; +} + +int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) { + if (!frame_tree_node_->opener()) + return MSG_ROUTING_NONE; + + return frame_tree_node_->opener() + ->render_manager() + ->GetRoutingIdForSiteInstance(instance); } } // namespace content diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h index 7e1f86af5ec0..9c96338c1698 100644 --- a/content/browser/frame_host/render_frame_host_manager.h +++ b/content/browser/frame_host/render_frame_host_manager.h @@ -121,7 +121,7 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { // appropriate RenderWidgetHostView type is used. virtual bool CreateRenderViewForRenderManager( RenderViewHost* render_view_host, - int opener_route_id, + int opener_frame_routing_id, int proxy_routing_id, const FrameReplicationState& replicated_frame_state, bool for_main_frame_navigation) = 0; @@ -353,7 +353,6 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { // the frame. scoped_ptr CreateRenderFrame(SiteInstance* instance, WebUIImpl* web_ui, - int opener_route_id, int flags, int* view_routing_id_ptr); @@ -414,7 +413,7 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { // of WebContentsImpl. void ResetProxyHosts(); - // Returns the routing id for a RenderFrameHost or RenderFrameHostProxy + // Returns the routing id for a RenderFrameHost or RenderFrameProxyHost // that has the given SiteInstance and is associated with this // RenderFrameHostManager. Returns MSG_ROUTING_NONE if none is found. int GetRoutingIdForSiteInstance(SiteInstance* site_instance); @@ -462,10 +461,15 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { // this frame's FrameTree and for its opener chain in the given SiteInstance. // This allows other tabs to send cross-process JavaScript calls to their // opener(s) and to any other frames in the opener's FrameTree (e.g., - // supporting calls like window.opener.opener.frames[x][y]). Returns the - // route ID of this frame's RenderView for |instance|. - // TODO(alexmos): Switch this to return RenderFrame routing IDs. - int CreateOpenerProxies(SiteInstance* instance); + // supporting calls like window.opener.opener.frames[x][y]). + void CreateOpenerProxies(SiteInstance* instance); + + // Returns a routing ID for the current FrameTreeNode's opener node in the + // given SiteInstance. May return a routing ID of either a RenderFrameHost + // (if opener's current or pending RFH has SiteInstance |instance|) or a + // RenderFrameProxyHost. Returns MSG_ROUTING_NONE if there is no opener, or + // if the opener node doesn't have a proxy for |instance|. + int GetOpenerRoutingID(SiteInstance* instance); // Called on the RFHM of the inner WebContents to create a // RenderFrameProxyHost in its outer WebContents's SiteInstance, @@ -611,19 +615,15 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { // (2) Create proxies for the new RFH's SiteInstance in its own frame tree; // (3) set any additional flags for the new RenderFrame with // |create_render_frame_flags|. - // Returns the opener's RVH route ID to be used for the new RenderFrame. - // TODO(alexmos): switch this to return opener's RFH routing ID instead. - int CreateProxiesForNewRenderFrameHost(SiteInstance* old_instance, - SiteInstance* new_instance, - int* create_render_frame_flags); + void CreateProxiesForNewRenderFrameHost(SiteInstance* old_instance, + SiteInstance* new_instance, + int* create_render_frame_flags); // Create swapped out RenderViews and RenderFrameProxies in the given // SiteInstance for all frames on the opener chain of this frame. Same as - // CreateOpenerProxies, but starts from this frame's opener, returning - // MSG_ROUTING_NONE if it doesn't exist, and calling CreateOpenerProxies if - // it does. - // TODO(alexmos): Switch this to return RenderFrame routing IDs. - int CreateOpenerProxiesIfNeeded(SiteInstance* instance); + // CreateOpenerProxies, but starts from this frame's opener, calling + // CreateOpenerProxies on it if it exists and returning otherwise. + void CreateOpenerProxiesIfNeeded(SiteInstance* instance); // Creates a RenderFrameHost and corresponding RenderViewHost if necessary. scoped_ptr CreateRenderFrameHost(SiteInstance* instance, @@ -640,15 +640,12 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { SiteInstance* new_instance, int bindings); - // Sets up the necessary state for a new RenderViewHost with the given opener, - // if necessary. It creates a RenderFrameProxy in the target renderer process - // with the given |proxy_routing_id|, which is used to route IPC messages when - // in swapped out state. Returns early if the RenderViewHost has already been + // Sets up the necessary state for a new RenderViewHost. Creates a + // RenderFrameProxy in the target renderer process with the given + // |proxy_routing_id|, which is used to route IPC messages when in swapped + // out state. Returns early if the RenderViewHost has already been // initialized for another RenderFrameHost. - // TODO(creis): opener_route_id is currently for the RenderViewHost but should - // be for the RenderFrame, since frames can have openers. bool InitRenderView(RenderViewHostImpl* render_view_host, - int opener_route_id, int proxy_routing_id, bool for_main_frame_navigation); 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 7fc11299e538..ad04a4aaea10 100644 --- a/content/browser/frame_host/render_frame_host_manager_unittest.cc +++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc @@ -1654,9 +1654,9 @@ TEST_F(RenderFrameHostManagerTest, CleanUpSwappedOutRVHOnProcessCrash) { EXPECT_TRUE( opener1_manager->GetSwappedOutRenderViewHost(rfh1->GetSiteInstance()) ->IsRenderViewLive()); - EXPECT_EQ(opener1_manager->GetSwappedOutRenderViewHost( - rfh1->GetSiteInstance())->GetRoutingID(), - contents()->GetMainFrame()->GetRenderViewHost()->opener_route_id()); + EXPECT_EQ( + opener1_manager->GetRoutingIdForSiteInstance(rfh1->GetSiteInstance()), + contents()->GetMainFrame()->GetRenderViewHost()->opener_frame_route_id()); } // Test that RenderViewHosts created for WebUI navigations are properly diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index 08e220bde732..c54e4e9dce2b 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc @@ -274,7 +274,7 @@ SiteInstanceImpl* RenderViewHostImpl::GetSiteInstance() const { } bool RenderViewHostImpl::CreateRenderView( - int opener_route_id, + int opener_frame_route_id, int proxy_route_id, int32 max_page_id, const FrameReplicationState& replicated_frame_state, @@ -316,7 +316,7 @@ bool RenderViewHostImpl::CreateRenderView( params.session_storage_namespace_id = delegate_->GetSessionStorageNamespace(instance_.get())->id(); // Ensure the RenderView sets its opener correctly. - params.opener_route_id = opener_route_id; + params.opener_frame_route_id = opener_frame_route_id; params.swapped_out = !is_active_; params.replicated_frame_state = replicated_frame_state; params.proxy_routing_id = proxy_route_id; diff --git a/content/browser/renderer_host/render_view_host_impl.h b/content/browser/renderer_host/render_view_host_impl.h index 6e14b189003d..3eae4c386217 100644 --- a/content/browser/renderer_host/render_view_host_impl.h +++ b/content/browser/renderer_host/render_view_host_impl.h @@ -204,7 +204,7 @@ class CONTENT_EXPORT RenderViewHostImpl // state. |replicated_frame_state| contains replicated data for the // top-level frame, such as its name and sandbox flags. virtual bool CreateRenderView( - int opener_route_id, + int opener_frame_route_id, int proxy_route_id, int32 max_page_id, const FrameReplicationState& replicated_frame_state, diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc index ca97ea2a4e94..c5514f65877f 100644 --- a/content/browser/site_per_process_browsertest.cc +++ b/content/browser/site_per_process_browsertest.cc @@ -2542,17 +2542,36 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, PostMessageAndWaitForReply(root->child_at(0), "postToPopup('subframe-msg')", "\"done-subframe1\""); - // TODO(alexmos): Once we propagate subframe opener information to new - // RenderViews, try sending a postMessage from the popup to window.opener and - // ensure that it reaches subframe1. - - // Verify the total number of received messages for each subframe. First - // subframe and popup should have one message each, and other frames - // shouldn't have received any messages. + // Send a postMessage from the popup to window.opener and ensure that it + // reaches subframe1. This verifies that the subframe opener information + // propagated to the popup's RenderFrame. Wait for subframe1 to send a reply + // message to the popup. + EXPECT_TRUE(ExecuteScript(popup->web_contents(), "window.name = 'popup';")); + PostMessageAndWaitForReply(popup_root, "postToOpener('subframe-msg', '*')", + "\"done-popup\""); + + // Second a postMessage from popup2 to window.opener.opener, which should + // resolve to subframe1. This tests opener chains of length greater than 1. + // As before, subframe1 will send a reply to popup2. + FrameTreeNode* popup2_root = + static_cast(popup2->web_contents()) + ->GetFrameTree() + ->root(); + EXPECT_TRUE(ExecuteScript(popup2->web_contents(), "window.name = 'popup2';")); + PostMessageAndWaitForReply(popup2_root, + "postToOpenerOfOpener('subframe-msg', '*')", + "\"done-popup2\""); + + // Verify the total number of received messages for each subframe: + // - 3 for first subframe (two from first popup, one from second popup) + // - 2 for popup (both from first subframe) + // - 1 for popup2 (reply from first subframe) + // - 0 for other frames EXPECT_EQ(0, GetReceivedMessages(root)); - EXPECT_EQ(1, GetReceivedMessages(root->child_at(0))); + EXPECT_EQ(3, GetReceivedMessages(root->child_at(0))); EXPECT_EQ(0, GetReceivedMessages(root->child_at(1))); - EXPECT_EQ(1, GetReceivedMessages(popup_root)); + EXPECT_EQ(2, GetReceivedMessages(popup_root)); + EXPECT_EQ(1, GetReceivedMessages(popup2_root)); } // Check that parent.frames[num] references correct sibling frames when the diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index 361643010c43..00e524c44865 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc @@ -4042,7 +4042,7 @@ int WebContentsImpl::CreateSwappedOutRenderView( GetRenderManager()->CreateRenderFrameProxy(instance); } else { GetRenderManager()->CreateRenderFrame( - instance, nullptr, MSG_ROUTING_NONE, + instance, nullptr, CREATE_RF_SWAPPED_OUT | CREATE_RF_FOR_MAIN_FRAME_NAVIGATION | CREATE_RF_HIDDEN, &render_view_routing_id); @@ -4218,7 +4218,7 @@ NavigationEntry* bool WebContentsImpl::CreateRenderViewForRenderManager( RenderViewHost* render_view_host, - int opener_route_id, + int opener_frame_routing_id, int proxy_routing_id, const FrameReplicationState& replicated_frame_state, bool for_main_frame_navigation) { @@ -4251,12 +4251,10 @@ bool WebContentsImpl::CreateRenderViewForRenderManager( int32 max_page_id = GetMaxPageIDForSiteInstance(render_view_host->GetSiteInstance()); - if (!static_cast( - render_view_host)->CreateRenderView(opener_route_id, - proxy_routing_id, - max_page_id, - replicated_frame_state, - created_with_opener_)) { + if (!static_cast(render_view_host) + ->CreateRenderView(opener_frame_routing_id, proxy_routing_id, + max_page_id, replicated_frame_state, + created_with_opener_)) { return false; } diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h index f580046a189a..0ebb78bdd454 100644 --- a/content/browser/web_contents/web_contents_impl.h +++ b/content/browser/web_contents/web_contents_impl.h @@ -578,7 +578,7 @@ class CONTENT_EXPORT WebContentsImpl bool CreateRenderViewForRenderManager( RenderViewHost* render_view_host, - int opener_route_id, + int opener_frame_routing_id, int proxy_routing_id, const FrameReplicationState& replicated_frame_state, bool for_main_frame_navigation) override; diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc index 52e6eccf9d04..5e419f078163 100644 --- a/content/browser/web_contents/web_contents_impl_unittest.cc +++ b/content/browser/web_contents/web_contents_impl_unittest.cc @@ -912,16 +912,27 @@ TEST_F(WebContentsImplTest, FindOpenerRVHWhenPending) { url2, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string()); orig_rfh->PrepareForCommit(); TestRenderFrameHost* pending_rfh = contents()->GetPendingMainFrame(); + SiteInstance* instance = pending_rfh->GetSiteInstance(); // While it is still pending, simulate opening a new tab with the first tab // as its opener. This will call CreateOpenerProxies on the opener to ensure // that an RVH exists. - int opener_routing_id = contents()->GetRenderManager()->CreateOpenerProxies( - pending_rfh->GetSiteInstance()); - - // We should find the pending RVH and not create a new one. - EXPECT_EQ(pending_rfh->GetRenderViewHost()->GetRoutingID(), - opener_routing_id); + scoped_ptr popup( + TestWebContents::Create(browser_context(), instance)); + popup->SetOpener(contents()); + contents()->GetRenderManager()->CreateOpenerProxies(instance); + + // If swapped out is allowed, we should find the pending RFH and not create a + // new one. Otherwise, a new proxy should be created for the opener in + // |instance|, and we should ensure that its routing ID is returned here. + // + // TODO(alexmos): Add the latter check once CreateOpenerProxies is fixed to + // create a proxy in this case. + if (!RenderFrameHostManager::IsSwappedOutStateForbidden()) { + int opener_frame_routing_id = + popup->GetRenderManager()->GetOpenerRoutingID(instance); + EXPECT_EQ(pending_rfh->GetRoutingID(), opener_frame_routing_id); + } } // Tests that WebContentsImpl uses the current URL, not the SiteInstance's site, diff --git a/content/common/view_messages.h b/content/common/view_messages.h index 7433e50ca2fc..433f04ad558f 100644 --- a/content/common/view_messages.h +++ b/content/common/view_messages.h @@ -501,9 +501,9 @@ IPC_STRUCT_BEGIN(ViewMsg_New_Params) // The session storage namespace ID this view should use. IPC_STRUCT_MEMBER(int64, session_storage_namespace_id) - // The route ID of the opener RenderView if we need to set one - // (MSG_ROUTING_NONE otherwise). - IPC_STRUCT_MEMBER(int, opener_route_id) + // The route ID of the opener RenderFrame or RenderFrameProxy, if we need to + // set one (MSG_ROUTING_NONE otherwise). + IPC_STRUCT_MEMBER(int, opener_frame_route_id) // Whether the RenderView should initially be swapped out. IPC_STRUCT_MEMBER(bool, swapped_out) diff --git a/content/public/test/render_view_test.cc b/content/public/test/render_view_test.cc index e6eef8c3043c..52ea9c2bd028 100644 --- a/content/public/test/render_view_test.cc +++ b/content/public/test/render_view_test.cc @@ -58,7 +58,6 @@ using blink::WebURLRequest; namespace { -const int32 kOpenerId = -2; const int32 kRouteId = 5; const int32 kMainFrameRouteId = 6; const int32 kNewWindowRouteId = 7; @@ -237,7 +236,7 @@ void RenderViewTest::SetUp() { mock_process_.reset(new MockRenderProcess); ViewMsg_New_Params view_params; - view_params.opener_route_id = kOpenerId; + view_params.opener_frame_route_id = MSG_ROUTING_NONE; view_params.window_was_created_with_opener = false; view_params.renderer_preferences = RendererPreferences(); view_params.web_preferences = WebPreferences(); diff --git a/content/public/test/test_renderer_host.h b/content/public/test/test_renderer_host.h index fc12cbebf659..c429876fc5b3 100644 --- a/content/public/test/test_renderer_host.h +++ b/content/public/test/test_renderer_host.h @@ -132,12 +132,11 @@ class RenderViewHostTester { virtual ~RenderViewHostTester() {} // Gives tests access to RenderViewHostImpl::CreateRenderView. - virtual bool CreateTestRenderView( - const base::string16& frame_name, - int opener_route_id, - int proxy_routing_id, - int32 max_page_id, - bool created_with_opener) = 0; + virtual bool CreateTestRenderView(const base::string16& frame_name, + int opener_frame_route_id, + int proxy_routing_id, + int32 max_page_id, + bool created_with_opener) = 0; // Makes the WasHidden/WasShown calls to the RenderWidget that // tell it it has been hidden or restored from having been hidden. diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc index ce57049fa60e..3fb5b008e1fa 100644 --- a/content/renderer/render_view_impl.cc +++ b/content/renderer/render_view_impl.cc @@ -611,6 +611,42 @@ void ApplyBlinkSettings(const base::CommandLine& command_line, } } +// Looks up and returns the WebFrame corresponding to a given opener frame +// routing ID. Also stores the opener's RenderView routing ID into +// |opener_view_routing_id|. +WebFrame* ResolveOpener(int opener_frame_routing_id, + int* opener_view_routing_id) { + *opener_view_routing_id = MSG_ROUTING_NONE; + if (opener_frame_routing_id == MSG_ROUTING_NONE) + return nullptr; + + // Opener routing ID could refer to either a RenderFrameProxy or a + // RenderFrame, so need to check both. + RenderFrameProxy* opener_proxy = + RenderFrameProxy::FromRoutingID(opener_frame_routing_id); + if (opener_proxy) { + *opener_view_routing_id = opener_proxy->render_view()->GetRoutingID(); + + // TODO(nasko,alexmos): This check won't be needed once swapped-out:// is + // gone. + if (opener_proxy->IsMainFrameDetachedFromTree()) { + DCHECK(!RenderFrameProxy::IsSwappedOutStateForbidden()); + return opener_proxy->render_view()->webview()->mainFrame(); + } else { + return opener_proxy->web_frame(); + } + } + + RenderFrameImpl* opener_frame = + RenderFrameImpl::FromRoutingID(opener_frame_routing_id); + if (opener_frame) { + *opener_view_routing_id = opener_frame->render_view()->GetRoutingID(); + return opener_frame->GetWebFrame(); + } + + return nullptr; +} + } // namespace RenderViewImpl::RenderViewImpl(const ViewMsg_New_Params& params) @@ -662,8 +698,13 @@ void RenderViewImpl::Initialize(const ViewMsg_New_Params& params, bool was_created_by_renderer) { routing_id_ = params.view_id; surface_id_ = params.surface_id; - if (params.opener_route_id != MSG_ROUTING_NONE && was_created_by_renderer) - opener_id_ = params.opener_route_id; + + int opener_view_routing_id; + WebFrame* opener_frame = + ResolveOpener(params.opener_frame_route_id, &opener_view_routing_id); + if (opener_view_routing_id != MSG_ROUTING_NONE && was_created_by_renderer) + opener_id_ = opener_view_routing_id; + display_mode_= params.initial_size.display_mode; // Ensure we start with a valid next_page_id_ from the browser. @@ -822,13 +863,10 @@ void RenderViewImpl::Initialize(const ViewMsg_New_Params& params, GetContentClient()->renderer()->RenderViewCreated(this); - // If we have an opener_id but we weren't created by a renderer, then - // it's the browser asking us to set our opener to another RenderView. - if (params.opener_route_id != MSG_ROUTING_NONE && !was_created_by_renderer) { - RenderViewImpl* opener_view = FromRoutingID(params.opener_route_id); - if (opener_view) - webview()->mainFrame()->setOpener(opener_view->webview()->mainFrame()); - } + // If we have an opener_frame but we weren't created by a renderer, then it's + // the browser asking us to set our opener to another frame. + if (opener_frame && !was_created_by_renderer) + webview()->mainFrame()->setOpener(opener_frame); // If we are initially swapped out, navigate to kSwappedOutURL. // This ensures we are in a unique origin that others cannot script. @@ -1627,7 +1665,11 @@ WebView* RenderViewImpl::createView(WebLocalFrame* creator, // and rely on the browser sending a WasHidden / WasShown message if it // disagrees. ViewMsg_New_Params view_params; - view_params.opener_route_id = routing_id_; + + RenderFrameImpl* creator_frame = RenderFrameImpl::FromWebFrame(creator); + view_params.opener_frame_route_id = creator_frame->GetRoutingID(); + DCHECK_EQ(routing_id_, creator_frame->render_view()->GetRoutingID()); + view_params.window_was_created_with_opener = true; view_params.renderer_preferences = renderer_preferences_; view_params.web_preferences = webkit_preferences_; diff --git a/content/test/data/post_message.html b/content/test/data/post_message.html index b293a21a61c6..b56764d1c3c2 100644 --- a/content/test/data/post_message.html +++ b/content/test/data/post_message.html @@ -8,6 +8,12 @@ return true; } + // Send a message to our opener's opener. + function postToOpenerOfOpener(msg, origin) { + window.opener.opener.postMessage(msg, origin); + return true; + } + // Send a message to a window named "foo". function postToFoo(msg) { var w = window.open("", "foo"); diff --git a/content/test/test_render_view_host.cc b/content/test/test_render_view_host.cc index a3603f5270a0..520ac89106ce 100644 --- a/content/test/test_render_view_host.cc +++ b/content/test/test_render_view_host.cc @@ -225,7 +225,7 @@ TestRenderViewHost::TestRenderViewHost( false /* hidden */, false /* has_initialized_audio_host */), delete_counter_(NULL), - opener_route_id_(MSG_ROUTING_NONE) { + opener_frame_route_id_(MSG_ROUTING_NONE) { // TestRenderWidgetHostView installs itself into this->view_ in its // constructor, and deletes itself when TestRenderWidgetHostView::Destroy() is // called. @@ -239,18 +239,18 @@ TestRenderViewHost::~TestRenderViewHost() { bool TestRenderViewHost::CreateTestRenderView( const base::string16& frame_name, - int opener_route_id, + int opener_frame_route_id, int proxy_route_id, int32 max_page_id, bool window_was_created_with_opener) { FrameReplicationState replicated_state; replicated_state.name = base::UTF16ToUTF8(frame_name); - return CreateRenderView(opener_route_id, proxy_route_id, max_page_id, + return CreateRenderView(opener_frame_route_id, proxy_route_id, max_page_id, replicated_state, window_was_created_with_opener); } bool TestRenderViewHost::CreateRenderView( - int opener_route_id, + int opener_frame_route_id, int proxy_route_id, int32 max_page_id, const FrameReplicationState& replicated_frame_state, @@ -258,7 +258,7 @@ bool TestRenderViewHost::CreateRenderView( DCHECK(!IsRenderViewLive()); set_renderer_initialized(true); DCHECK(IsRenderViewLive()); - opener_route_id_ = opener_route_id; + opener_frame_route_id_ = opener_frame_route_id; RenderFrameHost* main_frame = GetMainFrame(); if (main_frame) static_cast(main_frame)->SetRenderFrameCreated(true); diff --git a/content/test/test_render_view_host.h b/content/test/test_render_view_host.h index 112879c7a3d7..6eb3a2432059 100644 --- a/content/test/test_render_view_host.h +++ b/content/test/test_render_view_host.h @@ -227,21 +227,21 @@ class TestRenderViewHost delete_counter_ = delete_counter; } - // The opener route id passed to CreateRenderView(). - int opener_route_id() const { return opener_route_id_; } + // The opener frame route id passed to CreateRenderView(). + int opener_frame_route_id() const { return opener_frame_route_id_; } // RenderWidgetHost overrides (same value, but in the Mock* type) MockRenderProcessHost* GetProcess() const override; bool CreateTestRenderView(const base::string16& frame_name, - int opener_route_id, + int opener_frame_route_id, int proxy_route_id, int32 max_page_id, bool window_was_created_with_opener) override; // RenderViewHost overrides -------------------------------------------------- - bool CreateRenderView(int opener_route_id, + bool CreateRenderView(int opener_frame_route_id, int proxy_route_id, int32 max_page_id, const FrameReplicationState& replicated_frame_state, @@ -270,8 +270,8 @@ class TestRenderViewHost // See set_delete_counter() above. May be NULL. int* delete_counter_; - // See opener_route_id() above. - int opener_route_id_; + // See opener_frame_route_id() above. + int opener_frame_route_id_; DISALLOW_COPY_AND_ASSIGN(TestRenderViewHost); }; diff --git a/content/test/test_web_contents.cc b/content/test/test_web_contents.cc index f93024c09155..9bf027f12b71 100644 --- a/content/test/test_web_contents.cc +++ b/content/test/test_web_contents.cc @@ -137,18 +137,15 @@ bool TestWebContents::CrossProcessNavigationPending() { bool TestWebContents::CreateRenderViewForRenderManager( RenderViewHost* render_view_host, - int opener_route_id, + int opener_frame_routing_id, int proxy_routing_id, const FrameReplicationState& replicated_frame_state, bool for_main_frame) { UpdateMaxPageIDIfNecessary(render_view_host); // This will go to a TestRenderViewHost. - static_cast( - render_view_host)->CreateRenderView(opener_route_id, - proxy_routing_id, - -1, - replicated_frame_state, - false); + static_cast(render_view_host) + ->CreateRenderView(opener_frame_routing_id, proxy_routing_id, -1, + replicated_frame_state, false); return true; } diff --git a/content/test/test_web_contents.h b/content/test/test_web_contents.h index fd3a3be46569..ffc7619c5b9d 100644 --- a/content/test/test_web_contents.h +++ b/content/test/test_web_contents.h @@ -69,7 +69,7 @@ class TestWebContents : public WebContentsImpl, public WebContentsTester { // Prevent interaction with views. bool CreateRenderViewForRenderManager( RenderViewHost* render_view_host, - int opener_route_id, + int opener_frame_routing_id, int proxy_routing_id, const FrameReplicationState& replicated_frame_state, bool for_main_frame) override; -- 2.11.4.GIT