Preparation steps for adding speculative renderer creation.

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 7780cc05e2c90df1464137488038106a0f2316f6..c20a8bd3462a1d8c4f59c987f9ac4c9aad00e396 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -134,23 +134,25 @@ RenderFrameProxyHost* RenderFrameHostManager::GetProxyToParent() {
   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 nullptr;
   }
+  return new_web_ui;
 }
 
 RenderFrameHostImpl* RenderFrameHostManager::Navigate(
@@ -557,6 +559,30 @@ void RenderFrameHostManager::SwapOutOldFrame(
   }
 }
 
+void RenderFrameHostManager::DiscardRenderFrameHost(
+    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
@@ -997,9 +1023,24 @@ void RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(
   int create_render_frame_flags = 0;
   if (is_main_frame)
     create_render_frame_flags |= CREATE_RF_FOR_MAIN_FRAME_NAVIGATION;
-  // 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).
+  if (delegate_->IsHidden())

[nasko, 2014/11/24 17:00:27]

nit: empty line before the if statement

[carlosk, 2014/11/24 18:15:29]

Done.

+    create_render_frame_flags |= CREATE_RF_HIDDEN;
+  int opener_route_id =

[nasko, 2014/11/24 17:00:28]

nit: Same here, empty line above it. Unrelated statements are easier to read if
they are visually separated.

[carlosk, 2014/11/24 18:15:29]

Done.

+      CreateOpenerRenderViewsIfNeeded(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
+  // successfully created.
+  if (pending_render_frame_host_)
+    CancelPending();
+  // Create a non-swapped-out RFH with the given opener.
+  pending_render_frame_host_ =
+      CreateRenderFrame(new_instance, opener_route_id,
+                        create_render_frame_flags, pending_web_ui(), nullptr);
+}
+
+int RenderFrameHostManager::CreateOpenerRenderViewsIfNeeded(
+    SiteInstance* old_instance,
+    SiteInstance* new_instance) {
   int opener_route_id = MSG_ROUTING_NONE;
   if (new_instance->IsRelatedSiteInstance(old_instance)) {
     opener_route_id =
@@ -1012,17 +1053,7 @@ void RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(
           frame_tree_node_, new_instance);
     }
   }
-
-  if (delegate_->IsHidden())
-    create_render_frame_flags |= CREATE_RF_HIDDEN;
-
-  // Create a non-swapped-out RFH with the given opener.
-  int route_id = CreateRenderFrame(new_instance, opener_route_id,
-                                   create_render_frame_flags);
-  if (route_id == MSG_ROUTING_NONE) {
-    pending_render_frame_host_.reset();
-    return;
-  }
+  return opener_route_id;
 }
 
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
@@ -1056,9 +1087,12 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
   return render_frame_host.Pass();
 }
 
-int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
-                                              int opener_route_id,
-                                              int flags) {
+scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
+    SiteInstance* instance,
+    int opener_route_id,
+    int flags,
+    const WebUIImpl* web_ui,
+    int* routing_id_ptr) {

[nasko, 2014/11/24 17:00:27]

Why take in this as param? The callers can use the returned RFH to get whichever
routing ID they are interested in.

[carlosk, 2014/11/24 18:15:29]

Because if the RFH is flagged to be created swapped out its pointer is not
returned because its ownership is immediately given to the proxy. As this is
always the case for the external calls (not from RHFM itself) and it's them that
need the routing ID, it is still needed.

   bool swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   CHECK(instance);
   // Swapped out views should always be hidden.
@@ -1071,20 +1105,21 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
   }
 
   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)

[nasko, 2014/11/24 17:00:27]

Not passing in this pointer as argument will also make the code a lot easier to
read, no need for if checks all over the place.

[carlosk, 2014/11/24 18:15:29]

I don't think that's possible as explained above.

+    *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.
+  // never create it in the same SiteInstance as our current RFH.

[nasko, 2014/11/24 17:00:27]

nit: "never create" appears twice.

[carlosk, 2014/11/24 18:15:29]

Done.

   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) {
@@ -1098,13 +1133,13 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
       // 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);
         }
       }
     }
@@ -1129,7 +1164,7 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
         proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
     }
 
-    bool success =
+    success =
         InitRenderView(render_view_host, opener_route_id, proxy_routing_id,
                        !!(flags & CREATE_RF_FOR_MAIN_FRAME_NAVIGATION));
     if (success) {
@@ -1141,22 +1176,27 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
         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 nullptr;

[nasko, 2014/11/24 17:00:27]

Is this a valid return value for a scoped_ptr?

[carlosk, 2014/11/24 18:15:29]

I was also in doubt but:
a) the code compiled without issue (and a lot of times code didn't compile with
scoped_ptr because of tall checks it has around it)
b) All tests (automatic and my manual ones) did work so far.

 }
 
 int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
@@ -1558,24 +1598,7 @@ void RenderFrameHostManager::CancelPending() {
   // 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();
-  }
+  DiscardRenderFrameHost(pending_render_frame_host.Pass());
 
   pending_web_ui_.reset();
   pending_and_current_web_ui_.reset();