PlzNavigate: implement process reuse for ServiceWorkers

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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/navigation_handle_impl.h"
 
 #include <iterator>
 
 #include "base/logging.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
@@ skipping 25 matching lines @@
 #include "content/public/common/url_constants.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/redirect_info.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
 namespace content {
 
 namespace {
 
+const int kInvalidRenderProcessHostId = -1;

[Charlie Reis, 2017/05/15 03:41:52]

We should use ChildProcessHost::kInvalidUniqueID when comparing against RPH IDs.

[clamy, 2017/05/16 14:50:45]

Done.

+
 void UpdateThrottleCheckResult(
     NavigationThrottle::ThrottleCheckResult* to_update,
     NavigationThrottle::ThrottleCheckResult result) {
   *to_update = result;
 }
 
 void NotifyAbandonedTransferNavigation(const GlobalRequestID& id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get())
     rdh->CancelRequest(id.child_id, id.request_id);
@@ skipping 56 matching lines @@
       should_replace_current_entry_(false),
       redirect_chain_(redirect_chain),
       is_download_(false),
       is_stream_(false),
       started_from_context_menu_(started_from_context_menu),
       reload_type_(ReloadType::NONE),
       restore_type_(RestoreType::NONE),
       navigation_type_(NAVIGATION_TYPE_UNKNOWN),
       should_check_main_world_csp_(should_check_main_world_csp),
       is_form_submission_(is_form_submission),
+      speculative_render_process_host_id_(kInvalidRenderProcessHostId),
+      should_inform_process_on_redirects_(false),
       weak_factory_(this) {
   TRACE_EVENT_ASYNC_BEGIN2("navigation", "NavigationHandle", this,
                            "frame_tree_node",
                            frame_tree_node_->frame_tree_node_id(), "url",
                            url_.possibly_invalid_spec());
   DCHECK(!navigation_start.is_null());
   if (redirect_chain_.empty())
     redirect_chain_.push_back(url);
 
   starting_site_instance_ =
@@ skipping 27 matching lines @@
         navigation_start, "Initial URL", url_.spec());
   }
 
   if (is_same_page_) {
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                  "Same document");
   }
 }
 
 NavigationHandleImpl::~NavigationHandleImpl() {
+  // Inform the RenderProcessHost to no longer expect a navigation.
+  if (speculative_render_process_host_id_ != kInvalidRenderProcessHostId) {
+    RenderProcessHost* process =
+        RenderProcessHost::FromID(speculative_render_process_host_id_);
+    if (process) {
+      RenderProcessHostImpl::RemoveExpectedNavigationToSite(
+          frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+          process, url_);
+    }
+  }
+
   // Transfer requests that have not matched up with another navigation request
   // from the renderer need to be cleaned up. These are marked as protected in
   // the RDHI, so they do not get cancelled when frames are destroyed.
   if (is_transferring()) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&NotifyAbandonedTransferNavigation, GetGlobalRequestID()));
   }
 
   if (!IsRendererDebugURL(url_))
