Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.

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 <algorithm>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 12 matching lines @@
 #include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_factory.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/frame_host/render_frame_proxy_host.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #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"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host_observer.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/browser_plugin_guest_mode.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
 
 namespace content {
 
 namespace {
 
 // Helper function to add the FrameTree of the given node's opener to the list
@@ skipping 160 matching lines @@
     RenderViewHostDelegate* render_view_delegate,
     RenderWidgetHostDelegate* render_widget_delegate,
     Delegate* delegate)
     : frame_tree_node_(frame_tree_node),
       delegate_(delegate),
       render_frame_delegate_(render_frame_delegate),
       render_view_delegate_(render_view_delegate),
       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_);
 }
 
 RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_) {
     scoped_ptr<RenderFrameHostImpl> relic = UnsetPendingRenderFrameHost();
     ShutdownProxiesIfLastActiveFrameInSiteInstance(relic.get());
   }
 
   if (speculative_render_frame_host_) {
     scoped_ptr<RenderFrameHostImpl> relic = UnsetSpeculativeRenderFrameHost();
     ShutdownProxiesIfLastActiveFrameInSiteInstance(relic.get());
   }
 
   ShutdownProxiesIfLastActiveFrameInSiteInstance(render_frame_host_.get());
 
   // Delete any RenderFrameProxyHosts and swapped out RenderFrameHosts.
   // It is important to delete those prior to deleting the current
   // RenderFrameHost, since the CrossProcessFrameConnector (owned by
   // RenderFrameProxyHost) points to the RenderWidgetHostView associated with
   // 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>());
 }
 
 void RenderFrameHostManager::Init(SiteInstance* site_instance,
                                   int32 view_routing_id,
                                   int32 frame_routing_id,
                                   int32 widget_routing_id) {
   DCHECK(site_instance);
   // TODO(avi): While RenderViewHostImpl is-a RenderWidgetHostImpl, this must
@@ skipping 20 matching lines @@
     return nullptr;
   return render_frame_host_->render_view_host();
 }
 
 RenderViewHostImpl* RenderFrameHostManager::pending_render_view_host() const {
   if (!pending_render_frame_host_)
     return nullptr;
   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();
   if (render_frame_host_)
     return render_frame_host_->GetView();
   return nullptr;
 }
 
 bool RenderFrameHostManager::ForInnerDelegate() {
   return delegate_->GetOuterDelegateFrameTreeNodeID() !=
@@ skipping 48 matching lines @@
 
 void RenderFrameHostManager::RemoveOuterDelegateFrame() {
   FrameTreeNode* outer_delegate_frame_tree_node =
       FrameTreeNode::GloballyFindByID(
           delegate_->GetOuterDelegateFrameTreeNodeID());
   DCHECK(outer_delegate_frame_tree_node->parent());
   outer_delegate_frame_tree_node->frame_tree()->RemoveFrame(
       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,
     const NavigationEntryImpl& entry) {
   TRACE_EVENT1("navigation", "RenderFrameHostManager:Navigate",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
   // Create a pending RenderFrameHost to use for the navigation.
   RenderFrameHostImpl* dest_render_frame_host = UpdateStateForNavigate(
       dest_url,
       // TODO(creis): Move source_site_instance to FNE.
@@ skipping 28 matching lines @@
     // site that is handled via Mojo, then Mojo WebUI code in //chrome will
     // add a service to this RFH's ServiceRegistry).
     dest_render_frame_host->SetUpMojoIfNeeded();
 
     // Recreate the opener chain.
     CreateOpenerProxies(dest_render_frame_host->GetSiteInstance(),
                         frame_tree_node_);
     if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
       return nullptr;
 
+    if (GetNavigatingWebUI()) {
+      GetNavigatingWebUI()->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.
     if (dest_render_frame_host != render_frame_host_) {
       if (dest_render_frame_host->GetView())
         dest_render_frame_host->GetView()->Hide();
     } else {
       // After a renderer crash we'd have marked the host as invisible, so we
       // need to set the visibility of the new View to the correct value here
       // after reload.
@@ skipping 230 matching lines @@
 
   // Make sure any dynamic changes to this frame's sandbox flags that were made
   // prior to navigation take effect.
   CommitPendingSandboxFlags();
 }
 
 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;
   }
 
   if (render_frame_host == pending_render_frame_host_ ||
       render_frame_host == speculative_render_frame_host_) {
     // The pending cross-process navigation completed, so show the renderer.
     CommitPending();
   } else if (render_frame_host == render_frame_host_) {
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
@@ skipping 322 matching lines @@
   // Subframe navigations will use the current renderer, unless
   // --site-per-process is enabled.
   // TODO(carlosk): Have renderer-initated main frame navigations swap processes
   // if needed when it no longer breaks OAuth popups (see
   // https://crbug.com/440266).
   bool is_main_frame = frame_tree_node_->IsMainFrame();
   if (current_site_instance == dest_site_instance.get() ||
       (!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.
+    CHECK(!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 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());
       }
     }
+
+    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 (!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(

[nasko, 2015/11/04 17:16:10]

The comment above says reuser, yet the call is to RenderViewCreated. If we still
call this method, it seems to me that the code to UpdateWebUI can be pulled
after the if statement, as it is identical and the checks are very similar.

[carlosk, 2015/11/04 21:43:59]

We reuse the speculative RFH but if a new WebUI is created it will be the first
time it interacts with the speculative RFH's RVH, hence the call to
RenderViewCreated.

But indeed that makes so that we can merge the two versions of this
update-and-notify logic as you described. Nice catch.

Done.

[nasko, 2015/11/05 00:36:38]

Awesome!

+            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.
     if (!render_frame_host_->IsRenderFrameLive()) {
       // The current RFH is not live.  There's no reason to sit around with a
       // sad tab or a newly created RFH while we wait for the navigation to
       // complete. Just switch to the speculative RFH now and go back to normal.
       // (Note that we don't care about on{before}unload handlers if the current
       // RFH isn't live.)
       CommitPending();
     }
+    DCHECK(!current_web_ui_is_navigating_);
   }
   DCHECK(navigation_rfh &&
          (navigation_rfh == render_frame_host_.get() ||
           navigation_rfh == speculative_render_frame_host_.get()));
 
   // If the RenderFrame that needs to navigate is not live (its process was just
   // created or has crashed), initialize it.
   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_) {
       // TODO(nasko): This is a very ugly hack. The Chrome extensions process
       // manager still uses NotificationService and expects to see a
       // RenderViewHost changed notification after WebContents and
       // RenderFrameHostManager are completely initialized. This should be
       // removed once the process manager moves away from NotificationService.
       // See https://crbug.com/462682.
       delegate_->NotifyMainFrameSwappedFromRenderManager(
           nullptr, render_frame_host_->render_view_host());
     }
   }
 
   return navigation_rfh;
 }
 
 // PlzNavigate
 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());
 }
 
 // PlzNavigate
 scoped_ptr<RenderFrameHostImpl>
 RenderFrameHostManager::UnsetSpeculativeRenderFrameHost() {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   speculative_render_frame_host_->GetProcess()->RemovePendingView();
@@ skipping 172 matching lines @@
   // We can't switch a RenderView between view source and non-view source mode
   // without screwing up the session history sometimes (when navigating between
   // "view-source:http://foo.com/" and "http://foo.com/", Blink doesn't treat
   // it as a new navigation). So require a BrowsingInstance switch.
   if (current_is_view_source_mode != new_is_view_source_mode)
     return true;
 
   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,
     SiteInstance* dest_instance,
     SiteInstance* candidate_instance,
     ui::PageTransition transition,
     bool dest_is_restore,
     bool dest_is_view_source_mode) {
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
 
@@ skipping 312 matching lines @@
   // 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
   // Init multiple times will be ignored, so this is safe.
   if (!new_instance->GetProcess()->Init())
     return;
 
   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(
     SiteInstance* old_instance,
     SiteInstance* new_instance) {
   // Only create opener proxies if they are in the same BrowsingInstance.
   if (new_instance->IsRelatedSiteInstance(old_instance)) {
     CreateOpenerProxies(new_instance, frame_tree_node_);
   } else if (SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
     // Ensure that the frame tree has RenderFrameProxyHosts for the
@@ skipping 75 matching lines @@
   }
 
   return RenderFrameHostFactory::Create(
       site_instance, render_view_host, render_frame_delegate_,
       render_widget_delegate_, frame_tree, frame_tree_node_, frame_routing_id,
       widget_routing_id, flags);
 }
 
 // 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
   // process or the existing process crashed) have been initialized. Calling
   // Init multiple times will be ignored, so this is safe.
   if (!new_instance->GetProcess()->Init())
     return false;
 
   CreateProxiesForNewRenderFrameHost(old_instance, new_instance);
 
   int create_render_frame_flags = 0;
   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);
   bool swapped_out_forbidden =
       SiteIsolationPolicy::IsSwappedOutStateForbidden();
 
   CHECK(instance);
   CHECK(!swapped_out_forbidden || !swapped_out);
   CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible() ||
         frame_tree_node_->IsMainFrame());
@@ skipping 97 matching lines @@
         success = InitRenderFrame(new_render_frame_host.get());
       }
     }
 
     if (success) {
       if (view_routing_id_ptr)
         *view_routing_id_ptr = render_view_host->GetRoutingID();
     }
   }
 
