Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.

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 7e53ce84586f542c2573c4409bf1691a49415278..4baf0efeaf4205fb815c7e72cfd08f8f7f3c10c9 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -30,7 +30,6 @@
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
-#include "content/browser/webui/web_ui_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/view_messages.h"
@@ -39,7 +38,6 @@
 #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/browser_plugin_guest_mode.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
@@ -245,10 +243,6 @@ RenderFrameHostManager::~RenderFrameHostManager() {
   // the current RenderFrameHost and uses it during its destructor.
   ResetProxyHosts();
 
-  // Release the WebUI prior to resetting the current RenderFrameHost, as the
-  // WebUI accesses the RenderFrameHost during cleanup.
-  web_ui_.reset();
-
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
 }
@@ -289,6 +283,21 @@ RenderViewHostImpl* RenderFrameHostManager::pending_render_view_host() const {
   return pending_render_frame_host_->render_view_host();
 }
 
+WebUIImpl* RenderFrameHostManager::pending_web_ui() const {
+  if (should_reuse_web_ui_)
+    return render_frame_host_->web_ui();
+
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    if (speculative_render_frame_host_)
+      return speculative_render_frame_host_->web_ui();
+  } else {
+    if (pending_render_frame_host_)
+      return pending_render_frame_host_->web_ui();
+  }
+  return nullptr;
+}
+
 RenderWidgetHostView* RenderFrameHostManager::GetRenderWidgetHostView() const {
   if (interstitial_page_)
     return interstitial_page_->GetView();
@@ -351,27 +360,6 @@ void RenderFrameHostManager::RemoveOuterDelegateFrame() {
       outer_delegate_frame_tree_node);
 }
 
-void RenderFrameHostManager::SetPendingWebUI(const GURL& url, int bindings) {
-  pending_web_ui_ = CreateWebUI(url, bindings);
-  pending_and_current_web_ui_.reset();
-}
-
-scoped_ptr<WebUIImpl> RenderFrameHostManager::CreateWebUI(const GURL& url,
-                                                          int bindings) {
-  scoped_ptr<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 (new_web_ui && bindings != NavigationEntryImpl::kInvalidBindings &&
-      new_web_ui->GetBindings() != bindings) {
-    RecordAction(base::UserMetricsAction("ProcessSwapBindingsMismatch_RVHM"));
-    return nullptr;
-  }
-  return new_web_ui.Pass();
-}
-
 RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     const GURL& dest_url,
     const FrameNavigationEntry& frame_entry,
@@ -420,6 +408,11 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
       return nullptr;
 
+    if (pending_web_ui()) {
+      pending_web_ui()->RenderViewCreated(
+          dest_render_frame_host->render_view_host());
+    }
+
     // Now that we've created a new renderer, be sure to hide it if it isn't
     // our primary one.  Otherwise, we might crash if we try to call Show()
     // on it later.
