📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14aab505a40000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14aab505a40000

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

commit a77a2f59e3a2887dfa7d232851228b5c1581b8bb
Author: Aleks Totic <atotic@chromium.org>
Date: Wed Sep 26 19:29:00 2018

[LayoutNG] Fix float double-painting crash

The crash trigger was Legacy and NG painting the same float.

This is how it happens:
NGBlockNode::CopyChildFragmentPosition adds float to ContainingBlock()
float list.
Float might propagate to ContainingBlock()'s ancestor and set the
ShouldPaint() flag. How this happens is still mysterious to me, the
obvious suspect UpdateAncestorShouldPaintFloatingObject does not
get called.

NG will paint the float if ContainingBlock has PaintFragment().

My fix is a bit of a hack: in Legacy, if we know that float will
get painted by NG, do not paint it.

This fixes the crash. Test still fails, but that is block layout
algorithm problem.

Bug:  864398 
Cq-Include-Trybots: 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: I7ebe82a2188baaa076476e4f5cbd78bab7127875
Reviewed-on: https://chromium-review.googlesource.com/1244246
Commit-Queue: Aleks Totic <atotic@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594428}
[modify] https://crrev.com/a77a2f59e3a2887dfa7d232851228b5c1581b8bb/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/a77a2f59e3a2887dfa7d232851228b5c1581b8bb/third_party/blink/renderer/core/paint/block_painter.cc

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

commit 4b2392e52102a19f67b89a3bbd86312223af09b1
Author: Aleks Totic <atotic@chromium.org>
Date: Fri Sep 28 04:50:38 2018

[LayoutNG] Fix float ShouldPaint flag

This is not the kind of fix I like, that gets to the core of the problem
and fixes it for good.

BUG ANALYSIS:
The bug was that the floats were not being painted.

Floats were supposed to be painted by NG container.

They were not being painted because layer did not
paint with PaintPhase::kFloat.

PaintLayer did not paint with kFloat because it did not
have needs_paint_phase_float_ flag set.

The flag was not set because float's
ContainingBlock().ContainsFloats() returned false.

ContainsFloats() returned false because none of the floats
it knew about had ShouldPaint() == true.

ShouldPaint() was false because in first part of the test
float was self-painting. In 2nd part of the test, float
becomes not self-painting, and should have its flag reset.

This does not happen in NG, because LayoutBlockFlow::InsertFloatingObject
never resets the flag because flags do not get set if
floating object is already there.

How does the flag get reset in Legacy? In Legacy,
all floats get cleared on Layout, so when float is
added again, the flag gets set correctly.

Removing all floats in NG on layout would be the
"correct" fix, matching existing behavior. But,
this cannot be done easily. Float descendants
from cached fragments do not get added back on relayout.
Adding descendants from cached fragments would be a big
fix, we'd need to keep a list in cache. We can talk about whether
we want to do this. I can imagine there are other
not-tested float scenarios, that fail even with this patch.
But since I am not a float expert, I can't come up with
any.
Long term, the need for legacy float list will go away.
I'd hold off on complicated fix until we encounter
real-world problems.

THE FIX:
Instead of resetting flag by removing the float, NG
resets the flag after insertion.

Bug:  864398 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ib09b59669644ebe574a6e41cb1ef6426598296c5
Reviewed-on: https://chromium-review.googlesource.com/1249839
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Aleks Totic <atotic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594983}
[modify] https://crrev.com/4b2392e52102a19f67b89a3bbd86312223af09b1/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[add] https://crrev.com/4b2392e52102a19f67b89a3bbd86312223af09b1/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/block/float/nopaint-after-layer-destruction2-expected.png
[add] https://crrev.com/4b2392e52102a19f67b89a3bbd86312223af09b1/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/block/float/nopaint-after-layer-destruction2-expected.txt
[modify] https://crrev.com/4b2392e52102a19f67b89a3bbd86312223af09b1/third_party/blink/renderer/core/layout/ng/ng_block_node.cc

