Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 43be1e9382c12832304e21b2ff4e4dd38b18cfb0..1357d95760f8ee653c927b0fce36448bf15c94fb 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -42,6 +42,7 @@
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
+#include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/accessibility_messages.h"
 #include "content/common/frame_messages.h"
 #include "content/common/input_messages.h"
@@ -193,6 +194,9 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
       accessibility_reset_token_(0),
       accessibility_reset_count_(0),
       no_create_browser_accessibility_manager_for_testing_(false),
+      web_ui_type_(WebUI::kNoWebUI),
+      pending_web_ui_type_(WebUI::kNoWebUI),
+      should_reuse_web_ui_(false),
       weak_ptr_factory_(this) {
   bool is_swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   bool hidden = !!(flags & CREATE_RF_HIDDEN);
@@ -221,6 +225,13 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
 }
 
 RenderFrameHostImpl::~RenderFrameHostImpl() {
+  // Release the WebUI before all else as the WebUI accesses the RenderFrameHost
+  // during cleanup.
+  web_ui_type_ = WebUI::kNoWebUI;
+  web_ui_.reset();
+  pending_web_ui_type_ = WebUI::kNoWebUI;
+  pending_web_ui_.reset();
+
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
@@ -1026,6 +1037,11 @@ void RenderFrameHostImpl::SwapOut(
   swapout_event_monitor_timeout_->Start(
       base::TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS));
 
+  web_ui_type_ = WebUI::kNoWebUI;

[Charlie Reis, 2015/10/23 06:39:25]

Sanity check: Why is this the right time to revoke WebUI?  The RenderFrameHost
will still be running its unload handler in the background.  Are there no cases
that it will try to use the WebUI object during unload?

[carlosk, 2015/10/27 14:35:44]

Comparing with the previous behavior, RFHI:SwapOut is called only by two methods
from RFHM (excluding tests) [1]:
- RFHM::SwapOutOldFrame: which itself is only called by RFHM::CommitPending for
the previously active RFH after the new one was put in place and any previous
WebUI replaced.
- RFHM::DiscardUnusedFrame which was always used to discard a speculative or
pending RFH so I don't think we need to care about the unload handler for them.

Given these two this location seemed good enough. WDYT?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

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

I'm not sure how that makes much of a difference. The unload handler can be
considered complete only at OnSwapOut. Until then, unload handler could be
sending IPCs to the browser process. We might not have any of those today, but
I'd rather make this code work properly than rely on no such WebUI existing.

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

OK, I moved the destruction of the WebUI to when the swap-out ack is received.

To add to the work on making sure this makes a difference: *if* received IPCs
come through WebContentsImpl::OnMessageReceived then they will not reach a
non-active or non-pending RFH's WebUI given its implementation uses
WebContentsImpl::GetWebUI().

See
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

+  web_ui_.reset();
+  pending_web_ui_type_ = WebUI::kNoWebUI;
+  pending_web_ui_.reset();

[Charlie Reis, 2015/10/23 06:39:25]

Can we make these 4 lines into a private helper (ResetWebUI), also used in
~RenderFrameHostImpl?

[carlosk, 2015/10/27 14:35:44]

Done.

+
   // There may be no proxy if there are no active views in the process.
   int proxy_routing_id = MSG_ROUTING_NONE;
   FrameReplicationState replication_state;
@@ -1972,6 +1988,49 @@ bool RenderFrameHostImpl::IsFocused() {
           frame_tree_->GetFocusedFrame()->IsDescendantOf(frame_tree_node()));
 }
 
+void RenderFrameHostImpl::UpdatePendingWebUI(const GURL& dest_url,
+                                             int entry_bindings) {
+  WebUI::TypeID new_web_ui_type =
+      WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
+          GetSiteInstance()->GetBrowserContext(), dest_url);
+
+  // The current WebUI should be reused when dest_url requires a WebUI and its
+  // type matches the current.
+  should_reuse_web_ui_ = false;
+  if (new_web_ui_type != WebUI::kNoWebUI && web_ui_type_ == new_web_ui_type) {
+    DCHECK(web_ui_);
+    should_reuse_web_ui_ = true;
+
+    // Reset the pending WebUI as the current one will be reused.
+    pending_web_ui_.reset();
+    pending_web_ui_type_ = WebUI::kNoWebUI;
+  } else if (pending_web_ui_type_ != new_web_ui_type) {
+    // Otherwise create, update or reset the pending one if its type doesn't
+    // match the new type (if new type is kNoWebUI then it will be reset).
+    pending_web_ui_ =
+        CreateWebUI(dest_url, entry_bindings, &pending_web_ui_type_);
+  }
+
+  DCHECK_EQ(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI);
+}
+
+void RenderFrameHostImpl::CommitPendingWebUI() {
+  if (should_reuse_web_ui_) {
+    DCHECK(!pending_web_ui_ && pending_web_ui_type_ == WebUI::kNoWebUI);
+    should_reuse_web_ui_ = false;
+  } else {
+    web_ui_ = pending_web_ui_.Pass();
+    web_ui_type_ = pending_web_ui_type_;
+    pending_web_ui_type_ = WebUI::kNoWebUI;
+  }
+}
+
+void RenderFrameHostImpl::DiscardPendingWebUI() {
+  pending_web_ui_.reset();
+  pending_web_ui_type_ = WebUI::kNoWebUI;
+  should_reuse_web_ui_ = false;
+}
+
 const image_downloader::ImageDownloaderPtr&
 RenderFrameHostImpl::GetMojoImageDownloader() {
   if (!mojo_image_downloader_.get() && GetServiceRegistry()) {
@@ -2276,4 +2335,47 @@ void RenderFrameHostImpl::AXContentNodeDataToAXNodeData(
   }
 }
 
+scoped_ptr<WebUIImpl> RenderFrameHostImpl::CreateWebUI(
+    const GURL& dest_url,
+    int entry_bindings,
+    WebUI::TypeID* web_ui_type) {
+  if (!dest_url.is_valid()) {
+    *web_ui_type = WebUI::kNoWebUI;
+    return nullptr;
+  }
+
+  scoped_ptr<WebUIImpl> web_ui;
+  *web_ui_type = WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
+      GetSiteInstance()->GetBrowserContext(), dest_url);
+  if (*web_ui_type != WebUI::kNoWebUI) {
+    web_ui = delegate_->CreateWebUIForRenderFrameHost(dest_url);
+    DCHECK(web_ui);
+
+    // If we have assigned (zero or more) bindings to the 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 (entry_bindings != NavigationEntryImpl::kInvalidBindings &&
+        web_ui->GetBindings() != entry_bindings) {
+      RecordAction(base::UserMetricsAction("ProcessSwapBindingsMismatch_RVHM"));
+      *web_ui_type = WebUI::kNoWebUI;
+      return nullptr;
+    }
+  }
+
+  // Check RenderViewHost for proper bindings.
+  if (web_ui && !render_view_host_->GetProcess()->IsForGuestsOnly()) {
+    // If a WebUI was created for the URL and the RenderView is not in a guest
+    // process, then enable missing bindings with the RenderViewHost.
+    int new_bindings = web_ui->GetBindings();
+    if ((render_view_host_->GetEnabledBindings() & new_bindings) !=
+        new_bindings) {
+      render_view_host_->AllowBindings(new_bindings);
+    }
+  }
+
+  CHECK_EQ(!web_ui, *web_ui_type == WebUI::kNoWebUI);
+  return web_ui.Pass();
+}
+
 }  // namespace content