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 06ab825123e4b1fd294dd577e6aee18ab9ff94fe..5cee577366c8128fb9aa37559555aa4e72ea5443 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -61,6 +61,7 @@ RenderFrameHostManager::RenderFrameHostManager(
       render_view_delegate_(render_view_delegate),
       render_widget_delegate_(render_widget_delegate),
       interstitial_page_(NULL),
+      should_reuse_web_ui_(false),
       weak_factory_(this) {
   DCHECK(frame_tree_node_);
 }
@@ -69,6 +70,11 @@ RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     CancelPending();
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    CleanUpNavigation();

[ncarter (slow), 2014/12/12 22:37:54]

A CL I'm working on -- https://codereview.chromium.org/799593004/ -- will fix a
bug in ~RenderFrameHostManager that's due to the save-for-reuse work of
DiscardUnusedFrame being problematic during destruction.

Since CleanUpNavigation() also calls DiscardUnusedFrame, I think this will
introduce a new instance of that bug. See if you can use the test I added in the
CL as a starting point for tickling the CleanUpNavigation() instance?

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

Thanks for pointing this out! I refactored the code to follow pretty much
exactly your fix for the pending case.

+  }
+
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
 
@@ -288,6 +294,8 @@ void RenderFrameHostManager::OnBeforeUnloadACK(
     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;
@@ -419,6 +427,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_);
 
@@ -574,7 +588,10 @@ void RenderFrameHostManager::DiscardUnusedFrame(
     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 {
@@ -619,35 +636,158 @@ void RenderFrameHostManager::ResetProxyHosts() {
 }
 
 // PlzNavigate
+void RenderFrameHostManager::BeginNavigation(
+    const FrameHostMsg_BeginNavigation_Params& params,
+    const CommonNavigationParams& common_params) {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  // Cleans up any state in case there's an ongoing navigation.
+  CleanUpNavigation();
+
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  // TODO(carlosk): Replace the default values with the right ones for
+  // dest_instance, dest_is_restore, dest_is_view_source_mode.
+  scoped_refptr<SiteInstanceImpl> new_instance =
+      static_cast<SiteInstanceImpl*>(GetSiteInstanceForNavigation(
+          common_params.url, nullptr, common_params.transition, false, false));
+
+  // Will keep the current RFH if its SiteInstance matches the new one from
+  // the navigation or if this is a subframe navigation. If --site-per-process
+  // is enabled the RFH is never kept when sites don't match.
+  // TODO(carlosk): do not swap processes for renderer initiated navigations
+  // (see crbug.com/440266).
+  if (current_instance == new_instance.get() ||
+      (!frame_tree_node_->IsMainFrame() &&
+       !CommandLine::ForCurrentProcess()->HasSwitch(
+           switches::kSitePerProcess))) {
+    // 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;

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

These are the returns I'd like to eliminate but I'd have to use if-blocks to
hold DCHECKs.

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

While it is fine in general to have an if-block with only a DCHECK inside, I
don't think it would be better in that case. IMO the early return makes it
clearer that something went wrong if InitRenderView did not succeed, compared to
writing:
if (InitRenderView(...))
  DCHECK(current_instance->GetProcess()->HasConnection())
return;

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

+1 to both.

(DCHECKs inside a conditional mostly make sense if the conditional can be
optimized out.)

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

Finally I didn't need the if's. I simply DCHECK the |success|es.

[clamy, 2014/12/16 15:22:48]

That is not equivalent to the previous condition.

[carlosk, 2014/12/19 04:27:14]

You are right, it is not equivalent for the Debug case as it will crash on
renderer creation failure.

Now for the Release case it is still equivalent.

As this version has stronger guarantees and content tests are still passing I
thought it was OK to keep it that way. WDYT?

+      }
+    }
+    DCHECK(current_instance->GetProcess()->HasConnection());
+    return;
+  }
+  // Speculatively create a new RFH.

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

Please add a line before that comment and remove the one below.

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

Done.

+
+  // 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());
+}
+
+// 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(

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

Please modify this function so that it does not commit the pending RFH. Instead
it should either return the current renderer, or the speculative RFH. If it
turns out we need a speculative RFH and we don't have one, or the one we have is
wrong, a new speculative RFh should be created and returned.

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

Done.

       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.

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

I removed this TODO in favor of the new ones I'm adding related to the newly
created bug.

But from the CR comments I had understood this policy applied for any kind of
renderer-initiated navigations, and not only for subframe. Is that so?

-
   // Pick the right RenderFrameHost to commit the navigation.
-  // TODO(clamy): Replace the default values by the right ones.
-  RenderFrameHostImpl* render_frame_host = UpdateStateForNavigate(
-      url, NULL, transition, false, false, GlobalRequestID(),
-      NavigationEntryImpl::kInvalidBindings);
+
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  // TODO(clamy): Replace the default values with the right ones for
+  // dest_instance, dest_is_restore, dest_is_view_source_mode.
+  scoped_refptr<SiteInstance> new_instance =
+      GetSiteInstanceForNavigation(url, NULL, transition, false, false);
+
+  // Will keep the current RFH if its SiteInstance matches the new one from
+  // the navigation or if this is a subframe navigation. If --site-per-process
+  // is enabled the RFH is never kept when sites don't match.
+  // TODO(carlosk): do not swap processes for renderer initiated navigations
+  // (see crbug.com/440266).
+  if (current_instance == new_instance.get() ||
+      (!frame_tree_node_->IsMainFrame() &&
+       !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(clamy): Replace the binding value with the right one.
+      bool success = CreateSpeculativeRenderFrameHost(
+          url, current_instance, new_instance.get(),
+          NavigationEntryImpl::kInvalidBindings);
+      CHECK(success);
+    }
+    DCHECK(speculative_render_frame_host_);
+    CommitPending();

[nasko, 2014/12/10 23:11:34]

Why is this method calling CommitPending?

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

Because it was here that we would switch the active RFHs. It was moved back to
the later step at DidNavigateFrame as per other discussions.

+    DCHECK(!speculative_render_frame_host_);
+  }
 
   // If the renderer that needs to navigate is not live (it was just created or

[clamy, 2014/12/16 15:22:48]

As mentionned above this _must_ absolutely stay there. We can't have an
initialized renderer try to commit the page. The renderer selected here is going
to try to request the navigation url from the browser and get the body of the
response (and it absolutely needs to be live to do that).

[carlosk, 2014/12/19 04:27:14]

Acknowledged and moved it back here from DidNavigateFrame.

   // it crashed), initialize it.
-  if (!render_frame_host->render_view_host()->IsRenderViewLive()) {
+  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())) {
+        render_frame_host_->GetSiteInstance());
+    if (!InitRenderView(render_frame_host_->render_view_host(), opener_route_id,
+                        MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame())) {
       return NULL;
     }
   }
-  return render_frame_host;
+  return render_frame_host_.get();
+}
+
+// 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(
@@ -1110,8 +1250,8 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
   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
@@ -1288,29 +1428,39 @@ int RenderFrameHostManager::GetRoutingIdForSiteInstance(
 void RenderFrameHostManager::CommitPending() {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CommitPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
+  bool browser_side_navigation = 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 (!browser_side_navigation) {
+    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();
+    // PlzNavigate
+    if (!should_reuse_web_ui_)
+      web_ui_.reset(speculative_web_ui_.release());
+    DCHECK(!speculative_web_ui_);
   }
 
   // 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_ && !speculative_render_frame_host_) {
     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
@@ -1324,10 +1474,20 @@ 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 (!browser_side_navigation) {
+    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 {
+    // PlzNavigate
+    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();