diff options
author | pkotwicz@chromium.org <pkotwicz@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-12 19:29:47 +0000 |
---|---|---|
committer | pkotwicz@chromium.org <pkotwicz@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-12 19:30:59 +0000 |
commit | 4e49497a7ec0da1dbc00c569b7457a221c9214d3 (patch) | |
tree | f529169dfa9b317e5b7a9ab91683fe05d2b4ab42 | |
parent | c3f5a257d99feea7645d93cd7586200b980290f8 (diff) | |
download | chromium_src-4e49497a7ec0da1dbc00c569b7457a221c9214d3.zip chromium_src-4e49497a7ec0da1dbc00c569b7457a221c9214d3.tar.gz chromium_src-4e49497a7ec0da1dbc00c569b7457a221c9214d3.tar.bz2 |
Revert of [site isolation] cross-site transfers should track the RenderFrameHost, not the View (https://codereview.chromium.org/457003002/)
Reason for revert:
Speculatively reverting CL in case it is causing WebNavigationApiTest.OpenTab to fail http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20Chromium&testType=browser_tests&tests=WebNavigationApiTest.OpenTab
Original issue's description:
> site isolation: cross-site transfers should track the RenderFrameHost, not the View
>
> This fixes some issues that came up while debugging bug 400850, which inadvertently caused OOPIF-like view reuse in RenderFrameHostManagerTest.AllowTargetedNavigationsAfterSwap.
>
> 1. Make navigation_suspended state be per-RenderFrameHost. What was happening, was that we'd suspend navigations on the non-main frame of the view, and upon resume, we'd switch frames.
>
> 2. Make the map of pending cross site requests be a map of RenderFrameHost IDs instead of RenderViewHost IDs, instead of just assuming that it's the main frame of the view that's navigating.
>
> 3. Add a pending_main_rfh() to the test apparatus, for use by unittests that query the navigation_suspended state.
>
> BUG=304341, 400850
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288867
TBR=creis@chromium.org,nick@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=304341, 400850
Review URL: https://codereview.chromium.org/462083003
Cr-Commit-Position: refs/heads/master@{#289049}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289049 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | content/browser/cross_site_request_manager.cc | 16 | ||||
-rw-r--r-- | content/browser/cross_site_request_manager.h | 26 | ||||
-rw-r--r-- | content/browser/frame_host/render_frame_host_impl.cc | 51 | ||||
-rw-r--r-- | content/browser/frame_host/render_frame_host_impl.h | 48 | ||||
-rw-r--r-- | content/browser/frame_host/render_frame_host_manager.cc | 33 | ||||
-rw-r--r-- | content/browser/frame_host/render_frame_host_manager_unittest.cc | 11 | ||||
-rw-r--r-- | content/browser/loader/cross_site_resource_handler.cc | 9 | ||||
-rw-r--r-- | content/browser/loader/resource_dispatcher_host_impl.cc | 1 | ||||
-rw-r--r-- | content/browser/renderer_host/render_view_host_impl.cc | 45 | ||||
-rw-r--r-- | content/browser/renderer_host/render_view_host_impl.h | 48 | ||||
-rw-r--r-- | content/browser/web_contents/web_contents_impl_unittest.cc | 39 | ||||
-rw-r--r-- | content/public/test/test_renderer_host.cc | 16 | ||||
-rw-r--r-- | content/public/test/test_renderer_host.h | 1 | ||||
-rw-r--r-- | content/test/test_render_view_host.cc | 4 | ||||
-rw-r--r-- | content/test/test_render_view_host.h | 1 |
15 files changed, 172 insertions, 177 deletions
diff --git a/content/browser/cross_site_request_manager.cc b/content/browser/cross_site_request_manager.cc index 5ce71bf..2fa38b1 100644 --- a/content/browser/cross_site_request_manager.cc +++ b/content/browser/cross_site_request_manager.cc @@ -9,24 +9,24 @@ namespace content { bool CrossSiteRequestManager::HasPendingCrossSiteRequest(int renderer_id, - int render_frame_id) { + int render_view_id) { base::AutoLock lock(lock_); - std::pair<int, int> key(renderer_id, render_frame_id); - return pending_cross_site_frames_.find(key) != - pending_cross_site_frames_.end(); + std::pair<int, int> key(renderer_id, render_view_id); + return pending_cross_site_views_.find(key) != + pending_cross_site_views_.end(); } void CrossSiteRequestManager::SetHasPendingCrossSiteRequest(int renderer_id, - int render_frame_id, + int render_view_id, bool has_pending) { base::AutoLock lock(lock_); - std::pair<int, int> key(renderer_id, render_frame_id); + std::pair<int, int> key(renderer_id, render_view_id); if (has_pending) { - pending_cross_site_frames_.insert(key); + pending_cross_site_views_.insert(key); } else { - pending_cross_site_frames_.erase(key); + pending_cross_site_views_.erase(key); } } diff --git a/content/browser/cross_site_request_manager.h b/content/browser/cross_site_request_manager.h index bf04c8a..29417d7 100644 --- a/content/browser/cross_site_request_manager.h +++ b/content/browser/cross_site_request_manager.h @@ -17,7 +17,7 @@ namespace content { // CrossSiteRequestManager is used to handle bookkeeping for cross-site // requests and responses between the UI and IO threads. Such requests involve -// a transition from one RenderFrameHost to another within WebContentsImpl, and +// a transition from one RenderViewHost to another within WebContentsImpl, and // involve coordination with ResourceDispatcherHost. // // CrossSiteRequestManager is a singleton that may be used on any thread. @@ -27,22 +27,22 @@ class CrossSiteRequestManager { // Returns the singleton instance. static CrossSiteRequestManager* GetInstance(); - // Returns whether the RenderFrameHost specified by the given IDs currently + // Returns whether the RenderViewHost specified by the given IDs currently // has a pending cross-site request. If so, we will have to delay the - // response until the previous RenderFrameHost runs its onunload handler. - // Called by ResourceDispatcherHost on the IO thread and RenderFrameHost on + // response until the previous RenderViewHost runs its onunload handler. + // Called by ResourceDispatcherHost on the IO thread and RenderViewHost on // the UI thread. - bool HasPendingCrossSiteRequest(int renderer_id, int render_frame_id); + bool HasPendingCrossSiteRequest(int renderer_id, int render_view_id); - // Sets whether the RenderFrameHost specified by the given IDs currently has a - // pending cross-site request. Called by RenderFrameHost on the UI thread. + // Sets whether the RenderViewHost specified by the given IDs currently has a + // pending cross-site request. Called by RenderViewHost on the UI thread. void SetHasPendingCrossSiteRequest(int renderer_id, - int render_frame_id, + int render_view_id, bool has_pending); private: friend struct DefaultSingletonTraits<CrossSiteRequestManager>; - typedef std::set<std::pair<int, int> > RenderFrameSet; + typedef std::set<std::pair<int, int> > RenderViewSet; CrossSiteRequestManager(); ~CrossSiteRequestManager(); @@ -51,10 +51,10 @@ class CrossSiteRequestManager { // class. You must not block while holding this lock. base::Lock lock_; - // Set of (render_process_host_id, render_frame_id) pairs of all - // RenderFrameHosts that have pending cross-site requests. Used to pass - // information about the RenderFrameHosts between the UI and IO threads. - RenderFrameSet pending_cross_site_frames_; + // Set of (render_process_host_id, render_view_id) pairs of all + // RenderViewHosts that have pending cross-site requests. Used to pass + // information about the RenderViewHosts between the UI and IO threads. + RenderViewSet pending_cross_site_views_; DISALLOW_COPY_AND_ASSIGN(CrossSiteRequestManager); }; diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc index 832911c..c9125d8 100644 --- a/content/browser/frame_host/render_frame_host_impl.cc +++ b/content/browser/frame_host/render_frame_host_impl.cc @@ -13,7 +13,6 @@ #include "content/browser/accessibility/browser_accessibility_manager.h" #include "content/browser/accessibility/browser_accessibility_state_impl.h" #include "content/browser/child_process_security_policy_impl.h" -#include "content/browser/cross_site_request_manager.h" #include "content/browser/frame_host/cross_process_frame_connector.h" #include "content/browser/frame_host/cross_site_transferring_request.h" #include "content/browser/frame_host/frame_tree.h" @@ -167,7 +166,6 @@ RenderFrameHostImpl::RenderFrameHostImpl(RenderViewHostImpl* render_view_host, routing_id_(routing_id), is_swapped_out_(is_swapped_out), renderer_initialized_(false), - navigations_suspended_(false), weak_ptr_factory_(this) { frame_tree_->RegisterRenderFrameHost(this); GetProcess()->AddRoute(routing_id_, this); @@ -190,10 +188,6 @@ RenderFrameHostImpl::~RenderFrameHostImpl() { GetProcess()->RemoveRoute(routing_id_); g_routing_id_frame_map.Get().erase( RenderFrameHostID(GetProcess()->GetID(), routing_id_)); - // Clean up any leftover state from cross-site requests. - CrossSiteRequestManager::GetInstance()->SetHasPendingCrossSiteRequest( - GetProcess()->GetID(), routing_id_, false); - if (delegate_) delegate_->RenderFrameDeleted(this); @@ -1017,13 +1011,14 @@ void RenderFrameHostImpl::Navigate(const FrameMsg_Navigate_Params& params) { // Only send the message if we aren't suspended at the start of a cross-site // request. - if (navigations_suspended_) { + if (render_view_host_->navigations_suspended_) { // Shouldn't be possible to have a second navigation while suspended, since // navigations will only be suspended during a cross-site request. If a // second navigation occurs, RenderFrameHostManager will cancel this pending // RFH and create a new pending RFH. - DCHECK(!suspended_nav_params_.get()); - suspended_nav_params_.reset(new FrameMsg_Navigate_Params(params)); + DCHECK(!render_view_host_->suspended_nav_params_.get()); + render_view_host_->suspended_nav_params_.reset( + new FrameMsg_Navigate_Params(params)); } else { // Get back to a clean state, in case we start a new navigation without // completing a RVH swap or unload handler. @@ -1146,17 +1141,6 @@ void RenderFrameHostImpl::NotificationClosed(int notification_id) { cancel_notification_callbacks_.erase(notification_id); } -bool RenderFrameHostImpl::HasPendingCrossSiteRequest() { - return CrossSiteRequestManager::GetInstance()->HasPendingCrossSiteRequest( - GetProcess()->GetID(), routing_id_); -} - -void RenderFrameHostImpl::SetHasPendingCrossSiteRequest( - bool has_pending_request) { - CrossSiteRequestManager::GetInstance()->SetHasPendingCrossSiteRequest( - GetProcess()->GetID(), routing_id_, has_pending_request); -} - void RenderFrameHostImpl::PlatformNotificationPermissionRequestDone( int request_id, blink::WebNotificationPermission permission) { Send(new PlatformNotificationMsg_PermissionRequestComplete( @@ -1214,31 +1198,4 @@ void RenderFrameHostImpl::ClearPendingTransitionRequestData() { routing_id_)); } -void RenderFrameHostImpl::SetNavigationsSuspended( - bool suspend, - const base::TimeTicks& proceed_time) { - // This should only be called to toggle the state. - DCHECK(navigations_suspended_ != suspend); - - navigations_suspended_ = suspend; - if (!suspend && suspended_nav_params_) { - // There's navigation message params waiting to be sent. Now that we're not - // suspended anymore, resume navigation by sending them. If we were swapped - // out, we should also stop filtering out the IPC messages now. - render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT); - - DCHECK(!proceed_time.is_null()); - suspended_nav_params_->browser_navigation_start = proceed_time; - Send(new FrameMsg_Navigate(routing_id_, *suspended_nav_params_)); - suspended_nav_params_.reset(); - } -} - -void RenderFrameHostImpl::CancelSuspendedNavigations() { - // Clear any state if a pending navigation is canceled or preempted. - if (suspended_nav_params_) - suspended_nav_params_.reset(); - navigations_suspended_ = false; -} - } // namespace content diff --git a/content/browser/frame_host/render_frame_host_impl.h b/content/browser/frame_host/render_frame_host_impl.h index de6a58e..cb9f60d 100644 --- a/content/browser/frame_host/render_frame_host_impl.h +++ b/content/browser/frame_host/render_frame_host_impl.h @@ -189,30 +189,6 @@ class CONTENT_EXPORT RenderFrameHostImpl // Load the specified URL; this is a shortcut for Navigate(). void NavigateToURL(const GURL& url); - // Returns whether navigation messages are currently suspended for this - // RenderFrameHost. Only true during a cross-site navigation, while waiting - // for the onbeforeunload handler. - bool are_navigations_suspended() const { return navigations_suspended_; } - - // Suspends (or unsuspends) any navigation messages from being sent from this - // RenderFrameHost. This is called when a pending RenderFrameHost is created - // for a cross-site navigation, because we must suspend any navigations until - // we hear back from the old renderer's onbeforeunload handler. Note that it - // is important that only one navigation event happen after calling this - // method with |suspend| equal to true. If |suspend| is false and there is a - // suspended_nav_message_, this will send the message. This function should - // only be called to toggle the state; callers should check - // are_navigations_suspended() first. If |suspend| is false, the time that the - // user decided the navigation should proceed should be passed as - // |proceed_time|. - void SetNavigationsSuspended(bool suspend, - const base::TimeTicks& proceed_time); - - // Clears any suspended navigation state after a cross-site navigation is - // canceled or suspended. This is important if we later return to this - // RenderFrameHost. - void CancelSuspendedNavigations(); - // Runs the beforeunload handler for this frame. |for_cross_site_transition| // indicates whether this call is for the current frame during a cross-process // navigation. False means we're closing the entire tab. @@ -267,17 +243,6 @@ class CONTENT_EXPORT RenderFrameHostImpl gfx::NativeViewAccessible GetParentNativeViewAccessible() const; #endif - // Returns whether this RenderFrameHost has an outstanding cross-site request. - // Cleared when we hear the response and start to swap out the old - // RenderFrameHost, or if we hear a commit here without a network request. - bool HasPendingCrossSiteRequest(); - - // Sets whether this RenderFrameHost has an outstanding cross-site request, - // for which another renderer will need to run an onunload event handler. - // This is called before the first navigation event for this RenderFrameHost, - // and cleared when we hear the response or commit. - void SetHasPendingCrossSiteRequest(bool has_pending_request); - protected: friend class RenderFrameHostFactory; @@ -409,19 +374,6 @@ class CONTENT_EXPORT RenderFrameHostImpl bool is_swapped_out_; bool renderer_initialized_; - // Whether we should buffer outgoing Navigate messages rather than sending - // them. This will be true when a RenderFrameHost is created for a cross-site - // request, until we hear back from the onbeforeunload handler of the old - // RenderFrameHost. - bool navigations_suspended_; - - // We only buffer the params for a suspended navigation while this RFH is the - // pending RenderFrameHost of a RenderFrameHostManager. There will only ever - // be one suspended navigation, because RenderFrameHostManager will destroy - // the pending RenderFrameHost and create a new one if a second navigation - // occurs. - scoped_ptr<FrameMsg_Navigate_Params> suspended_nav_params_; - // When the last BeforeUnload message was sent. base::TimeTicks send_before_unload_start_time_; diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc index b40cc2b..618391f 100644 --- a/content/browser/frame_host/render_frame_host_manager.cc +++ b/content/browser/frame_host/render_frame_host_manager.cc @@ -273,8 +273,9 @@ bool RenderFrameHostManager::ShouldCloseTabOnUnresponsiveRenderer() { // that the beforeunload handler will later finish and possibly return // false (meaning the navigation should not proceed), but we'll ignore it // in this case because it took too long. - if (pending_render_frame_host_->are_navigations_suspended()) { - pending_render_frame_host_->SetNavigationsSuspended( + if (pending_render_frame_host_->render_view_host()-> + are_navigations_suspended()) { + pending_render_frame_host_->render_view_host()->SetNavigationsSuspended( false, base::TimeTicks::Now()); } } @@ -297,9 +298,10 @@ void RenderFrameHostManager::OnBeforeUnloadACK( // already made by ShouldCloseTabOnUnresponsiveRenderer. In that case, it // is ok to do nothing here. if (pending_render_frame_host_ && - pending_render_frame_host_->are_navigations_suspended()) { - pending_render_frame_host_->SetNavigationsSuspended(false, - proceed_time); + pending_render_frame_host_->render_view_host()-> + are_navigations_suspended()) { + pending_render_frame_host_->render_view_host()-> + SetNavigationsSuspended(false, proceed_time); } } else { // Current page says to cancel. @@ -456,7 +458,8 @@ void RenderFrameHostManager::DidNavigateFrame( // then we still need to swap out the old RFH first and run its unload // handler, only if it hasn't happened yet. OK for that to happen in the // background. - if (pending_render_frame_host_->HasPendingCrossSiteRequest() && + if (pending_render_frame_host_->render_view_host()-> + HasPendingCrossSiteRequest() && pending_render_frame_host_->render_view_host()->rvh_state() == RenderViewHostImpl::STATE_DEFAULT) { SwapOutOldPage(); @@ -546,7 +549,8 @@ void RenderFrameHostManager::SwapOutOldPage() { // navigation. Thus, we no longer need to remember that the RenderFrameHost // is part of a pending cross-site request. if (pending_render_frame_host_) { - pending_render_frame_host_->SetHasPendingCrossSiteRequest(false); + pending_render_frame_host_->render_view_host()-> + SetHasPendingCrossSiteRequest(false); } } @@ -1405,7 +1409,8 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate( // Navigate message) until we hear back from the old renderer's // beforeunload handler. If the handler returns false, we'll have to // cancel the request. - DCHECK(!pending_render_frame_host_->are_navigations_suspended()); + DCHECK(!pending_render_frame_host_->render_view_host()-> + are_navigations_suspended()); bool is_transfer = entry.transferred_global_request_id() != GlobalRequestID(); if (is_transfer) { @@ -1420,13 +1425,15 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate( render_frame_host_->render_view_host()->Send(new ViewMsg_Stop( render_frame_host_->render_view_host()->GetRoutingID())); - pending_render_frame_host_->SetNavigationsSuspended(true, - base::TimeTicks()); + pending_render_frame_host_->render_view_host()->SetNavigationsSuspended( + true, base::TimeTicks()); - // Tell the CrossSiteRequestManager that this RFH has a pending cross-site + // Tell the CrossSiteRequestManager that this RVH has a pending cross-site // request, so that ResourceDispatcherHost will know to tell us to run the // old page's unload handler before it sends the response. - pending_render_frame_host_->SetHasPendingCrossSiteRequest(true); + // TODO(creis): This needs to be on the RFH. + pending_render_frame_host_->render_view_host()-> + SetHasPendingCrossSiteRequest(true); } // We now have a pending RFH. @@ -1501,7 +1508,7 @@ void RenderFrameHostManager::CancelPending() { pending_render_frame_host->GetSiteInstance()); if (site_instance->active_view_count() > 1) { // Any currently suspended navigations are no longer needed. - pending_render_frame_host->CancelSuspendedNavigations(); + pending_render_frame_host->render_view_host()->CancelSuspendedNavigations(); RenderFrameProxyHost* proxy = new RenderFrameProxyHost(site_instance, frame_tree_node_); 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 e8db0e1..1a76c36 100644 --- a/content/browser/frame_host/render_frame_host_manager_unittest.cc +++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc @@ -925,7 +925,7 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) { // Check that the navigation is still suspended because the old RVH // is not swapped out, yet. - EXPECT_TRUE(host2->are_navigations_suspended()); + EXPECT_TRUE(host2->render_view_host()->are_navigations_suspended()); MockRenderProcessHost* test_process_host2 = static_cast<MockRenderProcessHost*>(host2->GetProcess()); test_process_host2->sink().ClearMessages(); @@ -976,7 +976,8 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) { EXPECT_NE(host3->GetProcess()->GetID(), host2_process_id); // Navigations in the new RVH should be suspended. - EXPECT_TRUE(host3->are_navigations_suspended()); + EXPECT_TRUE(static_cast<RenderViewHostImpl*>( + host3->render_view_host())->are_navigations_suspended()); EXPECT_EQ(host, manager->current_frame_host()); EXPECT_FALSE(manager->current_frame_host()->is_swapped_out()); @@ -1047,10 +1048,10 @@ TEST_F(RenderFrameHostManagerTest, NewCrossNavigationBetweenSwapOutAndCommit) { // Pending rvh2 is already deleted. contents()->ProceedWithCrossSiteNavigation(); - TestRenderFrameHost* rfh3 = pending_main_test_rfh(); - EXPECT_TRUE(rfh3); + TestRenderViewHost* rvh3 = pending_test_rvh(); + EXPECT_TRUE(rvh3); // Navigation should be already unblocked by rvh1. - EXPECT_FALSE(rfh3->are_navigations_suspended()); + EXPECT_FALSE(rvh3->are_navigations_suspended()); } // Tests WebUI creation. diff --git a/content/browser/loader/cross_site_resource_handler.cc b/content/browser/loader/cross_site_resource_handler.cc index 486f6f9..03589bf 100644 --- a/content/browser/loader/cross_site_resource_handler.cc +++ b/content/browser/loader/cross_site_resource_handler.cc @@ -201,10 +201,9 @@ bool CrossSiteResourceHandler::OnNormalResponseStarted( return DeferForNavigationPolicyCheck(info, response, defer); } - bool swap_needed = - should_transfer || - CrossSiteRequestManager::GetInstance()->HasPendingCrossSiteRequest( - info->GetChildID(), info->GetRenderFrameID()); + bool swap_needed = should_transfer || + CrossSiteRequestManager::GetInstance()-> + HasPendingCrossSiteRequest(info->GetChildID(), info->GetRouteID()); // If this is a download, just pass the response through without doing a // cross-site check. The renderer will see it is a download and abort the @@ -294,7 +293,7 @@ void CrossSiteResourceHandler::OnResponseCompleted( if (has_started_response_ || status.status() != net::URLRequestStatus::FAILED || !CrossSiteRequestManager::GetInstance()->HasPendingCrossSiteRequest( - info->GetChildID(), info->GetRenderFrameID())) { + info->GetChildID(), info->GetRouteID())) { next_handler_->OnResponseCompleted(status, security_info, defer); return; } diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc index e30a0f6..17cfcf7 100644 --- a/content/browser/loader/resource_dispatcher_host_impl.cc +++ b/content/browser/loader/resource_dispatcher_host_impl.cc @@ -27,6 +27,7 @@ #include "content/browser/appcache/chrome_appcache_service.h" #include "content/browser/cert_store_impl.h" #include "content/browser/child_process_security_policy_impl.h" +#include "content/browser/cross_site_request_manager.h" #include "content/browser/download/download_resource_handler.h" #include "content/browser/download/save_file_manager.h" #include "content/browser/download/save_file_resource_handler.h" diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index 5006500..84d4b93 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc @@ -24,6 +24,7 @@ #include "base/values.h" #include "cc/base/switches.h" #include "content/browser/child_process_security_policy_impl.h" +#include "content/browser/cross_site_request_manager.h" #include "content/browser/dom_storage/session_storage_namespace_impl.h" #include "content/browser/frame_host/frame_tree.h" #include "content/browser/gpu/compositor_util.h" @@ -188,6 +189,7 @@ RenderViewHostImpl::RenderViewHostImpl( instance_(static_cast<SiteInstanceImpl*>(instance)), waiting_for_drag_context_response_(false), enabled_bindings_(0), + navigations_suspended_(false), main_frame_routing_id_(main_frame_routing_id), run_modal_reply_msg_(NULL), run_modal_opener_id_(MSG_ROUTING_NONE), @@ -238,6 +240,10 @@ RenderViewHostImpl::~RenderViewHostImpl() { delegate_->RenderViewDeleted(this); + // Be sure to clean up any leftover state from cross-site requests. + CrossSiteRequestManager::GetInstance()->SetHasPendingCrossSiteRequest( + GetProcess()->GetID(), GetRoutingID(), false); + // If this was swapped out, it already decremented the active view // count of the SiteInstance it belongs to. if (IsRVHStateActive(rvh_state_)) @@ -494,6 +500,34 @@ void RenderViewHostImpl::NavigateToURL(const GURL& url) { delegate_->GetFrameTree()->GetMainFrame()->NavigateToURL(url); } +void RenderViewHostImpl::SetNavigationsSuspended( + bool suspend, + const base::TimeTicks& proceed_time) { + // This should only be called to toggle the state. + DCHECK(navigations_suspended_ != suspend); + + navigations_suspended_ = suspend; + if (!suspend && suspended_nav_params_) { + // There's navigation message params waiting to be sent. Now that we're not + // suspended anymore, resume navigation by sending them. If we were swapped + // out, we should also stop filtering out the IPC messages now. + SetState(STATE_DEFAULT); + + DCHECK(!proceed_time.is_null()); + suspended_nav_params_->browser_navigation_start = proceed_time; + Send(new FrameMsg_Navigate( + main_frame_routing_id_, *suspended_nav_params_.get())); + suspended_nav_params_.reset(); + } +} + +void RenderViewHostImpl::CancelSuspendedNavigations() { + // Clear any state if a pending navigation is canceled or pre-empted. + if (suspended_nav_params_) + suspended_nav_params_.reset(); + navigations_suspended_ = false; +} + void RenderViewHostImpl::SuppressDialogsUntilSwapOut() { Send(new ViewMsg_SuppressDialogsUntilSwapOut(GetRoutingID())); } @@ -614,6 +648,17 @@ void RenderViewHostImpl::ClosePageIgnoringUnloadEvents() { delegate_->Close(this); } +bool RenderViewHostImpl::HasPendingCrossSiteRequest() { + return CrossSiteRequestManager::GetInstance()->HasPendingCrossSiteRequest( + GetProcess()->GetID(), GetRoutingID()); +} + +void RenderViewHostImpl::SetHasPendingCrossSiteRequest( + bool has_pending_request) { + CrossSiteRequestManager::GetInstance()->SetHasPendingCrossSiteRequest( + GetProcess()->GetID(), GetRoutingID(), has_pending_request); +} + #if defined(OS_ANDROID) void RenderViewHostImpl::ActivateNearestFindResult(int request_id, float x, diff --git a/content/browser/renderer_host/render_view_host_impl.h b/content/browser/renderer_host/render_view_host_impl.h index fa76384..6d69007 100644 --- a/content/browser/renderer_host/render_view_host_impl.h +++ b/content/browser/renderer_host/render_view_host_impl.h @@ -260,6 +260,30 @@ class CONTENT_EXPORT RenderViewHostImpl // RenderFrameHostImpl. void NavigateToURL(const GURL& url); + // Returns whether navigation messages are currently suspended for this + // RenderViewHost. Only true during a cross-site navigation, while waiting + // for the onbeforeunload handler. + bool are_navigations_suspended() const { return navigations_suspended_; } + + // Suspends (or unsuspends) any navigation messages from being sent from this + // RenderViewHost. This is called when a pending RenderViewHost is created + // for a cross-site navigation, because we must suspend any navigations until + // we hear back from the old renderer's onbeforeunload handler. Note that it + // is important that only one navigation event happen after calling this + // method with |suspend| equal to true. If |suspend| is false and there is + // a suspended_nav_message_, this will send the message. This function + // should only be called to toggle the state; callers should check + // are_navigations_suspended() first. If |suspend| is false, the time that the + // user decided the navigation should proceed should be passed as + // |proceed_time|. + void SetNavigationsSuspended(bool suspend, + const base::TimeTicks& proceed_time); + + // Clears any suspended navigation state after a cross-site navigation is + // canceled or suspended. This is important if we later return to this + // RenderViewHost. + void CancelSuspendedNavigations(); + // Whether this RenderViewHost has been swapped out to be displayed by a // different process. bool IsSwappedOut() const { return rvh_state_ == STATE_SWAPPED_OUT; } @@ -292,6 +316,17 @@ class CONTENT_EXPORT RenderViewHostImpl // and the user has agreed to continue with closing the page. void ClosePageIgnoringUnloadEvents(); + // Returns whether this RenderViewHost has an outstanding cross-site request. + // Cleared when we hear the response and start to swap out the old + // RenderViewHost, or if we hear a commit here without a network request. + bool HasPendingCrossSiteRequest(); + + // Sets whether this RenderViewHost has an outstanding cross-site request, + // for which another renderer will need to run an onunload event handler. + // This is called before the first navigation event for this RenderViewHost, + // and cleared when we hear the response or commit. + void SetHasPendingCrossSiteRequest(bool has_pending_request); + // Tells the renderer view to focus the first (last if reverse is true) node. void SetInitialFocus(bool reverse); @@ -499,6 +534,19 @@ class CONTENT_EXPORT RenderViewHostImpl // See BindingsPolicy for details. int enabled_bindings_; + // Whether we should buffer outgoing Navigate messages rather than sending + // them. This will be true when a RenderViewHost is created for a cross-site + // request, until we hear back from the onbeforeunload handler of the old + // RenderViewHost. + // TODO(nasko): Move to RenderFrameHost, as this is per-frame state. + bool navigations_suspended_; + + // We only buffer the params for a suspended navigation while we have a + // pending RVH for a WebContentsImpl. There will only ever be one suspended + // navigation, because WebContentsImpl will destroy the pending RVH and create + // a new one if a second navigation occurs. + // TODO(nasko): Move to RenderFrameHost, as this is per-frame state. + scoped_ptr<FrameMsg_Navigate_Params> suspended_nav_params_; // The current state of this RVH. // TODO(nasko): Move to RenderFrameHost, as this is per-frame state. diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc index a3e2055..fdee01a 100644 --- a/content/browser/web_contents/web_contents_impl_unittest.cc +++ b/content/browser/web_contents/web_contents_impl_unittest.cc @@ -500,12 +500,13 @@ TEST_F(WebContentsImplTest, CrossSiteBoundaries) { static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost()); int pending_rvh_delete_count = 0; pending_rvh->set_delete_counter(&pending_rvh_delete_count); - RenderFrameHostImpl* pending_rfh = pending_main_test_rfh(); + RenderFrameHostImpl* pending_rfh = contents()->GetFrameTree()->root()-> + render_manager()->pending_frame_host(); - // Navigations should be suspended in pending_rfh until BeforeUnloadACK. - EXPECT_TRUE(pending_rfh->are_navigations_suspended()); + // Navigations should be suspended in pending_rvh until BeforeUnloadACK. + EXPECT_TRUE(pending_rvh->are_navigations_suspended()); orig_rvh->SendBeforeUnloadACK(true); - EXPECT_FALSE(pending_rfh->are_navigations_suspended()); + EXPECT_FALSE(pending_rvh->are_navigations_suspended()); // DidNavigate from the pending page contents()->TestDidNavigate( @@ -531,22 +532,23 @@ TEST_F(WebContentsImplTest, CrossSiteBoundaries) { // Going back should switch SiteInstances again. The first SiteInstance is // stored in the NavigationEntry, so it should be the same as at the start. - // We should use the same RFH as before, swapping it back in. + // We should use the same RVH as before, swapping it back in. controller().GoBack(); - TestRenderFrameHost* goback_rfh = pending_main_test_rfh(); - EXPECT_EQ(orig_rfh, goback_rfh); + TestRenderViewHost* goback_rvh = + static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost()); + EXPECT_EQ(orig_rvh, goback_rvh); EXPECT_TRUE(contents()->cross_navigation_pending()); - // Navigations should be suspended in goback_rfh until BeforeUnloadACK. - EXPECT_TRUE(goback_rfh->are_navigations_suspended()); + // Navigations should be suspended in goback_rvh until BeforeUnloadACK. + EXPECT_TRUE(goback_rvh->are_navigations_suspended()); pending_rvh->SendBeforeUnloadACK(true); - EXPECT_FALSE(goback_rfh->are_navigations_suspended()); + EXPECT_FALSE(goback_rvh->are_navigations_suspended()); // DidNavigate from the back action contents()->TestDidNavigate( - goback_rfh->GetRenderViewHost(), 1, url2, PAGE_TRANSITION_TYPED); + goback_rvh, 1, url2, PAGE_TRANSITION_TYPED); EXPECT_FALSE(contents()->cross_navigation_pending()); - EXPECT_EQ(goback_rfh->GetRenderViewHost(), contents()->GetRenderViewHost()); + EXPECT_EQ(goback_rvh, contents()->GetRenderViewHost()); EXPECT_EQ(instance1, contents()->GetSiteInstance()); // The pending RFH should now be swapped out, not deleted. EXPECT_TRUE(contents()->GetRenderManagerForTesting()-> @@ -674,7 +676,8 @@ TEST_F(WebContentsImplTest, NavigateFromSitelessUrl) { SetBrowserClientForTesting(&browser_client); TestRenderViewHost* orig_rvh = test_rvh(); - RenderFrameHostImpl* orig_rfh = main_test_rfh(); + RenderFrameHostImpl* orig_rfh = + contents()->GetFrameTree()->root()->current_frame_host(); int orig_rvh_delete_count = 0; orig_rvh->set_delete_counter(&orig_rvh_delete_count); SiteInstanceImpl* orig_instance = @@ -724,17 +727,15 @@ TEST_F(WebContentsImplTest, NavigateFromSitelessUrl) { EXPECT_TRUE(contents()->cross_navigation_pending()); EXPECT_EQ(url, contents()->GetLastCommittedURL()); EXPECT_EQ(url2, contents()->GetVisibleURL()); - TestRenderFrameHost* pending_rfh = pending_main_test_rfh(); - TestRenderViewHost* pending_rvh = pending_test_rvh(); - EXPECT_EQ(pending_rfh->GetRenderViewHost(), pending_rvh); + TestRenderViewHost* pending_rvh = + static_cast<TestRenderViewHost*>(contents()->GetPendingRenderViewHost()); int pending_rvh_delete_count = 0; pending_rvh->set_delete_counter(&pending_rvh_delete_count); // Navigations should be suspended in pending_rvh until BeforeUnloadACK. - EXPECT_TRUE(pending_rfh->are_navigations_suspended()); + EXPECT_TRUE(pending_rvh->are_navigations_suspended()); orig_rvh->SendBeforeUnloadACK(true); - EXPECT_FALSE(pending_rfh->are_navigations_suspended()); - EXPECT_EQ(pending_rfh->GetRenderViewHost(), pending_rvh); + EXPECT_FALSE(pending_rvh->are_navigations_suspended()); // DidNavigate from the pending page. contents()->TestDidNavigate( diff --git a/content/public/test/test_renderer_host.cc b/content/public/test/test_renderer_host.cc index c9136b7..70d7e66 100644 --- a/content/public/test/test_renderer_host.cc +++ b/content/public/test/test_renderer_host.cc @@ -112,19 +112,9 @@ RenderViewHost* RenderViewHostTestHarness::active_rvh() { } RenderFrameHost* RenderViewHostTestHarness::main_rfh() { - WebContentsImpl* web_contents = - static_cast<WebContentsImpl*>(this->web_contents()); - RenderFrameHostManager* main_frame_render_manager = - web_contents->GetFrameTree()->root()->render_manager(); - return main_frame_render_manager->current_frame_host(); -} - -RenderFrameHost* RenderViewHostTestHarness::pending_main_rfh() { - WebContentsImpl* web_contents = - static_cast<WebContentsImpl*>(this->web_contents()); - RenderFrameHostManager* main_frame_render_manager = - web_contents->GetFrameTree()->root()->render_manager(); - return main_frame_render_manager->pending_frame_host(); + WebContentsImpl* web_contents = static_cast<WebContentsImpl*>( + this->web_contents()); + return web_contents->GetFrameTree()->GetMainFrame(); } BrowserContext* RenderViewHostTestHarness::browser_context() { diff --git a/content/public/test/test_renderer_host.h b/content/public/test/test_renderer_host.h index a3785a9..a514d30 100644 --- a/content/public/test/test_renderer_host.h +++ b/content/public/test/test_renderer_host.h @@ -160,7 +160,6 @@ class RenderViewHostTestHarness : public testing::Test { RenderViewHost* pending_rvh(); RenderViewHost* active_rvh(); RenderFrameHost* main_rfh(); - RenderFrameHost* pending_main_rfh(); BrowserContext* browser_context(); MockRenderProcessHost* process(); diff --git a/content/test/test_render_view_host.cc b/content/test/test_render_view_host.cc index 5bc9478f..8ee9cf4 100644 --- a/content/test/test_render_view_host.cc +++ b/content/test/test_render_view_host.cc @@ -389,10 +389,6 @@ TestRenderFrameHost* RenderViewHostImplTestHarness::main_test_rfh() { return static_cast<TestRenderFrameHost*>(main_rfh()); } -TestRenderFrameHost* RenderViewHostImplTestHarness::pending_main_test_rfh() { - return static_cast<TestRenderFrameHost*>(pending_main_rfh()); -} - TestWebContents* RenderViewHostImplTestHarness::contents() { return static_cast<TestWebContents*>(web_contents()); } diff --git a/content/test/test_render_view_host.h b/content/test/test_render_view_host.h index 81ff5d2..5dd8a94 100644 --- a/content/test/test_render_view_host.h +++ b/content/test/test_render_view_host.h @@ -368,7 +368,6 @@ class RenderViewHostImplTestHarness : public RenderViewHostTestHarness { TestRenderViewHost* pending_test_rvh(); TestRenderViewHost* active_test_rvh(); TestRenderFrameHost* main_test_rfh(); - TestRenderFrameHost* pending_main_test_rfh(); TestWebContents* contents(); private: |