New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 757767 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Oct 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[LayoutNG] Layout tests crash and time out because of multicol

Reported by msten...@opera.com, Aug 22 2017

Issue description

With https://chromium-review.googlesource.com/c/chromium/src/+/591429 a bunch of layout tests will start to crash or time out. Crashes and timeouts slow down the test runner, so better get rid of these problems in a timely manner.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 31 2017

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

commit 5aca1690c0ade593dbaa61e1165f02949da1408b
Author: Morten Stenshorne <mstensho@opera.com>
Date: Thu Aug 31 11:19:25 2017

[LayoutNG] Skip siblings preceding the first break token.

When we resume layout in a next fragmentainer, assume that all siblings
preceding the one associated with the first break token have been finished.
Assuming the opposite - that all preceding siblings needed layout - caused
infinite loops.

Example:
<div style="columns:5; column-fill:auto; height:60px;">
    <div id="child1" style="height:100px;"></div>
    <div id="child2" style="height:100px;"></div>
</div>

After the first column, we'll have an unfinished break token for #child1, which
is what we'll resume with in the second column.
After the second column, we'll have a finished break token for #child1 and an
unfinished break token for #child2.
After the third column, we'll only have an unfinished break token for #child2,
since we skipped #child1 (it was finished).
The fourth column obviously only contains #child2. We shouldn't start at
#child1 just because it has no break token, or we'll get an infinite loop.

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I845a75bb646615223f9acfcc51e2fab04b05b058
Bug:  757767 
Reviewed-on: https://chromium-review.googlesource.com/639410
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498817}
[modify] https://crrev.com/5aca1690c0ade593dbaa61e1165f02949da1408b/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/5aca1690c0ade593dbaa61e1165f02949da1408b/third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator.cc
[modify] https://crrev.com/5aca1690c0ade593dbaa61e1165f02949da1408b/third_party/WebKit/Source/core/layout/ng/ng_block_child_iterator_test.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 4 2017

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

commit 5e611227e94309d4fb81069384a094cf6f93876b
Author: Morten Stenshorne <mstensho@opera.com>
Date: Mon Sep 04 15:21:08 2017

[LayoutNG] Proper overflow legacy write-back inside multicol.

Only calculate overflow when at the last fragment. It's only then that we can
be sure that all children have been laid out. This used to trigger DCHECK
failures in legacy layout.

Also don't let a line break fool us into believing that the block fragmented
(by creating unfinished break tokens).

Re-enable and update unit test OverflowedBlock, now that overflow no longer
causes DCHECK failures in legacy layout.

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I9a104c3a7392db66c251e6f8da8ea8c7e16ba357
Bug:  757767 
Reviewed-on: https://chromium-review.googlesource.com/645968
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499503}
[modify] https://crrev.com/5e611227e94309d4fb81069384a094cf6f93876b/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/5e611227e94309d4fb81069384a094cf6f93876b/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
[modify] https://crrev.com/5e611227e94309d4fb81069384a094cf6f93876b/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
[modify] https://crrev.com/5e611227e94309d4fb81069384a094cf6f93876b/third_party/WebKit/Source/core/layout/ng/ng_column_layout_algorithm_test.cc
[modify] https://crrev.com/5e611227e94309d4fb81069384a094cf6f93876b/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 11 2017

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

commit 0c8918a74706408a1c76517381984a6e22bd5930
Author: Morten Stenshorne <mstensho@opera.com>
Date: Mon Sep 11 15:54:59 2017

Adjustments for LayoutNG in LayoutBlockFlow line layout.

LayoutNG ignores LayoutMultiColumnFlowThread objects, so that the DOM
children of a multicol container become actual layout children of said
multicol container (on the NG side), without any intervening flow
thread block. However, the flow thread is still created even in NG (to
be able to paint and hit-test using the legacy layout structure), so
when NG invokes the legacy engine to lay out lines, we need to be able
to stop walking the ancestry when reaching the flow thread.

This fixes a bunch of crashing tests. They will now either pass or
fail (without crashing) instead.

Bug:  757767 
Change-Id: I55693c34aefe53b47ceb7d7490059cc1182e5ff8
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/660297
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Cr-Commit-Position: refs/heads/master@{#500925}
[modify] https://crrev.com/0c8918a74706408a1c76517381984a6e22bd5930/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/0c8918a74706408a1c76517381984a6e22bd5930/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/0c8918a74706408a1c76517381984a6e22bd5930/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 12 2017

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

commit 86f5a4374843e256b995841b51e44df02af71abe
Author: Morten Stenshorne <mstensho@opera.com>
Date: Tue Sep 12 23:13:42 2017

[LayoutNG] Prevent breaks from escaping the containing fragmentation context.

The call sites that call NGFragmentBuilder::AddChild() also need to explicitly
propagate breaks to their container, if that's what they want. The column
layout algoirithm *doesn't* want this.

Bug:  757767 
Change-Id: I203c045fc85a65303dfe4c0cdad20eb60e64fba2
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/663859
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501444}
[modify] https://crrev.com/86f5a4374843e256b995841b51e44df02af71abe/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/86f5a4374843e256b995841b51e44df02af71abe/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
[modify] https://crrev.com/86f5a4374843e256b995841b51e44df02af71abe/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc
[modify] https://crrev.com/86f5a4374843e256b995841b51e44df02af71abe/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 16 2017

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

commit bcde8e580019343b322b4265fc9664e5646fd2c0
Author: Morten Stenshorne <mstensho@opera.com>
Date: Mon Oct 16 18:54:25 2017

[LayoutNG] Update some test expectations for multicol.

When multicol support was introduced in NG, a lot of tests started to crash and
time out. I wanted to get rid of those as soon as possible, so I created a
separate bug report. It seems that all the crashes and timeouts introduced by
multicol are now gone.

Some caret-range-outside-columns tests still occasionally time out (they're
actually just very slow), but that doesn't seem multicol related.

Bug:  757767 
Change-Id: I4827a090b23b5220e182759a458f3c5a05f115e7
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/720930
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Cr-Commit-Position: refs/heads/master@{#509123}
[modify] https://crrev.com/bcde8e580019343b322b4265fc9664e5646fd2c0/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

Comment 6 by msten...@opera.com, Oct 17 2017

Status: Fixed (was: Assigned)

Sign in to add a comment