PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects

content/browser/frame_host/navigation_request.h

 // Copyright 2014 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.
 
 #ifndef CONTENT_BROWSER_FRAME_HOST_NAVIGATION_REQUEST_H_
 #define CONTENT_BROWSER_FRAME_HOST_NAVIGATION_REQUEST_H_
 
 #include <memory>
 
 #include "base/callback_forward.h"
@@ skipping 251 matching lines @@
   int bindings_;
   int nav_entry_id_ = 0;
 
   // 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_;
 
   // 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.
   // Note: the RenderFrameHostManager may still decide to have the navigation
   // commit in a different renderer process if it detects that a renderer
   // transfer is needed. This is the case in particular when --site-per-process
   // is enabled.
   bool may_transfer_;
 
   std::unique_ptr<NavigationHandleImpl> navigation_handle_;
@@ skipping 12 matching lines @@
   mojom::URLLoaderFactoryPtrInfo subresource_loader_factory_info_;
 
   base::WeakPtrFactory<NavigationRequest> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NavigationRequest);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_NAVIGATION_REQUEST_H_