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..7f90ad1ebacef628851c5441c8bb65134146e938 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(

[nasko, 2014/11/24 23:59:08]

Now that I think about it more, I'm not sure why all this work will be needed
for the speculative RFH. Since the new RFH wouldn't have loaded any document or
be swapped out already, there is no reason for us to swap it out, just kill it.
Also, since we know we created a speculative RFH, we don't need to stash it into
a proxy either, so I think we don't need all this code for speculative RFH.

[carlosk, 2014/11/25 10:54:30]

The same thing is done for non-utilized pending RFHs and that's why it's
replicated here. Mind that the speculative RFH might have been resurrected from
a swapped out state so we might not have just created it and it might have
loaded documents before being swapped out.

IDK if these facts make any difference to what you're suggesting so let me know
if I should still remove this logic for the speculative case.

[nasko, 2014/11/26 00:13:02]

One thing is clear, we don't need to call rfh->SwapOut(proxy) in the case of
speculative RFH deletion. I think it might still be fine to leave this factored
out.

[carlosk, 2014/11/27 11:02:43]

Acknowledged.

+    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,26 @@ 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())
+    create_render_frame_flags |= CREATE_RF_HIDDEN;
+
+  int opener_route_id =
+      CreateOpenerRenderViewsIfNeeded(old_instance, new_instance);
+  // TODO(carlosk): does this "earlier" call for CancelPending affects anything?

[nasko, 2014/11/24 23:59:08]

I didn't add the same comment about empty line to each occurrence. I expected
one here and after CancelPending below.

[carlosk, 2014/11/25 10:54:30]

Acknowledged and done.

+  // It used to happen inside the creation method iff the new RFH was
+  // successfully created.

[carlosk, 2014/11/25 10:54:30]

I'm also removing this TODO here. My testing showed no issues but I can't know
for sure by looking at the code if there are any. I'm not super worried as the
pending case should go away at some point...

Any comments?

+  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 +1055,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 +1089,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) {
   bool swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   CHECK(instance);
   // Swapped out views should always be hidden.
@@ -1071,10 +1107,11 @@ 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;

[nasko, 2014/11/24 23:59:08]

Why do you need to pre-declare success here? It is only used in the else block
where it used to be declared before.

[carlosk, 2014/11/25 10:54:30]

It's also used in the last top-level if-block to decide what value this method
should be return.

+  if (routing_id_ptr)
+    *routing_id_ptr = MSG_ROUTING_NONE;
 
-  // We are creating a pending or swapped out RFH here.  We should never create
+  // We are creating a pending 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);
 
@@ -1082,9 +1119,9 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
   // 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 +1135,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 +1166,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 +1178,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

[nasko, 2014/11/24 23:59:08]

Let's verify this TODO before committing this code. It has to have a clear
answer.

[carlosk, 2014/11/25 10:54:30]

Yes. For this case I found no problems in any of my testing and i went over the
call tree for it and it doesn't seem they chain to a call to try and get the
pending RFH for non-test code (that one might in deeper levels but we'll have to
deal with it anyway).

I'm removing this this one already but let me know if any of you have extra
comments.

+        // 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) {

[nasko, 2014/11/24 23:59:08]

Why not return always if new_render_frame_host is non-null?

[carlosk, 2014/11/25 10:54:30]

Because of line 1179. If that one fails new_render_frame_host is not null but
should not be returned but simply destroyed. Hence the success check here.

+    DCHECK(new_render_frame_host->GetSiteInstance() == instance);
+    return new_render_frame_host.Pass();
+  }
+  return nullptr;
 }
 
 int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
@@ -1558,24 +1600,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();