-  // 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);
     return new_render_frame_host.Pass();
   }
   return nullptr;
 }
 
 int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
   // A RenderFrameProxyHost should never be created in the same SiteInstance as
@@ skipping 98 matching lines @@
     RenderFrameProxyHost* proxy) {
   // Ensure the renderer process is initialized before creating the
   // RenderView.
   if (!render_view_host->GetProcess()->Init())
     return false;
 
   // We may have initialized this RenderViewHost for another RenderFrameHost.
   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());
 
   bool created = delegate_->CreateRenderViewForRenderManager(
       render_view_host, opener_frame_routing_id,
       proxy ? proxy->GetRoutingID() : MSG_ROUTING_NONE,
       frame_tree_node_->current_replication_state());
 
   if (created && proxy)
     proxy->set_render_frame_proxy_created(true);
@@ skipping 71 matching lines @@
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(site_instance);
   if (proxy)
     return proxy->GetRoutingID();
 
   return MSG_ROUTING_NONE;
 }
 
 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;
   }
 
   // Remember if the page was focused so we can focus the new renderer in
   // that case.
   bool focus_render_view = !will_focus_location_bar &&
                            render_frame_host_->GetView() &&
                            render_frame_host_->GetView()->HasFocus();
 
   bool is_main_frame = frame_tree_node_->IsMainFrame();
 
   // 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());
   } else {
     // PlzNavigate
     DCHECK(speculative_render_frame_host_);
     old_render_frame_host =
         SetRenderFrameHost(speculative_render_frame_host_.Pass());
   }
 
