Reland #1 of: 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 1f6101d375b5172ea843a7e52505c5285766a6aa..e4c7fd7f0eb44e0417a87ccb26a498e45084f89e 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"
@@ -220,7 +218,7 @@ RenderFrameHostManager::RenderFrameHostManager(
       render_widget_delegate_(render_widget_delegate),
       proxy_hosts_(new RenderFrameProxyHostMap(this)),
       interstitial_page_(nullptr),
-      should_reuse_web_ui_(false),
+      current_web_ui_is_navigating_(false),
       weak_factory_(this) {
   DCHECK(frame_tree_node_);
 }
@@ -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::GetNavigatingWebUI() const {
+  if (current_web_ui_is_navigating_)
+    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();
@@ -357,27 +366,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,
@@ -426,6 +414,11 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
       return nullptr;
 
+    if (GetNavigatingWebUI()) {
+      GetNavigatingWebUI()->RenderViewCreated(

[Charlie Reis, 2015/11/09 07:01:00]

I don't understand the 5 separate RenderViewCreated calls spread through this
class.  This used to be called in a single place
(WebContentsImpl::RenderViewCreated), which was easy to reason about.  Now it's
not clear why they are where they are (which makes it hard to know if we've
covered all the cases, or when future cases might matter).

This one in particular confuses me, because the CreateRenderViewForRenderManager
call on line 394 looks like it used to trigger a RenderViewCreated call but now
it doesn't.  Why does this post-InitRenderView case need it and the earlier case
does not?

Is there a simpler approach we can use?  Is there a reason it can't stay in
WebContentsImpl::RenderViewCreated?

[carlosk, 2015/11/10 15:02:30]

There are a few reasons for that, some discusses in previous comment rounds in
the original change:
- Creation order was changed and now the WebUI is created after the RFH is.
Before WebContentsImpl::RenderViewCreated could call RFHM::pending_web_ui() and
it would return a valid value but now it won't (when creating a fully new
RFH/WebUI).
- It was decided the RFHI constructor should not be changed to immediately
initialize the WebUI. And also that we should not change all
RFH-creation-related methods to pump down the needed arguments for that. That's
the reason why this doesn't happen inside RFHM::InitRenderView either. All WebUI
update happens at a more "shallow" method level in RFHM.
- Before there was at least one missing call to RenderView(Created|Reused).

There was one more reason during the development of the refactor that might now
be deprecated: in some cases only RFHM had the information needed to decide
which of RenderView(Created|Reused) should be called. As right now there's only
one case of RenderViewReused. The reason for this is that *during this
development* we arrived at the conclusion that RenderViewCreated should be
called whenever either element in the WebUI/RVH pair is changed

And for the future:
- There's 2 "extra" calls right now because there's duplication between
PlzNavigate and the current navigation implementation (two will go away 
- There's some discussion around eliminating RenderViewReused so that would
simplify it even further.

One point though that I think is better now is that all of these notification
calls happen from one single class, RFHM, what makes them easier to track and
understand.

FYI, some related discussions I could dig from the previous change:
[1]
https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_...
[2]
https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_...
[3]
https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_...
[4]
https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_...

[Charlie Reis, 2015/11/11 01:11:00]

Thanks for the context.  It sounds like there's a lot of constraints on it that
we may be stuck with for now.

Would it be possible to add brief comments to the call sites about why it's
necessary there, and update the declaration comment in web_ui_impl.h?  That
would make it easier to reason about whether the calls are correct and when they
need changes or additions.

[carlosk, 2015/11/13 16:43:53]

Done.

+          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.
@@ -676,13 +669,14 @@ 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(!current_web_ui_is_navigating_ || 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_)
+    // A commit is required if there is a navigating WebUI, even without a
+    // pending or speculative RenderFrameHost.
+    if (GetNavigatingWebUI())
       CommitPending();
     return;
   }
@@ -1025,41 +1019,44 @@ 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

[Charlie Reis, 2015/11/09 07:01:00]

nit: "the the"

[carlosk, 2015/11/10 15:02:30]

Done.

+    // 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());
-      }
-    }
+    UpdateWebUIOnCurrentFrameHost(
+        request.common_params().url,
+        request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
+        request.bindings());
+
+    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
-    // this new SiteInstance.
+    // If the current RenderFrameHost cannot be used a new speculative one is
+    // created with the SiteInstance for the current URL.
+
+    // Check if an existing speculative RenderFrameHost can be reused.
     if (!speculative_render_frame_host_ ||
         speculative_render_frame_host_->GetSiteInstance() !=
             dest_site_instance.get()) {
+      // If a previous speculative RenderFrameHost didn't exist or if its
+      // SiteInstace differs from the one for the current URL, a new one needs

[Charlie Reis, 2015/11/09 07:01:00]

nit: SiteInstance

[carlosk, 2015/11/10 15:02:30]

Done.

+      // to be created.
       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);
     }