@@ skipping 402 matching lines @@
     const std::string& new_method,
     const GURL& new_referrer_url,
     bool new_is_external_protocol,
     scoped_refptr<net::HttpResponseHeaders> response_headers,
     net::HttpResponseInfo::ConnectionInfo connection_info,
     const ThrottleChecksFinishedCallback& callback) {
   TRACE_EVENT_ASYNC_STEP_INTO1("navigation", "NavigationHandle", this,
                                "WillRedirectRequest", "url",
                                new_url.possibly_invalid_spec());
 
+  // Update the speculative process when the redirect is cross-site.
+  BrowserContext* browser_context =
+      frame_tree_node_->navigator()->GetController()->GetBrowserContext();
+  GURL old_site = SiteInstance::GetSiteForURL(browser_context, url_);
+  GURL new_site = SiteInstance::GetSiteForURL(browser_context, new_url);
+  if (speculative_render_process_host_id_ != kInvalidRenderProcessHostId &&
+      old_site != new_site) {
+    RenderProcessHost* process =
+        RenderProcessHost::FromID(speculative_render_process_host_id_);
+    if (process) {
+      RenderProcessHostImpl::RemoveExpectedNavigationToSite(browser_context,
+                                                            process, url_);
+      if (should_inform_process_on_redirects_) {
+        RenderProcessHostImpl::AddExpectedNavigationToSite(browser_context,
+                                                           process, new_url);
+      } else {
+        speculative_render_process_host_id_ = kInvalidRenderProcessHostId;

[nasko, 2017/05/05 05:21:59]

Shouldn't speculative_render_process_host_id_ be cleared in both of these cases?
Once old site and new site don't agree, we know we will be going to a different
process.

[clamy, 2017/05/05 15:10:11]

Not in renderer-initiated navigations: we match the current process model and
don't switch process in that case.

[Charlie Reis, 2017/05/15 03:41:52]

Yes, the confusion here is part of my concern.  See my suggestion about
listening more directly to speculative RFH changes rather than trying to infer
it here indirectly.

(We might not need most of the NavHandleImpl changes if so.)

[clamy, 2017/05/15 15:21:15]

The problem with that is that we don't execute RFH change logic on redirects. I
would probably need to factor out the code that determines whether we are going
to change in RFHM so that it can be called without actually triggering the
change. Since it's a bit complicated, I think I'll aim for a first version here
that always clear the navigation tracking if we get a cross-site redirect. Then
I'll look into doing the clear more selectively.

[Charlie Reis, 2017/05/15 23:46:18]

Can you put some more detail on this redirect case into the design doc for us to
discuss more there?  I'm curious about the timeline of when speculative RFHs
change.

For example, suppose we have a redirect chain like A -> B -> C -> D, and each
has a ServiceWorker.  Presumably we create a speculative RFH for A.  Do we ever
change it for B and C, or do we wait until OnResponseStarted for D to possibly
replace it?  And in the BeginNavigation case with no process swaps, presumably
there's no speculative RFH at all, unless we get to D and find it requires a
process swap (e.g., CWS)?

I'm curious which process we should put the B and C ServiceWorkers in, even if
we had perfect knowledge.

[clamy, 2017/05/16 14:50:45]

I am now tracking the site url and updating it from there. The code in
UpdateSiteURL will deal with the updating of the RPH.

[clamy, 2017/05/16 14:50:45]

Ok I'll update the design doc so we can follow up on the redirect case.

In the case we mention, we don't change the speculative RFH for B & C. We
thought about it but decided it was probably wasteful.

+      }
+    }
+  }
+
   // Update the navigation parameters.
   url_ = new_url;
   method_ = new_method;
 
   if (!(transition_ & ui::PAGE_TRANSITION_CLIENT_REDIRECT)) {
     sanitized_referrer_.url = new_referrer_url;
     sanitized_referrer_ =
         Referrer::SanitizeForRequest(url_, sanitized_referrer_);
   }
 
@@ skipping 73 matching lines @@
 
 void NavigationHandleImpl::ReadyToCommitNavigation(
     RenderFrameHostImpl* render_frame_host) {
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                "ReadyToCommitNavigation");
 
   DCHECK(!render_frame_host_ || render_frame_host_ == render_frame_host);
   render_frame_host_ = render_frame_host;
   state_ = READY_TO_COMMIT;
 
+  if (IsBrowserSideNavigationEnabled()) {
+    // Update the processes about expected navigations if the speculative
+    // RenderFrameHost changed.
+    RenderProcessHost* final_process = render_frame_host->GetProcess();
+    bool should_inform_final_process =
+        speculative_render_process_host_id_ == kInvalidRenderProcessHostId;
+
+    if (speculative_render_process_host_id_ != kInvalidRenderProcessHostId) {

[Charlie Reis, 2017/05/15 03:41:52]

nit: Redundant with should_inform_final_process

[clamy, 2017/05/16 14:50:45]

I have removed this part in favor of calling SetSpeculativeProcessHostID again.

+      RenderProcessHost* process =
+          RenderProcessHost::FromID(speculative_render_process_host_id_);
+      should_inform_final_process = final_process != process;
+      if (process && final_process != process) {
+        RenderProcessHostImpl::RemoveExpectedNavigationToSite(
+            frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+            process, url_);
+      }
+    }
+
+    if (should_inform_final_process) {
+      RenderProcessHostImpl::AddExpectedNavigationToSite(
+          frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+          final_process, url_);
+      speculative_render_process_host_id_ = final_process->GetID();
+    }
+    DCHECK_EQ(speculative_render_process_host_id_, final_process->GetID());
+  }
+
   if (!IsRendererDebugURL(url_) && !IsSameDocument())
     GetDelegate()->ReadyToCommitNavigation(this);
 }
 
 void NavigationHandleImpl::DidCommitNavigation(
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
     bool navigation_entry_committed,
     bool did_replace_entry,
     const GURL& previous_url,
     NavigationType navigation_type,
@@ skipping 25 matching lines @@
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                  "DidCommitNavigation: error page");
     state_ = DID_COMMIT_ERROR_PAGE;
   } else {
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                  "DidCommitNavigation");
     state_ = DID_COMMIT;
   }
 }
 
+void NavigationHandleImpl::SetSpeculativeProcessID(
+    int render_process_host_id,
+    bool should_inform_on_redirects) {
+  RenderProcessHost* process =
+      RenderProcessHost::FromID(render_process_host_id);
+  if (!process)

[Charlie Reis, 2017/05/15 03:41:52]

This seems like it should be an error, rather than an early return where we
ignore it.

Actually, why not just pass in the RenderProcessHost instead?  We have it at the
call site, so we shouldn't need to look it up again.

[clamy, 2017/05/16 14:50:45]

This function now takes the RenderProcessHost and expects to be called multiple
times so it has changed quite a bit.

+    return;
+  RenderProcessHostImpl::AddExpectedNavigationToSite(
+      frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
+      process, url_);
+  speculative_render_process_host_id_ = render_process_host_id;
+  should_inform_process_on_redirects_ = should_inform_on_redirects;
+}
+
 void NavigationHandleImpl::Transfer() {
   DCHECK(!IsBrowserSideNavigationEnabled());
   // This is an actual transfer. Inform the NavigationResourceThrottle. This
   // will allow to mark the URLRequest as transferring. When it is marked as
   // transferring, the URLRequest can no longer be cancelled by its original
   // RenderFrame. Instead it will persist until being picked up by the transfer
   // RenderFrame, even if the original RenderFrame is destroyed.
   // Note: |transfer_callback_| can be null in unit tests.
   if (!transfer_callback_.is_null())
     BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, transfer_callback_);
@@ skipping 308 matching lines @@
     if (node->current_url().EqualsIgnoringRef(url_)) {
       if (found_self_reference)
         return true;
       found_self_reference = true;
     }
   }
   return false;
 }
 
 }  // namespace content