New issue
Advanced search Search tips

Issue 803649 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 2
Type: Bug

Blocked on:
issue 869264
issue 870680
issue 879983



Sign in to add a comment

Clip hierarchy issues

Project Member Reported by wangxianzhu@chromium.org, Jan 18 2018

Issue description

For now fragment clips are not parents of any other clips. For example,

div style="width: 200px; height: 100px; columns: 2">
  <div style="overflow: hidden; width: 80px; height: 180px; border: 2px solid blue; background: yellow">
    <div style="height: 2000px; background: green"></div>
  </div>
</div>

The clip tree is like:
root 0x26d8b3f64100 {"localTransformSpace":"0x26d8b3eb88d0","rect":"InfiniteIntRect"}
  ContentClip (FrameView) 0x26d8b3f642e0 {"parent":"0x26d8b3f64100","localTransformSpace":"0x26d8b3eb8c90","rect":"0,0 800x600"}
    FragmentClip (LayoutMultiColumnFlowThread (anonymous)) 0x26d8b3f644c0 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"108,8 1.00001e+06x999909"}
    FragmentClip (LayoutMultiColumnFlowThread (anonymous)) 0x26d8b3f643d0 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"-999992,-999992 1.0001e+06x1.00009e+06"}
    FragmentClip (LayoutBlockFlow DIV) 0x26d8b3f646a0 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"108,8 1.00001e+06x999909"}
    FragmentClip (LayoutBlockFlow DIV) 0x26d8b3f645b0 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"-999992,-999992 1.0001e+06x1.00009e+06"}
    OverflowClip (LayoutBlockFlow DIV) 0x26d8b3f64790 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"9,9 80x180"}
    OverflowClip (LayoutBlockFlow DIV) 0x26d8b3f64880 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"117,-82 80x180"}

When we paint contents under the OverflowClips, no FragmentClip will be applied, causing the painting overflows fragments.

A better clip tree might be:
root 0x26d8b3f64100 {"localTransformSpace":"0x26d8b3eb88d0","rect":"InfiniteIntRect"}
  ContentClip (FrameView) 0x26d8b3f642e0 {"parent":"0x26d8b3f64100","localTransformSpace":"0x26d8b3eb8c90","rect":"0,0 800x600"}
    FragmentClip (LayoutMultiColumnFlowThread (anonymous)) 0x26d8b3f644c0 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"108,8 1.00001e+06x999909"}
      OverflowClip (LayoutBlockFlow DIV) 0x26d8b3f64790 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"9,9 80x180"}
    FragmentClip (LayoutMultiColumnFlowThread (anonymous)) 0x26d8b3f643d0 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"-999992,-999992 1.0001e+06x1.00009e+06"}
      OverflowClip (LayoutBlockFlow DIV) 0x26d8b3f64880 {"parent":"0x26d8b3f642e0","localTransformSpace":"0x26d8b3eb8c90","rect":"117,-82 80x180"}

 
What do you mean by "causing the painting overflows fragments"?
For the test case, the fragments of the green div are not clipped by fragment clips, just by overflow clip. You can see the wrong result by opening the test in content_shell with --enable-slimming-paint-v175.

The fragments will be clipped by fragment clips if we let the overflow clips be the children of the fragment clips.
Owner: chrishtr@chromium.org
I see. The green div has fragment clips in its FragmentData linked list, but they don't apply
because the div does not have a PaintLayer.  
Owner: ----
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Available)
Blockedon: 807382
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5844f7146bca1e7c91e8f03de044ab408850d611

commit 5844f7146bca1e7c91e8f03de044ab408850d611
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Thu Feb 15 06:48:36 2018

[SPv175+] Let FragmentClips be in clip tree hierarchy

Previously all FragmentClips under a fragmented subtree were at the
same level below the parent clip state of the top-level fragmented
object. A FragmentClip was used in a chunk property state only if
the chunk had no other clips under the parent clip state of the top-
level fragmented object, causing FragmentClip was not applied in
some cases.

Now let Fragments be in clip tree hierarchy. When we create a new
tree builder context for a fragment, we need to try to find the
parent context from which to inherit the paint properties:
1. For a new LayoutFlowThread:
  a) if it's the top level, use the first (and only) parent context;
  b) otherwise, convert logical_top_in_flow_thread in itself to
     logical top in the parent flow thread which is used to match
     the parent context.