@@ -670,13 +663,13 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
     RenderFrameHostImpl* render_frame_host,
     bool was_caused_by_user_gesture) {
   if (!pending_render_frame_host_ && !speculative_render_frame_host_) {
-    DCHECK(!should_reuse_web_ui_ || web_ui_);
+    DCHECK(!should_reuse_web_ui_ || render_frame_host_->web_ui());
 
     // We should only hear this from our current renderer.
     DCHECK_EQ(render_frame_host_, render_frame_host);
 
     // Even when there is no pending RVH, there may be a pending Web UI.
-    if (pending_web_ui() || speculative_web_ui_)
+    if (pending_web_ui())
       CommitPending();
     return;
   }
@@ -1019,27 +1012,33 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
       (!request.browser_initiated() && is_main_frame) ||
       (!is_main_frame && !dest_site_instance->RequiresDedicatedProcess() &&
        !current_site_instance->RequiresDedicatedProcess())) {
-    // Reuse the current RFH if its SiteInstance matches the the navigation's
-    // or if this is a subframe navigation. We only swap RFHs for subframes when
-    // --site-per-process is enabled.
+    // Reuse the current RenderFrameHost if its SiteInstance matches the the
+    // navigation's or if this is a subframe navigation. We only swap RFHs for
+    // subframes when --site-per-process is enabled.
     CleanUpNavigation();
     navigation_rfh = render_frame_host_.get();
 
-    // As SiteInstances are the same, check if the WebUI should be reused.
-    const NavigationEntry* current_navigation_entry =
-        delegate_->GetLastCommittedNavigationEntryForRenderManager();
-    should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry,
-                                            request.common_params().url);
-    if (!should_reuse_web_ui_) {
-      speculative_web_ui_ = CreateWebUI(request.common_params().url,
-                                        request.bindings());
-      // Make sure the current RenderViewHost has the right bindings.
-      if (speculative_web_ui() &&
-          !render_frame_host_->GetProcess()->IsForGuestsOnly()) {
-        render_frame_host_->render_view_host()->AllowBindings(
-            speculative_web_ui()->GetBindings());
-      }
+    WebUI::TypeID previous_web_ui_type = render_frame_host_->web_ui_type();
+    bool changed_web_ui = render_frame_host_->UpdateWebUI(
+        request.common_params().url, request.bindings());
+
+    // If a change in WebUI happened, check this is an acceptable case.
+    DCHECK(!changed_web_ui || (IsAcceptableWebUITransition(
+            previous_web_ui_type, request.common_params().url,
+            request.restore_type() != NavigationEntryImpl::RESTORE_NONE)));
+
+    // If there is a WebUI in the current RenderFrameHost, it will be reused.
+    should_reuse_web_ui_ = render_frame_host_->web_ui();

[nasko, 2015/10/29 22:13:15]

Wouldn't UpdateWebUI above create one if needed? So if before there wasn't one
or one that didn't match the type, the value of should_reuse_web_ui_ will be
incorrect?

[carlosk, 2015/10/30 10:35:23]

Conceptually this is incorrect as you described but, by chance, there is no side
effect in practice. The result from a call to pending_web_ui() would still
return the correct WebUI for the navigation. I renamed this boolean to
current_web_ui_is_navigating_ to fix the conceptual error.

But his brought up another error: as a new WebUI can finally be created here,
the notification section below must be aware of that and call RenderViewCreated
instead. What is now done.

+    DCHECK_EQ(pending_web_ui(), render_frame_host_->web_ui());
+
+    // If the current RenderFrameHost has a WebUI and the associated RenderFrame
+    // is alive, notify to the WebUI that the RenderView is being reused.
+    if (pending_web_ui() && render_frame_host_->IsRenderFrameLive()) {
+      pending_web_ui()->RenderViewReused(render_frame_host_->render_view_host(),
+                                         frame_tree_node_->IsMainFrame());
     }
+
+    DCHECK(!speculative_render_frame_host_);
   } else {
     // If the SiteInstance for the final URL doesn't match the one from the
     // speculatively created RenderFrameHost, create a new RenderFrameHost using
@@ -1047,13 +1046,31 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     if (!speculative_render_frame_host_ ||
         speculative_render_frame_host_->GetSiteInstance() !=
             dest_site_instance.get()) {
+      // Creates a new speculative RenderFrameHost if the previous one didn't
+      // exist or if its SiteInstace differs from the current one.
       CleanUpNavigation();
-      bool success = CreateSpeculativeRenderFrameHost(
-          request.common_params().url, current_site_instance,
-          dest_site_instance.get(), request.bindings());
+      bool success = CreateSpeculativeRenderFrameHost(current_site_instance,
+                                                      dest_site_instance.get());
       DCHECK(success);
+
+      speculative_render_frame_host_->UpdateWebUI(request.common_params().url,
+                                                  request.bindings());
+      if (speculative_render_frame_host_->web_ui()) {
+        speculative_render_frame_host_->web_ui()->RenderViewCreated(
+            speculative_render_frame_host_->render_view_host());
+      }
+    } else {
+      // Reuses the previous speculative RenderFrameHost.
+      bool changed_web_ui = speculative_render_frame_host_->UpdateWebUI(
+          request.common_params().url, request.bindings());
+      if (changed_web_ui && speculative_render_frame_host_->web_ui()) {
+        speculative_render_frame_host_->web_ui()->RenderViewCreated(
+            speculative_render_frame_host_->render_view_host());
+      }
     }
     DCHECK(speculative_render_frame_host_);
+    DCHECK_EQ(pending_web_ui(), speculative_render_frame_host_->web_ui());
+
     navigation_rfh = speculative_render_frame_host_.get();
 
     // Check if our current RFH is live.
@@ -1065,6 +1082,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
       // RFH isn't live.)
       CommitPending();
     }
