fast/frames/crash-frameset-CSS-content-property.html flaky |
|||
Issue description
This test switches a <frameset> to a <body>. For framesets, LayoutView::CalculateScrollbarModes forces the user scrollable bit to be false. So, we need to make sure to call LayoutView::SetNeedsPaintPropertyUpdate when the LayoutView's child changes to/from a frameset.
Fix:
virtual LayoutFramset::InsertedIntoTree() {
LayoutBox::InsertedIntoTree();
// User scrollability on the scroll paint property depends on framesets (see: LayoutView::CalculateScrollbarModes)
// so we need to ensure the property gets updated.
if (Parent() && Parent() == View())
Parent()->SetNeedsPaintPropertyUpdate();
}
virtual LayoutFramset::WillBeRemovedFromTree() {
LayoutBox::WillBeRemovedFromTree();
// User scrollability on the scroll paint property depends on framesets (see: LayoutView::CalculateScrollbarModes)
// so we need to ensure the property gets updated.
if (Parent() && Parent() == View())
Parent()->SetNeedsPaintPropertyUpdate();
}
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7f775d338e8eb3690412d443208b43025cd18f2 commit f7f775d338e8eb3690412d443208b43025cd18f2 Author: Sahel Sharify <sahel@chromium.org> Date: Thu Aug 23 20:58:45 2018 Revert "Fix fast/frames/crash-frameset-CSS-content-property.html" This reverts commit 2e954351680196ce8508ce8bb7b47af62c7e77e3. Reason for revert: "fast/frames/crash-frameset-CSS-content-property.html" is flaky example failure: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/171496 Original change's description: > Fix fast/frames/crash-frameset-CSS-content-property.html > > This test switches a <frameset> to a <body>. For framesets, LayoutView::CalculateScrollbarModes > forces the user scrollable bit to be false. So, we need to make sure to call > LayoutView::SetNeedsPaintPropertyUpdate when the LayoutView's child changes to/from a frameset. > > > Bug: 875287 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel > > Change-Id: I5ae0a7673ae8e325242465633749ffcd76ca705f > Reviewed-on: https://chromium-review.googlesource.com/1186181 > Reviewed-by: Philip Rogers <pdr@chromium.org> > Commit-Queue: Jianpeng Chao <chaopeng@chromium.org> > Cr-Commit-Position: refs/heads/master@{#585493} TBR=pdr@chromium.org,chaopeng@chromium.org Change-Id: Ibbfd46388eedd04af20242e4c87413880b9d9718 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 875287 , 877210 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/1187283 Reviewed-by: Sahel Sharify <sahel@chromium.org> Commit-Queue: Sahel Sharify <sahel@chromium.org> Cr-Commit-Position: refs/heads/master@{#585612} [modify] https://crrev.com/f7f775d338e8eb3690412d443208b43025cd18f2/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/f7f775d338e8eb3690412d443208b43025cd18f2/third_party/blink/renderer/core/layout/layout_frame_set.cc [modify] https://crrev.com/f7f775d338e8eb3690412d443208b43025cd18f2/third_party/blink/renderer/core/layout/layout_frame_set.h
,
Aug 23
Hi Philip, Do you have other idea about this?
,
Aug 24
fyi: locally bisected the crash to crrev.com/6b253b354 (it's not really flaky on my machine) [1:1:0824/092934.984738:FATAL:object_paint_properties.h(116)] Check failed: !is_immutable_ || result.Unchanged(). Value changed while immutable. New state: Scroll (LayoutView #document) 0x5c06d124490 {"parent":"0x5c06d1243d0","containerRect":"0,0 800x600","contentsRect":"0,0 800x600","userScrollable":"both"} #0 0x7f5b1e1fefbd base::debug::StackTrace::StackTrace() #1 0x7f5b1df0534c base::debug::StackTrace::StackTrace() #2 0x7f5b1df74f5a logging::LogMessage::~LogMessage() #3 0x7f5b0b26067e blink::ObjectPaintProperties::UpdateScroll() #4 0x7f5b0b24f296 blink::(anonymous namespace)::FragmentPaintPropertyTreeBuilder::UpdateScrollAndScrollTranslation() #5 0x7f5b0b24733d blink::(anonymous namespace)::FragmentPaintPropertyTreeBuilder::UpdateForChildren() #6 0x7f5b0b247098 blink::PaintPropertyTreeBuilder::UpdateForChildren() #7 0x7f5b0b26c15a blink::PrePaintTreeWalk::WalkInternal() #8 0x7f5b0b26b4ca blink::PrePaintTreeWalk::Walk() #9 0x7f5b0b26b066 blink::PrePaintTreeWalk::Walk() #10 0x7f5b0b26ab03 blink::PrePaintTreeWalk::WalkTree() #11 0x7f5b0a839cff blink::LocalFrameView::RunPrePaintLifecyclePhase()
,
Aug 24
The test replaces a body with a frameset which should invalidate using LayoutFramset::InsertedIntoTree/LayoutFramset::WillBeRemovedFromTree but doesn't. Does the frameset get WillBeRemovedFromTree called? If so, maybe we cannot do the Parent() check and need to always invalidate the View. If not, why is WillBeRemovedFromTree not being called?
,
Sep 28
The LayoutFrameset::WillBeRemovedFromTree does not get called because LayoutObjectChildList::RemoveChildNode actually called with owner "LayoutBlockFlow HTML" old_child "LayoutImage FRAMESET id='frameset'". LayoutImage::WillBeRemovedFromTree get called from HTML. I think maybe we should do thefix in LayoutObjectChildList::RemoveChildNode always set owner SetNeedsPaintPropertyUpdate at the end.
,
Sep 28
It still crash, the crash not happens in replaceChild but in test teardown.
Old state: Scroll (LayoutView #document) 0x164cc0ba06d0 {"parent":"0x164cc0ba0250","containerRect":"0,0 800x600","contentsSize":"800x600"}
New state: Scroll (LayoutView #document) 0x164cc0ba06d0 {"parent":"0x164cc0ba0250","containerRect":"0,0 800x600","contentsSize":"800x600","userScrollable":"both"}
The difference is "userScrollable":"both". Maybe I should go deep into what change userScrollable.
,
Sep 29
It still crashes after add SetNeedsPaintPropertyUpdate to LayoutObjectChildList::RemoveChildNode because SetNeedsPaintPropertyUpdate only set SetNeedsPaintPropertyUpdate on this layout object but set DescendantNeedsPaintPropertyUpdate on ancestors.
In this case, LayoutObjectChildList::RemoveChildNode owner is "LayoutBlockFlow HTML" but the scroll properties changes in LayoutView #document. LayoutView #document is ancestor of LayoutBlockFlow HTML.
```
LayoutView 0x432b5204010 at (0,0) size 800x600
paint_offset=(0,0) visual_rect=(0,0 800x600) state=(t:0xcf23d545890 c:0xcf23d5a0370 e:0xcf23d55c1f0)
positive z-order list(1)
layer 0x432b52140e8 at (0,0) size 800x600
LayoutBlockFlow 0x432b5224010 {HTML} at (0,0) size 800x600
paint_offset=(0,0) visual_rect=(0,0 800x600) state=(t:0xcf23d544550 c:0xcf23d5a0130 e:0xcf23d55c1f0)
LayoutImage 0x432b5234010 {FRAMESET} at (0,0) size 0x0 id="frameset"
paint_offset=(0,0) visual_rect=(0,0 0x0)
```
But in FindObjectPropertiesNeedingUpdateScope which we set ObjectProperties immutable we only skip set it immutable when layout object NeedsPaintPropertyUpdate() is true.
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/find_properties_needing_update.h?rcl=3bdf8f1ab362aee7edbd5cca0dc9fc3a1896050b&l=64
So the fix maybe:
1. In LayoutObjectChildList::RemoveChildNode set owner SetNeedsPaintPropertyUpdate
2. In FindObjectPropertiesNeedingUpdateScope early return if layout object DescendantNeedsPaintPropertyUpdate is true.
pdr@ WDYT?
,
Sep 29
I would like to avoid changing LayoutObjectChildList::RemoveChildNode because it is called on all removals but this logic is only needed for a very rare frameset scenario.
In comment #6 you wrote:
"""
The LayoutFrameset::WillBeRemovedFromTree does not get called because LayoutObjectChildList::RemoveChildNode actually called with owner "LayoutBlockFlow HTML" old_child "LayoutImage FRAMESET id='frameset'". LayoutImage::WillBeRemovedFromTree get called from HTML.
"""
Do you mean that we don't have a LayoutFrameset object at all and instead have a LayoutImage? If we don't have a LayoutFramset, I think it is a mistake that LayoutView::CalculateScrollbarModes does the frameset special-case logic. What do you think of reviving your original patch and changing LayoutView::CalculateScrollbarModes to check for the LayoutFrameset object like this:
--------------------------8<--------------------------
void LayoutView::CalculateScrollbarModes(ScrollbarMode& h_mode,
ScrollbarMode& v_mode) const {
...
Document& document = GetDocument();
if (Node* body = document.body()) {
// Framesets can't scroll.
if (body->GetLayoutObject() && body->GetLayoutObject()->IsFrameSet())
RETURN_SCROLLBAR_MODE(kScrollbarAlwaysOff);
}
...
}
--------------------------8<--------------------------
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2892d91b0ce2ae422957c9bd8b90e6002579597c commit 2892d91b0ce2ae422957c9bd8b90e6002579597c Author: chaopeng <chaopeng@chromium.org> Date: Mon Oct 01 19:33:06 2018 Fix fast/frames/crash-frameset-CSS-content-property.html This test switches a <frameset> to a <body>. For framesets, LayoutView::CalculateScrollbarModes forces the user scrollable bit to be false. It excluded layout object is not LayoutFrameset but it is of type of Frameset. In this patch, we changed the Frameset check to body->GetLayoutObject()->IsFrameSet(). Bug: 875287 Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Icefcc3801dc79b39576f1248a07e43f905279e13 Reviewed-on: https://chromium-review.googlesource.com/1254470 Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Jianpeng Chao <chaopeng@chromium.org> Cr-Commit-Position: refs/heads/master@{#595505} [modify] https://crrev.com/2892d91b0ce2ae422957c9bd8b90e6002579597c/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/2892d91b0ce2ae422957c9bd8b90e6002579597c/third_party/blink/renderer/core/layout/layout_view.cc
,
Oct 2
|
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Aug 23