Make sure new render is used when navigating away from about:blank in WebUI renderer.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/frame_host/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 18 matching lines @@
 #include "content/browser/webui/web_ui_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_ui_controller.h"
+#include "content/public/common/bindings_policy.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
 
 namespace content {
 
 // static
 bool RenderFrameHostManager::ClearRFHsPendingShutdown(FrameTreeNode* node) {
   node->render_manager()->pending_delete_hosts_.clear();
   return true;
@@ skipping 691 matching lines @@
 SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
     const GURL& dest_url,
     SiteInstance* dest_instance,
     ui::PageTransition dest_transition,
     bool dest_is_restore,
     bool dest_is_view_source_mode) {
   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);
+  if(current_instance->GetSiteURL().SchemeIs(kGuestScheme))

[Charlie Reis, 2014/10/20 19:37:53]

nit: Space after if.
(git cl format will help catch style errors like this.)

This change looks good-- I like returning early here rather than weaving
is_guest_scheme into everything.

[Krzysztof Olczyk, 2014/10/21 08:27:05]

Done.

+    return current_instance;
 
   // 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.
   // TODO(clamy): Remove the dependency on the current entry.
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
   BrowserContext* browser_context =
       delegate_->GetControllerForRenderManager().GetBrowserContext();
   const GURL& current_effective_url = current_entry ?
       SiteInstanceImpl::GetEffectiveURL(browser_context,
                                         current_entry->GetURL()) :
       render_frame_host_->GetSiteInstance()->GetSiteURL();
   bool current_is_view_source_mode = current_entry ?
       current_entry->IsViewSourceMode() : dest_is_view_source_mode;
-  bool force_swap = !is_guest_scheme &&
-      ShouldSwapBrowsingInstancesForNavigation(
-          current_effective_url,
-          current_is_view_source_mode,
-          dest_instance,
-          SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
-          dest_is_view_source_mode);
-  if (!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap)) {
+  int current_enabled_bindings =
+      render_frame_host_->render_view_host()->GetEnabledBindings();
+  bool navigating_away_from_blank_page_in_webui =
+      current_effective_url.spec() == url::kAboutBlankURL &&

[Charlie Reis, 2014/10/20 19:37:53]

This seems much too specific to me.  The underlying flaw here is that we aren't
checking the process bindings when deciding whether to force a swap.  The same
bug could come up with other URLs like data:, etc.

I suppose we don't usually notice this because GetSiteInstanceForURL normally
catches this case (via HasWrongProcessForURL).  However, I think
ShouldSwapBrowsingInstancesForNavigation should check both the URL and the
bindings when considering WebUI.  

I imagine we should fix this using the {Has/Use}WebUIBindings calls that also
are in RenderProcessHostImpl::IsSuitableHost.  I don't think we want to just
call HasWrongProcessForURL because that might force a swap for hosted apps,
which need to stay in the same BrowsingInstance.

[Krzysztof Olczyk, 2014/10/21 08:27:05]

Indeed, it looks like it makes more sense to put such a check there.
I modified ShouldSwapBrowsingInstancesForNavigation with HasWebUIBindings check.

+      (current_enabled_bindings & BINDINGS_POLICY_WEB_UI);
+  bool force_swap = ShouldSwapBrowsingInstancesForNavigation(
+      current_effective_url,
+      current_is_view_source_mode,
+      dest_instance,
+      SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
+      dest_is_view_source_mode);
+  if (ShouldTransitionCrossSite() || force_swap ||
+      navigating_away_from_blank_page_in_webui) {

[Charlie Reis, 2014/10/20 19:37:53]

I'd rather not make this more complicated in this location. 
ShouldSwapBrowsingInstancesForNavigation is the right place to be adding checks
that force a swap, rather than appending it as another condition here.  (And I
do think we should force a BrowsingInstance swap if the bindings situation
requires it.)

[Krzysztof Olczyk, 2014/10/21 08:27:05]

Done.

     new_instance = GetSiteInstanceForURL(
         dest_url,
         dest_instance,
         dest_transition,
         dest_is_restore,
         dest_is_view_source_mode,
         current_instance,
         force_swap);
   }
 
@@ skipping 808 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content