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; |
} |
} |