PlzNavigate: Speculatively spawns a renderer process for navigations.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 43 matching lines @@
     RenderViewHostDelegate* render_view_delegate,
     RenderWidgetHostDelegate* render_widget_delegate,
     Delegate* delegate)
     : frame_tree_node_(frame_tree_node),
       delegate_(delegate),
       cross_navigation_pending_(false),
       render_frame_delegate_(render_frame_delegate),
       render_view_delegate_(render_view_delegate),
       render_widget_delegate_(render_widget_delegate),
       interstitial_page_(NULL),
-      weak_factory_(this) {
+      weak_factory_(this),

[Charlie Reis, 2014/12/04 21:46:28]

weak_factory_ must be last.

[carlosk, 2014/12/09 07:55:41]

Done.

+      should_reuse_web_ui_(false) {
   DCHECK(frame_tree_node_);
 }
 
 RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     CancelPending();
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    CleanUpNavigation();
+  }
+
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
 
   // Delete any swapped out RenderFrameHosts.
   STLDeleteValues(&proxy_hosts_);
 }
 
 void RenderFrameHostManager::Init(BrowserContext* browser_context,
                                   SiteInstance* site_instance,
                                   int view_routing_id,
@@ skipping 199 matching lines @@
     }
   }
   return false;
 }
 
 void RenderFrameHostManager::OnBeforeUnloadACK(
     bool for_cross_site_transition,
     bool proceed,
     const base::TimeTicks& proceed_time) {
   if (for_cross_site_transition) {
+    DCHECK(!CommandLine::ForCurrentProcess()->HasSwitch(
+        switches::kEnableBrowserSideNavigation));
     // Ignore if we're not in a cross-site navigation.
     if (!cross_navigation_pending_)
       return;
 
     if (proceed) {
       // Ok to unload the current page, so proceed with the cross-site
       // navigation.  Note that if navigations are not currently suspended, it
       // might be because the renderer was deemed unresponsive and this call was
       // already made by ShouldCloseTabOnUnresponsiveRenderer.  In that case, it
       // is ok to do nothing here.
@@ skipping 111 matching lines @@
 
   response_started_id_.reset();
 }
 
 void RenderFrameHostManager::ClearNavigationTransitionData() {
   render_frame_host_->ClearPendingTransitionRequestData();
 }
 
 void RenderFrameHostManager::DidNavigateFrame(
     RenderFrameHostImpl* render_frame_host) {
+  DCHECK(render_frame_host);
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    return;

[Charlie Reis, 2014/12/04 21:46:28]

I'm not sure this is correct.

What happens if a DidNavigate from the previous RFH arrives just after we've
decided to commit in the new one?  We'll return without doing anything here, but
the NavigationController will probably update the last committed entry to the
URL from the previous RFH.  We'll end up with a mismatch between the address bar
and the actual page contents until the new RFH commits.

Not sure what the right behavior in that case is.  If we know we've already left
the previous RFH, maybe we should ignore the commit?  I'm concerned what impact
that has in the previous renderer process, but maybe it's ok.

(Also see this CL, which is changing the logic below:
https://codereview.chromium.org/746993003/)

[clamy, 2014/12/05 17:16:19]

If we go along with committing the NavigationEntry at the same time as we swap
renderers, then the only issue would be with same-document navigations (since
all other renderer initiated navigations should go to the browser). In that
case, would the behavior ignore this same-document navigation because it arrived
after the commit of another pending navigation be ok? This implies not
committing an entry for the same-document navigation in that case as well.

[Charlie Reis, 2014/12/05 19:06:23]

I'm nervous about this as well: ignoring a commit usually leads to URL spoofs. 
Also, the renderer process keeps track of the offset and length of history,
which would be wrong in this case.  (Today it also keeps track of page IDs for
the whole history, but Avi's removing that.)

[carlosk, 2014/12/09 07:55:42]

I'm having trouble following your lines of thought here because I don't know
about the cases you are referring to here.

But it seems to me that these issues might also be resolved by moving the RFH
swapping to this place as I mentioned in a previous comment.

[clamy, 2014/12/09 15:09:51]

Indeed, it seems we are in agreement that the speculative RFH should be
committed here.

[carlosk, 2014/12/16 01:53:47]

Done.

+  }
+
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
     // We should only hear this from our current renderer.
     DCHECK_EQ(render_frame_host_, render_frame_host);
 
     // Even when there is no pending RVH, there may be a pending Web UI.
     if (pending_web_ui())
       CommitPending();
     return;
@@ skipping 121 matching lines @@
     // --site-per-process is used.
     DCHECK(is_main_frame);
 
     // The old RenderFrameHost will stay alive inside the proxy so that existing
     // JavaScript window references to it stay valid.
     proxy->TakeFrameHostOwnership(old_render_frame_host.Pass());
   }
 }
 
 void RenderFrameHostManager::DiscardUnusedFrame(
     scoped_ptr<RenderFrameHostImpl> render_frame_host) {

[carlosk, 2014/12/03 16:25:25]

Continuing the discussion that was started in crrev.com/753173002 regarding
calling RFHM::DiscardUnusedFrame for the speculative case:

On 2014/12/01 20:12:46, Charlie Reis wrote:
I had a second look at this and now I understand the importance of not calling
SwapOut on a RFH that is already in that state.

Even so it seems like the current implementation (with the fix below) is correct
as it will properly handle both situations: 
* When the RFH was gotten from the swapped out map, it will recreate the
previously deleted proxy and re-add it to the map.
* When a newly created is used it's simply deleted.

Maybe the checks for this are a little convoluted for PlzNavigate (SiteInstance
active frame count instead of is_swapped_out()) but that makes it work both in
this case and for the current navigation.

[clamy, 2014/12/03 17:24:48]

Speaking of swapped out state, does the CommitPending method set the state of
the RFH we swap in to DEFAULT? Otherwise since PlzNavigate does not call
RFHI::Navigate, we may swap back in a RFH in state swapped out. If that's the
case, we should have go back to default state before swapping it in.

[Charlie Reis, 2014/12/04 21:46:28]

Sounds good.
Good question.  CommitPending does not, so we'll need to find the right place to
do it.

[clamy, 2014/12/05 17:16:19]

One option could be to have RFH::CommitNavigation call it, in the same way
RFH::Navigate currently reset the state of the RFH before navigating. Granted,
this happens just after the RFH has been swapped in in the PlzNavigate case.

[carlosk, 2014/12/09 07:55:42]

It seems we need a test for a speculative RFH that is swapped back in... I'll
work on that next so for now I just added a failing test.

Now for the actual moment where the RFH should be set to STATE_DEFAULT I am a
tad lost. Should it be done immediately at the moment it's taken out of the
swapped-out map? It seems that was how it works for the current navigation given
how the "RFH recycling" works.
 
Or maybe it should happen only later when the RFH is actually made active?

[clamy, 2014/12/09 15:09:51]

If we go with swapping the speculative RFH in after the DidCommitProvisionalLoad
IPC was received, then we should reset its state to default in
RenderFrameHostImpl::CommitNavigation (as it is currently done in
RenderFrameHostImpl::Navigate for a pending RFH in cross-site navigations).

[Charlie Reis, 2014/12/10 22:37:44]

That sounds right.  The only reason we swapped it back in during Navigate was so
that the renderer could send navigation-related IPCs (which is no longer
necessary with PlzNavigate).

[carlosk, 2014/12/16 01:53:47]

Done.

   // TODO(carlosk): this code is very similar to what can be found in
   // SwapOutOldFrame and we should see that these are unified at some point.
 
   // If the SiteInstance for the pending RFH is being used by others don't
   // delete the RFH. Just swap it out and it can be reused at a later point.
   SiteInstanceImpl* site_instance = render_frame_host->GetSiteInstance();
   if (site_instance->HasSite() && site_instance->active_frame_count() > 1) {
     // Any currently suspended navigations are no longer needed.
     render_frame_host->CancelSuspendedNavigations();
 
     RenderFrameProxyHost* proxy =
         new RenderFrameProxyHost(site_instance, frame_tree_node_);
     proxy_hosts_[site_instance->GetId()] = proxy;
-    render_frame_host->SwapOut(proxy);
+
+    if (!render_frame_host->is_swapped_out())
+      render_frame_host->SwapOut(proxy);
+
     if (frame_tree_node_->IsMainFrame())
       proxy->TakeFrameHostOwnership(render_frame_host.Pass());
   } else {
     // We won't be coming back, so delete this one.
     render_frame_host.reset();
   }
 }
 
 void RenderFrameHostManager::MoveToPendingDeleteHosts(
     scoped_ptr<RenderFrameHostImpl> render_frame_host) {
@@ skipping 24 matching lines @@
     }
   }
   return false;
 }
 
 void RenderFrameHostManager::ResetProxyHosts() {
   STLDeleteValues(&proxy_hosts_);
 }
 
 // PlzNavigate
