Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.

content/browser/frame_host/render_frame_host_impl.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_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/lazy_instance.h"
@@ skipping 23 matching lines @@
 #include "content/browser/permissions/permission_service_impl.h"
 #include "content/browser/presentation/presentation_service_impl.h"
 #include "content/browser/renderer_host/input/input_router.h"
 #include "content/browser/renderer_host/input/timeout_monitor.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/renderer_host/render_view_host_delegate_view.h"
 #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"
 #include "content/common/inter_process_time_ticks_converter.h"
 #include "content/common/navigation_params.h"
 #include "content/common/render_frame_setup.mojom.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/swapped_out_messages.h"
 #include "content/public/browser/ax_event_notification_details.h"
 #include "content/public/browser/browser_accessibility_state.h"
@@ skipping 109 matching lines @@
 }
 
 RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
                                          RenderViewHostImpl* render_view_host,
                                          RenderFrameHostDelegate* delegate,
                                          RenderWidgetHostDelegate* rwh_delegate,
                                          FrameTree* frame_tree,
                                          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)),
       process_(site_instance->GetProcess()),
       cross_process_frame_connector_(NULL),
       render_frame_proxy_host_(NULL),
       frame_tree_(frame_tree),
       frame_tree_node_(frame_tree_node),
       render_widget_host_(nullptr),
       routing_id_(routing_id),
       render_frame_created_(false),
       navigations_suspended_(false),
       is_waiting_for_beforeunload_ack_(false),
       unload_ack_is_for_navigation_(false),
       is_loading_(false),
       pending_commit_(false),
       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);
   frame_tree_->AddRenderViewHostRef(render_view_host_);
   GetProcess()->AddRoute(routing_id_, this);
   g_routing_id_frame_map.Get().insert(std::make_pair(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_),
       this));
 
   if (is_swapped_out) {
     rfh_state_ = STATE_SWAPPED_OUT;
   } else {
     rfh_state_ = STATE_DEFAULT;
     GetSiteInstance()->increment_active_frame_count();
   }
 
   SetUpMojoIfNeeded();
   swapout_event_monitor_timeout_.reset(new TimeoutMonitor(base::Bind(
       &RenderFrameHostImpl::OnSwappedOut, weak_ptr_factory_.GetWeakPtr())));
 
   if (widget_routing_id != MSG_ROUTING_NONE) {
     render_widget_host_ = new RenderWidgetHostImpl(rwh_delegate, GetProcess(),
                                                    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_));
 
   if (delegate_ && render_frame_created_)
     delegate_->RenderFrameDeleted(this);
 
   // If this was swapped out, it already decremented the active frame count of
   // the SiteInstance it belongs to.
   if (IsRFHStateActive(rfh_state_))
@@ skipping 782 matching lines @@
   // gone through this, therefore just return.
   if (rfh_state_ != RenderFrameHostImpl::STATE_DEFAULT) {
     NOTREACHED() << "RFH should be in default state when calling SwapOut.";
     return;
   }
 
   SetState(RenderFrameHostImpl::STATE_PENDING_SWAP_OUT);
   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;
   if (proxy) {
     set_render_frame_proxy_host(proxy);
     proxy_routing_id = proxy->GetRoutingID();
     replication_state = proxy->frame_tree_node()->current_replication_state();
   }
 
   if (IsRenderFrameLive()) {
@@ skipping 917 matching lines @@
 bool RenderFrameHostImpl::IsFocused() {
   // TODO(mlamouri,kenrb): call GetRenderWidgetHost() directly when it stops
   // returning nullptr in some cases. See https://crbug.com/455245.
   return RenderWidgetHostImpl::From(
             GetView()->GetRenderWidgetHost())->is_focused() &&
          frame_tree_->GetFocusedFrame() &&
          (frame_tree_->GetFocusedFrame() == frame_tree_node() ||
           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()) {
     GetServiceRegistry()->ConnectToRemoteService(
         mojo::GetProxy(&mojo_image_downloader_));
   }
   return mojo_image_downloader_;
 }
 
 bool RenderFrameHostImpl::IsSameSiteInstance(
@@ skipping 275 matching lines @@
             ui::AX_ATTR_CHILD_TREE_ID,
             BrowserPluginInstanceIDToAXTreeID(value)));
         break;
       case AX_CONTENT_INT_ATTRIBUTE_LAST:
         NOTREACHED();
         break;
     }
   }
 }
 
+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