I took a look at the compositing/iframes/floating-self-painting-frame.html failure. Not sure if you have a fix in progress for it yet, but anyway, here's what seems to go wrong:

The float must be inside a zero-height block. The float must be painted, then change from being composited to not being composited (without being relaid out?).

There may be more to the story. Anyway, I haven't been able to get rid of the iframe and PreferCompositingToLCDText (which seems to enable compositing if there's a document with a scrollbar inside the iframe, and disable it if there's none).

Deleted: tc-iframe-removing-scrollbars.html 541 bytes

tc-iframe-removing-scrollbars.html 541 bytes View Download

I've been looking at floating-self-painting-frame.html. The fix is not obvious. This is what I came up with so far:
https://chromium-review.googlesource.com/c/chromium/src/+/1254756

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/11552752e40000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/11552752e40000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/153257d8e40000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/153257d8e40000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16667bfce40000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16667bfce40000

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

commit 080afd762a5b339e78f912715353efb96162c7df
Author: Aleks Totic <atotic@chromium.org>
Date: Sat Oct 06 04:01:20 2018

[LayoutNG] Move ContentsInkOverflow out of NGPhysicalContainerFragment

ContentsInkOverflow cannot be cached on fragments, because
- self-paint flag affects overflow
- NG cannot recompute overflow without full layout
=> overflow cannot be cached on NG fragments.

I've initially tried this patch with no cacheing at all, and 4
additional tests passed, validating that incorrect cached overflow is
the cause of failures.

Because NG cannot store overflow, the only other place where overflow
can be stored is LayoutBox. Fragments whose LayoutObject() is LayoutBox
will store their ContentsOverflow in Legacy.
Fragments whose LayoutObject() is not LayoutBox (LineBox fragments,
LayoutInline), will recompute ContentsInkOverflow for every call.

This is ok, but not great. It does regress performance for up to
5% on some tests. Suggestions on how to improve are welcome.

My next step is to profile the worst regressed tests, and see
if anything jumps out.

Bug:  864398 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I7c6c7289b66803cf18d130ad941fae3766426a67
Reviewed-on: https://chromium-review.googlesource.com/c/1258044
Commit-Queue: Aleks Totic <atotic@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597407}
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.cc
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/inline/ng_physical_line_box_fragment.cc
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/inline/ng_physical_line_box_fragment.h
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.cc
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/ng_fragment_builder.cc
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.cc
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/ng_physical_container_fragment.cc
[modify] https://crrev.com/080afd762a5b339e78f912715353efb96162c7df/third_party/blink/renderer/core/layout/ng/ng_physical_container_fragment.h

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

commit 1bbabd2da36b92418645174c066d8519566930ff
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Tue Oct 09 13:06:23 2018

[LayoutNG] Mark failing float test with designated bug.

TBR=atotic@chromium.org

Bug:  864398 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I18646d765a98353e3cf2fb25b2e534fb5aef80c5
Reviewed-on: https://chromium-review.googlesource.com/c/1270855
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597892}
[modify] https://crrev.com/1bbabd2da36b92418645174c066d8519566930ff/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

All NG float painting bugs are gone with ikilpatrick and chrishtr fixes.

\o/

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

commit 62e424e43c0823c1b9fa5485b6b017e604c20dea
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Fri Jan 04 15:32:37 2019

[LayoutNG] Update test expectations.

4 new failing tests.
1 test that used to fail in NG now passes.
12 tests recently marked as failing in legacy, but pass in NG.

TBR=atotic@chromium.org, kojii@chromium.org

Bug:  864398 
Change-Id: I2f39ba62a4162da0834b3a29342315b1c334cd67
Reviewed-on: https://chromium-review.googlesource.com/c/1396028
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619956}
[modify] https://crrev.com/62e424e43c0823c1b9fa5485b6b017e604c20dea/third_party/blink/web_tests/FlagExpectations/enable-blink-features=LayoutNG