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(render_view_host == current_host());

[awong, 2012/08/21 19:27:28]

DCHECK_EQ?

[nasko, 2012/08/21 22:24:13]

Done.

+
+  RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(render_view_host);

[awong, 2012/08/21 19:27:28]

Shortenings like rvh aren't commonly used in this file.

Maybe render_view_host_impl? (yeah, it's longer :( )

[nasko, 2012/08/21 22:24:13]

Done.

+  if (!rvh)

[awong, 2012/08/21 19:27:28]

For parameter validation, do this as close to the top as possible (before the
cast even, but probably after the DCHECK which is enforcing a stronger
invariant).

However, I would have expected this function to never be called if rvh is NULL. 
When could rvh be NULL in normal operation?  If that's not supposed to happen,
then we should crash rather than try to silently recover and mask bad behavior.

[nasko, 2012/08/21 22:24:13]

Done.

+    return;
+
+  for (RenderViewHostMap::iterator iter = swapped_out_hosts_.begin();
+       iter != swapped_out_hosts_.end();
+       ++iter) {
+    DCHECK(iter->second->GetSiteInstance() !=

[awong, 2012/08/21 19:27:28]

DCHECK_NE?

[nasko, 2012/08/21 22:24:13]

Done.

+        current_host()->GetSiteInstance());
+
+    iter->second->UpdateFrameTree(
+        rvh->GetProcess()->GetID(), rvh->GetRoutingID(), rvh->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 blank SiteInstance to WebUI, make sure we

[awong, 2012/08/21 19:27:28]

from blank -> from a blank
to WebUI -> to a WebUI.=

[nasko, 2012/08/21 22:24:13]

Done.

+    // create a new one.

[awong, 2012/08/21 19:27:28]

"new one" is ambiguous. "new SiteInstance" is clearer.

[nasko, 2012/08/21 22:24:13]

Done.

+    const WebUIControllerFactory* web_ui_factory =
+        content::GetContentClient()->browser()->GetWebUIControllerFactory();
+    if (web_ui_factory &&
+        web_ui_factory->UseWebUIForURL(browser_context, dest_url))

[awong, 2012/08/21 19:27:28]

Use braces around the body of a conditional that has multiple lines in either
the condition or the statement.

[nasko, 2012/08/21 22:24:13]

Done.

+        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
+  // 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.

[awong, 2012/08/21 19:27:28]

File a bug for this?

Also, I can't tell how severe this behavior is. In this comment and/or bug, can
we describe what is the effect of having this workaround?

[nasko, 2012/08/21 22:24:13]

I'm not quite sure myself yet. I've hit almost 400 cases in tests where we are
creating a new RVH with the same site instance as the current one. This doesn't
seem right, but I also don't want to spend time investigating all of those cases
prior to committing this CL.

+    if (swapped_out) {

[awong, 2012/08/21 19:27:28]

remove brace here :)

[nasko, 2012/08/21 22:24:13]

Done.

+      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
+      // 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;
 }