Skip to content

GitLab

T
tangled
Commit 3197afbf authored by Tien-Ren Chen's avatar Tien-Ren Chen Committed by Commit Bot
Browse files
Options

Revert "[Blink/SPv175+] Change DCHECK(chunk clip escaped layer clip) to a DLOG" 

This reverts commit bb7972c1.

Reason for revert: crbug.com/882642

Original change's description:
> [Blink/SPv175+] Change DCHECK(chunk clip escaped layer clip) to a DLOG
> 
> The PaintChunkToCcLayer algorithm was originally designed for
> SPv2 compositor, and it was expected the layerization algorithm should
> never assign a chunk to a excessively clipped layer, thus the DCHECK.
> 
> Later this algorithm was adopted in SPv175 to be used with the
> SPv1 compositor. There is a known bug that in certain corner case we
> can fail to escape clip, and the bug is difficult to fix in the
> legacy architecture. The DCHECK is expected to be a "soft" one that
> we have a fail-safe path to recover in a sane way.
> 
> This CL converts the DCHECK to a DLOG in SPv1 mode, and it should still
> trap in SPv2 mode. In addition, it reverts a workaround to a nullptr
> bug in the fail-safe path, and add a test for the fail-safe.
> 
> BUG=881788,853357
> 
> Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I4606acf4885f3344bb45a901bb2e8e46dbcda49a
> Reviewed-on: https://chromium-review.googlesource.com/1213952
> Reviewed-by: vmpstr <vmpstr@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Commit-Queue: Tien-Ren Chen <trchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#590052}

TBR=vmpstr@chromium.org,trchen@chromium.org,pdr@chromium.org