2. For an object which has column-span:all or is out-of-flow
   positioned, skip matching against parent contexts because the
   fragment is definitely (column-span:all) or probably (out-of-flow)
   not the child of any fragment of the parent fragments.
3. Otherwise, match the parent context using
   logical_top_in_flow_thread.

If the above algorithm matches a parent context, make a copy the
parent context as the new context, and overwrite fragment_clip and
logical_top_in_flow_thread.

Otherwise, we'll use the first parent fragment as if it were the
parent fragment, but traverse up the Container tree (which includes
all containing flow threads) to find the correct clip state.

Bug: 803649
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I42e84263b709e184dde92b972c74da96becd8a23
Reviewed-on: https://chromium-review.googlesource.com/912551
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536953}
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v175
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/multicol/multicol-repaint-expected.txt
[rename] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/mac-mac10.12/paint/invalidation/multicol/multicol-as-paint-container-expected.txt
[add] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/mac-mac10.12/paint/invalidation/multicol/multicol-repaint-expected.txt
[add] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/mac-mac10.12/paint/invalidation/multicol/multicol-with-block-expected.txt
[rename] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/mac-mac10.12/paint/invalidation/multicol/multicol-with-inline-expected.txt
[add] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/mac-mac10.12/paint/invalidation/multicol/multicol-with-overflowing-block-rl-expected.txt
[add] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/mac-mac10.12/paint/invalidation/multicol/multicol-with-text-expected.txt
[add] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/mac-mac10.12/paint/invalidation/overflow/paged-with-overflowing-block-rl-expected.txt
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/multicol/multicol-as-paint-container-expected.txt
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/multicol/multicol-repaint-expected.txt
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/multicol/multicol-with-inline-expected.txt
[add] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/multicol/multicol-with-overflowing-block-rl-expected.txt
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/multicol/multicol-with-text-expected.txt
[add] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/overflow/paged-with-overflowing-block-rl-expected.txt
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/Source/core/layout/LayoutFlowThread.cpp
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/Source/core/layout/LayoutFlowThread.h
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/Source/core/layout/LayoutTreeAsText.cpp
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/Source/core/paint/FragmentData.h
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h
[modify] https://crrev.com/5844f7146bca1e7c91e8f03de044ab408850d611/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp

Blockedon: -807382
Cc: wangxianzhu@chromium.org
Labels: -Pri-2 Pri-3
Owner: ----
Status: Available (was: Assigned)
The remaining issues seems not significant in the cases described in TODO(crbug.com/803649) in the source code.

Blockedon: 870638
Blockedon: 869264
Blockedon: 870680
Blockedon: -870638
Cc: pnangunoori@chromium.org
 Issue 879946  has been merged into this issue.
Blockedon: 879946
Blockedon: 879983
Blockedon: -879946
With BGPT we start hitting this in property_tree_manager:
Check failed: IsCurrentCcEffectSynthetic(). 

Here is a minimal testcase:
<!doctype html>
<div style="columns: 1;">
  <div style="clip-path: circle(40%);">
    <div style="will-change: transform;"></div>
  </div>
</div>

The issue is that we have two paint chunks (or GraphicsLayers) with the same mask effect node but the second chunk (or GraphicsLayer) has a clip node above the first. This is impossible to render because all chunks sharing the same effect node must be rendered together. The clip of the second chunk can be above the clip of the first chunk due to the fragment code in paint_property_tree_builder that resets the clip of positioned descendants of a multicolumn so that they are not clipped by the fragment clip.
Cc: pdr@chromium.org
 Issue 898454  has been merged into this issue.
Project Member

Comment 20 by ClusterFuzz, Oct 26

Labels: OS-Linux
Project Member

Comment 21 by ClusterFuzz, Oct 26

Labels: Test-Predator-Auto-Owner
Owner: pdr@chromium.org
Status: Assigned (was: Available)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/4e8eb7fbb5ada77093c526013db61d2c438ca24d ([BlinkGenPropertyTrees] Promote BGPT to experimental).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Owner: ----
Status: Available (was: Assigned)
Clusterfuzz is right that I exposed another avenue to this bug by enabling BGPT. For now we're going to leave this available.
Project Member

Comment 23 by ClusterFuzz, Oct 30

Labels: ClusterFuzz-Verified
Status: Verified (was: Available)
ClusterFuzz testcase 5745432091951104 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Labels: -ClusterFuzz-Verified -Test-Predator-Auto-Owner ClusterFuzz-Wrong
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Verified)
Discussed with pdr@ offline, and we got more insight into the issue.

