Merge CrossSiteResourceHandler and NavigationResourceThrottle

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index bb59cb07a25bbb960e2cd5965a3b2cc73a0f9460..e2836bfe7adc72cafae4439ba9559c8f29a2890e 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "content/browser/browsing_data/clear_site_data_throttle.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigator.h"
@@ -15,7 +16,9 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/common/frame_messages.h"
 #include "content/common/resource_request_body_impl.h"
+#include "content/common/site_isolation_policy.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/site_instance.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
 #include "net/url_request/redirect_info.h"
@@ -68,15 +71,21 @@ NavigationHandleImpl::NavigationHandleImpl(
       is_synchronous_(is_synchronous),
       is_srcdoc_(is_srcdoc),
       was_redirected_(false),
+      original_url_(url),
       state_(INITIAL),
       is_transferring_(false),
       frame_tree_node_(frame_tree_node),
       next_index_(0),
       navigation_start_(navigation_start),
       pending_nav_entry_id_(pending_nav_entry_id),
-      request_context_type_(REQUEST_CONTEXT_TYPE_UNSPECIFIED) {
+      request_context_type_(REQUEST_CONTEXT_TYPE_UNSPECIFIED),
+      should_replace_current_entry_(false),
+      is_download_(false),
+      is_stream_(false),
+      weak_factory_(this) {
   DCHECK(!navigation_start.is_null());
   GetDelegate()->DidStartNavigation(this);
+  redirect_chain_.push_back(url);

[Charlie Reis, 2016/09/16 21:19:25]

Why is this after the DidStartNavigation call?  I guess it's not exposed
publicly, but if we did expose it later, delegates could query it from
DidStartNavigation.

[clamy, 2016/09/20 15:57:22]

Done.

 
   if (IsInMainFrame()) {
     TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1(
@@ -234,9 +243,11 @@ void NavigationHandleImpl::Resume() {
     result = CheckWillProcessResponse();
 
     // If the navigation is about to proceed after processing the response, then
-    // it's ready to commit.
-    if (result == NavigationThrottle::PROCEED)
-      ReadyToCommitNavigation(render_frame_host_);
+    // it's ready to commit. Determine the final RenderFrameHost handling it.

[Charlie Reis, 2016/09/16 21:19:25]

nit: Change last sentence to: Determine which RenderFrameHost should render the
response, based on its site (after any redirects).

[clamy, 2016/09/20 15:57:22]

Done.

+    // Note: if FindFinalRenderFrameHost() returns false, this means that this
+    // NavigationHandle was deleted, so return immediately.
+    if (result == NavigationThrottle::PROCEED && !FindFinalRenderFrameHost())
+      return;
   }
 
   if (result != NavigationThrottle::DEFER)
@@ -309,6 +320,7 @@ NavigationHandleImpl::CallWillProcessResponseForTesting(
   NavigationThrottle::ThrottleCheckResult result = NavigationThrottle::DEFER;
   WillProcessResponse(static_cast<RenderFrameHostImpl*>(render_frame_host),
                       scoped_refptr<net::HttpResponseHeaders>(), SSLStatus(),
+                      GlobalRequestID(), false, false, false, base::Closure(),
                       base::Bind(&UpdateThrottleCheckResult, &result));
 
   // Reset the callback to ensure it will not be called later.
@@ -372,6 +384,7 @@ void NavigationHandleImpl::WillRedirectRequest(
   is_external_protocol_ = new_is_external_protocol;
   response_headers_ = response_headers;
   was_redirected_ = true;
+  redirect_chain_.push_back(new_url);
   if (new_method != "POST")
     resource_request_body_ = nullptr;
 
@@ -390,20 +403,33 @@ void NavigationHandleImpl::WillProcessResponse(
     RenderFrameHostImpl* render_frame_host,
     scoped_refptr<net::HttpResponseHeaders> response_headers,
     const SSLStatus& ssl_status,
+    const GlobalRequestID& request_id,
+    bool should_replace_current_entry,
+    bool is_download,
+    bool is_stream,
+    const base::Closure& transfer_callback,
     const ThrottleChecksFinishedCallback& callback) {
   DCHECK(!render_frame_host_ || render_frame_host_ == render_frame_host);
   render_frame_host_ = render_frame_host;
   response_headers_ = response_headers;
+  request_id_ = request_id;
+  should_replace_current_entry_ = should_replace_current_entry;
+  is_download_ = is_download;
+  is_stream_ = is_stream;
   state_ = WILL_PROCESS_RESPONSE;
   ssl_status_ = ssl_status;
   complete_callback_ = callback;
+  transfer_callback_ = transfer_callback;
 
   // Notify each throttle of the response.
   NavigationThrottle::ThrottleCheckResult result = CheckWillProcessResponse();
 
-  // If the navigation is about to proceed, then it's ready to commit.
-  if (result == NavigationThrottle::PROCEED)
-    ReadyToCommitNavigation(render_frame_host);
+  // If the navigation is about to proceed, check for the final RenderFrameHost
+  // for the navigation.

[Charlie Reis, 2016/09/16 21:19:25]

Let's use the same comment as in Resume().

Why do we need this in both places?  I'm not sure how they relate to each other,
and why we need both. (Maybe the comment can clarify?)

[clamy, 2016/09/20 15:57:22]

Done. I've slightly modified the first part of the comments to make it clearer
what the 2 cases are:
- if no NavigationThrottle defers during WillProcessResponse, then we call
FindFinalRenderFrameHost from here.
- otherwise we call it from Resume when the navigation is resumed.

+  // Note: if FindFinalRenderFrameHost() returns false, this means that this
+  // NavigationHandle was deleted, so return immediately.
+  if (result == NavigationThrottle::PROCEED && !FindFinalRenderFrameHost())
+    return;
 
   // If the navigation is not deferred, run the callback.
   if (result != NavigationThrottle::DEFER)
@@ -416,10 +442,7 @@ void NavigationHandleImpl::ReadyToCommitNavigation(
   render_frame_host_ = render_frame_host;
   state_ = READY_TO_COMMIT;
 
-  // Only notify the WebContentsObservers when PlzNavigate is enabled, as
-  // |render_frame_host_| may be wrong in the case of transfer navigations.
-  if (IsBrowserSideNavigationEnabled())
-    GetDelegate()->ReadyToCommitNavigation(this);
+  GetDelegate()->ReadyToCommitNavigation(this);
 }
 
 void NavigationHandleImpl::DidCommitNavigation(
@@ -439,6 +462,21 @@ void NavigationHandleImpl::DidCommitNavigation(
   state_ = net_error_code_ == net::OK ? DID_COMMIT : DID_COMMIT_ERROR_PAGE;
 }
 
+void NavigationHandleImpl::TransferToSiteInstance(SiteInstance* site_instance) {
+  DCHECK(!IsBrowserSideNavigationEnabled());
+  if (site_instance->GetId() !=
+      render_frame_host_->GetSiteInstance()->GetId()) {

[Charlie Reis, 2016/09/16 21:19:26]

It's a bit unexpected to call this method if no transfer is happening.  Can we
check for that before calling it in RFHM and just ensure that this is only
called when a transfer is in progress?

[clamy, 2016/09/20 15:57:21]

Done.

+    // This is an actual transfer. Inform the NavigationResourceThrottle.

[Charlie Reis, 2016/09/16 21:19:25]

Please elaborate on why the IO thread needs to know.  There's a comment in RFHM
about marking the request as transferring, but that's lacking a bit of context
as well.

[clamy, 2016/09/20 15:57:21]

I have elaborated in the comments.

+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, transfer_callback_);
+    transfer_callback_.Reset();
+  }
+}
+
+void NavigationHandleImpl::TransferToRenderFrameHost(

[Charlie Reis, 2016/09/16 21:19:26]

This is very different from TransferToSiteInstance, even though the names sound
the same.  I think we should just keep it as set_render_frame_host (keeping it
all in the .h file) and continue documenting that it's used for transfers.

[clamy, 2016/09/20 15:57:21]

Done.

+    RenderFrameHostImpl* render_frame_host) {
+  render_frame_host_ = render_frame_host;
+}
+
 NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CheckWillStartRequest() {
   DCHECK(state_ == WILL_SEND_REQUEST || state_ == DEFERRING_START);
@@ -535,6 +573,79 @@ NavigationHandleImpl::CheckWillProcessResponse() {
   return NavigationThrottle::PROCEED;
 }
 
+bool NavigationHandleImpl::FindFinalRenderFrameHost() {

[Charlie Reis, 2016/09/16 21:19:25]

This method feels misnamed, because it doesn't do any of the RFH selection. 
This one would actually be a better candidate to call MaybeTransferAndProceed.

It appears that this is mainly a wrapper for MaybeTransferAndProceed below,
ensuring that we call ReadyToCommitNavigation if it returns true.  If so, we
could rename the one below to MaybeTransferAndProceedInternal (which has
precedent in some method names in NavigationControllerImpl).

[clamy, 2016/09/20 15:57:22]

Done.

+  DCHECK_EQ(WILL_PROCESS_RESPONSE, state_);
+
+  // Check if the navigation should transfer. This may result in the
+  // destruction of this NavigationHandle, and the cancellation of the request.
+  if (!MaybeTransferAndProceed())
+    return false;
+
+  // Inform observers that the navigation is now ready to commit, unless a
+  // transfer of the navigation failed.
+  ReadyToCommitNavigation(render_frame_host_);
+  return true;
+}
+
+bool NavigationHandleImpl::MaybeTransferAndProceed() {
+  DCHECK(render_frame_host_);
+  // PlzNavigate: the final RenderFrameHost handling this navigation has been
+  // decided before calling WillProcessResponse.

[Charlie Reis, 2016/09/16 21:19:26]

Where does this happen?  It would have been nice to move away from
CrossSiteResourceHandler to something shared with PlzNavigate, rather than a
different flow that will need to change again.  Why does PlzNavigate pick its
RFH at a different time than this new approach?

[clamy, 2016/09/20 15:57:22]

We already had code written to chose a RFH to commit navigation with
PlzNavigate, and it is quite tied to the speculative RFH. I have added a TODO to
try to use the code here instead. I don't want to do it in this patch, since
it's adding more complexity to an already complex patch.

[Charlie Reis, 2016/09/21 03:31:47]

Yes, no need to move PlzNavigate code in this CL.  I was mainly wondering if we
could use the existing PlzNavigate code instead of introducing something new and
different, but it sounds like that's not possible due to the speculative RFH
code.  That's fine.

+  if (IsBrowserSideNavigationEnabled())
+    return true;
+
+  // Subframes shouldn't swap processes unless out-of-process iframes are
+  // possible.
+  if (!IsInMainFrame() && !SiteIsolationPolicy::AreCrossProcessFramesPossible())
+    return true;
+
+  // If this is a download, do not do a cross-site check. The renderer will
+  // see it is a download and abort the request.
+  //
+  // Similarly, HTTP 204 (No Content) responses leave the renderer showing the
+  // previous page. The navigation should be allowed to finish without running
+  // the unload handler or swapping in the pending RenderFrameHost.
+  if (is_download_ || is_stream_ ||
+      (response_headers_.get() && response_headers_->response_code() == 204)) {
+    return true;
+  }
+
+  // The content embedder can decide that a transfer to a different process is
+  // required for this URL.
+  bool should_transfer =
+      GetContentClient()->browser()->ShouldSwapProcessesForRedirect(
+          frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+          original_url_, url_);
+
+  RenderFrameHostManager* manager =
+      render_frame_host_->frame_tree_node()->render_manager();
+
+  // In the site-per-process model, the RenderFrameHostManager may also decide
+  // (independently from the content embedder's ShouldSwapProcessesForRedirect
+  // above) that a process transfer is needed. Process transfers are skipped for
+  // WebUI processes for now, since e.g. chrome://settings has multiple
+  // "cross-site" chrome:// frames, and that doesn't yet work cross- process.
+  if (SiteIsolationPolicy::AreCrossProcessFramesPossible() &&
+      !ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
+          render_frame_host_->GetProcess()->GetID())) {
+    should_transfer |= manager->IsRendererTransferNeededForNavigation(
+        render_frame_host_, url_);
+  }
+
+  // Start the transfer if needed.
+  if (should_transfer) {
+    // This may destroy the NavigationHandle if the transfer fails.
+    base::WeakPtr<NavigationHandleImpl> weak_self = weak_factory_.GetWeakPtr();
+    manager->OnCrossSiteResponse(render_frame_host_, request_id_,
+                                 redirect_chain_, sanitized_referrer_,
+                                 transition_, should_replace_current_entry_);
+    if (!weak_self)
+      return false;
+  }
+
+  return true;
+}
+
 void NavigationHandleImpl::RunCompleteCallback(
     NavigationThrottle::ThrottleCheckResult result) {
   DCHECK(result != NavigationThrottle::DEFER);