PlzNavigate: implement process reuse for ServiceWorkers

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 37f1c0c581746f799c9578ace7b1a76f98236dcb..64671a11ad1f90e2cac56af145b2a115b4395bee 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -43,6 +43,8 @@ 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) {
@@ -119,6 +121,8 @@ NavigationHandleImpl::NavigationHandleImpl(
       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",
@@ -166,6 +170,17 @@ NavigationHandleImpl::NavigationHandleImpl(
 }
 
 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.
@@ -588,6 +603,27 @@ void NavigationHandleImpl::WillRedirectRequest(
                                "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;
@@ -681,6 +717,33 @@ void NavigationHandleImpl::ReadyToCommitNavigation(
   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);
 }
@@ -726,6 +789,20 @@ void NavigationHandleImpl::DidCommitNavigation(
   }
 }
 
+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