Summary of the issue:

1. When an object under a fragmented context need to escape the fragment clip, for now we just set the clip state to the parent of the fragment clip, causing the object also mistakenly escapes the clips under the fragment clip. Some clips under the fragment clip may need to apply to the object.

2. If any effect with explicit output clip under the fragment clip is applied on the object, escaping the clip will cause bad property tree state of the object, which will cause DCHECK failure in PaintPropertyManager, error message in PaintChunksToCcLayer, and/or corrupted rendering.

The solution will be to keep two clip states in fragmented context: one is with fragment clips and the other without. When an object needs to escape the fragment clips, we will use the clip state without fragment clips. Unused clip nodes in the unused clip states will be automatically removed after PrePaint when we remove their only references from the tree builder context.
> The solution will be to keep two clip states in fragmented context: one is with fragment clips and the other without.

Won't this mean duplicating all of the non-fragmented clips, to preserve parenting hierarchy?
> Won't this mean duplicating all of the non-fragmented clips, to preserve parenting hierarchy?

Yes. We may also need to duplicate the clip path (resulting total 3 clip states) for two usages: skipping the nearest fragment clip (for column-span:all), and skipping all fragment clips (for monolithic elements, including composited elements in SPv1).
 Issue 912828  has been merged into this issue.
The infinite wisdom of clusterfuzz produced an interesting testcase:
<!doctype html>
<html style="column-width: 10px;">
<body style="clip-path: circle(40%);">
<div style="column-span: all; will-change: transform;"></div>
</body>
This hits a slightly different DCHECK before the IsCurrentCcEffectSynthetic DCHECK:
compositing_layer_property_updater.cc(57)] Check failed: layout_snapped_paint_offset == snapped_paint_offset ("-0.3125,0" vs. "0,0")"LayoutBlockFlow (column spanner) DIV"

I think this is a variant of this bug.
Project Member

Comment 29 by ClusterFuzz, Dec 11

Labels: OS-Mac
 Issue 915364  has been merged into this issue.
We are getting so many crashes from this after enabling BlinkGenPropertyTrees in experimental that I think we may need to work around it.
Blocking: -771643
Labels: -Pri-3 Pri-1
Status: Started (was: Assigned)
I just noticed that we also need to duplicate clip tree path in other cases. Actually CssClipFixedPosition is one of it, but the current implementation is incomplete, e.g. in the following case:
<div style="overflow: hidden; height: 300px">
  <div style="clip-path: circle(70%); background: yellow; width: 300px; height: 300px">
    <div style="position: fixed; width: 100px; height: 100px; background: green"></div>
  </div>
</div>

the fixed-position object need to skip the overflow clip but not the clip-path clip. Currently we fail to skip the overflow clip.

Because such cases are rare, my plan is to clone the clip tree path (excluding skipped clips) only when needed instead of keeping multiple clip contexts.
Summary: Clip hierarchy issues (was: [SPv175+] Fragment clip hierarchy issues)
Project Member

Comment 35 by bugdroid1@chromium.org, Dec 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a31bb2899b906264ecb5ec895ff3302e6919d4a

commit 1a31bb2899b906264ecb5ec895ff3302e6919d4a
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Wed Dec 19 01:15:56 2018

[BlinkGenPropertyTrees] Don't skip fragment clip above effect's output clip

We don't allow an object's clip an object's clip to escape the output
clip of the object's effect, so don't skip fragment clip if there is any
effect between the object and the clip container has output clip.

This won't get correct rendering (because of the extra clips), but
avoids crash in PropertyTreeManager.

Bug: 803649
Change-Id: I63c7feda09755be1ac04d9a788c4110449f73f04
Reviewed-on: https://chromium-review.googlesource.com/c/1382870
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617701}
[modify] https://crrev.com/1a31bb2899b906264ecb5ec895ff3302e6919d4a/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
[add] https://crrev.com/1a31bb2899b906264ecb5ec895ff3302e6919d4a/third_party/blink/web_tests/external/wpt/css/css-multicol/multicol-span-all-under-clip-path-crash.html

Labels: -Pri-1 Pri-2
The #c35 CL should lower BGPT crash rate and unblock BGPT. We need more discussions to fix the root cause which is not BGPT-specific.

Sign in to add a comment