+    DCHECK(!should_reuse_web_ui_);
   }
   DCHECK(navigation_rfh &&
          (navigation_rfh == render_frame_host_.get() ||
@@ -1075,8 +1093,11 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
     CreateOpenerProxies(navigation_rfh->GetSiteInstance(), frame_tree_node_);
-    if (!InitRenderView(navigation_rfh->render_view_host(), nullptr)) {
+    if (!InitRenderView(navigation_rfh->render_view_host(), nullptr))
       return nullptr;
+
+    if (pending_web_ui()) {
+      pending_web_ui()->RenderViewCreated(navigation_rfh->render_view_host());
     }
 
     if (navigation_rfh == render_frame_host_) {
@@ -1098,7 +1119,8 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
 void RenderFrameHostManager::CleanUpNavigation() {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
-  speculative_web_ui_.reset();
+  // TODO(carlosk): the discarding of the current RFH WebUI and the cleanup of
+  // the speculative RFH should not always happen together.
   should_reuse_web_ui_ = false;
   if (speculative_render_frame_host_)
     DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
@@ -1292,18 +1314,6 @@ bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
   return false;
 }
 
-bool RenderFrameHostManager::ShouldReuseWebUI(
-    const NavigationEntry* current_entry,
-    const GURL& new_url) const {
-  NavigationControllerImpl& controller =
-      delegate_->GetControllerForRenderManager();
-  return current_entry && web_ui_ &&
-      (WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
-          controller.GetBrowserContext(), current_entry->GetURL()) ==
-       WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
-          controller.GetBrowserContext(), new_url));
-}
-
 SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
     const GURL& dest_url,
     SiteInstance* source_instance,
@@ -1636,8 +1646,8 @@ void RenderFrameHostManager::CreatePendingRenderFrameHost(
   CreateProxiesForNewRenderFrameHost(old_instance, new_instance);
 
   // Create a non-swapped-out RFH with the given opener.
-  pending_render_frame_host_ = CreateRenderFrame(
-      new_instance, pending_web_ui(), create_render_frame_flags, nullptr);
+  pending_render_frame_host_ =
+      CreateRenderFrame(new_instance, create_render_frame_flags, nullptr);
 }
 
 void RenderFrameHostManager::CreateProxiesForNewRenderFrameHost(
@@ -1733,19 +1743,12 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
 
 // PlzNavigate
 bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
-    const GURL& url,
     SiteInstance* old_instance,
-    SiteInstance* new_instance,
-    int bindings) {
+    SiteInstance* new_instance) {
   CHECK(new_instance);
   CHECK_NE(old_instance, new_instance);
   CHECK(!should_reuse_web_ui_);
 
-  // Note: |speculative_web_ui_| must be initialized before starting the
-  // |speculative_render_frame_host_| creation steps otherwise the WebUI
-  // won't be properly initialized.
-  speculative_web_ui_ = CreateWebUI(url, bindings);
-
   // The process for the new SiteInstance may (if we're sharing a process with
   // another host that already initialized it) or may not (we have our own
   // process or the existing process crashed) have been initialized. Calling
@@ -1759,19 +1762,13 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
   if (delegate_->IsHidden())
     create_render_frame_flags |= CREATE_RF_HIDDEN;
   speculative_render_frame_host_ =
-      CreateRenderFrame(new_instance, speculative_web_ui_.get(),
-                        create_render_frame_flags, nullptr);
+      CreateRenderFrame(new_instance, create_render_frame_flags, nullptr);
 
-  if (!speculative_render_frame_host_) {
-    speculative_web_ui_.reset();
-    return false;
-  }
-  return true;
+  return !!speculative_render_frame_host_;
 }
 
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
     SiteInstance* instance,
-    WebUIImpl* web_ui,
     int flags,
     int* view_routing_id_ptr) {
   bool swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
@@ -1889,21 +1886,6 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
     }
   }
 
