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),
+      should_reuse_web_ui_(false) {
   DCHECK(frame_tree_node_);
 }
 
 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>());
 
   // Delete any swapped out RenderFrameHosts.
   STLDeleteValues(&proxy_hosts_);
 }
 
 void RenderFrameHostManager::Init(BrowserContext* browser_context,
                                   SiteInstance* site_instance,
                                   int view_routing_id,
@@ skipping 47 matching lines @@
                             ->render_manager()
                             ->current_frame_host()
                             ->GetSiteInstance()
                             ->GetId());
   if (iter == proxy_hosts_.end())
     return NULL;
 
   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(
     const NavigationEntryImpl& entry) {
   TRACE_EVENT1("navigation", "RenderFrameHostManager:Navigate",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
   // Create a pending RenderFrameHost to use for the navigation.
   RenderFrameHostImpl* dest_render_frame_host = UpdateStateForNavigate(
       entry.GetURL(),
       entry.site_instance(),
@@ skipping 134 matching lines @@
       // is ok to do nothing here.
       if (pending_render_frame_host_ &&
           pending_render_frame_host_->are_navigations_suspended()) {
         pending_render_frame_host_->SetNavigationsSuspended(false,
                                                             proceed_time);
       }
     } else {
       // 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.
     bool proceed_to_fire_unload;
     delegate_->BeforeUnloadFiredFromRenderManager(proceed, proceed_time,
                                                   &proceed_to_fire_unload);
 
     if (proceed_to_fire_unload) {
       // If we're about to close the tab and there's a pending RFH, cancel it.
       // Otherwise, if the navigation in the pending RFH completes before the
       // close in the current RFH, we'll lose the tab close.
       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.
       render_frame_host_->render_view_host()->ClosePage();
     }
   }
 }
 
 void RenderFrameHostManager::OnCrossSiteResponse(
     RenderFrameHostImpl* pending_render_frame_host,
@@ skipping 78 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;
+  }
+
   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();
+      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
     // one.
     CancelPending();
     cross_navigation_pending_ = false;
   } else {
     // No one else should be sending us DidNavigate in this state.
     DCHECK(false);
   }
 }
 
+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
   // in case a process swap has occurred while the message was in flight.
   for (RenderFrameProxyHostMap::iterator iter = proxy_hosts_.begin();
        iter != proxy_hosts_.end();
        ++iter) {
     DCHECK_NE(iter->second->GetSiteInstance(),
               current_frame_host()->GetSiteInstance());
     iter->second->DisownOpener();
@@ skipping 93 matching lines @@
     // We shouldn't get here for subframes, since we only swap subframes when
     // --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::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
   // when the timer times out, or when the RFHM itself is deleted (whichever
   // comes first).
   pending_delete_hosts_.push_back(
       linked_ptr<RenderFrameHostImpl>(render_frame_host.release()));
 }
 
 bool RenderFrameHostManager::IsPendingDeletion(
@@ skipping 15 matching lines @@
       return true;
     }
   }
   return false;
 }
 
 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,
       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.
 
@@ skipping 175 matching lines @@
   bool current_is_view_source_mode = current_entry ?
       current_entry->IsViewSourceMode() : dest_is_view_source_mode;
   bool force_swap = ShouldSwapBrowsingInstancesForNavigation(
       current_effective_url,
       current_is_view_source_mode,
       dest_instance,
       SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
       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,
   // we would have two RenderFrameHosts in the same SiteInstance and the same
   // frame, resulting in page_id conflicts for their NavigationEntries.
   if (force_swap)
     CHECK_NE(new_instance, current_instance);
   return new_instance;
 }
 
