PlzNavigate: implement RequestNavigation in the no live renderer case

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 cb8148ca3d9486fa26f3fdadf70a5a0cf17cd3d4..f2df6621545e986e6449bd5a73d4e2b028d26d65 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -15,6 +15,7 @@
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/interstitial_page_impl.h"
+#include "content/browser/frame_host/navigation_before_commit_info.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
@@ -38,10 +39,42 @@
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
+#include "net/base/load_flags.h"
 
 namespace content {
 
+namespace {
+
+// PlzNavigate
+FrameHostMsg_BeginNavigation_Params BeginNavigationFromNavigate(

[Charlie Reis, 2014/08/08 20:25:28]

Let's add a comment here, since it's confusing to see a function that transforms
a renderer-bound message (FrameMsg) into a browser-bound message (FrameHostMsg).
 I think the intention is that it's simulating the response from the renderer
when the renderer doesn't exist?

[clamy, 2014/08/12 12:13:16]

Done.

+    const FrameMsg_Navigate_Params& navigate_params) {
+  FrameHostMsg_BeginNavigation_Params begin_navigation_params;
+  begin_navigation_params.method = navigate_params.is_post ?
+      "POST" : "GET";
+  begin_navigation_params.url = navigate_params.url;
+  begin_navigation_params.referrer =
+      Referrer(navigate_params.referrer.url, navigate_params.referrer.policy);
+
+  // TODO(clamy): This should be modified to take into account caching policy
+  // requirements (eg for POST reloads).
+  begin_navigation_params.load_flags =
+      net::LOAD_NORMAL | net::LOAD_ENABLE_LOAD_TIMING;
+
+  // TODO(clamy): Post data from the browser should be put in the request body.
+
+  begin_navigation_params.has_user_gesture = false;
+  begin_navigation_params.transition_type = navigate_params.transition;
+  begin_navigation_params.should_replace_current_entry =
+      navigate_params.should_replace_current_entry;
+  begin_navigation_params.allow_download =
+      navigate_params.allow_download;
+  return begin_navigation_params;
+}
+
+}  // namespace
+
 RenderFrameHostManager::PendingNavigationParams::PendingNavigationParams(
     const GlobalRequestID& global_request_id,
     scoped_ptr<CrossSiteTransferringRequest> cross_site_transferring_request,
@@ -180,43 +213,9 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
   if (!dest_render_frame_host)
     return NULL;  // We weren't able to create a pending render frame host.
 
-  // If the current render_frame_host_ isn't live, we should create it so
-  // that we don't show a sad tab while the dest_render_frame_host fetches
-  // its first page.  (Bug 1145340)
-  if (dest_render_frame_host != render_frame_host_ &&
-      !render_frame_host_->render_view_host()->IsRenderViewLive()) {
-    // Note: we don't call InitRenderView here because we are navigating away
-    // soon anyway, and we don't have the NavigationEntry for this host.
-    delegate_->CreateRenderViewForRenderManager(
-        render_frame_host_->render_view_host(), MSG_ROUTING_NONE,
-        MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame());
-  }
-
-  // If the renderer crashed, then try to create a new one to satisfy this
-  // navigation request.
-  if (!dest_render_frame_host->render_view_host()->IsRenderViewLive()) {
-    // Recreate the opener chain.
-    int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
-        dest_render_frame_host->GetSiteInstance());
-    if (!InitRenderView(dest_render_frame_host->render_view_host(),
-                        opener_route_id,
-                        MSG_ROUTING_NONE,
-                        frame_tree_node_->IsMainFrame()))
-      return NULL;
-
-    // Now that we've created a new renderer, be sure to hide it if it isn't
-    // our primary one.  Otherwise, we might crash if we try to call Show()
-    // on it later.
-    if (dest_render_frame_host != render_frame_host_ &&
-        dest_render_frame_host->render_view_host()->GetView()) {
-      dest_render_frame_host->render_view_host()->GetView()->Hide();
-    } else {
-      // Notify here as we won't be calling CommitPending (which does the
-      // notify).
-      delegate_->NotifySwappedFromRenderManager(
-          NULL, render_frame_host_.get(), frame_tree_node_->IsMainFrame());
-    }
-  }
+  // Initialize the destination RFH and the current RFH if they are not live.
+  if (!InitRenderFrameHostsBeforeNavigation(dest_render_frame_host))
+    return NULL;
 
   // If entry includes the request ID of a request that is being transferred,
   // the destination render frame will take ownership, so release ownership of
@@ -230,6 +229,25 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
   return dest_render_frame_host;
 }
 
+// PlzNavigate
+bool RenderFrameHostManager::RequestNavigation(
+    const NavigationEntryImpl& entry,
+    const FrameMsg_Navigate_Params& navigate_params) {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  // TODO(clamy): replace RenderViewHost::IsRenderViewLive by
+  // RenderFrameHost::IsLive.
+  if (render_frame_host_->render_view_host()->IsRenderViewLive())
+    // TODO(clamy): send a RequestNavigation IPC.
+    return true;
+
+  InitRenderFrameHostsBeforeNavigation(render_frame_host_.get());

[Charlie Reis, 2014/08/08 20:25:29]

I don't see why we're making this call in RequestNavigation.  We don't have a
destination RenderFrameHost yet, and passing the current RFH as
|dest_render_frame_host| makes the code in InitRenderFrameHostsBeforeNavigation
do unexpected things (e.g., skip the sad tab check and unexpectedly do more if
the current RFH is dead).

Is there a reason it's needed?  (If not, we probably don't need to make it into
a helper function.)

[clamy, 2014/08/12 12:13:16]

Done.

+  // The navigation request is sent directly to the IO thread.
+  OnBeginNavigation(BeginNavigationFromNavigate(navigate_params),
+                    entry);
+  return true;
+}
+
 void RenderFrameHostManager::Stop() {
   render_frame_host_->render_view_host()->Stop();
 
@@ -572,8 +590,12 @@ void RenderFrameHostManager::ResetProxyHosts() {
   STLDeleteValues(&proxy_hosts_);
 }
 
+// PlzNavigate
 void RenderFrameHostManager::OnBeginNavigation(
-    const FrameHostMsg_BeginNavigation_Params& params) {
+    const FrameHostMsg_BeginNavigation_Params& params,
+    const NavigationEntryImpl& entry) {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
   // TODO(clamy): Check if navigations are blocked and if so, return
   // immediately.
   NavigationRequestInfo info(params);
@@ -585,11 +607,45 @@ void RenderFrameHostManager::OnBeginNavigation(
       false : frame_tree_node_->parent()->IsMainFrame();
   info.is_showing = GetRenderWidgetHostView()->IsShowing();
 
-  navigation_request_.reset(
-      new NavigationRequest(info, frame_tree_node_->frame_tree_node_id()));
+  // TODO(clamy): Check if the current RFH should be initialized (in case it has
+  // crashed) not to display a sad tab while navigating.
+  // TODO(clamy): Spawn a speculative renderer process if we do not have one to
+  // use for the navigation.
+  navigation_request_.reset(new NavigationRequest(
+      info, entry, frame_tree_node_->frame_tree_node_id()));
   navigation_request_->BeginNavigation(params.request_body);

[Charlie Reis, 2014/08/08 20:25:28]

Looks like this goes to RDH::NavigationRequest, which is NOTIMPLEMENTED.  How
does the test pass?

[clamy, 2014/08/12 12:13:16]

Apparently NOTIMPLEMENTED just prints an error message and does not cause the
test to fail, even on Debug builds. However, in the future it would be better to
somehow mock the call to RDH::NavigationRequest.

-  // TODO(clamy): If we have no live RenderFrameHost to handle the request (eg
-  // cross-site navigation) spawn one speculatively here and keep track of it.
+}
+
+// PlzNavigate
+void RenderFrameHostManager::CommitNavigation(
+    const NavigationBeforeCommitInfo& info) {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  // Update the stored NavigationEntry for commit.
+  // TODO(clamy): Check if some more state should be updated at that point.
+  navigation_request_->UpdateEntryForCommit(info.navigation_url);

[Charlie Reis, 2014/08/08 20:25:28]

Since we haven't actually committed yet, I don't think we want to update the
NavigationEntry yet.

[clamy, 2014/08/12 12:13:16]

We don't get the new site instance right if we don't update the url.

+
+  // Pick the right RenderFrameHost to commit the navigation.
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  SiteInstance* new_instance = GetSiteInstanceForNavigation(

[Charlie Reis, 2014/08/08 20:25:28]

I'm really excited about waiting until this point to pick the SiteInstance!  No
more getting it wrong due to server redirects.

+      navigation_request_->entry());
+  DCHECK(!pending_render_frame_host_.get());
+
+  if (current_instance != new_instance) {

[Charlie Reis, 2014/08/08 20:25:28]

We may want a TODO to update how pending WebUI objects are handled.  (See the
code in the similar spot of UpdateStateForNavigate.)

[clamy, 2014/08/12 12:13:16]

Done.

+    RenderFrameHostImpl * new_rfh = CreateRenderFrameHostForNewSiteInstance(
+        current_instance, new_instance, frame_tree_node_->IsMainFrame());
+    DCHECK(new_rfh);
+    // TODO(clamy): The old RenderFrameHost should be asked to run its unload

[Charlie Reis, 2014/08/08 20:25:28]

Side note: I'm thinking we want to wait until the navigation has committed in
the renderer for this.  I'm working on that in http://crbug.com/402020.

[clamy, 2014/08/12 12:13:16]

Done.

+    // handler.
+    scoped_ptr<RenderFrameHostImpl> old_render_frame_host =
+        SetRenderFrameHost(pending_render_frame_host_.Pass());

[clamy, 2014/08/08 12:02:46]

Right now the newly created RenderFrameHost for the navigation is placed in
pending_render_frame_host_. We will eventually want to remove that, but I think
it is better to wait to see how the speculative renderer spawning optimization
ends up being implemented before doing so.

[Charlie Reis, 2014/08/08 20:25:28]

Acknowledged.

+    if (frame_tree_node_->IsMainFrame())
+      render_frame_host_->render_view_host()->AttachToFrameTree();
+  }
+
+  frame_tree_node_->navigator()->CommitNavigation(
+      render_frame_host_.get(), info);
+  navigation_request_.reset();
 }
 
 void RenderFrameHostManager::Observe(
@@ -736,6 +792,35 @@ bool RenderFrameHostManager::ShouldReuseWebUI(
           controller.GetBrowserContext(), new_entry->GetURL()));
 }
 
+SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
+    const NavigationEntryImpl& entry) {
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  SiteInstance* new_instance = current_instance;
+
+  // We do not currently swap processes for navigations in webview tag guests.
+  bool is_guest_scheme = current_instance->GetSiteURL().SchemeIs(kGuestScheme);
+
+  // Determine if we need a new BrowsingInstance for this entry.  If true, this
+  // implies that it will get a new SiteInstance (and likely process), and that
+  // other tabs in the current BrowsingInstance will be unable to script it.
+  // This is used for cases that require a process swap even in the
+  // process-per-tab model, such as WebUI pages.
+  const NavigationEntry* current_entry =
+      delegate_->GetLastCommittedNavigationEntryForRenderManager();
+  bool force_swap = !is_guest_scheme &&
+      ShouldSwapBrowsingInstancesForNavigation(current_entry, &entry);
+  if (!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap))
+    new_instance = GetSiteInstanceForEntry(entry, 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;
+}
+
 SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
     const NavigationEntryImpl& entry,
     SiteInstance* current_instance,
@@ -903,6 +988,38 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
   return current_instance->GetRelatedSiteInstance(dest_url);
 }
 
+RenderFrameHostImpl* RenderFrameHostManager::

[Charlie Reis, 2014/08/08 20:25:28]

It feels awkward to return a raw pointer of something that's stored in
pending_render_frame_host_'s scoped_ptr as a side effect.  Perhaps we should
make this void and just have callers check pending_render_frame_host_ afterward.

[clamy, 2014/08/12 12:13:16]

Done.

+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 = 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 NULL;
+  }
+  return pending_render_frame_host_.get();
+}
+
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
     SiteInstance* site_instance,
     int view_routing_id,
@@ -935,11 +1052,12 @@ 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::CreateRenderFrame(
+    SiteInstance* instance,
+    int opener_route_id,
+    bool swapped_out,
+    bool for_main_frame_navigation,
+    bool hidden) {
   CHECK(instance);
   DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.
 
@@ -1031,8 +1149,9 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
   }
 
   // Use this as our new pending RFH if it isn't swapped out.
-  if (!swapped_out)
+  if (!swapped_out) {

[Charlie Reis, 2014/08/08 20:25:29]

Nit: I don't think this change is necessary.

[clamy, 2014/08/12 12:13:16]

Done.

     pending_render_frame_host_ = new_render_frame_host.Pass();
+  }
 
   // If a brand new RFH was created, announce it to observers.
   if (frame_to_announce)
@@ -1118,6 +1237,48 @@ int RenderFrameHostManager::GetRoutingIdForSiteInstance(
   return MSG_ROUTING_NONE;
 }
 
+bool RenderFrameHostManager::InitRenderFrameHostsBeforeNavigation(
+    RenderFrameHostImpl* dest_render_frame_host) {
+  // If the current render_frame_host_ isn't live, we should create it so
+  // that we don't show a sad tab while the dest_render_frame_host fetches
+  // its first page.  (crbug.com/1145340)
+  if (dest_render_frame_host != render_frame_host_ &&
+      !render_frame_host_->render_view_host()->IsRenderViewLive()) {
+    // Note: we don't call InitRenderView here because we are navigating away
+    // soon anyway, and we don't have the NavigationEntry for this host.
+    delegate_->CreateRenderViewForRenderManager(
+        render_frame_host_->render_view_host(), MSG_ROUTING_NONE,
+        MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame());
+  }
+
+  // If the renderer crashed, then try to create a new one to satisfy this
+  // navigation request.
+  if (!dest_render_frame_host->render_view_host()->IsRenderViewLive()) {
+    // Recreate the opener chain.
+    int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
+        dest_render_frame_host->GetSiteInstance());
+    if (!InitRenderView(dest_render_frame_host->render_view_host(),
+                        opener_route_id,
+                        MSG_ROUTING_NONE,
+                        frame_tree_node_->IsMainFrame()))
+      return false;
+
+    // Now that we've created a new renderer, be sure to hide it if it isn't
+    // our primary one.  Otherwise, we might crash if we try to call Show()
+    // on it later.
+    if (dest_render_frame_host != render_frame_host_ &&
+        dest_render_frame_host->render_view_host()->GetView()) {
+      dest_render_frame_host->render_view_host()->GetView()->Hide();
+    } else {
+      // Notify here as we won't be calling CommitPending (which does the
+      // notify).
+      delegate_->NotifySwappedFromRenderManager(
+          NULL, render_frame_host_.get(), frame_tree_node_->IsMainFrame());
+    }
+  }
+  return true;
+}
+
 void RenderFrameHostManager::CommitPending() {
   // 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
@@ -1329,28 +1490,11 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   }
 
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
-  scoped_refptr<SiteInstance> new_instance = current_instance;
+  scoped_refptr<SiteInstance> new_instance =
+      GetSiteInstanceForNavigation(entry);
 
-  // We do not currently swap processes for navigations in webview tag guests.
-  bool is_guest_scheme = current_instance->GetSiteURL().SchemeIs(kGuestScheme);
-
-  // Determine if we need a new BrowsingInstance for this entry.  If true, this
-  // implies that it will get a new SiteInstance (and likely process), and that
-  // other tabs in the current BrowsingInstance will be unable to script it.
-  // This is used for cases that require a process swap even in the
-  // process-per-tab model, such as WebUI pages.
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
-  bool force_swap = !is_guest_scheme &&
-      ShouldSwapBrowsingInstancesForNavigation(current_entry, &entry);
-  if (!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap))
-    new_instance = GetSiteInstanceForEntry(entry, 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);
 
   if (new_instance != current_instance) {
     // New SiteInstance: create a pending RFH to navigate.
@@ -1364,32 +1508,11 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
     // not have its bindings set appropriately.
     SetPendingWebUI(entry);
 
-    // Ensure that we have created RFHs for the new RFH's opener chain if
-    // we are staying in the same BrowsingInstance. This allows the pending RFH
-    // to send cross-process script calls to its opener(s).
-    int opener_route_id = MSG_ROUTING_NONE;
-    if (new_instance->IsRelatedSiteInstance(current_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 pending RFH with the given opener and navigate
-    // it.
-    int route_id = CreateRenderFrame(new_instance,
-                                     opener_route_id,
-                                     false,
-                                     frame_tree_node_->IsMainFrame(),
-                                     delegate_->IsHidden());
-    if (route_id == MSG_ROUTING_NONE)
+    if (!CreateRenderFrameHostForNewSiteInstance(
+        current_instance, new_instance,
+        frame_tree_node_->IsMainFrame())) {
       return NULL;
+    }
 
     // Check if our current RFH is live before we set up a transition.
     if (!render_frame_host_->render_view_host()->IsRenderViewLive()) {