+void RenderFrameHostManager::BeginNavigation(

[Charlie Reis, 2014/12/04 21:46:28]

This code looks a lot like UpdateStateForNavigate, but it's missing a lot from
there.  Is the intention for this to replace that method eventually, once enough
of it is moved over?  If so, we should have a TODO to that effect.

[carlosk, 2014/12/09 07:55:42]

Both methods do check if the current SiteInstance for the navigation matches the
current/speculative SiteInstances but in different ways. Also the actions they
take from there are completely different. I was not considering merging them.

+    const FrameHostMsg_BeginNavigation_Params& params,
+    const CommonNavigationParams& common_params) {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  // If there is an ongoing navigation, cancel it.

[Charlie Reis, 2014/12/04 21:46:28]

cancel it -> cancel any state we have for it.

(RFHM is just a state machine for the process logic, so it doesn't handle
canceling the navigation itself.  Do you need the actual navigation to be
canceled as well?)

[clamy, 2014/12/05 17:16:19]

NavigatorImpl is normally responsible for canceling the actual navigation. This
should be a state clean up.

[carlosk, 2014/12/09 07:55:42]

Updated the comment.

This was more of a just-in-case action: whenever a new navigation begins any
previous state is cleaned up. It doesn't seem that there's a guarantee that the
cleanup happens otherwise.

[clamy, 2014/12/09 15:09:52]

The way the code is written, there would not have been a clean-up in the case
where a new navigation cancels an ongoing one.

I think we should have the CleanUpNavigation method be called by the destructor
of the NavigationRequest, instead of in separate places in the code. That way,
whenever a navigation is canceled, we are sure the associated state in RFHM is
cleaned up as well.

In that case, you could replace this call by a
DCHECK(!speculative_render_frame_host_.get()).

[carlosk, 2014/12/16 01:53:47]

It is already being called in NavigatorImpl::CancelNavigation, which is the
method that erases NavigationRequest instances from the request map.

But right now either of these options work right but crash or behave weirdly. So
I'll leave this as is for now and then get back to this issue once we sort out
how and when to cancel ongoing navigations upon receiving a new request. Added a
TODO for that.

+  CleanUpNavigation();
+
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  // TODO(carlosk): Replace the default values with the right ones.

[Charlie Reis, 2014/12/04 21:46:28]

I see this is the same TODO as in GetFrameHostForNavigation.  Both are a bit
vague-- are they referring to dest_is_restore and dest_is_view_source_mode? 
Let's call them out by name to make this TODO easier to fix in the future
(assuming we can't fix it now).

[clamy, 2014/12/05 17:16:19]

Note that it also concerns the SiteInstance pointer that may be provided by the
NavigationEntry.

[carlosk, 2014/12/09 07:55:42]

Updated comment.

+  scoped_refptr<SiteInstanceImpl> new_instance =
+      static_cast<SiteInstanceImpl*>(GetSiteInstanceForNavigation(
+          common_params.url, nullptr, common_params.transition, false, false));
+
+  if (new_instance.get() != current_instance &&
+      (frame_tree_node_->IsMainFrame() ||

[Charlie Reis, 2014/12/04 21:46:28]

These additional checks are non-obvious, so please add a comment for them.

They're making me realize that I'm not sure what process model you're intending
for PlzNavigate.  Today, Chrome is careful not to swap processes for most
renderer-initiated cross-site navigations (in the main frame).  This is for
important reasons, like OAuth popups where a cross-site iframe creates a popup
that needs to stay in the same process.

It looks like this check is creating a policy halfway between default Chrome
behavior and --site-per-process, in a way that breaks OAuth popups.  Perhaps we
should discuss (in a separate email?) what process model policy you all want
PlzNavigate to use, and then we can make it happen here.

[clamy, 2014/12/05 17:16:19]

In that case, we need to be able to distinguish here between a
renderer-initiated navigation and a browser-initiated one. We don't do it right
now, though it would be useful for other purposes (ie canceling or not an
ongoing navigation request). Would a TODO: do not swap processes for renderer
initiated navigations be sufficient for the time being, with the goal to revise
it when we have agreed on a process model to use?

[Charlie Reis, 2014/12/05 19:06:23]

Yes, but this is an important TODO we'll need to resolve.  Let's include a bug
number for it.

[carlosk, 2014/12/09 07:55:42]

I inverted the logic here to match the similar snippet from
GetRenderFrameForNavigation and added the proper comments. This made so that
there's a big number of return statements. I could eliminate some if it should
be OK to have if-blocks only with a DCHECK inside, is it?

Let's then discuss the correct policy we should use elsewhere.
Interesting you mentioned that. While looking at
NavigatorImpl::OnBeginNavigation I noticed there could be a problem when a
renderer-initiated navigation is requested while there's already an ongoing one.
It seemed it wouldn't cancel the old one and I understand you just confirmed
that.
I added the TODO and created the respective issue: crbug.com/440266

[clamy, 2014/12/09 15:09:51]

I think it is fine to have an if-block with just a DCHECK inside. Example in
RFHM (with a CHECK):
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
Generally the goal is to have simpler logical conditions and less indentation.
Actually it is not canceled. Instead we attempt to call
NavigationRequest::BeginNavigation again on the existing one. This fails because
of a DCHECK(!_loader.get()). I am currently working on fixing that behavior (and
have the current request be canceled by the render-initiated one if it was
user-initiated as well).

[carlosk, 2014/12/16 01:53:47]

Done!
Ack.

+       CommandLine::ForCurrentProcess()->HasSwitch(
+           switches::kSitePerProcess))) {
+    // Navigating to a new SiteInstance -> speculatively create a new RFH.
+
+    // TODO(carlosk): enable bindings check below.
+    bool success = CreateSpeculativeRenderFrameHost(
+        common_params.url, current_instance, new_instance.get(),
+        NavigationEntryImpl::kInvalidBindings);
+    if (!success)
+      return;
+    DCHECK(new_instance->GetProcess()->HasConnection());
+    DCHECK(new_instance->GetProcess()->GetBrowserContext());

[Charlie Reis, 2014/12/04 21:46:28]

This second check seems unnecessary.  Is there a way it could not have a
BrowserContext?

[carlosk, 2014/12/09 07:55:42]

I was going to say this was a copy/paste from another place but I can't find it
anymore. So I just removed this and the next one.

+  } else {

[Charlie Reis, 2014/12/04 21:46:28]

nit: Return here inside the previous block, so that the rest does not need to be
inside an else block.  (Similar to UpdateStateForNavigate.)

[carlosk, 2014/12/09 07:55:42]

Done.

+    // Navigating to the same SiteInstance -> make sure the current RFH is
+    // alive.
+    if (!render_frame_host_->render_view_host()->IsRenderViewLive()) {
+      // Recreate the opener chain.
+      int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
+          render_frame_host_->GetSiteInstance());
+      if (!InitRenderView(render_frame_host_->render_view_host(),
+                          opener_route_id, MSG_ROUTING_NONE,
+                          frame_tree_node_->IsMainFrame())) {
+        return;
+      }
+    }
+    DCHECK(current_instance->GetProcess()->HasConnection());
+    DCHECK(current_instance->GetProcess()->GetBrowserContext());
+  }
+}
+
+// PlzNavigate
+bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
+    const GURL& url,
+    SiteInstance* old_instance,
+    SiteInstance* new_instance,
+    int bindings) {
+  CHECK(new_instance);
+  CHECK_NE(old_instance, new_instance);
+
+  const NavigationEntry* current_navigation_entry =
+      delegate_->GetLastCommittedNavigationEntryForRenderManager();
+  scoped_ptr<WebUIImpl> new_web_ui;
+  should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry, url);
+  if (!should_reuse_web_ui_)
+    new_web_ui = CreateWebUI(url, bindings);
+
+  int opener_route_id =
+      CreateOpenerRenderViewsIfNeeded(old_instance, new_instance);
+
+  int create_render_frame_flags = 0;
+  if (frame_tree_node_->IsMainFrame())
+    create_render_frame_flags |= CREATE_RF_FOR_MAIN_FRAME_NAVIGATION;
+  if (delegate_->IsHidden())
+    create_render_frame_flags |= CREATE_RF_HIDDEN;
+  scoped_ptr<RenderFrameHostImpl> new_render_frame_host =
+      CreateRenderFrame(new_instance, new_web_ui.get(), opener_route_id,
+                        create_render_frame_flags, nullptr);
+  if (!new_render_frame_host) {
+    return false;
+  }
+  speculative_render_frame_host_.reset(new_render_frame_host.release());
+  speculative_web_ui_.reset(new_web_ui.release());
+  return true;
+}
+
+// PlzNavigate
 RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
       const GURL& url,
       ui::PageTransition transition) {
   CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   // TODO(clamy): When we handle renderer initiated navigations, make sure not
   // to use a different process for subframes if --site-per-process is not
   // enabled.
 
   // Pick the right RenderFrameHost to commit the navigation.
@@ skipping 11 matching lines @@
     if (!InitRenderView(render_frame_host->render_view_host(),
                         opener_route_id,
                         MSG_ROUTING_NONE,
                         frame_tree_node_->IsMainFrame())) {
       return NULL;
     }
   }
   return render_frame_host;
 }
 