+
+    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(GetNavigatingWebUI(), speculative_render_frame_host_->web_ui());
+
     navigation_rfh = speculative_render_frame_host_.get();
 
     // Check if our current RFH is live.
@@ -1071,6 +1068,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
       // RFH isn't live.)
       CommitPending();
     }
+    DCHECK(!current_web_ui_is_navigating_);
   }
   DCHECK(navigation_rfh &&
          (navigation_rfh == render_frame_host_.get() ||
@@ -1081,8 +1079,12 @@ 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 (GetNavigatingWebUI()) {
+      GetNavigatingWebUI()->RenderViewCreated(
+          navigation_rfh->render_view_host());
     }
 
     if (navigation_rfh == render_frame_host_) {
@@ -1104,8 +1106,9 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
 void RenderFrameHostManager::CleanUpNavigation() {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
-  speculative_web_ui_.reset();
-  should_reuse_web_ui_ = false;
+  // TODO(carlosk): the discarding of the current RFH WebUI and the cleanup of
+  // the speculative RFH should not always happen together.
+  current_web_ui_is_navigating_ = false;
   if (speculative_render_frame_host_)
     DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
 }
@@ -1298,18 +1301,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,
@@ -1642,8 +1633,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(
@@ -1739,18 +1730,11 @@ 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);
+  CHECK(!current_web_ui_is_navigating_);
 
   // 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
@@ -1765,19 +1749,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);
@@ -1895,21 +1873,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);
@@ -2028,31 +1991,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());
 
@@ -2144,42 +2082,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(current_web_ui_is_navigating_, !!render_frame_host_->web_ui());
+    current_web_ui_is_navigating_ = false;
+    // If there's no pending/speculative RenderFrameHost then the current
+    // RenderFrameHost is committing.
     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
@@ -2196,7 +2109,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());
@@ -2366,9 +2280,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) {
@@ -2381,17 +2292,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);
+    current_web_ui_is_navigating_ = false;
     CreatePendingRenderFrameHost(current_instance, new_instance.get());
+    DCHECK(pending_render_frame_host_);
     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(
+          pending_render_frame_host_->render_view_host());
+    }
+
+    // We now have a pending RFH and possibly an associated pending WebUI.
+    DCHECK_EQ(GetNavigatingWebUI(), 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
@@ -2404,16 +2319,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) {
@@ -2448,23 +2359,7 @@ 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());
-    }
-  }
-
-  if (pending_web_ui() && render_frame_host_->IsRenderFrameLive()) {
-    pending_web_ui()->RenderViewReused(render_frame_host_->render_view_host(),
-                                       frame_tree_node_->IsMainFrame());
-  }
+  UpdateWebUIOnCurrentFrameHost(dest_url, dest_is_restore, bindings);
 
   // The renderer can exit view source mode when any error or cancellation
   // happen. We must overwrite to recover the mode.
@@ -2477,9 +2372,39 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   return render_frame_host_.get();
 }
 
+void RenderFrameHostManager::UpdateWebUIOnCurrentFrameHost(const GURL& dest_url,

[Charlie Reis, 2015/11/09 07:01:00]

Nasko's https://codereview.chromium.org/1413863006/ was meant to eliminate this
complexity, because a RenderFrameHost can't change its WebUI object anymore. 
See below for details.

It seems like we should only need to take care when adding a WebUI object to an
RFH in the first place.

[carlosk, 2015/11/10 15:02:30]

I'd love to get to that point! But right now that's not true yet.

+                                                           bool dest_is_restore,
+                                                           int bindings) {
+  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.
+  if (changed_web_ui)
+    CheckWebUITransition(previous_web_ui_type, dest_url, dest_is_restore);
+
+  // If there is a WebUI in the current RenderFrameHost, it will navigate.
+  current_web_ui_is_navigating_ = !!render_frame_host_->web_ui();
+  DCHECK_EQ(GetNavigatingWebUI(), 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 created or
+  // reused depending on whether the WebUI was changed or not.
+  if (GetNavigatingWebUI() && render_frame_host_->IsRenderFrameLive()) {
+    if (changed_web_ui) {
+      GetNavigatingWebUI()->RenderViewCreated(
+          render_frame_host_->render_view_host());
+    } else {
+      GetNavigatingWebUI()->RenderViewReused(
+          render_frame_host_->render_view_host(),
+          frame_tree_node_->IsMainFrame());
+    }
+  }
+}
+
 void RenderFrameHostManager::CancelPending() {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CancelPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
+  current_web_ui_is_navigating_ = false;
   DiscardUnusedFrame(UnsetPendingRenderFrameHost());
 }
 
@@ -2492,12 +2417,11 @@ RenderFrameHostManager::UnsetPendingRenderFrameHost() {
       pending_render_frame_host.get(),
       render_frame_host_.get());
 
+  current_web_ui_is_navigating_ = 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();
 }
 