-  // When a new RenderView is created by the renderer process, the new
-  // WebContents 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 (web_ui && !new_render_frame_host->GetProcess()->IsForGuestsOnly()) {
-    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);
-    }
-  }
-
   // Returns the new RFH if it isn't swapped out.
   if (success && !swapped_out) {
     DCHECK(new_render_frame_host->GetSiteInstance() == instance);
@@ -2022,31 +2004,6 @@ bool RenderFrameHostManager::InitRenderView(
   if (render_view_host->IsRenderViewLive())
     return true;
 
-  // If |render_view_host| is not for a proxy and the navigation is to a WebUI,
-  // and if the RenderView is not in a guest process, tell |render_view_host|
-  // about any bindings it will need enabled.
-  // TODO(carlosk): Move WebUI to RenderFrameHost in https://crbug.com/508850.
-  WebUIImpl* dest_web_ui = nullptr;
-  if (!proxy) {
-    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-            switches::kEnableBrowserSideNavigation)) {
-      dest_web_ui =
-          should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
-    } else {
-      dest_web_ui = pending_web_ui();
-    }
-  }
-  if (dest_web_ui && !render_view_host->GetProcess()->IsForGuestsOnly()) {
-    render_view_host->AllowBindings(dest_web_ui->GetBindings());
-  } else {
-    // Ensure that we don't create an unprivileged RenderView in a WebUI-enabled
-    // process unless it's swapped out.
-    if (render_view_host->is_active()) {
-      CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
-                render_view_host->GetProcess()->GetID()));
-    }
-  }
-
   int opener_frame_routing_id =
       GetOpenerRoutingID(render_view_host->GetSiteInstance());
 
