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

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <string>
@@ skipping 786 matching lines @@
         render_frame_host_->IsRenderFrameLive() &&
         ShouldMakeNetworkRequestForURL(request.common_params().url) &&
         IsRendererTransferNeededForNavigation(render_frame_host_.get(),
                                               request.common_params().url);
 
     no_renderer_swap |=
         !request.may_transfer() && !can_renderer_initiate_transfer;
   } 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) {
     // GetFrameHostForNavigation will be called more than once during a
     // navigation (currently twice, on request and when it's about to commit in
     // the renderer). In the follow up calls an existing pending WebUI should
     // not be recreated if the URL didn't change. So instead of calling
     // CleanUpNavigation just discard the speculative RenderFrameHost if one
     // exists.
     if (speculative_render_frame_host_)
@@ skipping 1459 matching lines @@
 RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
     const GURL& dest_url,
     SiteInstance* source_instance,
     SiteInstance* dest_instance,
     ui::PageTransition transition,
     bool dest_is_restore,
     bool dest_is_view_source_mode,
     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,
       dest_is_restore, dest_is_view_source_mode);
 
   // Inform the transferring NavigationHandle of a transfer to a different
   // SiteInstance.  It is important do so now, in order to mark the request as
   // 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()) {
     transfer_navigation_handle_->Transfer();
   }
 
   // If we are currently navigating cross-process to a pending RFH for a
   // different SiteInstance, we want to get back to normal and then navigate as
   // usual.  We will reuse the pending RFH below if it matches the destination
   // SiteInstance.
   if (pending_render_frame_host_) {
     if (pending_render_frame_host_->GetSiteInstance() != new_instance) {
       CancelPending();
     } else {
       // When a pending RFH is reused, it should always be live, since it is
       // cleared whenever a process dies.
       CHECK(pending_render_frame_host_->IsRenderFrameLive());
     }
   }
 
-  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",
         TRACE_EVENT_SCOPE_THREAD,
         "current_instance id", current_instance->GetId(),
         "new_instance id", new_instance->GetId());
 
     // New SiteInstance: create a pending RFH to navigate.
 
     if (!pending_render_frame_host_)
@@ skipping 56 matching lines @@
       //
       // Also make sure the old RenderFrame stops, in case a load is in
       // progress.  (We don't want to do this for transfers, since it will
       // interrupt the transfer with an unexpected DidStopLoading.)
       render_frame_host_->Send(new FrameMsg_Stop(
           render_frame_host_->GetRoutingID()));
       pending_render_frame_host_->SetNavigationsSuspended(true,
                                                           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();
   }
 
   // Otherwise the same SiteInstance can be used.  Navigate render_frame_host_.
 
   // It's possible to swap out the current RFH and then decide to navigate in it
   // anyway (e.g., a cross-process navigation that redirects back to the
   // original site).  In that case, we have a proxy for the current RFH but
   // haven't deleted it yet.  The new navigation will swap it back in, so we can
   // delete the proxy.
@@ skipping 305 matching lines @@
     msg->set_routing_id(render_frame_host_->GetRoutingID());
     render_frame_host_->Send(msg);
   } else {
     delete 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
   // history navigations. The two cannot be set simultaneously.
   DCHECK(!source_instance || !dest_instance);
 
   // Don't swap for subframes unless we are in an OOPIF-enabled mode.  We can
   // get here in tests for subframes (e.g., NavigateFrameToURL).
   if (!SiteIsolationPolicy::AreCrossProcessFramesPossible())
     return false;
 
   // If dest_url is a unique origin like about:blank, then the need for a swap
   // is determined by the source_instance or dest_instance.
   GURL resolved_url = dest_url;
   if (url::Origin(resolved_url).unique()) {
     if (source_instance) {
       resolved_url = source_instance->GetSiteURL();
     } else if (dest_instance) {
       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;
     }
   }
 
   // If we are in an OOPIF mode that only applies to some sites, only swap if
   // the policy determines that a transfer would have been needed.  We can get
   // here for session restore.
   if (!IsRendererTransferNeededForNavigation(render_frame_host_.get(),
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content