@@ -2672,8 +2596,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);
       }
     }
   }
@@ -2688,4 +2611,51 @@ int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) {
       ->GetRoutingIdForSiteInstance(instance);
 }
 
+void RenderFrameHostManager::CheckWebUITransition(
+    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

[Charlie Reis, 2015/11/09 07:01:00]

I agree that this is not a sustainable approach, and I'm not sure what value
it's adding.  Wouldn't it be sufficient to say that an RFH can't change its
WebUI object once it's added?  (See below about renderer debug URLs, which only
commit a page in rare cases like chrome://crashdump or chrome://shorthang.)

[carlosk, 2015/11/10 15:02:30]

It doesn't seem like just that will be sufficient. Crash reports from this
method [1] show that all 3 different CHECKs got triggered. So there are still
unknown cases (not mapped by tests) where there was no change of SiteInstance
(same-site navigation) but there was a WebUI transition: no WebUI to having a
WebUI, vice-versa, or a change of WebUI type.

To fix that it seems we'd have to further change SiteInstance selection logic
and/or decide what should the WebUI selection logic do when a situation that is
not allowed happens.

[1]
https://crash.corp.google.com/browse?stbtiq=content%3A%3ARenderFrameHostManag...

[Charlie Reis, 2015/11/11 01:11:00]

Interesting.  This seems like something we should get to the bottom of, before
re-landing this CL.

Can we add some equivalent checks to trunk (without the rest of the changes in
this CL) along with some crash keys to see what's causing us to fail these
invariants?  There's an example of this style of crash debugging here:
https://codereview.chromium.org/1410543014/

I think this will be a worthwhile investigation (though I'm sure it feels like a
distraction), since the code would really benefit if a RenderFrameHost can't
change its WebUI object.

[carlosk, 2015/11/13 16:43:53]

This depends on our decision regarding the pending WebUI.

+  // the current RenderFrameHost.
+  BrowserContext* browser_context =
+      delegate_->GetControllerForRenderManager().GetBrowserContext();
+  if (render_frame_host_->web_ui()) {
+    // Switching WebUI from one type to another while keeping the
+    // RenderFrameHost is never acceptable.
+    CHECK(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 URL.

[Charlie Reis, 2015/11/09 07:01:00]

This seems like an unnecessary case.  Why not just leave the WebUI object in
place for renderer debug URLs?  Then the WebUI object of an RFH never changes
once it's assigned, and we only have to be careful when first granting a WebUI
object.

I think the only renderer debug URLs that can commit in a WebUI are
chrome://crashdump and chrome://shorthang (context:
https://crbug.com/477606#c16), and those don't seem to pose a risk that would
require dropping WebUI.  Even if javascript: URLs were allowed on WebUI, they
can run on the current page so dropping the WebUI if they commit doesn't seem
helpful.

[carlosk, 2015/11/10 15:02:30]

SGTM. But as for my previous questions should the logic to decide about that be
kept here in RFHM or in RFHI? (I guess that also depends on the answers from my
previous questions)
Acknowledged.

[Charlie Reis, 2015/11/11 01:11:00]

Probably RFHI, but that's just guessing at this point.

[carlosk, 2015/11/13 16:43:53]

This depends on our decision regarding the pending WebUI.

+    bool is_back_from_renderer_url =
+        current_entry && IsRendererDebugURL(current_entry->GetURL());
+
+    // - Navigating in webview tag guest.
+    bool is_webview_tag_guest =
+        render_frame_host_->GetSiteInstance()->GetSiteURL().SchemeIs(
+            kGuestScheme);
+    CHECK(is_first_navigation_for_frame || is_webui_restore ||
+          is_back_from_renderer_url || is_webview_tag_guest);
+  } else {
+    // Going from having to not having a WebUI is acceptable when Navigating to
+    // a special renderer URL.
+    CHECK(IsRendererDebugURL(
+        SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url)));
+  }
+}
+
 }  // namespace content