Navigation: Move members from NavigationHandleImpl to NavigationRequest.

This CL moves the usage of starting_site_instance_, site_url and expected_render_process_host_id
from NavigationHandleImpl to NavigationRequest.
With this, these functions were also moved:
NavigationHandleImpl::UpdateSiteURL
NavigationHandleImpl::SetExpectedProcess

A new function is introduced, NavigationRequest::ResetExpectedProcess
for refactoring.

Change-Id: I92fd557b05108b2bf6b786e6467311e67791ce7d
Bug: 916537
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1481322Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Arthur Hemery <ahemery@chromium.org>
Commit-Queue: Mohamed Abdelhalim <zetamoo@google.com>
Cr-Commit-Position: refs/heads/master@{#638122}

content/browser/frame_host/navigation_handle_impl.cc

@@ -33,7 +33,6 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/common/child_process_host.h"
-#include "content/public/common/content_client.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/common/url_utils.h"
 #include "net/base/net_errors.h"
@@ -155,7 +154,6 @@ NavigationHandleImpl::NavigationHandleImpl(
       reload_type_(ReloadType::NONE),
       restore_type_(RestoreType::NONE),
       navigation_type_(NAVIGATION_TYPE_UNKNOWN),
-      expected_render_process_host_id_(ChildProcessHost::kInvalidUniqueID),
       is_same_process_(true),
       throttle_runner_(this, this),
       weak_factory_(this) {
@@ -167,12 +165,6 @@ NavigationHandleImpl::NavigationHandleImpl(
   DCHECK(!navigation_request_->common_params().navigation_start.is_null());
   DCHECK(!IsRendererDebugURL(url));
 
-  starting_site_instance_ =
-      frame_tree_node()->current_frame_host()->GetSiteInstance();
-
-  site_url_ = SiteInstanceImpl::GetSiteForURL(
-      starting_site_instance_->GetBrowserContext(),
-      starting_site_instance_->GetIsolationContext(), url);
   if (redirect_chain_.empty())
     redirect_chain_.push_back(url);
 
@@ -230,17 +222,6 @@ NavigationHandleImpl::NavigationHandleImpl(
 }
 
 NavigationHandleImpl::~NavigationHandleImpl() {
-  // Inform the RenderProcessHost to no longer expect a navigation.
-  if (expected_render_process_host_id_ != ChildProcessHost::kInvalidUniqueID) {
-    RenderProcessHost* process =
-        RenderProcessHost::FromID(expected_render_process_host_id_);
-    if (process) {
-      RenderProcessHostImpl::RemoveExpectedNavigationToSite(
-          frame_tree_node()->navigator()->GetController()->GetBrowserContext(),
-          process, site_url_);
-    }
-  }
-
 #if defined(OS_ANDROID)
   navigation_handle_proxy_->DidFinish();
 #endif
@@ -269,7 +250,7 @@ const GURL& NavigationHandleImpl::GetURL() {
 }
 
 SiteInstanceImpl* NavigationHandleImpl::GetStartingSiteInstance() {
-  return starting_site_instance_.get();
+  return navigation_request_->starting_site_instance();
 }
 
 bool NavigationHandleImpl::IsInMainFrame() {
@@ -636,7 +617,7 @@ void NavigationHandleImpl::WillRedirectRequest(
                                "WillRedirectRequest", "url",
                                GetURL().possibly_invalid_spec());
   UpdateStateFollowingRedirect(new_referrer_url, std::move(callback));
-  UpdateSiteURL(post_redirect_process);
+  navigation_request_->UpdateSiteURL(post_redirect_process);
 
   if (IsSelfReferentialURL()) {
     state_ = CANCELING;
@@ -731,7 +712,7 @@ void NavigationHandleImpl::ReadyToCommitNavigation(bool is_error) {
     }
   }
 
-  SetExpectedProcess(GetRenderFrameHost()->GetProcess());
+  navigation_request_->SetExpectedProcess(GetRenderFrameHost()->GetProcess());
 
   if (!IsSameDocument())
     GetDelegate()->ReadyToCommitNavigation(this);
@@ -829,38 +810,6 @@ void NavigationHandleImpl::DidCommitNavigation(
   }
 }
 
-void NavigationHandleImpl::SetExpectedProcess(
-    RenderProcessHost* expected_process) {
-  if (expected_process &&
-      expected_process->GetID() == expected_render_process_host_id_) {
-    // This |expected_process| has already been informed of the navigation,
-    // no need to update it again.
-    return;
-  }
-
-  // If a RenderProcessHost was expecting this navigation to commit, have it
-  // stop tracking this site.
-  RenderProcessHost* old_process =
-      RenderProcessHost::FromID(expected_render_process_host_id_);
-  if (old_process) {
-    RenderProcessHostImpl::RemoveExpectedNavigationToSite(
-        frame_tree_node()->navigator()->GetController()->GetBrowserContext(),
-        old_process, site_url_);
-  }
-
-  if (expected_process == nullptr) {
-    expected_render_process_host_id_ = ChildProcessHost::kInvalidUniqueID;
-    return;
-  }
-
-  // Keep track of the speculative RenderProcessHost and tell it to expect a
-  // navigation to |site_url_|.
-  expected_render_process_host_id_ = expected_process->GetID();
-  RenderProcessHostImpl::AddExpectedNavigationToSite(
-      frame_tree_node()->navigator()->GetController()->GetBrowserContext(),
-      expected_process, site_url_);
-}
-
 void NavigationHandleImpl::OnNavigationEventProcessed(
     NavigationThrottleRunner::Event event,
     NavigationThrottle::ThrottleCheckResult result) {
@@ -1021,30 +970,6 @@ bool NavigationHandleImpl::IsSelfReferentialURL() {
   return false;
 }
 
-void NavigationHandleImpl::UpdateSiteURL(
-    RenderProcessHost* post_redirect_process) {
-  // TODO(alexmos): Using |starting_site_instance_|'s IsolationContext may not
-  // be correct for cross-BrowsingInstance redirects.
-  GURL new_site_url = SiteInstanceImpl::GetSiteForURL(
-      frame_tree_node()->navigator()->GetController()->GetBrowserContext(),
-      starting_site_instance_->GetIsolationContext(), GetURL());
-  int post_redirect_process_id = post_redirect_process
-                                     ? post_redirect_process->GetID()
-                                     : ChildProcessHost::kInvalidUniqueID;
-  if (new_site_url == site_url_ &&
-      post_redirect_process_id == expected_render_process_host_id_) {
-    return;
-  }
-
-  // Stop expecting a navigation to the current site URL in the current expected
-  // process.
-  SetExpectedProcess(nullptr);
-
-  // Update the site URL and the expected process.
-  site_url_ = new_site_url;
-  SetExpectedProcess(post_redirect_process);
-}
-
 void NavigationHandleImpl::RenderProcessBlockedStateChanged(bool blocked) {
   if (blocked)
     StopCommitTimeout();

content/browser/frame_host/navigation_handle_impl.h

@@ -326,11 +326,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle,
     return std::move(modified_request_headers_);
   }
 
-  // Sets ID of the RenderProcessHost we expect the navigation to commit in.
-  // This is used to inform the RenderProcessHost to expect a navigation to the
-  // url we're navigating to.
-  void SetExpectedProcess(RenderProcessHost* expected_process);
-
   NavigationThrottle* GetDeferringThrottleForTesting() const {
     return throttle_runner_.GetDeferringThrottle();
   }
@@ -392,12 +387,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle,
   // WillStartRequest and WillRedirectRequest to prevent the navigation.
   bool IsSelfReferentialURL();
 
-  // Updates the destination site URL for this navigation. This is called on
-  // redirects. |post_redirect_process| is the renderer process that should
-  // handle the navigation following the redirect if it can be handled by an
-  // existing RenderProcessHost. Otherwise, it should be null.
-  void UpdateSiteURL(RenderProcessHost* post_redirect_process);
-
   // Called if READY_TO_COMMIT -> COMMIT state transition takes an unusually
   // long time.
   void OnCommitTimeout();
@@ -413,7 +402,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle,
   NavigationRequest* navigation_request_;
 
   // See NavigationHandle for a description of those member variables.
-  scoped_refptr<SiteInstanceImpl> starting_site_instance_;
   Referrer sanitized_referrer_;
   net::Error net_error_code_;
   const bool is_same_document_;
@@ -422,9 +410,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle,
   bool should_update_history_;
   bool subframe_entry_committed_;
 
-  // The site URL of this navigation, as obtained from SiteInstance::GetSiteURL.
-  GURL site_url_;
-
   // The headers used for the request.
   net::HttpRequestHeaders request_headers_;
 
@@ -499,10 +484,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle,
   GURL base_url_;
   NavigationType navigation_type_;
 
-  // Used to inform a RenderProcessHost that we expect this navigation to commit
-  // in it.
-  int expected_render_process_host_id_;
-
   // Which proxy server was used for this navigation, if any.
   net::ProxyServer proxy_server_;
 

content/browser/frame_host/navigation_request.cc

@@ -496,6 +496,7 @@ NavigationRequest::NavigationRequest(
       from_begin_navigation_(from_begin_navigation),
       has_stale_copy_in_cache_(false),
       net_error_(net::OK),
+      expected_render_process_host_id_(ChildProcessHost::kInvalidUniqueID),
       devtools_navigation_token_(base::UnguessableToken::Create()),
       request_navigation_client_(nullptr),
       commit_navigation_client_(nullptr),
@@ -542,6 +543,18 @@ NavigationRequest::NavigationRequest(
     bindings_ = entry->bindings();
   }
 
+  // This is needed to get site URLs and assign the expected RenderProcessHost.
+  // This is not always the same as |source_site_instance|, as it only depends
+  // on the current frame host, and does not depend on |entry|.
+  starting_site_instance_ =
+      frame_tree_node->current_frame_host()->GetSiteInstance();
+
+  // TODO(alexmos): Using |starting_site_instance_|'s IsolationContext may not
+  // be correct for cross-BrowsingInstance redirects.
+  site_url_ = SiteInstanceImpl::GetSiteForURL(
+      starting_site_instance_->GetBrowserContext(),
+      starting_site_instance_->GetIsolationContext(), common_params_.url);
+
   // Update the load flags with cache information.
   UpdateLoadFlagsWithCacheFlags(&begin_params_->load_flags,
                                 common_params_.navigation_type,
@@ -617,6 +630,7 @@ NavigationRequest::NavigationRequest(
 
 NavigationRequest::~NavigationRequest() {
   TRACE_EVENT_ASYNC_END0("navigation", "NavigationRequest", this);
+  ResetExpectedProcess();
   if (state_ == STARTED) {
     devtools_instrumentation::OnNavigationRequestFailed(
         *this, network::URLLoaderCompletionStatus(net::ERR_ABORTED));
@@ -1333,7 +1347,7 @@ void NavigationRequest::OnRequestFailedInternal(
     // TODO(nasko): Investigate whether GetFrameHostForNavigation can properly
     // account for clearing the expected process if it clears the speculative
     // RenderFrameHost. See https://crbug.com/793127.
-    navigation_handle_->SetExpectedProcess(nullptr);
+    ResetExpectedProcess();
     render_frame_host =
         frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
   } else {
@@ -1449,7 +1463,7 @@ void NavigationRequest::OnStartChecksComplete(
           : frame_tree_node_->current_frame_host();
   DCHECK(navigating_frame_host);
 
-  navigation_handle_->SetExpectedProcess(navigating_frame_host->GetProcess());
+  SetExpectedProcess(navigating_frame_host->GetProcess());
 
   BrowserContext* browser_context =
       frame_tree_node_->navigator()->GetController()->GetBrowserContext();
@@ -1860,6 +1874,69 @@ void NavigationRequest::CommitNavigation() {
       render_frame_host_->GetSiteInstance()->GetBrowserContext());
 }
 
+void NavigationRequest::ResetExpectedProcess() {
+  if (expected_render_process_host_id_ == ChildProcessHost::kInvalidUniqueID) {
+    // No expected process is set, nothing to update.
+    return;
+  }
+  RenderProcessHost* process =
+      RenderProcessHost::FromID(expected_render_process_host_id_);
+  if (process) {
+    RenderProcessHostImpl::RemoveExpectedNavigationToSite(
+        frame_tree_node()->navigator()->GetController()->GetBrowserContext(),
+        process, site_url_);
+  }
+  expected_render_process_host_id_ = ChildProcessHost::kInvalidUniqueID;
+}
+
+void NavigationRequest::SetExpectedProcess(
+    RenderProcessHost* expected_process) {
+  if (expected_process &&
+      expected_process->GetID() == expected_render_process_host_id_) {
+    // This |expected_process| has already been informed of the navigation,
+    // no need to update it again.
+    return;
+  }
+
+  ResetExpectedProcess();
+
+  if (expected_process == nullptr) {
+    expected_render_process_host_id_ = ChildProcessHost::kInvalidUniqueID;
+    return;
+  }
+
+  // Keep track of the speculative RenderProcessHost and tell it to expect a
+  // navigation to |site_url_|.
+  expected_render_process_host_id_ = expected_process->GetID();
+  RenderProcessHostImpl::AddExpectedNavigationToSite(
+      frame_tree_node()->navigator()->GetController()->GetBrowserContext(),
+      expected_process, site_url_);
+}
+
+void NavigationRequest::UpdateSiteURL(
+    RenderProcessHost* post_redirect_process) {
+  // TODO(alexmos): Using |starting_site_instance_|'s IsolationContext may not
+  // be correct for cross-BrowsingInstance redirects.
+  GURL new_site_url = SiteInstanceImpl::GetSiteForURL(
+      frame_tree_node()->navigator()->GetController()->GetBrowserContext(),
+      starting_site_instance_->GetIsolationContext(), common_params_.url);
+  int post_redirect_process_id = post_redirect_process
+                                     ? post_redirect_process->GetID()
+                                     : ChildProcessHost::kInvalidUniqueID;
+  if (new_site_url == site_url_ &&
+      post_redirect_process_id == expected_render_process_host_id_) {
+    return;
+  }
+
+  // Stop expecting a navigation to the current site URL in the current expected
+  // process.
+  ResetExpectedProcess();
+
+  // Update the site URL and the expected process.
+  site_url_ = new_site_url;
+  SetExpectedProcess(post_redirect_process);
+}
+
 bool NavigationRequest::IsAllowedByCSPDirective(
     CSPContext* context,
     CSPDirective::Name directive,

content/browser/frame_host/navigation_request.h

@@ -164,6 +164,10 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
 
   int bindings() const { return bindings_; }
 
+  SiteInstanceImpl* starting_site_instance() const {
+    return starting_site_instance_.get();
+  }
+
   bool browser_initiated() const { return browser_initiated_ ; }
 
   bool from_begin_navigation() const { return from_begin_navigation_; }
@@ -213,6 +217,17 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
     on_start_checks_complete_closure_ = closure;
   }
 
+  // Sets ID of the RenderProcessHost we expect the navigation to commit in.
+  // This is used to inform the RenderProcessHost to expect a navigation to the
+  // url we're navigating to.
+  void SetExpectedProcess(RenderProcessHost* expected_process);
+
+  // Updates the destination site URL for this navigation. This is called on
+  // redirects. |post_redirect_process| is the renderer process that should
+  // handle the navigation following the redirect if it can be handled by an
+  // existing RenderProcessHost. Otherwise, it should be null.
+  void UpdateSiteURL(RenderProcessHost* post_redirect_process);
+
   int nav_entry_id() const { return nav_entry_id_; }
 
   // For automation driver-initiated navigations over the devtools protocol,
@@ -396,6 +411,9 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
   // Only used with PerNavigationMojoInterface enabled.
   void IgnoreInterfaceDisconnection();
 
+  // Inform the RenderProcessHost to no longer expect a navigation.
+  void ResetExpectedProcess();
+
   FrameTreeNode* frame_tree_node_;
 
   RenderFrameHostImpl* render_frame_host_ = nullptr;
@@ -442,6 +460,8 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
   int bindings_;
   int nav_entry_id_ = 0;
 
+  scoped_refptr<SiteInstanceImpl> starting_site_instance_;
+
   // Whether the navigation should be sent to a renderer a process. This is
   // true, except for 204/205 responses and downloads.
   bool response_should_be_rendered_;
@@ -476,6 +496,13 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
   bool has_stale_copy_in_cache_;
   int net_error_;
 
+  // Identifies in which RenderProcessHost this navigation is expected to
+  // commit.
+  int expected_render_process_host_id_;
+
+  // The site URL of this navigation, as obtained from SiteInstance::GetSiteURL.
+  GURL site_url_;
+
   std::unique_ptr<InitiatorCSPContext> initiator_csp_context_;
 
   base::Closure on_start_checks_complete_closure_;