Skip to content

Commit

Permalink
[BlinkGenPropertyTrees] Fix crash when handling fullscreen pepper layer
Browse files Browse the repository at this point in the history
RenderWidgetFullscreenPepper manages a cc layer without letting blink
know it, so the layer's paint property states are not set, causing
crashes.

This CL is temporary, by forcing cc::PropertyTreeBuilder to initialize
the paint properties of the pepper fullscreen root layer that is not
managed by blink.

TODO(crbug.com/925855): We should let blink manage the paint
properties for all content cc::Layers.

Test: https://chromium-review.googlesource.com/c/chromium/src/+/1437182
passes all CQ bots.

Bug: 913464,925855
Change-Id: I3438562f4671dce5224f8180616e2fe2af2f6cae
Reviewed-on: https://chromium-review.googlesource.com/c/1436493
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626745}
  • Loading branch information
wangxianzhu authored and Commit Bot committed Jan 28, 2019
1 parent 3ecc3d9 commit 41d634c
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 7 deletions.
27 changes: 22 additions & 5 deletions cc/trees/layer_tree_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ void LayerTreeHost::UpdateDeferMainFrameUpdateInternal() {
}

bool LayerTreeHost::IsUsingLayerLists() const {
return settings_.use_layer_lists;
return settings_.use_layer_lists && !force_use_property_tree_builder_;
}

void LayerTreeHost::CommitComplete() {
Expand Down Expand Up @@ -662,10 +662,11 @@ bool LayerTreeHost::UpdateLayers() {
property_trees_.clear();
return false;
}

DCHECK(!root_layer()->parent());
base::ElapsedTimer timer;

bool result = DoUpdateLayers(root_layer());
bool result = DoUpdateLayers();
micro_benchmark_controller_.DidUpdateLayers();

if (const char* client_name = GetClientNameForMetrics()) {
Expand Down Expand Up @@ -736,7 +737,7 @@ void LayerTreeHost::RecordGpuRasterizationHistogram(
gpu_rasterization_histogram_recorded_ = true;
}

bool LayerTreeHost::DoUpdateLayers(Layer* root_layer) {
bool LayerTreeHost::DoUpdateLayers() {
TRACE_EVENT1("cc,benchmark", "LayerTreeHost::DoUpdateLayers",
"source_frame_number", SourceFrameNumber());

Expand All @@ -750,13 +751,13 @@ bool LayerTreeHost::DoUpdateLayers(Layer* root_layer) {
if (!IsUsingLayerLists()) {
TRACE_EVENT0("cc", "LayerTreeHost::UpdateLayers::BuildPropertyTrees");
Layer* root_scroll =
PropertyTreeBuilder::FindFirstScrollableLayer(root_layer);
PropertyTreeBuilder::FindFirstScrollableLayer(root_layer_.get());
Layer* page_scale_layer = viewport_layers_.page_scale.get();
if (!page_scale_layer && root_scroll)
page_scale_layer = root_scroll->parent();
gfx::Transform identity_transform;
PropertyTreeBuilder::BuildPropertyTrees(
root_layer, page_scale_layer, inner_viewport_scroll_layer(),
root_layer_.get(), page_scale_layer, inner_viewport_scroll_layer(),
outer_viewport_scroll_layer(), overscroll_elasticity_element_id(),
elastic_overscroll_, page_scale_factor_, device_scale_factor_,
gfx::Rect(device_viewport_size_), identity_transform, &property_trees_);
Expand Down Expand Up @@ -792,6 +793,11 @@ bool LayerTreeHost::DoUpdateLayers(Layer* root_layer) {
DCHECK(property_trees_.clip_tree.Node(layer->clip_tree_index()));
DCHECK(property_trees_.scroll_tree.Node(layer->scroll_tree_index()));
}
#else
// This is a quick sanity check for readiness of paint properties.
// TODO(crbug.com/913464): This is to help analysis of crashes of the bug.
// Remove this CHECK when we close the bug.
CHECK(property_trees_.effect_tree.Node(root_layer_->effect_tree_index()));
#endif

draw_property_utils::UpdatePropertyTrees(this, &property_trees_);
Expand Down Expand Up @@ -1047,9 +1053,20 @@ void LayerTreeHost::SetRootLayer(scoped_refptr<Layer> root_layer) {
content_has_non_aa_paint_ = false;
gpu_rasterization_histogram_recorded_ = false;

force_use_property_tree_builder_ = false;

SetNeedsFullTreeSync();
}

void LayerTreeHost::SetNonBlinkManagedRootLayer(
scoped_refptr<Layer> root_layer) {
SetRootLayer(std::move(root_layer));

DCHECK(root_layer_->children().empty());
if (IsUsingLayerLists() && root_layer_)
force_use_property_tree_builder_ = true;
}

LayerTreeHost::ViewportLayers::ViewportLayers() = default;

LayerTreeHost::ViewportLayers::~ViewportLayers() = default;
Expand Down
17 changes: 16 additions & 1 deletion cc/trees/layer_tree_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,14 @@ class CC_EXPORT LayerTreeHost : public MutatorHostClient {
Layer* root_layer() { return root_layer_.get(); }
const Layer* root_layer() const { return root_layer_.get(); }

// Sets the root layer which is not managed by blink, and we will initialize
// its paint properties using PropertyTreeBuilder. For ui::Compositor, because
// for now we always use PropertyTreeBulder, this function is equivalent to
// SetRootLayer().
// TODO(crbug.com/925855): This is temporary. Eventually we should let the
// caller inform blink about the layer and remove the function.
void SetNonBlinkManagedRootLayer(scoped_refptr<Layer> root_layer);

// Viewport Layers are used to identify key layers to the compositor thread,
// so that it can perform viewport-based scrolling independently, such as
// for pinch-zoom or overscroll elasticity.
Expand Down Expand Up @@ -708,7 +716,7 @@ class CC_EXPORT LayerTreeHost : public MutatorHostClient {
void ApplyPageScaleDeltaFromImplSide(float page_scale_delta);
void InitializeProxy(std::unique_ptr<Proxy> proxy);

bool DoUpdateLayers(Layer* root_layer);
bool DoUpdateLayers();

void UpdateDeferMainFrameUpdateInternal();

Expand Down Expand Up @@ -843,6 +851,13 @@ class CC_EXPORT LayerTreeHost : public MutatorHostClient {
// for every layer during property tree building.
bool has_copy_request_ = false;

// When settings_.use_layer_lists is true, paint properties are generated by
// blink and we don't use PropertyTreeBuilder, except that the root layer
// is set by SetNonBlinkManagedRootLayer().
// TODO(crbug.com/925855): Remove this field when removing
// SetNonBlinkManagedRootLayer().
bool force_use_property_tree_builder_ = false;

MutatorHost* mutator_host_;

std::vector<std::pair<PaintImage, base::OnceCallback<void(bool)>>>
Expand Down
5 changes: 5 additions & 0 deletions content/renderer/compositor/layer_tree_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,11 @@ void LayerTreeView::ClearRootLayer() {
layer_tree_host_->SetRootLayer(nullptr);
}

void LayerTreeView::SetNonBlinkManagedRootLayer(
scoped_refptr<cc::Layer> layer) {
layer_tree_host_->SetNonBlinkManagedRootLayer(std::move(layer));
}

cc::AnimationHost* LayerTreeView::CompositorAnimationHost() {
return animation_host_.get();
}
Expand Down
1 change: 1 addition & 0 deletions content/renderer/compositor/layer_tree_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ class LayerTreeView : public blink::WebLayerTreeView,
// blink::WebLayerTreeView implementation.
viz::FrameSinkId GetFrameSinkId() override;
void SetRootLayer(scoped_refptr<cc::Layer> layer) override;
void SetNonBlinkManagedRootLayer(scoped_refptr<cc::Layer> layer);
void ClearRootLayer() override;
cc::AnimationHost* CompositorAnimationHost() override;
gfx::Size GetViewportSize() const override;
Expand Down
2 changes: 1 addition & 1 deletion content/renderer/render_widget_fullscreen_pepper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ void RenderWidgetFullscreenPepper::SetLayer(cc::Layer* layer) {
}
UpdateLayerBounds();
layer_->SetIsDrawable(true);
layer_tree_view()->SetRootLayer(layer_);
layer_tree_view()->SetNonBlinkManagedRootLayer(layer_);
}

bool RenderWidgetFullscreenPepper::OnMessageReceived(const IPC::Message& msg) {
Expand Down

0 comments on commit 41d634c

Please sign in to comment.