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..990ba4334ff9d479f0409c0d2a708324ec94b99e 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),

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

weak_factory_ must be last.

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

Done.

+      should_reuse_web_ui_(false) {
   DCHECK(frame_tree_node_);
 }
 
@@ -69,6 +70,11 @@ RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     CancelPending();
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    CleanUpNavigation();
+  }
+
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
 
@@ -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;

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

I'm not sure this is correct.

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

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

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

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

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

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

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

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

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

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

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

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

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

Done.

+  }
+
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
@@ -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,6 +636,88 @@ void RenderFrameHostManager::ResetProxyHosts() {
 }
 
 // PlzNavigate
+void RenderFrameHostManager::BeginNavigation(

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

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

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

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

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

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

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

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

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

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

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

Updated the comment.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Updated comment.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Done!
Ack.

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

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

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

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

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

+  } else {

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

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

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

Done.

+    // Navigating to the same SiteInstance -> make sure the current RFH is
+    // alive.
+    if (!render_frame_host_->render_view_host()->IsRenderViewLive()) {
+      // Recreate the opener chain.
+      int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
+          render_frame_host_->GetSiteInstance());
+      if (!InitRenderView(render_frame_host_->render_view_host(),
+                          opener_route_id, MSG_ROUTING_NONE,
+                          frame_tree_node_->IsMainFrame())) {
+        return;
+      }
+    }
+    DCHECK(current_instance->GetProcess()->HasConnection());
+    DCHECK(current_instance->GetProcess()->GetBrowserContext());
+  }
+}
+
+// PlzNavigate
+bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
+    const GURL& url,
+    SiteInstance* old_instance,
+    SiteInstance* new_instance,
+    int bindings) {
+  CHECK(new_instance);
+  CHECK_NE(old_instance, new_instance);
+
+  const NavigationEntry* current_navigation_entry =
+      delegate_->GetLastCommittedNavigationEntryForRenderManager();
+  scoped_ptr<WebUIImpl> new_web_ui;
+  should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry, url);
+  if (!should_reuse_web_ui_)
+    new_web_ui = CreateWebUI(url, bindings);
+
+  int opener_route_id =
+      CreateOpenerRenderViewsIfNeeded(old_instance, new_instance);
+
+  int create_render_frame_flags = 0;
+  if (frame_tree_node_->IsMainFrame())
+    create_render_frame_flags |= CREATE_RF_FOR_MAIN_FRAME_NAVIGATION;
+  if (delegate_->IsHidden())
+    create_render_frame_flags |= CREATE_RF_HIDDEN;
+  scoped_ptr<RenderFrameHostImpl> new_render_frame_host =
+      CreateRenderFrame(new_instance, new_web_ui.get(), opener_route_id,
+                        create_render_frame_flags, nullptr);
+  if (!new_render_frame_host) {
+    return false;
+  }
+  speculative_render_frame_host_.reset(new_render_frame_host.release());
+  speculative_web_ui_.reset(new_web_ui.release());
+  return true;
+}
+
+// PlzNavigate
 RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
       const GURL& url,
       ui::PageTransition transition) {
@@ -650,6 +749,19 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
   return render_frame_host;
 }
 
+// PlzNavigate
+void RenderFrameHostManager::CleanUpNavigation() {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  if (speculative_render_frame_host_) {
+    speculative_render_frame_host_->GetProcess()->RemovePendingView();
+    DiscardUnusedFrame(speculative_render_frame_host_.Pass());
+  }
+  if (speculative_web_ui_)
+    speculative_web_ui_.reset();
+  should_reuse_web_ui_ = false;
+}
+
 void RenderFrameHostManager::Observe(
     int type,
     const NotificationSource& source,
@@ -1110,8 +1222,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
@@ -1294,23 +1406,32 @@ void RenderFrameHostManager::CommitPending() {
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
-  // Next commit the Web UI, if any. Either replace |web_ui_| with
-  // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
-  // leave |web_ui_| as is if reusing it.
-  DCHECK(!(pending_web_ui_.get() && pending_and_current_web_ui_.get()));
-  if (pending_web_ui_) {
-    web_ui_.reset(pending_web_ui_.release());
-  } else if (!pending_and_current_web_ui_.get()) {
-    web_ui_.reset();
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(

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

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

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

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

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

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

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

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

Done.

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

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

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

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

Makes sense. Done.

     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
@@ -1324,10 +1445,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 (!CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    DCHECK(!speculative_render_frame_host_);
+    // Swap in the pending frame and make it active. Also ensure the FrameTree
+    // stays in sync.
+    old_render_frame_host =
+        SetRenderFrameHost(pending_render_frame_host_.Pass());
+  } else {
+    DCHECK(speculative_render_frame_host_);
+    old_render_frame_host =
+        SetRenderFrameHost(speculative_render_frame_host_.Pass());
+  }
+
   if (is_main_frame)
     render_frame_host_->render_view_host()->AttachToFrameTree();
 
@@ -1456,6 +1587,36 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
       url, instance, transition, is_restore, is_view_source_mode);
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(

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

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

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

Makes total sense and so I moved it.

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

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

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

Same concern about the process model policy.

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

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

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

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

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

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

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

+        // because in this case they are not speculative. Suggestions are
+        // very welcome!
+        bool success = CreateSpeculativeRenderFrameHost(
+            url, current_instance, new_instance.get(), bindings);
+        if (!success)
+          return nullptr;
+      }
+      DCHECK(speculative_render_frame_host_);
+      CommitPending();
+      DCHECK(!speculative_render_frame_host_);
+    }
+    return render_frame_host_.get();
+  }
+
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();