Fix cross-site subframe navigations that transfer back to original RFH.

content/browser/frame_host/render_frame_host_manager.cc

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 PlzNavigate.  As mentioned below on
line 813, this method is called both before and after redirects in PlzNavigate
mode (once when picking a speculative RFH, and once when it's time to commit). 
I imagine that the transfer bug with redirects to data URLs will apply during
the second call.

In that sense, we should probably avoid having the fix depend on whether it's a
transfer or not (especially since PlzNavigate doesn't have the same concept of
transfers).  Is there something else that we can use to distinguish the case
that data/about URLs require a process swap?

[alexmos, 2017/02/14 21:16:06]

Yes, getting this working with PlzNavigate needed a bit more work.  I switched
my approach to be based on whether or not a redirect happened, rather than a
transfer, which also makes sense with PlzNavigate, and better matches the
scenario we care about (redirects to data: or about:).  

With PlzNavigate, however, passing that into CanSubframeSwapProcess still didn't
lead to a new SiteInstance for  the data: URL.  This is because PlzNavigate
preserves the |source_instance| through redirects (unlike the transfer path),
and as part of figuring out |dest_site_instance| above, the following path ran
in RFHM::DetermineSiteInstanceForURL:

  // Use the source SiteInstance in case of data URLs, about:srcdoc pages and
  // about:blank pages because the content is then controlled and/or scriptable
  // by the source SiteInstance.
  GURL about_blank(url::kAboutBlankURL);
  GURL about_srcdoc(content::kAboutSrcDocURL);
  if (source_instance && (dest_url == about_srcdoc || dest_url == about_blank ||
                          dest_url.scheme() == url::kDataScheme)) {
    return SiteInstanceDescriptor(source_instance);
  }

I.e., if we have A-embed-B, and A navigates the subframe to a URL that gets
redirected to data:, this would cause the data: URL to be placed into the source
(A) SiteInstance.  The |source_instance| shouldn't apply if we reach this via a
redirect (since the content is controlled by an extension, not source_instance),
so I ended up plumbing the redirect info there as well.  Let me know what you
think about that.

[Charlie Reis, 2017/02/28 00:57:52]

Yes, that sounds like the right thing to do.  Thanks!

   }
 
   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 spot where the current RFH is
returned.  Now it's always returned at the bottom of this function.  There's
some more work (WebUI/view-source) that is done on the current RFH below; it
seems that it was another bug that we weren't doing those things previously when
returning the current RFH early due to CanSubframeSwapProcess.

[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 performed and explicitly
allowed:

https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_r...

    // Explicitly mark the URL as safe for redirection, to prevent the request
    // from being blocked because of net::ERR_UNSAFE_REDIRECT.
    *allowed_unsafe_redirect_url = new_url;

The APIs docs are here:

https://developer.chrome.com/extensions/declarativeWebRequest

Namely, RedirectToEmptyDocument is implemented as a redirect to "data:," (this
is what that test was hitting), and RedirectRequest can be used to redirect an
arbitrary URL to another arbitrary URL (I've tried both data: URLs and
about:blank in a sample extension).

I noticed that these APIs are currently only available on beta and dev, but not
stable.

[Charlie Reis, 2017/02/02 22:40:46]

Interesting!  I did just hear Devlin mention that the declarativeWebRequest API
didn't ship.  Is that the only way for extensions to do this, or can they do it
via the normal web request API?

(I'm wondering if we can avoid this problem by disallowing the behavior in
extensions.  That might be the case if the declarativeWebRequest API is going to
be removed and if nothing else can do this.  Probably overly optimistic, but
worth checking.)

[alexmos, 2017/02/14 21:16:06]

Unfortunately, this is a problem for regular webRequest as well.  I've confirmed
this with a toy extension by setting |redirectUrl| in onBeforeRequest.  The docs
also explicitly allow this: "Redirections to non-HTTP schemes such as data: are
allowed."  (https://developer.chrome.com/extensions/webRequest).  I've updated
the  comment.  It might be worth adding an explicit test for this too (for
regular webRequest redirecting to a data URL).

I did talk to Devlin about the declarativeWebRequest also, and he mentioned that
while it didn't ship, some of the stuff there might be the only way to do things
for extensions on mobile, so it's better to support them if we can.

+      // 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 cases, but it looks like it
depends on what IsRendererTransferNeededForNavigation returns for dest_url. 
There's a lot of ways for that to return false, so I'm wondering if that can be
a problem.

[alexmos, 2017/02/14 21:16:06]

Yes, for instance we don't want to return true here when we're not in an
appropriate OOPIF mode.  (--site-per-process or TDI)

As for IsRendererTransferNeededForNavigation returning false, I don't think
that's a problem because the only way we could get into here for a redirect to
data: is if IsRendererTransferNeededForNavigation returned true earlier when
deciding whether or not a transfer was needed.  (This is the call in
MaybeTransferAndProceedInternal.)  So it should also return true here the second
time.

+      //
+      // (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 strictly necessary; I did it to
match the transfer condition in UpdateStateForNavigate, but perhaps we could
also just directly check whether transfer_navigation_handle_ exists?  (Not sure
if it's possible to get here with it being valid but referencing another global
request ID)

[Charlie Reis, 2017/02/02 22:40:46]

(See my earlier question about PlzNavigate.)

+        return false;
     }
   }