Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1234)

Unified Diff: content/browser/frame_host/render_frame_host_manager.cc

Issue 1352813006: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: More test fixes and improvements but the code is stil a WIP! Created 5 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 376a185061a21a8264d233b1512345ba407dd3ec..fc43128bdb31c12bbf0a04332eb966a966f9d76f 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -31,7 +31,7 @@
#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/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"
@@ -221,7 +221,6 @@ RenderFrameHostManager::RenderFrameHostManager(
render_widget_delegate_(render_widget_delegate),
proxy_hosts_(new RenderFrameProxyHostMap(this)),
interstitial_page_(nullptr),
- should_reuse_web_ui_(false),
weak_factory_(this) {
DCHECK(frame_tree_node_);
}
@@ -246,10 +245,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>());
}
@@ -353,27 +348,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,
@@ -672,13 +646,12 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
RenderFrameHostImpl* render_frame_host,
bool was_caused_by_user_gesture) {
if (!pending_render_frame_host_ && !speculative_render_frame_host_) {
- DCHECK_IMPLIES(should_reuse_web_ui_, 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_)
+ // If current RenderFrameHost has a pending Web UI, commit it.
+ if (render_frame_host_->pending_web_ui())
CommitPending();
return;
}
@@ -1006,21 +979,23 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
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());
- }
+ // As SiteInstances are the same, make the RFH update its possible pending
+ // WebUI.
+ render_frame_host_->UpdatePendingWebUI(request.common_params().url,
+ request.bindings());
+ DCHECK(speculative_web_ui() == render_frame_host_->pending_web_ui());
+ if (speculative_web_ui() && render_frame_host_->IsRenderFrameLive()) {
+ speculative_web_ui()->GetController()->RenderViewReused(
+ render_frame_host_->render_view_host());
}
+
+ // // If a speculative WebUI was set, 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());
+ // }
} else {
// If the SiteInstance for the final URL doesn't match the one from the
// speculatively created RenderFrameHost, create a new RenderFrameHost using
@@ -1079,8 +1054,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
void RenderFrameHostManager::CleanUpNavigation() {
CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableBrowserSideNavigation));
- speculative_web_ui_.reset();
- should_reuse_web_ui_ = false;
+ render_frame_host_->DiscardPendingWebUI();
if (speculative_render_frame_host_)
DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
}
@@ -1264,18 +1238,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,
@@ -1559,7 +1521,9 @@ const GURL& RenderFrameHostManager::GetCurrentURLForSiteInstance(
void RenderFrameHostManager::CreatePendingRenderFrameHost(
SiteInstance* old_instance,
- SiteInstance* new_instance) {
+ SiteInstance* new_instance,
+ const GURL& url,
+ int bindings) {
int create_render_frame_flags = 0;
if (delegate_->IsHidden())
create_render_frame_flags |= CREATE_RF_HIDDEN;
@@ -1577,8 +1541,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_ = CreateRenderFrameInternal(
+ new_instance, url, bindings, create_render_frame_flags, nullptr);
}
void RenderFrameHostManager::CreateProxiesForNewRenderFrameHost(
@@ -1667,12 +1631,6 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
int bindings) {
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);
// 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
@@ -1686,22 +1644,27 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
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);
+ speculative_render_frame_host_ = CreateRenderFrameInternal(
+ new_instance, url, bindings, create_render_frame_flags, nullptr);
- if (!speculative_render_frame_host_) {
- speculative_web_ui_.reset();
- return false;
- }
return true;
}
scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
SiteInstance* instance,
- WebUIImpl* web_ui,
int flags,
int* view_routing_id_ptr) {
+ return CreateRenderFrameInternal(instance, GURL(),
+ NavigationEntryImpl::kInvalidBindings, flags,
+ view_routing_id_ptr);
+}
+
+scoped_ptr<RenderFrameHostImpl>
+RenderFrameHostManager::CreateRenderFrameInternal(SiteInstance* instance,
+ const GURL& url,
+ int past_bindings,
+ int flags,
+ int* view_routing_id_ptr) {
bool swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
bool swapped_out_forbidden =
SiteIsolationPolicy::IsSwappedOutStateForbidden();
@@ -1736,6 +1699,7 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
// Prevent the process from exiting while we're trying to use it.
if (!swapped_out) {
new_render_frame_host = proxy->PassFrameHostOwnership();
+ new_render_frame_host->InitializeWebUI(url, past_bindings);
new_render_frame_host->GetProcess()->AddPendingView();
proxy_hosts_->Remove(instance->GetId());
@@ -1764,6 +1728,7 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
new_render_frame_host =
CreateRenderFrameHost(instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE,
widget_routing_id, surface_id, flags);
+ new_render_frame_host->InitializeWebUI(url, past_bindings);
RenderViewHostImpl* render_view_host =
new_render_frame_host->render_view_host();
int proxy_routing_id = MSG_ROUTING_NONE;
@@ -1821,20 +1786,20 @@ 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);
- }
- }
+ // // 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 (new_render_frame_host && new_render_frame_host->web_ui() &&
+ // !new_render_frame_host->GetProcess()->IsForGuestsOnly()) {
+ // RenderViewHost* render_view_host =
+ // new_render_frame_host->render_view_host();
+ // int bindings = new_render_frame_host->web_ui()->GetBindings();
+ // if ((render_view_host->GetEnabledBindings() & bindings) != bindings) {
+ // render_view_host->AllowBindings(bindings);
+ // }
+ // }
// Returns the new RFH if it isn't swapped out.
if (success && !swapped_out) {
@@ -1956,27 +1921,26 @@ bool RenderFrameHostManager::InitRenderView(
if (render_view_host->IsRenderViewLive())
return true;
- // If the ongoing navigation is to a WebUI and the RenderView is not in a
- // guest process, tell the RenderViewHost about any bindings it will need
- // enabled.
- WebUIImpl* dest_web_ui = nullptr;
- 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()));
- }
- }
+ // // If the ongoing navigation is to a WebUI and the RenderView is not in a
+ // // guest process, tell the RenderViewHost about any bindings it will need
+ // // enabled.
+ // WebUIImpl* dest_web_ui = nullptr;
+ // if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+ // switches::kEnableBrowserSideNavigation)) {
+ // dest_web_ui = speculative_web_ui();
+ // } 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());
@@ -2063,42 +2027,16 @@ 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_) {
+ // If the current RenderFrameHost has a pending Web UI then commit it.
+ if (render_frame_host_->pending_web_ui()) {
+ DCHECK(!pending_render_frame_host_ && !speculative_render_frame_host_);
+ render_frame_host_->CommitPendingWebUI();
if (will_focus_location_bar)
delegate_->SetFocusToLocationBar(false);
return;
@@ -2115,7 +2053,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());
@@ -2277,9 +2216,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) {
@@ -2292,14 +2228,8 @@ 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);
- CreatePendingRenderFrameHost(current_instance, new_instance.get());
+ CreatePendingRenderFrameHost(current_instance, new_instance.get(), dest_url,
+ bindings);
if (!pending_render_frame_host_)
return nullptr;
@@ -2359,21 +2289,20 @@ 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());
- }
- }
+ render_frame_host_->UpdatePendingWebUI(dest_url, bindings);
+
+ // TODO(carlosk): try moving this into RenderFrameHostImpl::CreateWebUI
+ // // Make sure the new RenderViewHost has the right bindings.
+ // if (render_frame_host_->pending_web_ui() &&
+ // !render_frame_host_->GetProcess()->IsForGuestsOnly()) {
+ // render_frame_host_->render_view_host()->AllowBindings(
+ // render_frame_host_->pending_web_ui()->GetBindings());
+ // }
- if (pending_web_ui() && render_frame_host_->IsRenderFrameLive()) {
- pending_web_ui()->GetController()->RenderViewReused(
+ // TODO(carlosk): try moving this into RenderFrameHostImpl::UpdatePendingWebUI
+ if (render_frame_host_->pending_web_ui() &&
+ render_frame_host_->IsRenderFrameLive()) {
+ render_frame_host_->pending_web_ui()->GetController()->RenderViewReused(
render_frame_host_->render_view_host());
}
@@ -2392,6 +2321,8 @@ void RenderFrameHostManager::CancelPending() {
TRACE_EVENT1("navigation", "RenderFrameHostManager::CancelPending",
"FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
DiscardUnusedFrame(UnsetPendingRenderFrameHost());
+ // TODO(carlosk): not sure yet this is the right place for this call...
+ render_frame_host_->DiscardPendingWebUI();
}
scoped_ptr<RenderFrameHostImpl>
@@ -2406,9 +2337,6 @@ RenderFrameHostManager::UnsetPendingRenderFrameHost() {
// 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();
}
@@ -2583,8 +2511,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);
}
}
}

Powered by Google App Engine
This is Rietveld 408576698