Clip hierarchy issues |
|||||||||||||||||||||||
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"}
,
Jan 18 2018
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.
,
Jan 19 2018
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.
,
Jan 19 2018
,
Feb 5 2018
,
Feb 8 2018
,
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
,
Feb 15 2018
,
Mar 9 2018
The remaining issues seems not significant in the cases described in TODO(crbug.com/803649) in the source code.
,
Aug 6
,
Aug 6
,
Aug 6
,
Aug 6
,
Sep 4
,
Sep 4
,
Sep 4
,
Sep 4
,
Oct 26
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.
,
Oct 26
,
Oct 26
,
Oct 26
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.
,
Oct 26
Clusterfuzz is right that I exposed another avenue to this bug by enabling BGPT. For now we're going to leave this available.
,
Oct 30
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.
,
Oct 30
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.
,
Nov 20
> 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?
,
Nov 20
> 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).
,
Dec 7
Issue 912828 has been merged into this issue.
,
Dec 7
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.
,
Dec 11
,
Dec 17
Issue 915364 has been merged into this issue.
,
Dec 17
We are getting so many crashes from this after enabling BlinkGenPropertyTrees in experimental that I think we may need to work around it.
,
Dec 17
,
Dec 17
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.
,
Dec 17
,
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
,
Dec 19
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 |
|||||||||||||||||||||||
Comment 1 by chrishtr@chromium.org
, Jan 18 2018