Skip to content

Commit

Permalink
content/browser: Reuse compositor for subframe navs with RenderDocument.
Browse files Browse the repository at this point in the history
Reuse LayerTreeView instances in the renderer for subframe navigations
when RenderDocument is enabled. This change adds the browser side code
required for enabling this. The high level changes are as follows:

- Compositing reuse is only done for same-site same-process
navigations.

- An optional FrameSinkId is plumbed through RenderWidgetHost, to
avoid generating a new ID when we're reusing the compositor. The
FrameSinkId is tied to the mojo connections + resources we want to
reuse.

- By default, the renderer side WebFrameWidget is initiated when the
RenderFrameImpl is created. The WebFrameWidget requires initializing
the compositor when its created.
So when reusing the compositor, we defer creating the WebFrameWidget
until commit. This state is now tracked on RenderFrameHostImpl to
defer work which must happen after the renderer side WebFrameWidget
has been created.

- By default, RenderWidgetHostView assumes that its FrameSinkId is
bound to it throughout its lifetime. However when reusing the
compositor, we transfer the FrameSinkId from the old RWHV to the new
RWHV.
RWHVBase now tracks whether its FrameSinkId is bound to it. This is
used to manage ownership of FrameSinkId with both Viz and
RenderWidgetHostInputEventRouter.

Since this is a perf optimization, which
is likely to have issues, this is flag guarded with a default on
switch to ease debugging.

R=rakina@chromium.org

Bug: 1464791
Change-Id: I7c5a92b0fc961aa192e7ee57a28171a822e3c0ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912959
Auto-Submit: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: Khushal Sagar <khushalsagar@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1214375}
  • Loading branch information