Change-Id: I8f669d48422eb2cc3bb21db2d4866857710c3fbb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 881788, 853357
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1217712Reviewed-by: default avatarTien-Ren Chen <trchen@chromium.org>
Commit-Queue: Tien-Ren Chen <trchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590114}
parent 973da034
Show whitespace changes
Inline Side-by-side
Showing with 35 additions and 78 deletions
third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer.cc
View file @ 3197afbf
......@@ -21,6 +21,17 @@ namespace blink {
namespace {
// TODO(crbug.com/853357): This is a temporary work-around for a bug. Unalias on
// a node should always succeed if the node is not null. The code below assumes
// that the node is not null, so we should be calling Unalias directly. However,
// because of the referenced bug, which triggers a DCHECK, the node can in fact
// sometimes be null. This turns the bug symptom from a DCHECK to a
// null-dereference, which crashes release.
template <typename NodeType>
const NodeType* Unalias(const NodeType* node) {
return node ? node->Unalias() : nullptr;
}
class ConversionContext {
public:
ConversionContext(const PropertyTreeState& layer_state,
......@@ -29,8 +40,8 @@ class ConversionContext {
cc::DisplayItemList& cc_list)
: layer_state_(layer_state),
layer_offset_(layer_offset),
current_transform_(layer_state.Transform()->Unalias()),
current_clip_(layer_state.Clip()->Unalias()),
current_transform_(Unalias(layer_state.Transform())),
current_clip_(Unalias(layer_state.Clip())),
current_effect_(layer_state.Effect()),
chunk_to_layer_mapper_(layer_state_,
layer_offset_,
......@@ -311,7 +322,7 @@ static bool CombineClip(const ClipPaintPropertyNode* clip,
}
void ConversionContext::SwitchToClip(const ClipPaintPropertyNode* target_clip) {
target_clip = target_clip->Unalias();
target_clip = Unalias(target_clip);
if (target_clip == current_clip_)
return;
......@@ -319,23 +330,16 @@ void ConversionContext::SwitchToClip(const ClipPaintPropertyNode* target_clip) {
const ClipPaintPropertyNode* lca_clip =
LowestCommonAncestor(*target_clip, *current_clip_).Unalias();
while (current_clip_ != lca_clip) {
if (!state_stack_.size() || state_stack_.back().type != StateEntry::kClip) {
// There is a known bug in SPv1 compositing that a chunk may be assigned
// to a layer with a deeper clip than the chunk wants. We should recover
// in a sane way in that case.
#if DCHECK_IS_ON()
DLOG(ERROR)
<< "Error: Chunk has a clip that escaped its layer's or effect's"
<< " clip.\n"
<< "target_clip:\n"
DCHECK(state_stack_.size() && state_stack_.back().type == StateEntry::kClip)
<< "Error: Chunk has a clip that escaped its layer's or effect's clip."
<< "\ntarget_clip:\n"
<< target_clip->ToTreeString().Utf8().data() << "current_clip_:\n"
<< current_clip_->ToTreeString().Utf8().data();
#endif
if (RuntimeEnabledFeatures::SlimmingPaintV2Enabled())
NOTREACHED();
if (!state_stack_.size() || state_stack_.back().type != StateEntry::kClip)
break;
}
current_clip_ = current_clip_->Parent()->Unalias();
current_clip_ = Unalias(current_clip_->Parent());
StateEntry& previous_state = state_stack_.back();
if (current_clip_ == lca_clip) {
// |lca_clip| is an intermediate clip in a series of combined clips.
......@@ -346,18 +350,22 @@ void ConversionContext::SwitchToClip(const ClipPaintPropertyNode* target_clip) {
EndClip();
}
// Step 2: Collect all clips between the target clip and the LCA clip.
// If everything goes normally, the current clip should at the LCA clip,
// unless an error was detected in step 1.
if (target_clip == current_clip_)
return;
// Step 2: Collect all clips between the target clip and the current clip.
// At this point the current clip must be an ancestor of the target.
Vector<const ClipPaintPropertyNode*, 1u> pending_clips;
for (const ClipPaintPropertyNode* clip = target_clip; clip != lca_clip;
clip = clip->Parent()->Unalias()) {
for (const ClipPaintPropertyNode* clip = target_clip; clip != current_clip_;
clip = Unalias(clip->Parent())) {
// This should never happen unless the DCHECK in step 1 failed.
if (!clip)
break;
pending_clips.push_back(clip);
}
if (!pending_clips.size())
return;
// Step 3: Now apply the list of clips in top-down order.
DCHECK(pending_clips.size());
auto pending_combined_clip_rect = pending_clips.back()->ClipRect();
const auto* lowest_combined_clip_node = pending_clips.back();
for (size_t i = pending_clips.size() - 1; i--;) {
......@@ -381,9 +389,9 @@ void ConversionContext::SwitchToClip(const ClipPaintPropertyNode* target_clip) {
void ConversionContext::StartClip(
const FloatRoundedRect& combined_clip_rect,
const ClipPaintPropertyNode* lowest_combined_clip_node) {
DCHECK_EQ(lowest_combined_clip_node, lowest_combined_clip_node->Unalias());
DCHECK_EQ(lowest_combined_clip_node, Unalias(lowest_combined_clip_node));
auto* local_transform =
lowest_combined_clip_node->LocalTransformSpace()->Unalias();
Unalias(lowest_combined_clip_node->LocalTransformSpace());
if (local_transform != current_transform_)
EndTransform();
cc_list_.StartPaint();
......@@ -615,7 +623,7 @@ void ConversionContext::PopState() {
void ConversionContext::SwitchToTransform(
const TransformPaintPropertyNode* target_transform) {
target_transform = target_transform->Unalias();
target_transform = Unalias(target_transform);
if (target_transform == current_transform_)
return;
......
third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer_test.cc
View file @ 3197afbf
......@@ -1186,56 +1186,5 @@ TEST_F(PaintChunksToCcLayerTest, StartWithAliasClip) {
EXPECT_THAT(*output, PaintRecordMatcher::Make({cc::PaintOpType::DrawRecord}));
}
// These tests are testing error recovery path that are only used in
// release builds. A DCHECK'd build will trap instead.
#if !DCHECK_IS_ON()
TEST_F(PaintChunksToCcLayerTest, ChunkEscapeLayerClipFailSafe) {
// This test verifies the fail-safe path correctly recovers from a malformed
// chunk that escaped its layer's clip.
FloatRoundedRect clip_rect(0.f, 0.f, 1.f, 1.f);
auto c1 = CreateClip(c0(), &t0(), clip_rect);
TestChunks chunks;
chunks.AddChunk(t0(), c0(), e0());
sk_sp<PaintRecord> output =
PaintChunksToCcLayer::Convert(
chunks.chunks, PropertyTreeState(&t0(), c1.get(), &e0()),
gfx::Vector2dF(), chunks.items,
cc::DisplayItemList::kToBeReleasedAsPaintOpBuffer)
->ReleaseAsRecord();
EXPECT_THAT(*output, PaintRecordMatcher::Make({cc::PaintOpType::DrawRecord}));
}
TEST_F(PaintChunksToCcLayerTest, ChunkEscapeEffectClipFailSafe) {
// This test verifies the fail-safe path correctly recovers from a malformed
// chunk that escaped its effect's clip.
FloatRoundedRect clip_rect(0.f, 0.f, 1.f, 1.f);
auto c1 = CreateClip(c0(), &t0(), clip_rect);
CompositorFilterOperations filter;
filter.AppendBlurFilter(5);
auto e1 = CreateFilterEffect(e0(), &t0(), c1.get(), std::move(filter));
TestChunks chunks;
chunks.AddChunk(t0(), *c1.get(), *e1.get());
chunks.AddChunk(t0(), c0(), *e1.get());
sk_sp<PaintRecord> output =
PaintChunksToCcLayer::Convert(
chunks.chunks, PropertyTreeState(&t0(), &c0(), &e0()),
gfx::Vector2dF(), chunks.items,
cc::DisplayItemList::kToBeReleasedAsPaintOpBuffer)
->ReleaseAsRecord();
EXPECT_THAT(*output,
PaintRecordMatcher::Make({cc::PaintOpType::Save,
cc::PaintOpType::ClipRect, // <c1>
cc::PaintOpType::SaveLayer, // <e1>
cc::PaintOpType::DrawRecord, // <p0/>
cc::PaintOpType::DrawRecord, // <p1/>
cc::PaintOpType::Restore, // </e1>
cc::PaintOpType::Restore})); // </c1>
}
#endif
} // namespace
} // namespace blink
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment