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 9973a8657557899523f41b4e45c838445fc7a3ce..c7ab5c6a734c5350a7cf20e01fe3d25b1e8f8763 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -39,9 +39,39 @@
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
+#include "net/base/load_flags.h"
 
 namespace content {
 
+namespace {
+
+FrameHostMsg_BeginNavigation_Params BeginNavigationFromNavigate(
+    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 = navigate_params.referrer.url;
+  begin_navigation_params.referrer_policy = 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 +210,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.

[nasko, 2014/07/17 12:05:08]

If they are not alive, then they will be the same, right? The comment implies
that they can be different.

[clamy, 2014/07/17 13:07:53]

What if the current renderer crashed and we navigate to a brand new different
site, do we reuse the RFH in that case?

[nasko, 2014/07/21 13:02:05]

No, we don't. I was thinking initial creation only and didn't take into account
the crashed renderer.

+  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 +226,40 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
   return dest_render_frame_host;
 }
 
+bool RenderFrameHostManager::RequestNavigation(
+    const NavigationEntryImpl& entry,
+    const FrameMsg_Navigate_Params& navigate_params) {
+  if (render_frame_host_->render_view_host()->IsRenderViewLive())

[nasko, 2014/07/17 12:05:08]

Let's put a TODO to put IsLive on RenderFrameHost, as it doesn't make sense to
go to the RVH long term.

[clamy, 2014/07/17 13:07:53]

Done.

+    // TODO(clamy): send a RequestNavigation IPC.
+    return true;
+
+  // The navigation request is sent directly to the IO thread.
+  OnBeginNavigation(BeginNavigationFromNavigate(navigate_params));

[nasko, 2014/07/17 12:05:08]

There is a comment in OnBeginNavigation to create the speculative RFH. You are
doing it below. One of the two has to be corrected.

[clamy, 2014/07/17 13:07:53]

The comment in OnBeginNavigation had more to do with creating a RFH
speculatively in case we realize we have to do a cross-site navigation and we
don't have a proper renderer for that. I guess I could move the RFH creation
part below into OnBeginNavigation, so that we only have speculative RFH spawning
in one place.

+
+  // Create a new RFH for the navigation if necessary.
+  RenderFrameHostImpl* dest_render_frame_host = render_frame_host_.get();
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  SiteInstance* new_instance = GetSiteInstanceForNavigation(entry);
+  if (current_instance != new_instance) {
+    // 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);
+    }
+
+    // Create a non-swapped-out speculative RFH with the given opener.
+    int route_id = CreateRenderFrame(new_instance, opener_route_id, false,
+                                     delegate_->IsHidden(), true);
+    if (route_id == MSG_ROUTING_NONE)
+      return false;
+    dest_render_frame_host = speculative_render_frame_host_.get();
+  }
+  return InitRenderFrameHostsBeforeNavigation(dest_render_frame_host);

[nasko, 2014/07/17 12:05:08]

Do we need to init in the case where we don't create a new RFH? 

[clamy, 2014/07/17 13:07:53]

We are in the case where the current RFH is not live, so if we don't create a
new one, we are likely to use the not live current_render_frame_host_ when the
navigation commits. I think it should be initialized, but I may be mistaken.

[nasko, 2014/07/21 13:02:05]

I don't think we need a live current renderer to start the new navigation, but
handling that case might just make the code more complex, which I'm not a fan
of.

[clamy, 2014/07/21 13:28:17]

So should I let the initialization in place?

+}
+
 void RenderFrameHostManager::Stop() {
   render_frame_host_->render_view_host()->Stop();
 
@@ -944,7 +974,8 @@ int RenderFrameHostManager::CreateRenderFrame(
     SiteInstance* instance,
     int opener_route_id,
     bool swapped_out,
-    bool hidden) {
+    bool hidden,
+    bool is_speculative) {
   CHECK(instance);
   DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.

[nasko, 2014/07/17 12:05:08]

is_speculative should never be true when swapped_out is true, right? If yes,
then let's put a DCHECK to enforce it.

[clamy, 2014/07/17 13:07:53]

Done.

 
@@ -1020,8 +1051,12 @@ int RenderFrameHostManager::CreateRenderFrame(
   }
 
   // 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 (!swapped_out) {
+    if (is_speculative)
+      speculative_render_frame_host_ = new_render_frame_host.Pass();
+    else
+      pending_render_frame_host_ = new_render_frame_host.Pass();
+  }
 
   // If a brand new RFH was created, announce it to observers.
   if (frame_to_announce)
@@ -1058,6 +1093,77 @@ bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
       render_view_host, opener_route_id, proxy_routing_id, for_main_frame);
 }
 
+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.  (Bug 1145340)

[nasko, 2014/07/17 12:05:08]

nit: convert this to a crbug link.

[clamy, 2014/07/17 13:07:53]

Done.

+  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;
+}
+
+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;
+}
+
 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
@@ -1266,28 +1372,9 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   // before the end of this method, so we don't have to worry about their ref
   // counts dropping to zero.
   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.
+  SiteInstance* new_instance = GetSiteInstanceForNavigation(entry);
   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.
@@ -1313,7 +1400,7 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
     // Create a non-swapped-out pending RFH with the given opener and navigate
     // it.
     int route_id = CreateRenderFrame(new_instance, opener_route_id, false,
-                                     delegate_->IsHidden());
+                                     delegate_->IsHidden(), false);
     if (route_id == MSG_ROUTING_NONE)
       return NULL;