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 16e89ef367050c8467f2e46a5e5606bd5fa29da3..b4cdd674e3bcd521398d0bd6ace306285711e785 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -41,6 +41,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"
@@ -170,7 +171,9 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
                                          FrameTreeNode* frame_tree_node,
                                          int32 routing_id,
                                          int32 widget_routing_id,
-                                         int flags)
+                                         int flags,
+                                         const GURL& dest_url,
+                                         int past_bindings)
     : render_view_host_(render_view_host),
       delegate_(delegate),
       site_instance_(static_cast<SiteInstanceImpl*>(site_instance)),
@@ -190,6 +193,8 @@ 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),
+      should_reuse_web_ui_(false),
       weak_ptr_factory_(this) {
   bool is_swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   bool hidden = !!(flags & CREATE_RF_HIDDEN);
@@ -215,9 +220,18 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
                                                    widget_routing_id, hidden);
     render_widget_host_->set_owned_by_render_frame_host(true);
   }
+
+  web_ui_ = CreateWebUI(dest_url, past_bindings, &web_ui_type_);
+  if (web_ui_ && frame_tree_node_->IsMainFrame())
+    web_ui_->RenderViewCreated(render_view_host_);

[nasko, 2015/10/01 20:05:16]

Urgh! This is ugly and we need to figure out why it is needed and make it
better. Not in this CL, just general comment.

[carlosk, 2015/10/05 16:59:57]

Acknowledged.

[Dan Beam, 2015/10/06 17:50:33]

this CL may make it irrelevant

[nasko, 2015/10/07 23:49:32]

Care to add some context why it will be irrelevant? I'm all for making it so,
but want to understand what is its purpose and why this CL will help.

[Dan Beam, 2015/10/07 23:55:11]

I originally thought this was solely to support subframe webUIs but no, this'll
continue to be necessary as handlers are created before renderframes (sorry!)

[Dan Beam, 2015/10/08 00:01:35]

if handlers are created before renderframes**

[carlosk, 2015/10/09 17:29:10]

OK, so I'm keeping them. But it's still not clear to me when exactly to call
RenderViewCreated versus RenderViewReused. Are my if-expressions here and in
UpdatePendingWebUI enough? 

For instance, @nasko mentioned an existing RenderView could be reused for a new
RFHI being created for the main frame. With the this implementation that would
cause RenderViewCreated to be invoked. Would that be a problem?

@dbeam, can you shed some light on this?

[Dan Beam, 2015/10/13 16:24:21]

RenderViewCreated() is useful as the WebUI exists before the RenderView.  when
the RenderView comes up it may require some initial state to be sent to it (i.e.
here's your bookmarks!).

on refresh (or same-origin navigation?), the same initial state may be required
to be sent even though the C++ browser-side handler was never torn
down/recreated.

/cc estade@ as he added this performance optimization:
http://codereview.chromium.org/10154004

As long as the C++ handlers don't get torn down and rebuilt too often, it
doesn't matter to me how we accomplish this behavior (i.e. if we just call
RenderFrameCreated each time rather than RenderFrameReused).

 }
 
 RenderFrameHostImpl::~RenderFrameHostImpl() {
+  // Release the WebUI before all else as the WebUI accesses the RenderFrameHost
+  // during cleanup.
+  web_ui_.reset();
+  pending_web_ui_.reset();
+
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
@@ -1020,6 +1034,9 @@ void RenderFrameHostImpl::SwapOut(
   swapout_event_monitor_timeout_->Start(
       base::TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS));
 
+  web_ui_.reset();
+  web_ui_type_ = WebUI::kNoWebUI;
+
   // There may be no proxy if there are no active views in the process.
   int proxy_routing_id = MSG_ROUTING_NONE;
   FrameReplicationState replication_state;
@@ -1957,6 +1974,53 @@ bool RenderFrameHostImpl::IsFocused() {
           frame_tree_->GetFocusedFrame()->IsDescendantOf(frame_tree_node()));
 }
 
+void RenderFrameHostImpl::UpdatePendingWebUI(const GURL& dest_url,
+                                             int past_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_ =
+      new_web_ui_type != WebUI::kNoWebUI && web_ui_type_ == new_web_ui_type;

[nasko, 2015/10/01 20:05:16]

This will be more readable if the boolean is initialized to false and an if
statement with the checks turns it to true.

[carlosk, 2015/10/05 16:59:57]

Done.

+
+  if (should_reuse_web_ui_) {
+    DCHECK(web_ui_);
+    // Reset the pending WebUI if 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, past_bindings, &pending_web_ui_type_);
+
+    if (pending_web_ui_ && IsRenderFrameLive()) {
+      pending_web_ui_->RenderViewReused(render_view_host_,
+                                        frame_tree_node_->IsMainFrame());
+    }
+  }
+  CHECK_IMPLIES(pending_web_ui_, pending_web_ui_type_ != WebUI::kNoWebUI);
+  CHECK_IMPLIES(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI);
+}
+
+void RenderFrameHostImpl::CommitPendingWebUI() {
+  if (should_reuse_web_ui_) {
+    CHECK(!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()) {
@@ -2252,4 +2316,42 @@ void RenderFrameHostImpl::AXContentNodeDataToAXNodeData(
   }
 }
 
+scoped_ptr<WebUIImpl> RenderFrameHostImpl::CreateWebUI(
+    const GURL& dest_url,
+    int past_bindings,
+    WebUI::TypeID* web_ui_type) {
+  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);

[nasko, 2015/10/01 20:05:16]

Why do we need the delegate to create it. Can't we just create it here?

[carlosk, 2015/10/05 16:59:57]

There is some code sharing for the creation logic in WebContentsImpl with
another WebUI creation method (CreateSubframeWebUI) invoked from the UberUI
WebUI. The latter doesn't seem to hold a reference to the RFH what would
complicate moving all logic here.

The WebUI constructor also needs a pointer to the WebContents (WebContentsImpl
passes |this|) so the RFH might not be considered the right place
architecturally to do that. But I'm not sure of this...

So I'm thinking it should stay as is. WDYT?

WebContentsImpl::CreateWebUI and its callers: https://goo.gl/86By1k

[nasko, 2015/10/07 23:49:32]

Ah, I missed that. Yeah, passing WebContents as a parameter to WebUI creation
means that it stays in WebContents.
 

+    CHECK(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 (web_ui && past_bindings != NavigationEntryImpl::kInvalidBindings &&
+        web_ui->GetBindings() != past_bindings) {
+      RecordAction(base::UserMetricsAction("ProcessSwapBindingsMismatch_RVHM"));
+      web_ui.reset();
+      *web_ui_type = WebUI::kNoWebUI;
+    }
+  }
+
+  // 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_IMPLIES(web_ui, *web_ui_type != WebUI::kNoWebUI);
+  CHECK_IMPLIES(!web_ui, *web_ui_type == WebUI::kNoWebUI);
+  return web_ui.Pass();
+}
+
 }  // namespace content