khushalsagar authored and Chromium LUCI CQ committed Oct 24, 2023
1 parent d574a59 commit d7bdc38
Show file tree
Hide file tree
Showing 33 changed files with 334 additions and 117 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ class FlingSchedulerTest : public testing::Test,
delegate_ = std::make_unique<MockRenderWidgetHostDelegate>();
widget_host_ = TestRenderWidgetHost::Create(
/* frame_tree= */ nullptr, delegate_.get(),
RenderWidgetHostImpl::DefaultFrameSinkId(*site_instance_group_,
routing_id),
site_instance_group_->GetSafeRef(), routing_id, false);
delegate_->set_widget_host(widget_host_.get());
return std::make_unique<TestRenderWidgetHostView>(widget_host_.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,13 @@ class TracingRenderWidgetHost : public RenderWidgetHostImpl {
public:
TracingRenderWidgetHost(FrameTree* frame_tree,
RenderWidgetHostDelegate* delegate,
viz::FrameSinkId frame_sink_id,
base::SafeRef<SiteInstanceGroup> site_instance_group,
int32_t routing_id,
bool hidden)
: RenderWidgetHostImpl(frame_tree,
/*self_owned=*/false,
frame_sink_id,
delegate,
std::move(site_instance_group),
routing_id,
Expand Down Expand Up @@ -118,12 +120,13 @@ class TracingRenderWidgetHostFactory : public RenderWidgetHostFactory {
std::unique_ptr<RenderWidgetHostImpl> CreateRenderWidgetHost(
FrameTree* frame_tree,
RenderWidgetHostDelegate* delegate,
viz::FrameSinkId frame_sink_id,
base::SafeRef<SiteInstanceGroup> site_instance_group,
int32_t routing_id,
bool hidden) override {
return std::make_unique<TracingRenderWidgetHost>(
frame_tree, delegate, std::move(site_instance_group), routing_id,
hidden);
frame_tree, delegate, frame_sink_id, std::move(site_instance_group),
routing_id, hidden);
}
};

Expand Down
3 changes: 2 additions & 1 deletion content/browser/renderer_host/mock_render_widget_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ MockRenderWidgetHost::MockRenderWidgetHost(
mojo::PendingAssociatedRemote<blink::mojom::Widget> pending_blink_widget)
: RenderWidgetHostImpl(frame_tree,
/*self_owned=*/false,
DefaultFrameSinkId(*site_instance_group, routing_id),
delegate,
std::move(site_instance_group),
site_instance_group,
routing_id,
/*hidden=*/false,
/*renderer_initiated_creation=*/false,
Expand Down
98 changes: 90 additions & 8 deletions content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1638,11 +1638,24 @@ RenderFrameHostImpl::RenderFrameHostImpl(
int32_t widget_routing_id =
site_instance->GetProcess()->GetNextRoutingID();
DCHECK_EQ(nullptr, GetLocalRenderWidgetHost());

auto* previous_rfh =
lifecycle_state_ == LifecycleStateImpl::kSpeculative &&
frame_tree_node_->current_frame_host()
->ShouldReuseCompositing(*GetSiteInstance())
? frame_tree_node_->current_frame_host()
: nullptr;
auto frame_sink_id =
previous_rfh
? previous_rfh->GetLocalRenderWidgetHost()->GetFrameSinkId()
: RenderWidgetHostImpl::DefaultFrameSinkId(
*site_instance_->group(), widget_routing_id);
owned_render_widget_host_ = RenderWidgetHostFactory::Create(
frame_tree_, frame_tree_->render_widget_delegate(),
frame_tree_, frame_tree_->render_widget_delegate(), frame_sink_id,
site_instance_->group()->GetSafeRef(), widget_routing_id,
/*hidden=*/true,
/*renderer_initiated_creation=*/false);
owned_render_widget_host_->SetViewIsFrameSinkIdOwner(!previous_rfh);
}

if (is_main_frame())
Expand Down Expand Up @@ -3404,6 +3417,18 @@ bool RenderFrameHostImpl::CreateRenderFrame(

if (auto* rwh = GetLocalRenderWidgetHost()) {
params->widget_params = rwh->BindAndGenerateCreateFrameWidgetParams();

auto* previous_rfh =
lifecycle_state_ == LifecycleStateImpl::kSpeculative &&
frame_tree_node_->current_frame_host()->ShouldReuseCompositing(
*GetSiteInstance())
? frame_tree_node_->current_frame_host()
: nullptr;
if (previous_rfh) {
waiting_for_renderer_widget_creation_after_commit_ = true;
params->widget_params->previous_frame_token_for_compositor_reuse =
previous_rfh->GetFrameToken();
}
}
mojo::PendingAssociatedRemote<mojom::Frame> pending_frame_remote;
params->frame = pending_frame_remote.InitWithNewEndpointAndPassReceiver();
Expand Down Expand Up @@ -3536,13 +3561,8 @@ void RenderFrameHostImpl::RenderFrameCreated() {

// Initialize the RenderWidgetHost which marks it and the RenderViewHost as
// live before calling to the `delegate_`.
if (GetLocalRenderWidgetHost()) {
#if BUILDFLAG(IS_ANDROID)
GetLocalRenderWidgetHost()->SetForceEnableZoom(
delegate_->GetOrCreateWebPreferences().force_enable_zoom);
#endif // BUILDFLAG(IS_ANDROID)
GetLocalRenderWidgetHost()->RendererWidgetCreated(
/*for_frame_widget=*/true);
if (!waiting_for_renderer_widget_creation_after_commit_) {
RendererWidgetCreated();
}

// Set up mojo connections to the renderer from the `frame_` connection before
Expand All @@ -3558,6 +3578,19 @@ void RenderFrameHostImpl::RenderFrameCreated() {
web_ui_->SetUpMojoConnection();
}

void RenderFrameHostImpl::RendererWidgetCreated() {
if (GetLocalRenderWidgetHost()) {
#if BUILDFLAG(IS_ANDROID)
GetLocalRenderWidgetHost()->SetForceEnableZoom(
delegate_->GetOrCreateWebPreferences().force_enable_zoom);
#endif // BUILDFLAG(IS_ANDROID)
GetLocalRenderWidgetHost()->RendererWidgetCreated(
/*for_frame_widget=*/true);
}

waiting_for_renderer_widget_creation_after_commit_ = false;
}

void RenderFrameHostImpl::RenderFrameDeleted() {
// In https://crbug.com/1146573 a WebContentsObserver was causing the frame to
// be reinitialized during deletion. It is not valid to re-enter navigation
Expand Down Expand Up @@ -12962,6 +12995,23 @@ bool RenderFrameHostImpl::DidCommitNavigationInternal(
return false;
}

// NOTE: The navigation has committed in the renderer so it is safe
// to pass ownership of the FrameSinkId to the new RFH.
if (waiting_for_renderer_widget_creation_after_commit_) {
RendererWidgetCreated();

CHECK_NE(frame_tree_node_->current_frame_host(), this);
auto* previous_rfh =
frame_tree_node_->current_frame_host()->ShouldReuseCompositing(
*GetSiteInstance())
? frame_tree_node_->current_frame_host()
: nullptr;
if (owned_render_widget_host_ && previous_rfh) {
previous_rfh->GetRenderWidgetHost()->SetViewIsFrameSinkIdOwner(false);
owned_render_widget_host_->SetViewIsFrameSinkIdOwner(true);
}
}

// TODO(clamy): We should stop having a special case for same-document
// navigation and just put them in the general map of NavigationRequests.
if (navigation_request &&
Expand Down Expand Up @@ -15954,4 +16004,36 @@ bool RenderFrameHostImpl::ShouldChangeRenderFrameHostOnSameSiteNavigation()
must_be_replaced());
}

bool RenderFrameHostImpl::ShouldReuseCompositing(
SiteInstanceImpl& speculative_site_instance) const {
if (!ShouldChangeRenderFrameHostOnSameSiteNavigation()) {
return false;
}

// This indicates that the renderer process corresponding to this frame has
// crashed and there is no compositor to reuse.
if (must_be_replaced_) {
return false;
}

if (!base::FeatureList::IsEnabled(features::kRenderDocumentCompositorReuse)) {
return false;
}

if (GetSiteInstance()->group() != speculative_site_instance.group()) {
return false;
}

CHECK_EQ(GetProcess(), speculative_site_instance.GetProcess());

// TODO(khushalsagar): We'll need this for main frames as well but the
// code for RFH creation/init limits it to subframes for now.
bool is_main_frame = !GetParent();
if (is_main_frame) {
return false;
}

return true;
}

} // namespace content
14 changes: 14 additions & 0 deletions content/browser/renderer_host/render_frame_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -3390,6 +3390,12 @@ class CONTENT_EXPORT RenderFrameHostImpl
mojo::PendingReceiver<network::mojom::SharedDictionaryAccessObserver>
observer) override;

// Returns true if this RFH's compositor should be reused by a speculattive
// RFH with the `speculative_site_instance`.
// Returns false if the speculative RFH should initialize a new compositor.
bool ShouldReuseCompositing(
SiteInstanceImpl& speculative_site_instance) const;

// Resets any waiting state of this RenderFrameHost that is no longer
// relevant.
void ResetWaitingState();
Expand Down Expand Up @@ -4101,6 +4107,10 @@ class CONTENT_EXPORT RenderFrameHostImpl
// https://github.com/ivansandrk/additional-windowing-controls/blob/main/awc-explainer.md
bool CanUseWindowingControls(base::StringPiece js_api_name);

// Notifies when the renderer side Widget instance has been created and mojo
// interfaces to it can be bound.
void RendererWidgetCreated();

// The RenderViewHost that this RenderFrameHost is associated with.
//
// It is kept alive as long as any RenderFrameHosts or RenderFrameProxyHosts
Expand Down Expand Up @@ -5238,6 +5248,10 @@ class CONTENT_EXPORT RenderFrameHostImpl
absl::optional<blink::DocumentToken>
fullscreen_document_on_document_element_ready_ = absl::nullopt;

// If true, the renderer side widget is created after the navigation is
// committed.
bool waiting_for_renderer_widget_creation_after_commit_ = false;

// WeakPtrFactories are the last members, to ensure they are destroyed before
// all other fields of `this`.
base::WeakPtrFactory<RenderFrameHostImpl> weak_ptr_factory_{this};
Expand Down
6 changes: 5 additions & 1 deletion content/browser/renderer_host/render_view_host_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,18 @@ RenderViewHost* RenderViewHostFactory::Create(
std::move(main_browsing_context_state), create_case);
}

const auto frame_sink_id =
RenderWidgetHostImpl::DefaultFrameSinkId(*group, widget_routing_id);
RenderViewHostImpl* view_host = new RenderViewHostImpl(
frame_tree, group, storage_partition_config,
RenderWidgetHostFactory::Create(
frame_tree, widget_delegate, group->GetSafeRef(), widget_routing_id,
frame_tree, widget_delegate, frame_sink_id, group->GetSafeRef(),
widget_routing_id,
/*hidden=*/true, renderer_initiated_creation),
delegate, routing_id, main_frame_routing_id,
true /* has_initialized_audio_host */,
std::move(main_browsing_context_state), create_case);
view_host->GetWidget()->SetViewIsFrameSinkIdOwner(true);
return view_host;
}

Expand Down
8 changes: 5 additions & 3 deletions content/browser/renderer_host/render_widget_host_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,20 @@ RenderWidgetHostFactory* RenderWidgetHostFactory::factory_ = nullptr;
std::unique_ptr<RenderWidgetHostImpl> RenderWidgetHostFactory::Create(
FrameTree* frame_tree,
RenderWidgetHostDelegate* delegate,
viz::FrameSinkId frame_sink_id,
base::SafeRef<SiteInstanceGroup> site_instance_group,
int32_t routing_id,
bool hidden,
bool renderer_initiated_creation) {
if (factory_) {
return factory_->CreateRenderWidgetHost(frame_tree, delegate,
return factory_->CreateRenderWidgetHost(frame_tree, delegate, frame_sink_id,
std::move(site_instance_group),
routing_id, hidden);
}
return RenderWidgetHostImpl::Create(
frame_tree, delegate, std::move(site_instance_group), routing_id, hidden,
renderer_initiated_creation, std::make_unique<FrameTokenMessageQueue>());
frame_tree, delegate, frame_sink_id, std::move(site_instance_group),
routing_id, hidden, renderer_initiated_creation,
std::make_unique<FrameTokenMessageQueue>());
}

// static
Expand Down
3 changes: 3 additions & 0 deletions content/browser/renderer_host/render_widget_host_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <memory>

#include "base/memory/safe_ref.h"
#include "components/viz/common/surfaces/frame_sink_id.h"
#include "content/common/content_export.h"
#include "mojo/public/cpp/bindings/pending_remote.h"

Expand All @@ -32,6 +33,7 @@ class RenderWidgetHostFactory {
static std::unique_ptr<RenderWidgetHostImpl> Create(
FrameTree* frame_tree,
RenderWidgetHostDelegate* delegate,
viz::FrameSinkId frame_sink_id,
base::SafeRef<SiteInstanceGroup> site_instance_group,
int32_t routing_id,
bool hidden,
Expand All @@ -49,6 +51,7 @@ class RenderWidgetHostFactory {
virtual std::unique_ptr<RenderWidgetHostImpl> CreateRenderWidgetHost(
FrameTree* frame_tree,
RenderWidgetHostDelegate* delegate,
viz::FrameSinkId frame_sink_id,
base::SafeRef<SiteInstanceGroup> site_instance_group,
int32_t routing_id,
bool hidden) = 0;
Expand Down
Loading

0 comments on commit d7bdc38

Please sign in to comment.