@@ skipping 149 matching lines @@
   // If we are currently navigating cross-process, we want to get back to normal
   // and then navigate as usual.
   if (pending_render_frame_host_)
     CancelPending();
 
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
       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) {
     TRACE_EVENT_INSTANT2(
         "navigation",
         "RenderFrameHostManager::UpdateStateForNavigate:New SiteInstance",
         TRACE_EVENT_SCOPE_THREAD,
         "current_instance id", current_instance->GetId(),
         "new_instance id", new_instance->GetId());
 
     // 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
       // sad tab or a newly created RFH while we wait for the pending RFH to
       // navigate.  Just switch to the pending RFH now and go back to normal.
       // (Note that we don't care about on{before}unload handlers if the current
       // RFH isn't live.)
       CommitPending();
       return render_frame_host_.get();
     }
     // 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) {
       // We don't need to stop the old renderer or run beforeunload/unload
       // handlers, because those have already been done.
       DCHECK(cross_site_transferring_request_->request_id() ==
              transferred_request_id);
     } else {
       // Also make sure the old render view stops, in case a load is in
       // progress.  (We don't want to do this for transfers, since it will
@@ skipping 14 matching lines @@
 
   // Otherwise the same SiteInstance can be used.  Navigate render_frame_host_.
 
   // It's possible to swap out the current RFH and then decide to navigate in it
   // anyway (e.g., a cross-process navigation that redirects back to the
   // original site).  In that case, we have a proxy for the current RFH but
   // haven't deleted it yet.  The new navigation will swap it back in, so we can
   // 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.
+  CHECK(!changed_web_ui ||
+        IsAcceptableWebUITransition(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());

[nasko, 2015/11/04 17:16:10]

All of the new code here is very similar to the code for PlzNavigate. I'm not
sure if it is worthwhile trying to consolidate it, but it will likely be useful
to put a comment that if anyone is changing this code, should also change the
PlzNavigate version of it.

[carlosk, 2015/11/04 21:43:59]

My first look at consolidating this made me choose not to. But on a 2nd look,
now that we have the single GetNavigatingWebUI, it looks much nicer! Also I
think a comment like that is easily overlooked.

So done!

[nasko, 2015/11/05 00:36:38]

Acknowledged.

     }
   }
 
