Shortcut cross-process frame transfers to avoid pending NavigationEntries.

content/browser/frame_host/navigator_impl.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/navigator_impl.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 561 matching lines @@
           GetSiteInstance();
   // If this came from a swapped out RenderFrameHost, we only allow the request
   // if we are still in the same BrowsingInstance.
   // TODO(creis): Move this to RenderFrameProxyHost::OpenURL.
   if (render_frame_host->is_swapped_out() &&
       !render_frame_host->GetSiteInstance()->IsRelatedSiteInstance(
           current_site_instance)) {
     return;
   }
 
-  // Delegate to RequestTransferURL because this is just the generic
-  // case where |old_request_id| is empty.
   // TODO(creis): Pass the redirect_chain into this method to support client
   // redirects.  http://crbug.com/311721.
   std::vector<GURL> redirect_chain;
-  RequestTransferURL(render_frame_host, url, source_site_instance,
-                     redirect_chain, referrer, ui::PAGE_TRANSITION_LINK,
-                     disposition, GlobalRequestID(),
-                     should_replace_current_entry, user_gesture);
-}
 
-void NavigatorImpl::RequestTransferURL(
-    RenderFrameHostImpl* render_frame_host,
-    const GURL& url,
-    SiteInstance* source_site_instance,
-    const std::vector<GURL>& redirect_chain,
-    const Referrer& referrer,
-    ui::PageTransition page_transition,
-    WindowOpenDisposition disposition,
-    const GlobalRequestID& transferred_global_request_id,
-    bool should_replace_current_entry,
-    bool user_gesture) {
   GURL dest_url(url);
-  RenderFrameHostImpl* current_render_frame_host =
-      GetRenderManager(render_frame_host)->current_frame_host();
-  SiteInstance* current_site_instance =
-      current_render_frame_host->GetSiteInstance();
   if (!GetContentClient()->browser()->ShouldAllowOpenURL(
           current_site_instance, url)) {
     dest_url = GURL(url::kAboutBlankURL);
   }
 
   int frame_tree_node_id = -1;
 
   // Send the navigation to the current FrameTreeNode if it's destined for a
   // subframe in the current tab.  We'll assume it's for the main frame
   // (possibly of a new or different WebContents) otherwise.
   if (SiteIsolationPolicy::AreCrossProcessFramesPossible() &&
       disposition == CURRENT_TAB && render_frame_host->GetParent()) {
     frame_tree_node_id =
         render_frame_host->frame_tree_node()->frame_tree_node_id();
   }
 
-  OpenURLParams params(
-      dest_url, referrer, frame_tree_node_id, disposition, page_transition,
-      true /* is_renderer_initiated */);
+  OpenURLParams params(dest_url, referrer, frame_tree_node_id, disposition,
+                       ui::PAGE_TRANSITION_LINK,
+                       true /* is_renderer_initiated */);
   params.source_site_instance = source_site_instance;
   if (redirect_chain.size() > 0)
     params.redirect_chain = redirect_chain;
-  params.transferred_global_request_id = transferred_global_request_id;
+  // TODO(creis): Remove transferred_global_request_id from OpenURLParams.
+  // See https://crbug.com/495161.
+  params.transferred_global_request_id = GlobalRequestID();
   params.should_replace_current_entry = should_replace_current_entry;
   params.user_gesture = user_gesture;
 
-  if (current_render_frame_host->web_ui()) {
+  if (render_frame_host->web_ui()) {

[alexmos, 2015/12/15 23:44:02]

Just to help me understand: previously, it seems this would've used the root's
RFH without --site-per-process (looking at navigator_impl's GetRenderManager),
but now it will use the current RFH.  Is this ok?

[Charlie Reis, 2015/12/16 00:27:30]

Nice catch-- GetRenderManager looks pretty fragile.  I suppose we were counting
on not getting here for subframes in default mode before.  It's only used in
RequestOpenURL and RequestTransferURL, so maybe we can kill the main frame
behavior.  I'll spin up a separate CL for the remaining uses.  (We definitely
should be using the same frame here, not the main frame.)

However, it looks like I also accidentally switched from
current_render_frame_host (last committed) to render_frame_host (which sent us
the IPC, and might not be last committed).

That said, using current_render_frame_host for this seems wrong to me, and is
likely a bug introduced in https://codereview.chromium.org/1472703004.  We don't
want a previous RFH to be able to send us an IPC after we've switched to WebUI
and thus get treated like WebUI below.

So, I think the new code is doing the right thing now.

[alexmos, 2015/12/16 00:40:04]

Yes, this makes sense to me now, and killing the main frame behavior in a
followup CL sounds great.  Nice catch on the previous vs. current switch in
1472703004.

     // Web UI pages sometimes want to override the page transition type for
     // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for
     // automatically generated suggestions).  We don't override other types
     // like TYPED because they have different implications (e.g., autocomplete).
     if (ui::PageTransitionCoreTypeIs(
         params.transition, ui::PAGE_TRANSITION_LINK))
-      params.transition =
-          current_render_frame_host->web_ui()->GetLinkTransitionType();
+      params.transition = render_frame_host->web_ui()->GetLinkTransitionType();
 
     // Note also that we hide the referrer for Web UI pages. We don't really
     // want web sites to see a referrer of "chrome://blah" (and some
     // chrome: URLs might have search terms or other stuff we don't want to
     // send to the site), so we send no referrer.
     params.referrer = Referrer();
 
     // Navigations in Web UI pages count as browser-initiated navigations.
     params.is_renderer_initiated = false;
   }
 
   if (delegate_)
     delegate_->RequestOpenURL(render_frame_host, params);
 }
 
+void NavigatorImpl::RequestTransferURL(
+    RenderFrameHostImpl* render_frame_host,
+    const GURL& url,
+    SiteInstance* source_site_instance,
+    const std::vector<GURL>& redirect_chain,
+    const Referrer& referrer,
+    ui::PageTransition page_transition,
+    WindowOpenDisposition disposition,
+    const GlobalRequestID& transferred_global_request_id,
+    bool should_replace_current_entry,
+    bool user_gesture) {

[alexmos, 2015/12/15 23:44:02]

This looks unused now.  Any reason to still keep it as an arg?

[Charlie Reis, 2015/12/16 00:27:30]

Good catch.  It didn't seem to make it through to LoadURLParams in the old code,
so we didn't seem to need it.  I've removed it, which cleans up an old comment
in RFHM as well.

+  // Allow the delegate to cancel the transfer.
+  if (!delegate_->ShouldTransferNavigation())
+    return;
+
+  GURL dest_url(url);
+  Referrer referrer_to_use(referrer);
+  FrameTreeNode* node = render_frame_host->frame_tree_node();
+  SiteInstance* current_site_instance = render_frame_host->GetSiteInstance();
+  if (!GetContentClient()->browser()->ShouldAllowOpenURL(current_site_instance,
+                                                         url)) {
+    dest_url = GURL(url::kAboutBlankURL);
+  }
+
+  DCHECK_EQ(CURRENT_TAB, disposition);
+
+  // TODO(creis): Determine if this transfer started as a browser-initiated
+  // navigation.  See https://crbug.com/495161.
+  bool is_renderer_initiated = true;
+  if (render_frame_host->web_ui()) {

[alexmos, 2015/12/15 23:44:02]

Is it ok that this block is now duplicated between RequestTransferURL and
RequestOpenURL?  Maybe we could combine them in a helper in a future CL, so that
they won't get out of sync in the future?

[Charlie Reis, 2015/12/16 00:27:30]

Good idea.  I'll abstract them out separately.

+    // Web UI pages sometimes want to override the page transition type for
+    // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for
+    // automatically generated suggestions).  We don't override other types
+    // like TYPED because they have different implications (e.g., autocomplete).
+    if (ui::PageTransitionCoreTypeIs(page_transition, ui::PAGE_TRANSITION_LINK))
+      page_transition = render_frame_host->web_ui()->GetLinkTransitionType();
+
+    // Note also that we hide the referrer for Web UI pages. We don't really
+    // want web sites to see a referrer of "chrome://blah" (and some
+    // chrome: URLs might have search terms or other stuff we don't want to
+    // send to the site), so we send no referrer.
+    referrer_to_use = Referrer();
+
+    // Navigations in Web UI pages count as browser-initiated navigations.
+    is_renderer_initiated = false;
+  }
+
+  NavigationController::LoadURLParams load_url_params(dest_url);
+  load_url_params.source_site_instance = source_site_instance;
+  load_url_params.transition_type = page_transition;
+  load_url_params.frame_tree_node_id = node->frame_tree_node_id();
+  load_url_params.referrer = referrer_to_use;
+  load_url_params.redirect_chain = redirect_chain;
+  load_url_params.is_renderer_initiated = is_renderer_initiated;
+  load_url_params.transferred_global_request_id = transferred_global_request_id;
+  load_url_params.should_replace_current_entry = should_replace_current_entry;
+
+  controller_->LoadURLWithParams(load_url_params);
+}
+
 // PlzNavigate
 void NavigatorImpl::OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
                                       bool proceed) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
 
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
 
   // The NavigationRequest may have been canceled while the renderer was
@@ skipping 315 matching lines @@
       entry->set_should_replace_entry(pending_entry->should_replace_entry());
       entry->SetRedirectChain(pending_entry->GetRedirectChain());
     }
     controller_->SetPendingEntry(entry.Pass());
     if (delegate_)
       delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content