PlzNavigate: Speculatively spawns a renderer process for navigations.

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 49b9495e32d2cb4d0da64a6b03a606afce8c3006..3de39fac6086c342b3eb29d3c9c7a4e3c6c7b315 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -61,7 +61,8 @@ RenderFrameHostManager::RenderFrameHostManager(
       render_view_delegate_(render_view_delegate),
       render_widget_delegate_(render_widget_delegate),
       interstitial_page_(NULL),
-      weak_factory_(this) {
+      weak_factory_(this),
+      should_reuse_web_ui_(false) {
   DCHECK(frame_tree_node_);
 }
 
@@ -69,6 +70,11 @@ RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     CancelPending();
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    CleanUpSpeculativeRenderFrameHost();
+  }
+
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
 
@@ -136,23 +142,25 @@ RenderFrameProxyHost* RenderFrameHostManager::GetProxyToParent() {
   return iter->second;
 }
 
-void RenderFrameHostManager::SetPendingWebUI(const GURL& url,
-                                             int bindings) {
-  pending_web_ui_.reset(
-      delegate_->CreateWebUIForRenderManager(url));
+void RenderFrameHostManager::SetPendingWebUI(const GURL& url, int bindings) {
+  pending_web_ui_.reset(CreateWebUI(url, bindings));
   pending_and_current_web_ui_.reset();
+}
+
+WebUIImpl* RenderFrameHostManager::CreateWebUI(const GURL& url, int bindings) {
+  WebUIImpl* new_web_ui = delegate_->CreateWebUIForRenderManager(url);
 
   // If we have assigned (zero or more) bindings to this NavigationEntry in the
   // past, make sure we're not granting it different bindings than it had
   // before.  If so, note it and don't give it any bindings, to avoid a
   // potential privilege escalation.
-  if (pending_web_ui_.get() &&
-      bindings != NavigationEntryImpl::kInvalidBindings &&
-      pending_web_ui_->GetBindings() != bindings) {
-    RecordAction(
-        base::UserMetricsAction("ProcessSwapBindingsMismatch_RVHM"));
-    pending_web_ui_.reset();
+  if (new_web_ui && bindings != NavigationEntryImpl::kInvalidBindings &&
+      new_web_ui->GetBindings() != bindings) {
+    RecordAction(base::UserMetricsAction("ProcessSwapBindingsMismatch_RVHM"));
+    delete new_web_ui;
+    return NULL;

[nasko, 2014/11/19 01:00:23]

nit: nullptr

[carlosk, 2014/11/19 17:24:29]

I was in doubt about this as I understood we are changing to using nullptr but
we also tend to try to keep the standard that is currently used (NULL in this
file). So, should I replace all cases?

[nasko, 2014/11/19 19:01:29]

New code should be using current(new) conventions, which means nullptr. Existing
code shouldn't be modified for the sake of migrating to the new style. So please
don't replace all cases, but update code you have written/touched.

[carlosk, 2014/11/21 14:36:34]

Acknowledged and done.

   }
+  return new_web_ui;
 }
 
 RenderFrameHostImpl* RenderFrameHostManager::Navigate(
@@ -307,6 +315,10 @@ void RenderFrameHostManager::OnBeforeUnloadACK(
       // Current page says to cancel.
       CancelPending();
       cross_navigation_pending_ = false;
+      if (CommandLine::ForCurrentProcess()->HasSwitch(

[clamy, 2014/11/19 15:03:29]

We should not receive a beforeunload ack in the case of a cross-site transition
when PlzNavigate is enabled. The goal is to have the ack included in
BeginNavigation. Therefore this block unnecessary, and could be replaced with a
DCHECK(!CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableBrowserSideNavigation)) at the beginning of the block for
cross-site transition.

As for the second added block, I think that it should prove to be unnecessary as
well. The speculative render frame host exists between BeginNavigation and
CommitNavigation. So the BeforeUnload event for the old page has already
executed (in response to RequestNavigation from the browser), and we don't have
a new page committed yet. Therefore, we should not be waiting for the
BeforeUnload event to execute when trying to close the page. In practice, I
think we do. But this should not be the case.

[carlosk, 2014/11/19 17:24:29]

Done for the 1st one.

I didn't understood what you meant about the 2nd block.

But FYI, I added all these added clean-up calls just following the thought that
"whenever a navigation is cancelled, speculative stuff should be cleared".

[carlosk, 2014/11/21 14:36:33]

Removed second block as discussed in person.

+              switches::kEnableBrowserSideNavigation)) {
+        CleanUpSpeculativeRenderFrameHost();
+      }
     }
   } else {
     // Non-cross site transition means closing the entire tab.
@@ -321,6 +333,10 @@ void RenderFrameHostManager::OnBeforeUnloadACK(
       if (pending_render_frame_host_) {
         CancelPending();
         cross_navigation_pending_ = false;
+        if (CommandLine::ForCurrentProcess()->HasSwitch(
+                switches::kEnableBrowserSideNavigation)) {
+          CleanUpSpeculativeRenderFrameHost();
+        }
       }
 
       // This is not a cross-site navigation, the tab is being closed.
@@ -419,6 +435,12 @@ void RenderFrameHostManager::ClearNavigationTransitionData() {
 
 void RenderFrameHostManager::DidNavigateFrame(
     RenderFrameHostImpl* render_frame_host) {
+  DCHECK(render_frame_host);
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    return;
+  }
+
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
@@ -427,13 +449,13 @@ void RenderFrameHostManager::DidNavigateFrame(
 
     // Even when there is no pending RVH, there may be a pending Web UI.
     if (pending_web_ui())
-      CommitPending();
+      CommitPending(false);
     return;
   }
 
   if (render_frame_host == pending_render_frame_host_) {
     // The pending cross-site navigation completed, so show the renderer.
-    CommitPending();
+    CommitPending(false);
     cross_navigation_pending_ = false;
   } else if (render_frame_host == render_frame_host_) {
     // A navigation in the original page has taken place.  Cancel the pending
@@ -446,6 +468,10 @@ void RenderFrameHostManager::DidNavigateFrame(
   }
 }
 
+void RenderFrameHostManager::CancelNavigation() {
+  CleanUpSpeculativeRenderFrameHost();

[clamy, 2014/11/19 15:03:29]

Considering that CancelNavigation is now just a call to
CleanUpSpeculativeRenderFrameHost, how about removing
CleanUpSpeculativeRenderFrameHost entirely?

[carlosk, 2014/11/19 17:24:29]

My reasoning is:
- From a public perspective, one should want to cancel a navigation and not
fiddle with internal state. So "cancel" is clearer.
- Internally I wanted to tell apart the cases when the intention is to cancel
the navigation from the ones where I just want to clean-up the speculative
stuff.
- Also internally I'm expecting that moving forward the "cancel" operation will
involve more things.

[nasko, 2014/11/19 19:01:29]

We shouldn't be speculating about the future. Let's keep code simple until we
really need it. I do agree the cleanup should be inlined here.

[carlosk, 2014/11/21 14:36:34]

Done. After discussing with clamy@ I left a single public method called
CleanUpNavigation. Reason is that it's called internally midway through the
navigation and it would be weird to invoke CancelNavigation when it's not being
cancelled at all.

+}
+
 void RenderFrameHostManager::DidDisownOpener(
     RenderFrameHost* render_frame_host) {
   // Notify all RenderFrameHosts but the one that notified us. This is necessary
@@ -559,6 +585,30 @@ void RenderFrameHostManager::SwapOutOldFrame(
   }
 }
 
+void RenderFrameHostManager::RecycleRenderFrameHost(
+    scoped_ptr<RenderFrameHostImpl> render_frame_host) {
+  // 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 (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) {
   // |render_frame_host| will be deleted when its SwapOut ACK is received, or
@@ -594,6 +644,80 @@ void RenderFrameHostManager::ResetProxyHosts() {
   STLDeleteValues(&proxy_hosts_);
 }
 
+void RenderFrameHostManager::BeginNavigation(

[clamy, 2014/11/19 15:03:29]

nit: Add above // PlzNavigate

[carlosk, 2014/11/19 17:24:29]

Done.

[carlosk, 2014/11/21 14:36:33]

Done.

+    const FrameHostMsg_BeginNavigation_Params& params,
+    const CommonNavigationParams& common_params) {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  // If there is an ongoing navigation, cancel it.
+  CancelNavigation();
+
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  // TODO(carlosk): Replace the default values by the right ones.
+  scoped_refptr<SiteInstanceImpl> new_instance =
+      static_cast<SiteInstanceImpl*>(GetSiteInstanceForNavigation(
+          common_params.url, NULL, common_params.transition, false, false));
+
+  if (new_instance.get() != current_instance) {
+    // Navigating to a new SiteInstance -> speculatively create a new RFH

[clamy, 2014/11/19 15:03:29]

nit: add a . at the end of the sentence.

[carlosk, 2014/11/19 17:24:29]

Done.

[carlosk, 2014/11/21 14:36:34]

Done.

+
+    // TODO(carlosk): what the TRACE_EVENT_INSTANT2 for New SiteInstance found
+    // in UpdateStateForNavigate be copied here?

[nasko, 2014/11/19 01:00:23]

I think we discussed this in the sync-up meeting. You can copy it over and
change the strings as appropriate.

[carlosk, 2014/11/19 17:24:29]

Yes, and I will. :)

I rephrased the TODO to make it clearer.

[carlosk, 2014/11/21 14:36:33]

It was indeed a "bad" TODO so I removed it for now and will add TRACE_EVENTs
later.

+
+    // TODO(carlosk): enable bindings check below

[clamy, 2014/11/19 15:03:29]

nit: add a . at the end of the comment.

[carlosk, 2014/11/19 17:24:29]

Done.

[carlosk, 2014/11/21 14:36:33]

Done.

+    bool success = CreateSpeculativeRenderFrameHost(
+        common_params.url, current_instance, new_instance.get(),
+        NavigationEntryImpl::kInvalidBindings);
+    if (!success)
+      return;
+  } else {
+    // Navigating to the same SiteInstance -> make sure the current RFH is alive

[clamy, 2014/11/19 15:03:29]

nit: add a . at the end of the comment.

[carlosk, 2014/11/19 17:24:29]

Done.

[carlosk, 2014/11/21 14:36:34]

Done.

+    DCHECK(render_frame_host_->GetSiteInstance() == new_instance);
+    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(new_instance->GetProcess()->HasConnection());
+  DCHECK(new_instance->GetProcess()->GetBrowserContext());
+}
+
+bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(

[clamy, 2014/11/19 15:03:29]

nit: add above // PlzNavigate

[carlosk, 2014/11/19 17:24:29]

Done.

[carlosk, 2014/11/21 14:36:34]

Done.

+    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.reset(CreateWebUI(url, bindings));
+
+  int opener_route_id =
+      GetOpenerRouteIDForNewRenderView(old_instance, new_instance);
+
+  scoped_ptr<RenderFrameHostImpl> new_render_frame_host =
+      CreateRenderFrameInternal(new_instance, opener_route_id, false,
+                                frame_tree_node_->IsMainFrame(),
+                                delegate_->IsHidden(), new_web_ui.get(), NULL);
+  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,
@@ -789,13 +913,8 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
       dest_is_view_source_mode);
   if (ShouldTransitionCrossSite() || force_swap) {
     new_instance = GetSiteInstanceForURL(
-        dest_url,
-        dest_instance,
-        dest_transition,
-        dest_is_restore,
-        dest_is_view_source_mode,
-        current_instance,
-        force_swap);
+        dest_url, dest_instance, dest_transition, dest_is_restore,

[nasko, 2014/11/19 01:00:23]

No need to change format of code you aren't modifying.

[carlosk, 2014/11/19 17:24:29]

Reverted.

This was a consequence of my previous changing this method and running git cl
format which now prefers these kind of formatting.

[carlosk, 2014/11/21 14:36:33]

Done.

+        dest_is_view_source_mode, current_instance, force_swap);
   }
 
   // If force_swap is true, we must use a different SiteInstance.  If we didn't,
@@ -996,9 +1115,22 @@ void RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(
     SiteInstance* old_instance,
     SiteInstance* new_instance,
     bool is_main_frame) {
-  // Ensure that we have created RFHs for the new RFH's opener chain if
-  // we are staying in the same BrowsingInstance. This allows the new RFH
-  // to send cross-process script calls to its opener(s).
+  int opener_route_id =
+      GetOpenerRouteIDForNewRenderView(old_instance, new_instance);
+  // TODO(carlosk): does this "earlier" call for CancelPending affects anything?
+  // It used to happen inside the creation method iff the new RFH was

[clamy, 2014/11/19 15:03:29]

nit: s/iff/if

[carlosk, 2014/11/19 17:24:29]

This is not a typo: iff == if and only if (http://en.wikipedia.org/wiki/iff)

Also, this is a temporary TODO and should be removed before commit.

+  // successfully created.
+  if (pending_render_frame_host_)
+    CancelPending();

[nasko, 2014/11/19 01:00:23]

Why is this needed?

[carlosk, 2014/11/19 17:24:29]

See lines 1143-1144 from the previous version: this used to be done in
RFHM::CreateRenderFrame that I refactored to me more generic so I'm just trying
to keep the old behavior as much as possible. That's why I added the TODO above
as well.

+  // Create a non-swapped-out RFH with the given opener.
+  pending_render_frame_host_ = CreateRenderFrameInternal(
+      new_instance, opener_route_id, false, is_main_frame,
+      delegate_->IsHidden(), pending_web_ui(), NULL);
+}
+
+int RenderFrameHostManager::GetOpenerRouteIDForNewRenderView(

[nasko, 2014/11/19 01:00:23]

This method does a lot more than GetOpenerRouteID. 

[carlosk, 2014/11/19 17:24:29]

Agreed. I extracted the behavior I wanted to share and might have chosen a poor
name for the method. But in fact I have no idea of what to call this...
Suggestions?

[nasko, 2014/11/19 19:01:29]

For one, you lost a very important comment that was prior to the code you've
moved around. It will probably be a good source of inspiration of whether to
factor this out into its own method or embed somewhere else.

[carlosk, 2014/11/21 14:36:33]

I didn't lose it. As that comment seemed to describe the logic I extracted I
moved it to be the documentation for this method's declaration in the header.

I renamed the method to CreateOpenerRenderViewsIfNeeded. Does this sounds better
or should I simply not extract this into a new method?

+    SiteInstance* old_instance,
+    SiteInstance* new_instance) {
   int opener_route_id = MSG_ROUTING_NONE;
   if (new_instance->IsRelatedSiteInstance(old_instance)) {
     opener_route_id =
@@ -1011,15 +1143,7 @@ void RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(
           frame_tree_node_, new_instance);
     }
   }
-
-  // Create a non-swapped-out RFH with the given opener.
-  int route_id = CreateRenderFrame(
-      new_instance, opener_route_id, false, is_main_frame,
-      delegate_->IsHidden());
-  if (route_id == MSG_ROUTING_NONE) {
-    pending_render_frame_host_.reset();
-    return;
-  }
+  return opener_route_id;
 }
 
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
@@ -1054,11 +1178,25 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
   return render_frame_host.Pass();
 }
 
-int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
-                                              int opener_route_id,
-                                              bool swapped_out,
-                                              bool for_main_frame_navigation,
-                                              bool hidden) {
+int RenderFrameHostManager::CreateSwappedOutHiddenRenderFrame(

[nasko, 2014/11/19 01:00:24]

I don't see a clear advantage to having this wrapper, especially since the
booleans were refactored into flags.

[carlosk, 2014/11/19 17:24:29]

Calls made outside of RFHM are all swapped-out and consequentially must be also
hidden. The only variation was the for_main_frame_navigation parameter. I prefer
to keep the publicly available methods as simple as possible hence the change.

Note that this was written before I was aware of the incoming change. Even so I
did talk with the owner of that CL and he was also OK with this. He confirmed
the new render frame creation option flag that he will add soon will also be
only internal to RFHM.

I did already rebase my changes locally and this looks like an good
simplification to me (extracted from web_contents_impl.cc):

-  return GetRenderManager()->CreateRenderFrame(
-      instance, opener_route_id, CREATE_RF_FOR_MAIN_FRAME_NAVIGATION |
-                                     CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN);
+  return GetRenderManager()->CreateSwappedOutHiddenRenderFrame(
+      instance, opener_route_id, true);

[nasko, 2014/11/19 19:01:29]

While I agree this is in general a very good goal to aim for, I think in this
case this is code churn that isn't needed. WebContents and RFHM are very closely
tied together, so I don't see the advantage of adding a one-line wrapper. It
actually adds more overhead when reading the code.
Sure.
 
Until you need to read the code and jump through two levels to get to the actual
implementation. Also, since this code is hopefully going to go away in the
not-so-distant future, I don't think it is worth "improving" it right now.

[carlosk, 2014/11/21 14:36:33]

OK, I reverted to having one single public method. But the callers had to be
adapted to the changed signature needed for it to be re-usable by the
speculative RFH logic.

+    SiteInstance* instance,
+    int opener_route_id,
+    bool for_main_frame_navigation) {
+  int routing_id = MSG_ROUTING_NONE;
+  CreateRenderFrameInternal(instance, opener_route_id, true,
+                            for_main_frame_navigation, true, NULL, &routing_id);
+  return routing_id;
+}
+
+scoped_ptr<RenderFrameHostImpl>
+RenderFrameHostManager::CreateRenderFrameInternal(

[clamy, 2014/11/19 15:03:29]

Note: In theory, methods in the .cc file should have the same order as in the
header file, and here it is completely wrong. I know that the initial file was
not perfect in that regard, but we're adding even more mess. So please make it
less so. In particular, this function needs to move down, and so does
CreateSpeculativeRenderFrameHost.

[carlosk, 2014/11/19 17:24:28]

Acknowledged and done. From now on I will follow that rule of thumb instead of
trying to keep related methods together.

[nasko, 2014/11/19 19:01:29]

You can still keep them together, just reorder the header and .cc to match.

[carlosk, 2014/11/21 14:36:33]

I couldn't really as one method was public and the other private. But it's a
moot point now as there's only one.

+    SiteInstance* instance,
+    int opener_route_id,
+    bool swapped_out,
+    bool for_main_frame_navigation,
+    bool hidden,
+    const WebUIImpl* web_ui,
+    int* routing_id_ptr) {
   CHECK(instance);
   DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.
 
@@ -1069,20 +1207,21 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
   }
 
   scoped_ptr<RenderFrameHostImpl> new_render_frame_host;
-  RenderFrameHostImpl* frame_to_announce = NULL;
-  int routing_id = MSG_ROUTING_NONE;
+  bool success = true;
+  if (routing_id_ptr)
+    *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()) {
-    routing_id = proxy->GetRenderViewHost()->GetRoutingID();
+    if (routing_id_ptr)
+      *routing_id_ptr = proxy->GetRenderViewHost()->GetRoutingID();
     // Delete the existing RenderFrameProxyHost, but reuse the RenderFrameHost.
     // Prevent the process from exiting while we're trying to use it.
     if (!swapped_out) {
@@ -1096,13 +1235,13 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
       // gets a RenderViewHost in the SiteInstance of its opener WebContents.
       // If not used in the first navigation, this RVH is swapped out and is not
       // granted bindings, so we may need to grant them when swapping it in.
-      if (pending_web_ui() &&
-          !new_render_frame_host->GetProcess()->IsIsolatedGuest()) {
-        int required_bindings = pending_web_ui()->GetBindings();
-        RenderViewHost* rvh = new_render_frame_host->render_view_host();
-        if ((rvh->GetEnabledBindings() & required_bindings) !=
-                required_bindings) {
-          rvh->AllowBindings(required_bindings);
+      if (web_ui && !new_render_frame_host->GetProcess()->IsIsolatedGuest()) {
+        int required_bindings = web_ui->GetBindings();
+        RenderViewHost* render_view_host =
+            new_render_frame_host->render_view_host();
+        if ((render_view_host->GetEnabledBindings() & required_bindings) !=
+            required_bindings) {
+          render_view_host->AllowBindings(required_bindings);
         }
       }
     }
@@ -1127,10 +1266,8 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
         proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
     }
 
-    bool success = InitRenderView(render_view_host,
-                                  opener_route_id,
-                                  proxy_routing_id,
-                                  for_main_frame_navigation);
+    success = InitRenderView(render_view_host, opener_route_id,
+                             proxy_routing_id, for_main_frame_navigation);
     if (success) {
       if (frame_tree_node_->IsMainFrame()) {
         // Don't show the main frame's view until we get a DidNavigate from it.
@@ -1140,22 +1277,27 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
         DCHECK(new_render_frame_host.get());
         success = InitRenderFrame(new_render_frame_host.get());
       }
-    } else if (!swapped_out && pending_render_frame_host_) {
-      CancelPending();
+      if (success) {
+        if (routing_id_ptr)
+          *routing_id_ptr = render_view_host->GetRoutingID();
+        // If a brand new RFH was created, announce it to observers.
+        // TODO(carlosk): verify there's no problem that now this RFH will only
+        // be set as the pending one *after* this delegate call (used to be
+        // before).
+        if (new_render_frame_host) {
+          render_frame_delegate_->RenderFrameCreated(
+              new_render_frame_host.get());
+        }
+      }
     }
-    routing_id = render_view_host->GetRoutingID();
-    frame_to_announce = new_render_frame_host.get();
   }
 
-  // Use this as our new pending RFH if it isn't swapped out.
-  if (!swapped_out)
-    pending_render_frame_host_ = new_render_frame_host.Pass();
-
-  // If a brand new RFH was created, announce it to observers.
-  if (frame_to_announce)
-    render_frame_delegate_->RenderFrameCreated(frame_to_announce);
-
-  return routing_id;
+  // Returns the new RFH if it isn't swapped out.
+  if (success && !swapped_out) {
+    DCHECK(new_render_frame_host->GetSiteInstance() == instance);
+    return new_render_frame_host.Pass();
+  }
+  return NULL;
 }
 
 int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
@@ -1197,10 +1339,9 @@ bool RenderFrameHostManager::InitRenderView(
     }
   }
 
-  return delegate_->CreateRenderViewForRenderManager(render_view_host,
-                                                     opener_route_id,
-                                                     proxy_routing_id,
-                                                     for_main_frame_navigation);
+  return delegate_->CreateRenderViewForRenderManager(

[clamy, 2014/11/19 15:03:29]

You don't need to change formating for code that is not modified.

[carlosk, 2014/11/19 17:24:29]

Reverted. Same reason as before...

[carlosk, 2014/11/21 14:36:33]

Done.

+      render_view_host, opener_route_id, proxy_routing_id,
+      for_main_frame_navigation);
 }
 
 bool RenderFrameHostManager::InitRenderFrame(
@@ -1246,32 +1387,43 @@ int RenderFrameHostManager::GetRoutingIdForSiteInstance(
   return MSG_ROUTING_NONE;
 }
 
-void RenderFrameHostManager::CommitPending() {
+void RenderFrameHostManager::CommitPending(bool use_speculative_rfh) {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CommitPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
+  // If use_speculative_rfh then kEnableBrowserSideNavigation must be enabled.
+  CHECK(!use_speculative_rfh ||
+        CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kEnableBrowserSideNavigation));
+
   // 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 (!use_speculative_rfh) {
+    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());
   }
 
   // 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_ && !use_speculative_rfh) {
     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
@@ -1285,10 +1437,19 @@ void RenderFrameHostManager::CommitPending() {
 
   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 (!use_speculative_rfh) {
+    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();
 
@@ -1417,6 +1578,33 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
       url, instance, transition, is_restore, is_view_source_mode);
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    if (current_instance == new_instance.get()) {
+      CleanUpSpeculativeRenderFrameHost();
+    } else {
+      // If the SiteInstance for the final URL doesn't match the one form the
+      // speculatively created RenderFrameHost, create a new one using the
+      // former.
+      if (!speculative_render_frame_host_ ||
+          speculative_render_frame_host_->GetSiteInstance() !=
+              new_instance.get()) {
+        CleanUpSpeculativeRenderFrameHost();
+        // TODO(carlosk): Should rename this method and the speculative members
+        // 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 NULL;
+      }
+      DCHECK(speculative_render_frame_host_);
+      CommitPending(true);
+      DCHECK(!speculative_render_frame_host_);
+    }
+    return render_frame_host_.get();
+  }
+
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
 
@@ -1452,7 +1640,7 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
         // navigate.  Just switch to the pending RFH now and go back to non
         // cross-navigating (Note that we don't care about on{before}unload
         // handlers if the current RFH isn't live.)
-        CommitPending();
+        CommitPending(false);
         return render_frame_host_.get();
       } else {
         NOTREACHED();
@@ -1557,29 +1745,23 @@ void RenderFrameHostManager::CancelPending() {
   // We no longer need to prevent the process from exiting.
   pending_render_frame_host->GetProcess()->RemovePendingView();
 
-  // 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 =
-      pending_render_frame_host->GetSiteInstance();
-  if (site_instance->active_frame_count() > 1) {
-    // Any currently suspended navigations are no longer needed.
-    pending_render_frame_host->CancelSuspendedNavigations();
-
-    RenderFrameProxyHost* proxy =
-        new RenderFrameProxyHost(site_instance, frame_tree_node_);
-    proxy_hosts_[site_instance->GetId()] = proxy;
-    pending_render_frame_host->SwapOut(proxy);
-    if (frame_tree_node_->IsMainFrame())
-      proxy->TakeFrameHostOwnership(pending_render_frame_host.Pass());
-  } else {
-    // We won't be coming back, so delete this one.
-    pending_render_frame_host.reset();
-  }
+  RecycleRenderFrameHost(pending_render_frame_host.Pass());

[nasko, 2014/11/19 19:01:29]

Recycle sounds like we will like to use it again and seems the wrong semantics
for this code. Let's rename it to something else that implies that we are
cleaning it up. The fact that we stash it around in certain cases is an
implementation detail that will disappear in the not-so-distant future.

[carlosk, 2014/11/21 14:36:34]

Done. Renamed to DiscardRenderFrameHost. WDYT?

 
   pending_web_ui_.reset();
   pending_and_current_web_ui_.reset();
 }
 
+void RenderFrameHostManager::CleanUpSpeculativeRenderFrameHost() {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  if (speculative_render_frame_host_) {
+    speculative_render_frame_host_->GetProcess()->RemovePendingView();
+    RecycleRenderFrameHost(speculative_render_frame_host_.Pass());
+  }
+  if (speculative_web_ui_)
+    speculative_web_ui_.reset();
+}
+
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::SetRenderFrameHost(
     scoped_ptr<RenderFrameHostImpl> render_frame_host) {
   // Swap the two.