Simplified testcase. No need for display:contents, or to interrupt the parser by a script.

Deleted: tc2.html 371 bytes

tc2.html 371 bytes View Download

LayoutNG marks all floating objects as "placed", presumably to avoid DCHECK failures in the geometry getter methods.

This is normally harmless, although being "placed" also tells the legacy float machinery that the floating object has been placed into the FloatingObjectTree, but we never really add anything to that thing in LayoutNG. That usually still works fine, because we don't initialize the FloatingObjectTree - UNLESS there are certain legacy objects present (e.g. tables). In the test, it gets initialized (and populated with the floats that we have at that point) like this:

#0  blink::FloatingObjects::ComputePlacedFloatsTree (this=0x244d67b412a0) at ./../../third_party/blink/renderer/core/layout/floating_objects.cc:568
#1  0x00007fffe2e37954 in blink::FloatingObjects::PlacedFloatsTree (this=0x244d67b412a0) at ../../third_party/blink/renderer/core/layout/floating_objects.h:260
#2  0x00007fffe2d90c6a in blink::FloatingObjects::LogicalLeftOffsetForAvoidingFloats (this=0x244d67b412a0, fixed_offset=0px, logical_top=0px, logical_height=100px) at ./../../third_party/blink/renderer/core/layout/floating_objects.cc:632
#3  0x00007fffe2dd9326 in blink::LayoutBlockFlow::LogicalLeftFloatOffsetForAvoidingFloats (this=0x3b320bc24678, logical_top=0px, fixed_offset=0px, logical_height=100px) at ./../../third_party/blink/renderer/core/layout/layout_block_flow.cc:4342
#4  0x00007fffe2e5afd6 in blink::LayoutBlockFlow::LogicalLeftOffsetForAvoidingFloats (this=0x3b320bc24678, position=0px, logical_height=100px) at ../../third_party/blink/renderer/core/layout/layout_block_flow.h:164
#5  0x00007fffe2e43bef in blink::LayoutBlockFlow::AvailableLogicalWidthForAvoidingFloats (this=0x3b320bc24678, position=0px, logical_height=100px) at ../../third_party/blink/renderer/core/layout/layout_block_flow.h:158
#6  0x00007fffe2defa53 in blink::LayoutBox::ContainingBlockAvailableLineWidth (this=0x3b320bc3c010) at ./../../third_party/blink/renderer/core/layout/layout_box.cc:2146
#7  0x00007fffe2df1ef7 in blink::LayoutBox::ComputeMarginsForDirection (this=0x3b320bc3c010, flow_direction=blink::kInlineDirection, containing_block=0x3b320bc24678, container_width=301px, child_width=100px, margin_start=0px, margin_end=0px, margin_start_length=Length(0, Fixed), margin_end_length=Length(0, Fixed)) at ./../../third_party/blink/renderer/core/layout/layout_box.cc:3093
#8  0x00007fffe2ef77b1 in blink::LayoutTable::UpdateLogicalWidth (this=0x3b320bc3c010) at ./../../third_party/blink/renderer/core/layout/layout_table.cc:385
#9  0x00007fffe2efbf61 in blink::LayoutTable::UpdateLayout (this=0x3b320bc3c010) at ./../../third_party/blink/renderer/core/layout/layout_table.cc:621
#10 0x00007fffe2ee2292 in blink::LayoutObject::ForceLayout (this=0x3b320bc3c010) at ./../../third_party/blink/renderer/core/layout/layout_object.cc:3495
#11 0x00007fffe2d4e8af in blink::NGBlockNode::RunOldLayout (this=0x7fffa3ce6f70, constraint_space=...) at ../../third_party/blink/renderer/core/layout/ng/ng_block_node.cc:873
#12 0x00007fffe2d4de37 in blink::NGBlockNode::Layout (this=0x7fffa3ce6f70, constraint_space=..., break_token=0x0) at ../../third_party/blink/renderer/core/layout/ng/ng_block_node.cc:179

The next thing that needs to go wrong, is that we add another float after the FloatingObjectTree has been initialized like above. LayoutNG doesn't update the FloatingObjectTree in any way, but does set the FloatingObject as "placed", as usual.

Now, if we remove that float again, we're going to search for it in the FloatingObjectTree, but naturally fail to find it. And the DCHECK will fail.

There's one more detail to the story here: The table needs to be relaid out after the first float has been added to trigger the FloatingObjectTree initialization.

This happens in the testcase because the initially assumed viewport scrollbars get removed (on platforms that have scrollbars that take up layout). Attaching a better testcase.

Deleted: tc3.html 713 bytes

tc3.html 713 bytes View Download

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

commit d97346ebbaff708023638756cb95373eb8f63b22
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Tue Nov 20 12:28:31 2018

[LayoutNG] Don't mark floats as "placed".

Being "placed" means that they should be added to the
FloatingObjectTree, which is really only used consistently by the legacy
engine. NG only messes with it by accident. Have LayoutNG stay out of
that business more than before, by introducing a "has geometry" flag.

I really doubt that it's a splendid idea to populate the
FloatingObjectTree at all in LayoutNG formatting contexts, but it does
happen sometimes when laying out legacy objects in an NG container that
also has floats. See bug report for further analysis.

Bug:  906077 
Change-Id: Iacebf5b8ca64e16c954a25745c8c2b01a18e1440
Reviewed-on: https://chromium-review.googlesource.com/c/1341915
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Aleks Totic <atotic@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609671}
[add] https://crrev.com/d97346ebbaff708023638756cb95373eb8f63b22/third_party/WebKit/LayoutTests/fast/block/float/add-remove-float-after-table-relayout-crash.html
[modify] https://crrev.com/d97346ebbaff708023638756cb95373eb8f63b22/third_party/blink/renderer/core/layout/floating_objects.cc
[modify] https://crrev.com/d97346ebbaff708023638756cb95373eb8f63b22/third_party/blink/renderer/core/layout/floating_objects.h
[modify] https://crrev.com/d97346ebbaff708023638756cb95373eb8f63b22/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm_test.cc
[modify] https://crrev.com/d97346ebbaff708023638756cb95373eb8f63b22/third_party/blink/renderer/core/layout/ng/ng_block_node.cc