Support frame tree propagation between renderers in the same browsing instance.

content/browser/web_contents/render_view_host_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/render_view_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 221 matching lines @@
     // A navigation in the original page has taken place.  Cancel the pending
     // one.
     CancelPending();
     cross_navigation_pending_ = false;
   } else {
     // No one else should be sending us DidNavigate in this state.
     DCHECK(false);
   }
 }
 
+void RenderViewHostManager::DidUpdateFrameTree(
+    RenderViewHost* render_view_host) {
+  DCHECK_EQ(render_view_host, current_host());

[Charlie Reis, 2012/08/22 22:08:34]

Are there any races that could cause this to be called when render_view_host is
no longer active (e.g., right after the browser process has swapped in a new
one), or does the filtering of swapped out messages prevent that?

Also, should we use a CHECK here, or do we need to return early if the DCHECK
fails?

[nasko, 2012/08/23 21:55:53]

Based on the cross-site transition timeline, it shouldn't be possible to receive
a frame tree update because of the filtering. The renderer starts filtering the
moment it runs the unload handler. It is possible for a frame update to be sent
prior to that, but the swap of pending render to active renderer doesn't happen
until we do RVHM::CommitPending. At that point, we either have processed any
outstanding frame update messages or are filtering it in the renderer. 
As such, I believe a CHECK is useful here.

+
+  RenderViewHostImpl* render_view_host_impl = static_cast<RenderViewHostImpl*>(
+      render_view_host);
+
+  for (RenderViewHostMap::iterator iter = swapped_out_hosts_.begin();
+       iter != swapped_out_hosts_.end();
+       ++iter) {
+    DCHECK_NE(iter->second->GetSiteInstance(),
+        current_host()->GetSiteInstance());
+
+    iter->second->UpdateFrameTree(
+        render_view_host_impl->GetProcess()->GetID(),

[Charlie Reis, 2012/08/22 22:08:34]

Interesting...  I wonder what happens if one of these processes has crashed. 
Does its RenderViewHosts automatically get removed from the swapped out list, or
might we be creating a new process here in by calling GetProcess()?

[nasko, 2012/08/23 21:55:53]

GetProcess() returns the RenderProcessHost, which is the wrapper around the
actual underlying process. It's ID doesn't change, even though the underlying OS
process can crash and be re-spawned. So this line should not be creating a new
OS process.

[Charlie Reis, 2012/08/24 23:26:07]

Ah, right you are!  I was thinking of SiteInstance::GetProcess.

+        render_view_host_impl->GetRoutingID(),
+        render_view_host_impl->frame_tree());
+  }
+}
+
 void RenderViewHostManager::SetWebUIPostCommit(WebUIImpl* web_ui) {
   DCHECK(!web_ui_.get());
   web_ui_.reset(web_ui);
 }
 
 void RenderViewHostManager::RendererAbortedProvisionalLoad(
     RenderViewHost* render_view_host) {
   // We used to cancel the pending renderer here for cross-site downloads.
   // However, it's not safe to do that because the download logic repeatedly
   // looks for this WebContents based on a render view ID.  Instead, we just
@@ skipping 236 matching lines @@
     if (curr_site_instance->HasWrongProcessForURL(dest_url))
       return curr_site_instance->GetRelatedSiteInstance(dest_url);
 
     // View-source URLs must use a new SiteInstance and BrowsingInstance.
     // TODO(nasko): This is the same condition as later in the function. This
     // should be taken into account when refactoring this method as part of
     // http://crbug.com/123007.
     if (entry.IsViewSourceMode())
       return SiteInstance::CreateForURL(browser_context, dest_url);
 
+    // If we are navigating from a blank SiteInstance to a WebUI, make sure we
+    // create a new SiteInstance.

[Charlie Reis, 2012/08/22 22:08:34]

Wait, shouldn't HasWrongProcessForURL have caught this case?  The comment on
line 504 suggests that it's supposed to catch WebUI.

[nasko, 2012/08/23 21:55:53]

The first thing HasWrongProcessForURL checks is whether there is a process
attached to the site instance or not. If not, it returns false. This is the
reason why it doesn't catch it, since the site instance is empty and no process
has been created to render "about:blank".

[Charlie Reis, 2012/08/24 23:26:07]

:(

Good catch.  This is fine, then, and I'll clean it up as part of 123007.

+    const WebUIControllerFactory* web_ui_factory =
+        content::GetContentClient()->browser()->GetWebUIControllerFactory();
+    if (web_ui_factory &&
+        web_ui_factory->UseWebUIForURL(browser_context, dest_url)) {
+        return SiteInstance::CreateForURL(browser_context, dest_url);
+    }
+
     // Normally the "site" on the SiteInstance is set lazily when the load
     // actually commits. This is to support better process sharing in case
     // the site redirects to some other site: we want to use the destination
     // site in the site instance.
     //
     // In the case of session restore, as it loads all the pages immediately
     // we need to set the site first, otherwise after a restore none of the
     // pages would share renderers in process-per-site.
     if (entry.restore_type() != NavigationEntryImpl::RESTORE_NONE)
       curr_site_instance->SetSite(dest_url);
@@ skipping 62 matching lines @@
     return curr_instance->GetRelatedSiteInstance(dest_url);
   }
 }
 
 int RenderViewHostManager::CreateRenderView(
     SiteInstance* instance,
     int opener_route_id,
     bool swapped_out) {
   CHECK(instance);
 
+  // If the current host is already in the site instance we are looking for

[Charlie Reis, 2012/08/22 22:08:34]

nit: site instance -> SiteInstance

[nasko, 2012/08/23 21:55:53]

Done.

+  // and we are creating a swapped out renderer, just return the routing id.
+  if (current_host()->GetSiteInstance() == instance) {
+    // TODO(nasko): Convert this if statement to a DCHECK once all causes
+    // of creating a new non-swapped out RVH for the same SiteInstance as the
+    // active one are tracked down and resolved.

[Charlie Reis, 2012/08/22 22:08:34]

Nice.  Please mention http://crbug.com/123007 here.

[nasko, 2012/08/23 21:55:53]

Done.

+    if (swapped_out)

[Charlie Reis, 2012/08/22 22:08:34]

This is actually a little confusing.  The caller is requesting a swapped_out
RenderView, but we're returning the routing ID of an active one.  I'm now
wondering if it's clearer to have the caller check whether there's an active one
in the given SiteInstance before requesting a swapped out one.  Thoughts?

[nasko, 2012/08/23 21:55:53]

Yes, changing the caller makes a bit more sense. I've done it and removed this
piece of code. I will file a separate bug about creating duplicate renderers for
the same SiteInstance and we can track those cases separately.

[Charlie Reis, 2012/08/24 23:26:07]

Great.  Please CC me on the bug.

+      return current_host()->GetRoutingID();
+  }
+
   // Check if we've already created an RVH 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 swapped out hosts if it commits.
   RenderViewHostImpl* new_render_view_host = static_cast<RenderViewHostImpl*>(
       GetSwappedOutRenderViewHost(instance));
   if (new_render_view_host) {
     // Prevent the process from exiting while we're trying to use it.
     if (!swapped_out)
       new_render_view_host->GetProcess()->AddPendingView();
   } else {
@@ skipping 10 matching lines @@
     if (swapped_out) {
       swapped_out_hosts_[instance->GetId()] = new_render_view_host;
     } else {
       new_render_view_host->GetProcess()->AddPendingView();
     }
 
     bool success = InitRenderView(new_render_view_host, opener_route_id);
     if (success) {
       // Don't show the view until we get a DidNavigate from it.
       new_render_view_host->GetView()->Hide();
+
+      // If we are creating a swapped out RVH, send a message to update the

[Charlie Reis, 2012/08/22 22:08:34]

nit: the -> its

[nasko, 2012/08/23 21:55:53]

Done.

+      // frame tree based on the active RVH for this RenderViewHostManager.
+      if (swapped_out) {
+        new_render_view_host->UpdateFrameTree(
+            current_host()->GetProcess()->GetID(),
+            current_host()->GetRoutingID(),
+            current_host()->frame_tree());
+      }
     } else if (!swapped_out) {
       CancelPending();
     }
   }
 
   // Use this as our new pending RVH if it isn't swapped out.
   if (!swapped_out)
     pending_render_view_host_ = new_render_view_host;
 
   return new_render_view_host->GetRoutingID();
@@ skipping 293 matching lines @@
 }
 
 bool RenderViewHostManager::IsSwappedOut(RenderViewHost* rvh) {
   if (!rvh->GetSiteInstance())
     return false;
 
   return swapped_out_hosts_.find(rvh->GetSiteInstance()->GetId()) !=
       swapped_out_hosts_.end();
 }
 
-RenderViewHost* RenderViewHostManager::GetSwappedOutRenderViewHost(
+RenderViewHostImpl* RenderViewHostManager::GetSwappedOutRenderViewHost(
     SiteInstance* instance) {
   RenderViewHostMap::iterator iter = swapped_out_hosts_.find(instance->GetId());
   if (iter != swapped_out_hosts_.end())
     return iter->second;
 
   return NULL;
 }