+// PlzNavigate
+void RenderFrameHostManager::CleanUpNavigation() {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  if (speculative_render_frame_host_) {
+    speculative_render_frame_host_->GetProcess()->RemovePendingView();
+    DiscardUnusedFrame(speculative_render_frame_host_.Pass());
+  }
+  if (speculative_web_ui_)
+    speculative_web_ui_.reset();
+  should_reuse_web_ui_ = false;
+}
+
 void RenderFrameHostManager::Observe(
     int type,
     const NotificationSource& source,
     const NotificationDetails& details) {
   switch (type) {
     case NOTIFICATION_RENDERER_PROCESS_CLOSED:
     case NOTIFICATION_RENDERER_PROCESS_CLOSING:
       RendererProcessClosing(
           Source<RenderProcessHost>(source).ptr());
       break;
@@ skipping 440 matching lines @@
   // longer relies on swapped out RFH for the top-level frame.
   if (!frame_tree_node_->IsMainFrame()) {
     CHECK(!swapped_out);
   }
 
   scoped_ptr<RenderFrameHostImpl> new_render_frame_host;
   bool success = true;
   if (view_routing_id_ptr)
     *view_routing_id_ptr = MSG_ROUTING_NONE;
 
-  // We are creating a pending or swapped out RFH here. We should never create
-  // it in the same SiteInstance as our current RFH.
+  // We are creating a pending, speculative or swapped out RFH here. We should
+  // never create it in the same SiteInstance as our current RFH.
   CHECK_NE(render_frame_host_->GetSiteInstance(), instance);
 
   // Check if we've already created an RFH for this SiteInstance.  If so, try
   // to re-use the existing one, which has already been initialized.  We'll
   // remove it from the list of proxy hosts below if it will be active.
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
   if (proxy && proxy->render_frame_host()) {
     if (view_routing_id_ptr)
       *view_routing_id_ptr = proxy->GetRenderViewHost()->GetRoutingID();
     // Delete the existing RenderFrameProxyHost, but reuse the RenderFrameHost.
@@ skipping 162 matching lines @@
 
 void RenderFrameHostManager::CommitPending() {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CommitPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
   // First check whether we're going to want to focus the location bar after
   // this commit.  We do this now because the navigation hasn't formally
   // committed yet, so if we've already cleared |pending_web_ui_| the call chain
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
-  // Next commit the Web UI, if any. Either replace |web_ui_| with
-  // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
-  // leave |web_ui_| as is if reusing it.
-  DCHECK(!(pending_web_ui_.get() && pending_and_current_web_ui_.get()));
-  if (pending_web_ui_) {
-    web_ui_.reset(pending_web_ui_.release());
-  } else if (!pending_and_current_web_ui_.get()) {
-    web_ui_.reset();
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(

[Charlie Reis, 2014/12/04 21:46:28]

Please cache this in a bool so we don't repeatedly call it below.  (That will
also be easier to read.)

[carlosk, 2014/12/09 07:55:42]

Ha! I had just undone that. :) Re-added the boolean and added comments to make
it explicit that the else-blocks are PlzNavigate-only.

+          switches::kEnableBrowserSideNavigation)) {
+    DCHECK(!speculative_web_ui_);
+    // Next commit the Web UI, if any. Either replace |web_ui_| with
+    // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
+    // leave |web_ui_| as is if reusing it.
+    DCHECK(!(pending_web_ui_.get() && pending_and_current_web_ui_.get()));
+    if (pending_web_ui_) {
+      web_ui_.reset(pending_web_ui_.release());
+    } else if (!pending_and_current_web_ui_.get()) {
+      web_ui_.reset();
+    } else {
+      DCHECK_EQ(pending_and_current_web_ui_.get(), web_ui_.get());
+      pending_and_current_web_ui_.reset();
+    }
   } else {
-    DCHECK_EQ(pending_and_current_web_ui_.get(), web_ui_.get());
-    pending_and_current_web_ui_.reset();
+    if (!should_reuse_web_ui_)
+      web_ui_.reset(speculative_web_ui_.release());

[Charlie Reis, 2014/12/04 21:46:28]

Let's add a DCHECK(!speculative_web_ui_) after this block, which will verify
that it didn't exist if should_reuse_web_ui_ was true.

[carlosk, 2014/12/09 07:55:42]

Done.

   }
 
   // It's possible for the pending_render_frame_host_ to be NULL when we aren't
   // crossing process boundaries. If so, we just needed to handle the Web UI
   // committing above and we're done.
-  if (!pending_render_frame_host_) {
+  if (!pending_render_frame_host_ &&
+      !CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {

[Charlie Reis, 2014/12/04 21:46:28]

Can we make this !pending_render_frame_host_ && !speculative_render_frame_host_
instead?  That would be easier to reason about.

[carlosk, 2014/12/09 07:55:42]

Makes sense. Done.

     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
   }
 
   // Remember if the page was focused so we can focus the new renderer in
   // that case.
   bool focus_render_view = !will_focus_location_bar &&
       render_frame_host_->render_view_host()->GetView() &&
       render_frame_host_->render_view_host()->GetView()->HasFocus();
 
   bool is_main_frame = frame_tree_node_->IsMainFrame();
 
-  // Swap in the pending frame and make it active. Also ensure the FrameTree
-  // stays in sync.
-  scoped_ptr<RenderFrameHostImpl> old_render_frame_host =
-      SetRenderFrameHost(pending_render_frame_host_.Pass());
+  scoped_ptr<RenderFrameHostImpl> old_render_frame_host;
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    DCHECK(!speculative_render_frame_host_);
+    // Swap in the pending frame and make it active. Also ensure the FrameTree
+    // stays in sync.
+    old_render_frame_host =
+        SetRenderFrameHost(pending_render_frame_host_.Pass());
+  } else {
+    DCHECK(speculative_render_frame_host_);
+    old_render_frame_host =
+        SetRenderFrameHost(speculative_render_frame_host_.Pass());
+  }
+
   if (is_main_frame)
     render_frame_host_->render_view_host()->AttachToFrameTree();
 
   // The process will no longer try to exit, so we can decrement the count.
   render_frame_host_->GetProcess()->RemovePendingView();
 
   // Show the new view (or a sad tab) if necessary.
   bool new_rfh_has_view = !!render_frame_host_->render_view_host()->GetView();
   if (!delegate_->IsHidden() && new_rfh_has_view) {
     // In most cases, we need to show the new view.
@@ skipping 108 matching lines @@
   if (cross_navigation_pending_) {
     if (pending_render_frame_host_)
       CancelPending();
     cross_navigation_pending_ = false;
   }
 
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
       url, instance, transition, is_restore, is_view_source_mode);
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(

[Charlie Reis, 2014/12/04 21:46:28]

This doesn't feel like it fits here.  Most of UpdateStateForNavigate (below this
block) ends up not being used in PlzNavigate at all.  In that case, why not put
this logic in GetFrameHostForNavigation, and just copy the first few lines above
there as well?  (We've already copied them into BeginNavigation.)

[carlosk, 2014/12/09 07:55:42]

Makes total sense and so I moved it.

It might need some adjustment if we decide to move part of this to the
commit-provisional-load time.

+          switches::kEnableBrowserSideNavigation)) {
+    if (current_instance == new_instance.get() ||
+        (!frame_tree_node_->IsMainFrame() &&

[Charlie Reis, 2014/12/04 21:46:28]

Same concern about the process model policy.

[carlosk, 2014/12/09 07:55:41]

Done: TODO + reference in crbug.com/440266 .

+         !CommandLine::ForCurrentProcess()->HasSwitch(
+             switches::kSitePerProcess))) {
+      CleanUpNavigation();
+    } else {
+      // If the SiteInstance for the final URL doesn't match the one from the
+      // speculatively created RenderFrameHost, create a new one using the
+      // former.
+      if (!speculative_render_frame_host_ ||
+          speculative_render_frame_host_->GetSiteInstance() !=
+              new_instance.get()) {
+        CleanUpNavigation();
+        // TODO(carlosk): Should rename this method and the speculative members

[Charlie Reis, 2014/12/04 21:46:28]

I'm not concerned about the name.  We're creating them and then immediately
committing them, so it's not visible outside of this block.  We could
restructure so that CreateSpeculativeRenderFrameHost is a method that returns an
RFH instead of assigning it, but I'm not sure if that's worthwhile.

[carlosk, 2014/12/09 07:55:41]

Great! Later on I realized that as soon as we begin to monitor redirects all
created RFH are speculative until the last one. And this part here in fact will
cease to exist.

+        // because in this case they are not speculative. Suggestions are
+        // very welcome!
+        bool success = CreateSpeculativeRenderFrameHost(
+            url, current_instance, new_instance.get(), bindings);
+        if (!success)
+          return nullptr;
+      }
+      DCHECK(speculative_render_frame_host_);
+      CommitPending();
+      DCHECK(!speculative_render_frame_host_);
+    }
+    return render_frame_host_.get();
+  }
+
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
 
   if (new_instance.get() != current_instance) {
     TRACE_EVENT_INSTANT2(
         "navigation",
         "RenderFrameHostManager::UpdateStateForNavigate:New SiteInstance",
         TRACE_EVENT_SCOPE_THREAD,
         "current_instance id", current_instance->GetId(),
         "new_instance id", new_instance->GetId());
@@ skipping 205 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content