[LayoutNG] Clear layout results when the layout wasn't successful.

Today we can end up in a state where the layout result cache for a
LayoutBox contains a fragment tree which doesn't match the layout tree
state.

This occurs when we "abort" layout, i.e.
1) during the first pass children are positioned at a particular offset
2) a parent aborts layout due to resolving a BFC offset
3) the parent hits the layout result cache which has children at a
   different offset

Bug: 1003558
Change-Id: Iabc5dfefd74c517222ec435cea376ad000a0372f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1821674Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702507}

third_party/blink/renderer/core/layout/layout_box.cc

@@ -2304,20 +2304,21 @@ void LayoutBox::InLayoutNGInlineFormattingContextWillChange(bool new_value) {
 
 void LayoutBox::SetCachedLayoutResult(const NGLayoutResult& layout_result,
                                       const NGBreakToken* break_token) {
+  DCHECK_EQ(layout_result.Status(), NGLayoutResult::kSuccess);
+  DCHECK(!layout_result.GetConstraintSpaceForCaching().IsIntermediateLayout());
+
   if (break_token)
     return;
-  if (layout_result.Status() != NGLayoutResult::kSuccess)
-    return;
-  if (!layout_result.HasValidConstraintSpaceForCaching())
-    return;
-  if (layout_result.GetConstraintSpaceForCaching().IsIntermediateLayout())
-    return;
   if (layout_result.PhysicalFragment().BreakToken())
     return;
 
   cached_layout_result_ = &layout_result;
 }
 
+void LayoutBox::ClearCachedLayoutResult() {
+  cached_layout_result_ = nullptr;
+}
+
 scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
     const NGConstraintSpace& new_space,
     const NGBreakToken* break_token,
@@ -2340,6 +2341,8 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
   if (early_break)
     return nullptr;
 
+  DCHECK_EQ(cached_layout_result->Status(), NGLayoutResult::kSuccess);
+
   // Set our initial temporary cache status to "hit".
   NGLayoutCacheStatus cache_status = NGLayoutCacheStatus::kHit;
 
@@ -2473,10 +2476,6 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
   // not clear the child dirty bits.
   ClearNeedsLayout();
 
-  // The checks above should be enough to bail if layout is incomplete, but
-  // let's verify:
-  DCHECK(IsBlockLayoutComplete(old_space, *cached_layout_result));
-
   // OOF-positioned nodes have to two-tier cache. The additional cache check
   // runs before the OOF-positioned sizing, and positioning calculations.
   //

third_party/blink/renderer/core/layout/layout_box.h

@@ -953,6 +953,7 @@ class CORE_EXPORT LayoutBox : public LayoutBoxModelObject {
   void SetFirstInlineFragment(NGPaintFragment*) final;
 
   void SetCachedLayoutResult(const NGLayoutResult&, const NGBreakToken*);
+  void ClearCachedLayoutResult();
   const NGLayoutResult* GetCachedLayoutResult() const {
     return cached_layout_result_.get();
   }

third_party/blink/renderer/core/layout/ng/ng_block_node.cc

@@ -480,11 +480,20 @@ void NGBlockNode::FinishLayout(
     const NGConstraintSpace& constraint_space,
     const NGBreakToken* break_token,
     scoped_refptr<const NGLayoutResult> layout_result) {
-  if (!IsBlockLayoutComplete(constraint_space, *layout_result))
+  if (constraint_space.IsIntermediateLayout())
+    return;
+
+  // If we abort layout and don't clear the cached layout-result, we can end
+  // up in a state where the layout-object tree doesn't match fragment tree
+  // referenced by this layout-result.
+  if (layout_result->Status() != NGLayoutResult::kSuccess) {
+    box_->ClearCachedLayoutResult();
     return;
+  }
 
   if (!constraint_space.HasBlockFragmentation())
     box_->SetCachedLayoutResult(*layout_result, break_token);
+
   if (block_flow) {
     auto* child = GetLayoutObjectForFirstChildNode(block_flow);
     bool has_inline_children =
@@ -1174,7 +1183,8 @@ scoped_refptr<const NGLayoutResult> NGBlockNode::RunLegacyLayout(
     CopyBaselinesFromLegacyLayout(constraint_space, &builder);
     layout_result = builder.ToBoxFragment();
 
-    box_->SetCachedLayoutResult(*layout_result, /* break_token */ nullptr);
+    if (!constraint_space.IsIntermediateLayout())
+      box_->SetCachedLayoutResult(*layout_result, /* break_token */ nullptr);
 
     // If |SetCachedLayoutResult| did not update cached |LayoutResult|,
     // |NeedsLayout()| flag should not be cleared.

third_party/blink/renderer/core/layout/ng/ng_layout_result.cc

@@ -27,6 +27,10 @@ struct SameSizeAsNGLayoutResult : public RefCounted<SameSizeAsNGLayoutResult> {
   };
   LayoutUnit intrinsic_block_size;
   unsigned bitfields[1];
+
+#if DCHECK_IS_ON()
+  bool has_valid_space;
+#endif
 };
 
 static_assert(sizeof(NGLayoutResult) == sizeof(SameSizeAsNGLayoutResult),
@@ -38,8 +42,7 @@ NGLayoutResult::NGLayoutResult(
     scoped_refptr<const NGPhysicalContainerFragment> physical_fragment,
     NGBoxFragmentBuilder* builder)
     : NGLayoutResult(std::move(physical_fragment),
-                     builder,
-                     /* cache_space */ true) {
+                     static_cast<NGContainerFragmentBuilder*>(builder)) {
   bitfields_.is_initial_block_size_indefinite =
       builder->is_initial_block_size_indefinite_;
   bitfields_.subtree_modified_margin_strut =
@@ -64,13 +67,11 @@ NGLayoutResult::NGLayoutResult(
     scoped_refptr<const NGPhysicalContainerFragment> physical_fragment,
     NGLineBoxFragmentBuilder* builder)
     : NGLayoutResult(std::move(physical_fragment),
-                     builder,
-                     /* cache_space */ false) {}
+                     static_cast<NGContainerFragmentBuilder*>(builder)) {}
 
 NGLayoutResult::NGLayoutResult(EStatus status, NGBoxFragmentBuilder* builder)
     : NGLayoutResult(/* physical_fragment */ nullptr,
-                     builder,
-                     /* cache_space */ false) {
+                     static_cast<NGContainerFragmentBuilder*>(builder)) {
   bitfields_.status = status;
   DCHECK_NE(status, kSuccess)
       << "Use the other constructor for successful layout";
@@ -113,17 +114,19 @@ NGLayoutResult::NGLayoutResult(const NGLayoutResult& other,
 
   if (new_end_margin_strut != NGMarginStrut() || HasRareData())
     EnsureRareData()->end_margin_strut = new_end_margin_strut;
+
+#if DCHECK_IS_ON()
+  has_valid_space_ = other.has_valid_space_;
+#endif
 }
 
 NGLayoutResult::NGLayoutResult(
     scoped_refptr<const NGPhysicalContainerFragment> physical_fragment,
-    NGContainerFragmentBuilder* builder,
-    bool cache_space)
+    NGContainerFragmentBuilder* builder)
     : space_(builder->space_ ? NGConstraintSpace(*builder->space_)
                              : NGConstraintSpace()),
       physical_fragment_(std::move(physical_fragment)),
       bitfields_(
-          /* has_valid_space */ cache_space && builder->space_,
           /* is_self_collapsing */ builder->is_self_collapsing_,
           /* is_pushed_by_floats */ builder->is_pushed_by_floats_,
           /* adjoining_object_types */ builder->adjoining_object_types_,
@@ -177,6 +180,10 @@ NGLayoutResult::NGLayoutResult(
     bitfields_.is_bfc_block_offset_nullopt =
         !builder->bfc_block_offset_.has_value();
   }
+
+#if DCHECK_IS_ON()
+  has_valid_space_ = builder->space_;
+#endif
 }
 
 NGLayoutResult::~NGLayoutResult() {
@@ -231,7 +238,6 @@ void NGLayoutResult::CheckSameForSimplifiedLayout(
   DCHECK(EndMarginStrut() == other.EndMarginStrut());
   DCHECK_EQ(MinimalSpaceShortage(), other.MinimalSpaceShortage());
 
-  DCHECK_EQ(bitfields_.has_valid_space, other.bitfields_.has_valid_space);
   DCHECK_EQ(bitfields_.has_forced_break, other.bitfields_.has_forced_break);
   DCHECK_EQ(bitfields_.is_self_collapsing, other.bitfields_.is_self_collapsing);
   DCHECK_EQ(bitfields_.is_pushed_by_floats,

third_party/blink/renderer/core/layout/ng/ng_layout_result.h

@@ -205,14 +205,11 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     return bitfields_.subtree_modified_margin_strut;
   }
 
-  // Returns true if the space stored with this layout result, is valid.
-  bool HasValidConstraintSpaceForCaching() const {
-    return bitfields_.has_valid_space;
-  }
-
   // Returns the space which generated this object for caching purposes.
   const NGConstraintSpace& GetConstraintSpaceForCaching() const {
-    DCHECK(bitfields_.has_valid_space);
+#if DCHECK_IS_ON()
+    DCHECK(has_valid_space_);
+#endif
     return space_;
   }
 
@@ -287,8 +284,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
   // Delegate constructor that sets up what it can, based on the builder.
   NGLayoutResult(
       scoped_refptr<const NGPhysicalContainerFragment> physical_fragment,
-      NGContainerFragmentBuilder* builder,
-      bool cache_space);
+      NGContainerFragmentBuilder* builder);
 
   static NGExclusionSpace MergeExclusionSpaces(
       const NGLayoutResult& other,
@@ -330,14 +326,12 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     // never uninitialized (potentially allowing a dangling pointer).
     Bitfields()
         : Bitfields(
-              /* has_valid_space */ false,
               /* is_self_collapsing */ false,
               /* is_pushed_by_floats */ false,
               /* adjoining_object_types */ kAdjoiningNone,
               /* has_descendant_that_depends_on_percentage_block_size */
               false) {}
-    Bitfields(bool has_valid_space,
-              bool is_self_collapsing,
+    Bitfields(bool is_self_collapsing,
               bool is_pushed_by_floats,
               NGAdjoiningObjectTypes adjoining_object_types,
               bool has_descendant_that_depends_on_percentage_block_size)
@@ -345,7 +339,6 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
           has_rare_data_exclusion_space(false),
           has_oof_positioned_offset(false),
           is_bfc_block_offset_nullopt(false),
-          has_valid_space(has_valid_space),
           has_forced_break(false),
           is_self_collapsing(is_self_collapsing),
           is_pushed_by_floats(is_pushed_by_floats),
@@ -363,7 +356,6 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     unsigned has_oof_positioned_offset : 1;
     unsigned is_bfc_block_offset_nullopt : 1;
 
-    unsigned has_valid_space : 1;
     unsigned has_forced_break : 1;
 
     unsigned is_self_collapsing : 1;
@@ -382,7 +374,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
   };
 
   // The constraint space which generated this layout result, may not be valid
-  // as indicated by |Bitfields::has_valid_space|.
+  // as indicated by |has_valid_space_|.
   const NGConstraintSpace space_;
 
   scoped_refptr<const NGPhysicalContainerFragment> physical_fragment_;
@@ -406,6 +398,10 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
 
   LayoutUnit intrinsic_block_size_;
   Bitfields bitfields_;
+
+#if DCHECK_IS_ON()
+  bool has_valid_space_ = false;
+#endif
 };
 
 }  // namespace blink

third_party/blink/renderer/core/layout/ng/ng_layout_utils.cc

@@ -310,7 +310,6 @@ NGLayoutCacheStatus CalculateSizeBasedLayoutCacheStatus(
     const NGConstraintSpace& new_space,
     base::Optional<NGFragmentGeometry>* fragment_geometry) {
   DCHECK_EQ(cached_layout_result.Status(), NGLayoutResult::kSuccess);
-  DCHECK(cached_layout_result.HasValidConstraintSpaceForCaching());
 
   const NGConstraintSpace& old_space =
       cached_layout_result.GetConstraintSpaceForCaching();
@@ -346,7 +345,6 @@ bool MaySkipLegacyLayout(const NGBlockNode& node,
                          const NGLayoutResult& cached_layout_result,
                          const NGConstraintSpace& new_space) {
   DCHECK_EQ(cached_layout_result.Status(), NGLayoutResult::kSuccess);
-  DCHECK(cached_layout_result.HasValidConstraintSpaceForCaching());
 
   const NGConstraintSpace& old_space =
       cached_layout_result.GetConstraintSpaceForCaching();
@@ -372,7 +370,6 @@ bool MaySkipLayoutWithinBlockFormattingContext(
     LayoutUnit* block_offset_delta,
     NGMarginStrut* end_margin_strut) {
   DCHECK_EQ(cached_layout_result.Status(), NGLayoutResult::kSuccess);
-  DCHECK(cached_layout_result.HasValidConstraintSpaceForCaching());
   DCHECK(bfc_block_offset);
   DCHECK(block_offset_delta);
   DCHECK(end_margin_strut);
@@ -544,13 +541,4 @@ bool MaySkipLayoutWithinBlockFormattingContext(
   return true;
 }
 
-bool IsBlockLayoutComplete(const NGConstraintSpace& space,
-                           const NGLayoutResult& result) {
-  if (result.Status() != NGLayoutResult::kSuccess)
-    return false;
-  if (space.IsIntermediateLayout())
-    return false;
-  return true;
-}
-
 }  // namespace blink

third_party/blink/renderer/core/layout/ng/ng_layout_utils.h

@@ -62,12 +62,6 @@ bool MaySkipLayoutWithinBlockFormattingContext(
     LayoutUnit* block_offset_delta,
     NGMarginStrut* end_margin_strut);
 
-// Return true if layout is considered complete. In some cases we require more
-// than one layout pass.
-// This function never considers intermediate layouts with
-// |NGConstraintSpace::IsIntermediateLayout| to be complete.
-bool IsBlockLayoutComplete(const NGConstraintSpace&, const NGLayoutResult&);
-
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_NG_LAYOUT_UTILS_H_

third_party/blink/web_tests/external/wpt/css/CSS2/floats/adjoining-floats-dynamic.html

+<!DOCTYPE html>
+<link rel="match" href="../../reference/ref-filled-green-100px-square.xht">
+<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1003558">
+<style>
+#target {
+  height: 50px;
+  width: 0;
+  background: green;
+  float: right;
+}
+
+span {
+  float: left;
+  width: 50px;
+  height: 50px;
+  background: green;
+}
+</style>
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="display: flow-root; width: 100px; background: red;">
+  <div id="target"></div>
+  <div style="position: absolute; width: 50px; height: 50px; background: green;"></div>
+  <div>
+    <div style="clear: both; height: 10px;">
+      <div>
+        <span></span>
+        <span></span>
+      </div>
+    </div>
+  </div>
+</div>
+<script>
+document.body.offsetTop;
+document.getElementById('target').style.width = '50px';
+</script>