Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(562)

Unified Diff: content/browser/frame_host/render_frame_host_manager.cc

Issue 2636193003: Fix cross-site subframe navigations that transfer back to original RFH. (Closed)
Patch Set: Try #3 Created 3 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 1dc50e79c0d7f1b3b0a266f8493823f1367d18e1..6888a8d49169ccd70134548c419a7db5bfa933d1 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -804,9 +804,9 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
} else {
// Subframe navigations will use the current renderer, unless specifically
// allowed to swap processes.
- no_renderer_swap |= !CanSubframeSwapProcess(request.common_params().url,
- request.source_site_instance(),
- request.dest_site_instance());
+ no_renderer_swap |= !CanSubframeSwapProcess(
+ request.common_params().url, request.source_site_instance(),
+ request.dest_site_instance(), false);
Charlie Reis 2017/02/02 22:40:46 I think we might have a problem here for PlzNaviga
alexmos 2017/02/14 21:16:06 Yes, getting this working with PlzNavigate needed
Charlie Reis 2017/02/28 00:57:52 Yes, that sounds like the right thing to do. Than
}
if (no_renderer_swap) {
@@ -2286,13 +2286,6 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
const GlobalRequestID& transferred_request_id,
int bindings,
bool is_reload) {
- if (!frame_tree_node_->IsMainFrame() &&
- !CanSubframeSwapProcess(dest_url, source_instance, dest_instance)) {
- // Note: Do not add code here to determine whether the subframe should swap
- // or not. Add it to CanSubframeSwapProcess instead.
- return render_frame_host_.get();
- }
-
SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
dest_url, source_instance, dest_instance, nullptr, transition,
@@ -2303,9 +2296,10 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
// transferring on the IO thread before attempting to destroy the pending RFH.
// This ensures the network request will not be destroyed along the pending
// RFH but will persist until it is picked up by the new RFH.
- if (transfer_navigation_handle_.get() &&
- transfer_navigation_handle_->GetGlobalRequestID() ==
- transferred_request_id &&
+ bool dest_is_transfer = transfer_navigation_handle_.get() &&
+ transfer_navigation_handle_->GetGlobalRequestID() ==
+ transferred_request_id;
+ if (dest_is_transfer &&
new_instance.get() !=
transfer_navigation_handle_->GetRenderFrameHost()
->GetSiteInstance()) {
@@ -2326,7 +2320,14 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
}
}
- if (new_instance.get() != current_instance) {
+ // Note: Do not add code here to determine whether the subframe should swap
+ // or not. Add it to CanSubframeSwapProcess instead.
+ bool allowed_to_swap_process =
alexmos 2017/01/25 22:30:55 I turned this into a flag so that there's just one
Charlie Reis 2017/02/02 22:40:46 Acknowledged.
+ frame_tree_node_->IsMainFrame() ||
+ CanSubframeSwapProcess(dest_url, source_instance, dest_instance,
+ dest_is_transfer);
+
+ if (new_instance.get() != current_instance && allowed_to_swap_process) {
TRACE_EVENT_INSTANT2(
"navigation",
"RenderFrameHostManager::UpdateStateForNavigate:New SiteInstance",
@@ -2403,7 +2404,6 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
base::TimeTicks());
render_frame_host_->DispatchBeforeUnload(true, is_reload);
}
-
Charlie Reis 2017/02/02 22:40:46 nit: Let's avoid this churn.
alexmos 2017/02/14 21:16:06 Done.
return pending_render_frame_host_.get();
}
@@ -2729,7 +2729,8 @@ void RenderFrameHostManager::SendPageMessage(IPC::Message* msg,
bool RenderFrameHostManager::CanSubframeSwapProcess(
const GURL& dest_url,
SiteInstance* source_instance,
- SiteInstance* dest_instance) {
+ SiteInstance* dest_instance,
+ bool for_transfer) {
// On renderer-initiated navigations, when the frame initiating the navigation
// and the frame being navigated differ, |source_instance| is set to the
// SiteInstance of the initiating frame. |dest_instance| is present on session
@@ -2751,8 +2752,23 @@ bool RenderFrameHostManager::CanSubframeSwapProcess(
resolved_url = dest_instance->GetSiteURL();
} else {
// If there is no SiteInstance this unique origin can be associated with,
- // then we should avoid a process swap.
- return false;
+ // there are two cases:
+ //
+ // (1) If we are in a transfer, allow a process swap. Normally,
+ // redirects to data: or about: URLs are disallowed as
+ // net::ERR_UNSAFE_REDIRECT. However, extensions can still redirect
+ // arbitary requests to those URLs using the chrome.declarativeWebRequest
+ // API, which will end up here (for an example, see
+ // ExtensionWebRequestApiTest.WebRequestDeclarative1). It's safest to
alexmos 2017/01/25 22:30:55 For reference, here's where these redirects are pe
Charlie Reis 2017/02/02 22:40:46 Interesting! I did just hear Devlin mention that
alexmos 2017/02/14 21:16:06 Unfortunately, this is a problem for regular webRe
+ // swap processes for those redirects if we are in an appropriate
+ // OOPIF-enabled mode.
Charlie Reis 2017/02/02 22:40:46 Sounds like we want this to return true for these
alexmos 2017/02/14 21:16:06 Yes, for instance we don't want to return true her
+ //
+ // (2) If we are not in a transfer, avoid a process swap. We can get
+ // here during session restore, and this avoids putting all data: and
+ // about:blank subframes in OOPIFs. We can also get here in tests with
+ // browser-initiated subframe navigations (NavigateFrameToURL).
+ if (!for_transfer)
alexmos 2017/01/25 22:30:55 I don't know if passing in this as a flag is stric
Charlie Reis 2017/02/02 22:40:46 (See my earlier question about PlzNavigate.)
+ return false;
}
}
« no previous file with comments | « content/browser/frame_host/render_frame_host_manager.h ('k') | content/browser/site_per_process_browsertest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698