-  if (pending_web_ui() && render_frame_host_->IsRenderFrameLive()) {
-    pending_web_ui()->RenderViewReused(render_frame_host_->render_view_host(),
-                                       frame_tree_node_->IsMainFrame());
-  }
-
   // The renderer can exit view source mode when any error or cancellation
   // happen. We must overwrite to recover the mode.
   if (dest_is_view_source_mode) {
     render_frame_host_->render_view_host()->Send(
         new ViewMsg_EnableViewSourceMode(
             render_frame_host_->render_view_host()->GetRoutingID()));
   }
 
   return render_frame_host_.get();
 }
 
 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());
 }
 
 scoped_ptr<RenderFrameHostImpl>
 RenderFrameHostManager::UnsetPendingRenderFrameHost() {
   scoped_ptr<RenderFrameHostImpl> pending_render_frame_host =
       pending_render_frame_host_.Pass();
 
   RenderFrameDevToolsAgentHost::OnCancelPendingNavigation(
       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();
 }
 
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::SetRenderFrameHost(
     scoped_ptr<RenderFrameHostImpl> render_frame_host) {
   // Swap the two.
   scoped_ptr<RenderFrameHostImpl> old_render_frame_host =
       render_frame_host_.Pass();
   render_frame_host_ = render_frame_host.Pass();
 
@@ skipping 154 matching lines @@
     if (rvh && !rvh->IsRenderViewLive()) {
       EnsureRenderViewInitialized(rvh, instance);
     } else {
       // Create a swapped out RenderView in the given SiteInstance if none
       // exists. Since an opener can point to a subframe, do this on the root
       // frame of the current opener's frame tree.
       if (SiteIsolationPolicy::IsSwappedOutStateForbidden()) {
         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);
       }
     }
   }
 }
 
 int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) {
   if (!frame_tree_node_->opener())
     return MSG_ROUTING_NONE;
 
   return frame_tree_node_->opener()
       ->render_manager()
       ->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.

[nasko, 2015/11/04 17:16:10]

It is acceptable, as long as we change RFHs, right?

[carlosk, 2015/11/04 21:43:59]

This method only cares about WebUI transitions on the current RFH (see comment
above and documentation in header file) so no, switching types is never
acceptable, hence the DCHECK below.

[nasko, 2015/11/05 00:36:38]

Let's clarify it in the comment then. Appending " for the same RenderFrameHost."

[carlosk, 2015/11/05 13:19:06]

Done.

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

[nasko, 2015/11/04 17:16:10]

Let's leave an empty line between the different cases, as it is easier to
visually distinguish them.

[carlosk, 2015/11/04 21:43:59]

Done.

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

[nasko, 2015/11/04 17:16:10]

Let's drop the "controlled" word and just have it as "special renderer URL". 

[carlosk, 2015/11/04 21:43:59]

Done.

+    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);
+    DCHECK(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 for:
+    // - Navigating to a special, renderer controlled URL.

[nasko, 2015/11/04 17:16:10]

Same comment as above.

[carlosk, 2015/11/04 21:43:59]

Done.

+    DCHECK(IsRendererDebugURL(
+        SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url)));
+  }
+  return true;

[nasko, 2015/11/04 17:16:10]

So this method always returns true? Why is it then that we do a CHECK on its
result? It will never hit. Don't we want to return false if none of the above is
the case?

[carlosk, 2015/11/04 21:43:59]

This made sense then these were called from inside DCHECKs:
a) Returning a bool allowed me to keep then in the DCHECK and so only be called
from debug builds
b) DCHECK-ing from inside them would make it easier to identify what case was
failing.

But now that it is in fact called from a CHECK those reasons are not valid
anymore. Furthermore having these DCHECKs inside are wrong.

I renamed the method and made it consistent again.

[nasko, 2015/11/05 00:36:38]

Cool!

+}
+
 }  // namespace content