PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects

content/browser/frame_host/navigation_request.h

Index: content/browser/frame_host/navigation_request.h
diff --git a/content/browser/frame_host/navigation_request.h b/content/browser/frame_host/navigation_request.h
index a4b2e010dec67638a18330a2bb638e80b5ca2e2b..42d01e8e9420ad08fe791f5ac9eacddac4376fb4 100644
--- a/content/browser/frame_host/navigation_request.h
+++ b/content/browser/frame_host/navigation_request.h
@@ -269,6 +269,14 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
   // The type of SiteInstance associated with this navigation.
   AssociatedSiteInstanceType associated_site_instance_type_;
 
+  // Stores the SiteInstance created on redirects to check if there is an
+  // existing RenderProcessHost that can commit the navigation so that the
+  // renderer process is not deleted while the navigation is ongoing. If the
+  // SiteInstance was a brand new SiteInstance, it is not stored.
+  // TODO(clamy): We should use this at the navigation start as well, and delete
+  // the speculative RenderFrameHost.

[Charlie Reis, 2017/06/27 18:46:35]

I'm not entirely sure we want to do this TODO.  I chatted with Nasko to learn
that the motivation is to avoid a provisional local frame in the renderer
process, which does seem good.  However, there's a fair amount of work that we
do when creating a RenderFrameHost and RenderFrame, including a possibly large
number of IPCs to create any RenderFrameProxies it depends on.  It seems useful
to me to do this in advance, and in the common case (i.e., new process) there's
not really a provisional local frame either.

Maybe we should discuss this further before deciding to do it?  Is there a bug
filed for it where we can discuss?

[clamy, 2017/06/28 14:08:32]

I have removed the TODO.

+  scoped_refptr<SiteInstance> speculative_site_instance_;

[Charlie Reis, 2017/06/27 18:46:35]

Hmm, is there a reason we need both this and dest_site_instance_?  I notice that
dest_site_instance_ is cleared at the start of
NavigationRequest::OnRequestRedirected, and it seems like it might be a
conceptual fit for what we're doing here.  Is there a reason we need them to be
separate?

[clamy, 2017/06/28 14:08:32]

dest_site_instance_ comes from the NavigationEntry - we store it there in case
the NavigationEntry goes away while navigating. It is used when we determine the
final SiteInstance for navigation. I'm not sure it's safe to store the
SiteInstance we compute on redirects there.

[Charlie Reis, 2017/06/28 17:05:22]

Acknowledged.

+
   // Whether the request may be transferred to a different process upon commit.
   // True for browser-initiated navigations and renderer-inititated navigations
   // started via the OpenURL path.