@@ -2138,42 +2095,17 @@ int RenderFrameHostManager::GetRoutingIdForSiteInstance(
 void RenderFrameHostManager::CommitPending() {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CommitPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
-  bool browser_side_navigation =
-      base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableBrowserSideNavigation);
-
   // First check whether we're going to want to focus the location bar after
   // this commit.  We do this now because the navigation hasn't formally
-  // committed yet, so if we've already cleared |pending_web_ui_| the call chain
+  // committed yet, so if we've already cleared the pending WebUI the call chain
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
-  // Next commit the Web UI, if any. Either replace |web_ui_| with
-  // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
-  // leave |web_ui_| as is if reusing it.
-  DCHECK(!(pending_web_ui_ && pending_and_current_web_ui_));
-  if (pending_web_ui_ || speculative_web_ui_) {
-    DCHECK(!should_reuse_web_ui_);
-    web_ui_.reset(browser_side_navigation ? speculative_web_ui_.release()
-                                          : pending_web_ui_.release());
-  } else if (pending_and_current_web_ui_ || should_reuse_web_ui_) {
-    if (browser_side_navigation) {
-      DCHECK(web_ui_);
-      should_reuse_web_ui_ = false;
-    } else {
-      DCHECK_EQ(pending_and_current_web_ui_.get(), web_ui_.get());
-      pending_and_current_web_ui_.reset();
-    }
-  } else {
-    web_ui_.reset();
-  }
-  DCHECK(!speculative_web_ui_);
-  DCHECK(!should_reuse_web_ui_);
-
-  // It's possible for the pending_render_frame_host_ to be nullptr when we
-  // aren't crossing process boundaries. If so, we just needed to handle the Web
-  // UI committing above and we're done.
   if (!pending_render_frame_host_ && !speculative_render_frame_host_) {
+    DCHECK_EQ(should_reuse_web_ui_, !!render_frame_host_->web_ui());
+    should_reuse_web_ui_ = false;
+    // If there's no pending/speculative RenderFrameHost then the current
+    // RenderFrameHost is committing.
     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
@@ -2190,7 +2122,8 @@ void RenderFrameHostManager::CommitPending() {
   // Swap in the pending or speculative frame and make it active. Also ensure
   // the FrameTree stays in sync.
   scoped_ptr<RenderFrameHostImpl> old_render_frame_host;
-  if (!browser_side_navigation) {
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
     DCHECK(!speculative_render_frame_host_);
     old_render_frame_host =
         SetRenderFrameHost(pending_render_frame_host_.Pass());
@@ -2353,9 +2286,6 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
       dest_url, source_instance, dest_instance, nullptr, transition,
       dest_is_restore, dest_is_view_source_mode);
 
-  const NavigationEntry* current_entry =
-      delegate_->GetLastCommittedNavigationEntryForRenderManager();
-
   DCHECK(!pending_render_frame_host_);
 
   if (new_instance.get() != current_instance) {
@@ -2368,17 +2298,21 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
 
     // New SiteInstance: create a pending RFH to navigate.
 
-    // This will possibly create (set to nullptr) a Web UI object for the
-    // pending page. We'll use this later to give the page special access. This
-    // must happen before the new renderer is created below so it will get
-    // bindings. It must also happen after the above conditional call to
-    // CancelPending(), otherwise CancelPending may clear the pending_web_ui_
-    // and the page will not have its bindings set appropriately.
-    SetPendingWebUI(dest_url, bindings);
+    should_reuse_web_ui_ = false;
     CreatePendingRenderFrameHost(current_instance, new_instance.get());
     if (!pending_render_frame_host_)
       return nullptr;
 
+    pending_render_frame_host_->UpdateWebUI(dest_url, bindings);
+    if (pending_render_frame_host_->web_ui()) {
+      pending_render_frame_host_->web_ui()->RenderViewCreated(

[nasko, 2015/10/29 22:13:15]

I know the naming will be weird, but can't we fold those
RenderViewCreated/Reused notifications inside UpdateWebUI?

[carlosk, 2015/10/30 10:35:23]

That's what I tried before and I had to move them back out here. Finally the
knowledge of if and when to call one or the other is only fully known at this
level, not inside RFHI. So to not have these calls spread between these two
files, which would be confusing, I find it preferable to have them all here.

+          pending_render_frame_host_->render_view_host());
+    }
+
+    // We now have a pending RFH and possibly an associated pending WebUI.
+    DCHECK(pending_render_frame_host_);
+    DCHECK_EQ(pending_web_ui(), pending_render_frame_host_->web_ui());
+
     // Check if our current RFH is live before we set up a transition.
     if (!render_frame_host_->IsRenderFrameLive()) {
       // The current RFH is not live.  There's no reason to sit around with a
@@ -2391,16 +2325,12 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
     }
     // Otherwise, it's safe to treat this as a pending cross-process transition.
 
-    // We now have a pending RFH.
-    DCHECK(pending_render_frame_host_);
-
     // We need to wait until the beforeunload handler has run, unless we are
     // transferring an existing request (in which case it has already run).
     // Suspend the new render view (i.e., don't let it send the cross-process
     // Navigate message) until we hear back from the old renderer's
     // beforeunload handler.  If the handler returns false, we'll have to
     // cancel the request.
-    //
     DCHECK(!pending_render_frame_host_->are_navigations_suspended());
     bool is_transfer = transferred_request_id != GlobalRequestID();
     if (is_transfer) {
@@ -2435,19 +2365,20 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   // delete the proxy.
   proxy_hosts_->Remove(new_instance.get()->GetId());
 
-  if (ShouldReuseWebUI(current_entry, dest_url)) {
-    pending_web_ui_.reset();
-    pending_and_current_web_ui_ = web_ui_->AsWeakPtr();
-  } else {
-    SetPendingWebUI(dest_url, bindings);
-    // Make sure the new RenderViewHost has the right bindings.
-    if (pending_web_ui() &&
-        !render_frame_host_->GetProcess()->IsForGuestsOnly()) {
-      render_frame_host_->render_view_host()->AllowBindings(
-          pending_web_ui()->GetBindings());
-    }
-  }
+  WebUI::TypeID previous_web_ui_type = render_frame_host_->web_ui_type();
+  bool changed_web_ui = render_frame_host_->UpdateWebUI(dest_url, bindings);
+
+  // If a change in WebUI happened, check this is an acceptable case.

[nasko, 2015/10/29 22:13:15]

In general, what happens if this is not an acceptable case? DCHECK is compiled
out in release builds, so will we have a security problem if we continue with
unacceptable case?

[carlosk, 2015/10/30 10:35:23]

Right now the navigation would simply continue in release builds. If this is
really considered a security issue I could change this to a CHECK instead.

But I wouldn't feel comfortable with that right now because I am not sure I
mapped all the "special cases" -- as we're calling them -- where these
transitions are allowed, what cause crashes in the real world. I based them from
running tests and they might not cover all possibilities. And as the logic that
determines site instance changes is complex it's really hard to not do it
empirically.

[nasko, 2015/11/02 17:00:35]

I think it should be a CHECK. Allocating WebUI where one shouldn't be is a
serious issue. We can monitor crashes to ensure we've covered all cases.

[carlosk, 2015/11/03 09:50:30]

Done.

+  DCHECK(!changed_web_ui ||
+         IsAcceptableWebUITransition(previous_web_ui_type, dest_url,
+                                     dest_is_restore));
+
+  // If there is a WebUI in the current RenderFrameHost, it will be reused.
+  should_reuse_web_ui_ = render_frame_host_->web_ui();
+  DCHECK_EQ(pending_web_ui(), render_frame_host_->web_ui());
 
+  // If the current RenderFrameHost has a WebUI and the associated RenderFrame
+  // is alive, notify to the WebUI that the RenderView is being reused.
   if (pending_web_ui() && render_frame_host_->IsRenderFrameLive()) {
     pending_web_ui()->RenderViewReused(render_frame_host_->render_view_host(),
                                        frame_tree_node_->IsMainFrame());
@@ -2467,6 +2398,7 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
 void RenderFrameHostManager::CancelPending() {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CancelPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
+  should_reuse_web_ui_ = false;
   DiscardUnusedFrame(UnsetPendingRenderFrameHost());
 }
 
@@ -2479,12 +2411,11 @@ RenderFrameHostManager::UnsetPendingRenderFrameHost() {
       pending_render_frame_host.get(),
       render_frame_host_.get());
 
+  should_reuse_web_ui_ = false;
+
   // We no longer need to prevent the process from exiting.
   pending_render_frame_host->GetProcess()->RemovePendingView();
 
-  pending_web_ui_.reset();
-  pending_and_current_web_ui_.reset();
-
   return pending_render_frame_host.Pass();
 }
 
@@ -2659,8 +2590,7 @@ void RenderFrameHostManager::CreateOpenerProxiesForFrameTree(
         frame_tree->root()->render_manager()->CreateRenderFrameProxy(instance);
       } else {
         frame_tree->root()->render_manager()->CreateRenderFrame(
-            instance, nullptr, CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN,
-            nullptr);
+            instance, CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN, nullptr);
       }
     }
   }
@@ -2675,4 +2605,44 @@ int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) {
       ->GetRoutingIdForSiteInstance(instance);
 }
 
+bool RenderFrameHostManager::IsAcceptableWebUITransition(
+    WebUI::TypeID previous_web_ui_type,
+    const GURL& dest_url,
+    bool dest_is_restore) {
+  // There are a few navigation cases that allow for changes to the WebUI of
+  // the current RenderFrameHost.
+  BrowserContext* browser_context =
+      delegate_->GetControllerForRenderManager().GetBrowserContext();
+  if (render_frame_host_->web_ui()) {
+    // Switching WebUI from one type to another is never acceptable.
+    DCHECK(previous_web_ui_type == WebUI::kNoWebUI);
+
+    // Going from not having to having a WebUI is acceptable for:
+    // - The first navigation of this frame.
+    const NavigationEntry* current_entry =
+        delegate_->GetLastCommittedNavigationEntryForRenderManager();
+    bool is_first_navigation_for_frame = !current_entry;
+    // - A restore navigation to a WebUI URL.
+    const GURL& current_effective_url =
+        current_entry ? SiteInstanceImpl::GetEffectiveURL(
+                            browser_context, current_entry->GetURL())
+                      : render_frame_host_->GetSiteInstance()->GetSiteURL();
+    bool is_webui_restore =
+        dest_is_restore &&
+        WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
+            browser_context, current_effective_url);
+    // - Navigating back from a special renderer controlled URL.
+    bool is_back_from_renderer_url =
+        current_entry && IsRendererDebugURL(current_entry->GetURL());
+    DCHECK(is_first_navigation_for_frame || is_webui_restore ||
+           is_back_from_renderer_url);
+  } else {
+    // Going from having to not having a WebUI is acceptable for:
+    // - Navigating to a special, renderer controlled URL.
+    DCHECK(IsRendererDebugURL(
+        SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url)));
+  }
+  return true;
+}
+
 }  // namespace content