@@ skipping 180 matching lines @@
   // practice.)
   if (current_entry)
     return current_entry->GetURL();
   return current_instance->GetSiteURL();
 }
 
 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 =
         delegate_->CreateOpenerRenderViewsForRenderManager(new_instance);
     if (CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kSitePerProcess)) {
       // Ensure that the frame tree has RenderFrameProxyHosts for the new
       // SiteInstance in all nodes except the current one.
       frame_tree_node_->frame_tree()->CreateProxiesForSiteInstance(
           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(
     SiteInstance* site_instance,
     int view_routing_id,
     int frame_routing_id,
     bool swapped_out,
     bool hidden) {
   if (frame_routing_id == MSG_ROUTING_NONE)
     frame_routing_id = site_instance->GetProcess()->GetNextRoutingID();
@@ skipping 14 matching lines @@
   scoped_ptr<RenderFrameHostImpl> render_frame_host =
       make_scoped_ptr(RenderFrameHostFactory::Create(render_view_host,
                                                      render_frame_delegate_,
                                                      frame_tree,
                                                      frame_tree_node_,
                                                      frame_routing_id,
                                                      swapped_out).release());
   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.
 
   // TODO(nasko): Remove the following CHECK once cross-site navigation no
   // 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;
-  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) {
       new_render_frame_host = proxy->PassFrameHostOwnership();
       new_render_frame_host->GetProcess()->AddPendingView();
 
       proxy_hosts_.erase(instance->GetId());
       delete proxy;
 
       // When a new render view is created by the renderer, the new WebContents
       // 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);
         }
       }
     }
   } else {
     // Create a new RenderFrameHost if we don't find an existing one.
     new_render_frame_host = CreateRenderFrameHost(
         instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE, swapped_out, hidden);
     RenderViewHostImpl* render_view_host =
         new_render_frame_host->render_view_host();
     int proxy_routing_id = MSG_ROUTING_NONE;
 
     // Prevent the process from exiting while we're trying to navigate in it.
     // Otherwise, if the new RFH is swapped out already, store it.
     if (!swapped_out) {
       new_render_frame_host->GetProcess()->AddPendingView();
     } else {
       proxy = new RenderFrameProxyHost(
           new_render_frame_host->GetSiteInstance(), frame_tree_node_);
       proxy_hosts_[instance->GetId()] = proxy;
       proxy_routing_id = proxy->GetRoutingID();
       if (frame_tree_node_->IsMainFrame())
         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.
         render_view_host->GetView()->Hide();
       } else if (!swapped_out) {
         // Init the RFH, so a RenderFrame is created in the renderer.
         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) {
   // A RenderFrameProxyHost should never be created in the same SiteInstance as
   // the current RFH.
   CHECK(instance);
   CHECK_NE(instance, render_frame_host_->GetSiteInstance());
 
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
   if (proxy)
@@ skipping 21 matching lines @@
     render_view_host->AllowBindings(pending_web_ui()->GetBindings());
   } else {
     // Ensure that we don't create an unprivileged RenderView in a WebUI-enabled
     // process unless it's swapped out.
     if (render_view_host->is_active()) {
       CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
                 render_view_host->GetProcess()->GetID()));
     }
   }
 
-  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(
     RenderFrameHostImpl* render_frame_host) {
   if (render_frame_host->IsRenderFrameLive())
     return true;
 
   int parent_routing_id = MSG_ROUTING_NONE;
   int proxy_routing_id = MSG_ROUTING_NONE;
   if (frame_tree_node_->parent()) {
@@ skipping 25 matching lines @@
     return render_frame_host_->GetRoutingID();
 
   RenderFrameProxyHostMap::iterator iter =
       proxy_hosts_.find(site_instance->GetId());
   if (iter != proxy_hosts_.end())
     return iter->second->GetRoutingID();
 
   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;
   }
 
   // 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 (!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();
 
   // 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(
+          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();
 
   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 15 matching lines @@
     }
 
     // Check if our current RFH is live before we set up a transition.
     if (!render_frame_host_->IsRenderFrameLive()) {
       if (!cross_navigation_pending_) {
         // The current RFH is not live.  There's no reason to sit around with a
         // sad tab or a newly created RFH while we wait for the pending RFH to
         // 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();
         return render_frame_host_.get();
       }
     }
     // Otherwise, it's safe to treat this as a pending cross-site transition.
 
     // We now have a pending RFH.
     DCHECK(!cross_navigation_pending_);
@@ skipping 84 matching lines @@
   scoped_ptr<RenderFrameHostImpl> pending_render_frame_host =
       pending_render_frame_host_.Pass();
 
   RenderViewDevToolsAgentHost::OnCancelPendingNavigation(
       pending_render_frame_host->render_view_host(),
       render_frame_host_->render_view_host());
 
   // 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.
   scoped_ptr<RenderFrameHostImpl> old_render_frame_host =
       render_frame_host_.Pass();
   render_frame_host_ = render_frame_host.Pass();
 
   if (frame_tree_node_->IsMainFrame()) {
     // Update the count of top-level frames using this SiteInstance.  All
     // subframes are in the same BrowsingInstance as the main frame, so we only
@@ skipping 59 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