Remove LayoutBox::ClearLayoutResults().

Just be sure to miss the cache afterwards, when we reach any of the
former call sites of ClearLayoutResults().

Also just remove some code from a web view test, which used to call
ClearLayoutResults(). The issue wasn't fully analyzed, and the code
where it crashed (SubtreeLayoutScope) has since been removed.

Clearing the layout results seems problematic. It was responsible for
the crbug.com/1443633 crash, although the root cause was found to be
something else.

Change-Id: I3d1e4a83bf882a836670546d54cbac5e3d3e806d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4577201
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1153754}

third_party/blink/renderer/core/exported/web_view_test.cc

@@ -4957,15 +4957,6 @@ TEST_F(WebViewTest, PreferredSizeWithNGGridSkipped) {
                                    )HTML",
                                      base_url);
 
-  LocalFrame* main_frame = web_view->MainFrameImpl()->GetFrame();
-  Document* document = main_frame->GetDocument();
-  Element* element = document->getElementById("target");
-
-  // Note: Not entirely clear how we get into this state in release builds, we
-  // *should* be layout clean and have cached results for the preferred size
-  // query. See https://crbug.com/1245654 for how we saw this issue in the wild.
-  element->GetLayoutBox()->ClearLayoutResults();
-
   gfx::Size size = web_view->ContentsPreferredMinimumSize();
   EXPECT_EQ(10, size.width());
   EXPECT_EQ(10, size.height());

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

@@ -3024,18 +3024,6 @@ void LayoutBox::FinalizeLayoutResults() {
     NGFragmentItems::FinalizeAfterLayout(layout_results_);
 }
 
-void LayoutBox::ClearLayoutResults() {
-  NOT_DESTROYED();
-  if (measure_result_)
-    InvalidateItems(*measure_result_);
-  measure_result_ = nullptr;
-
-  if (HasFragmentItems())
-    NGFragmentItems::ClearAssociatedFragments(this);
-
-  ShrinkLayoutResults(0);
-}
-
 void LayoutBox::RebuildFragmentTreeSpine() {
   DCHECK(PhysicalFragmentCount());
   SCOPED_BLINK_UMA_HISTOGRAM_TIMER_HIGHRES(

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

@@ -984,8 +984,6 @@ class CORE_EXPORT LayoutBox : public LayoutBoxModelObject {
   // added.
   void FinalizeLayoutResults();
 
-  void ClearLayoutResults();
-
   void RebuildFragmentTreeSpine();
 
   // Call when NG fragment count or size changed. Only call if the fragment

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

@@ -74,6 +74,10 @@ const NGLayoutResult* LayoutBox::CachedLayoutResult(
   if (early_break)
     return nullptr;
 
+  if (ShouldSkipLayoutCache()) {
+    return nullptr;
+  }
+
   DCHECK_EQ(cached_layout_result->Status(), NGLayoutResult::kSuccess);
 
   // Set our initial temporary cache status to "hit".

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

@@ -3359,6 +3359,15 @@ class CORE_EXPORT LayoutObject : public GarbageCollected<LayoutObject>,
     bitfields_.SetScrollAnchorDisablingStyleChanged(changed);
   }
 
+  bool ShouldSkipLayoutCache() const {
+    NOT_DESTROYED();
+    return bitfields_.ShouldSkipLayoutCache();
+  }
+  void SetShouldSkipLayoutCache(bool b) {
+    NOT_DESTROYED();
+    bitfields_.SetShouldSkipLayoutCache(b);
+  }
+
   BackgroundPaintLocation GetBackgroundPaintLocation() const {
     NOT_DESTROYED();
     return static_cast<BackgroundPaintLocation>(background_paint_location_);
@@ -3942,6 +3951,7 @@ class CORE_EXPORT LayoutObject : public GarbageCollected<LayoutObject>,
           can_composite_background_attachment_fixed_(false),
           is_scroll_anchor_object_(false),
           scroll_anchor_disabling_style_changed_(false),
+          should_skip_layout_cache_(false),
           has_box_decoration_background_(false),
           background_needs_full_paint_invalidation_(true),
           outline_may_be_affected_by_descendants_(false),
@@ -4179,6 +4189,8 @@ class CORE_EXPORT LayoutObject : public GarbageCollected<LayoutObject>,
     ADD_BOOLEAN_BITFIELD(scroll_anchor_disabling_style_changed_,
                          ScrollAnchorDisablingStyleChanged);
 
+    ADD_BOOLEAN_BITFIELD(should_skip_layout_cache_, ShouldSkipLayoutCache);
+
     ADD_BOOLEAN_BITFIELD(has_box_decoration_background_,
                          HasBoxDecorationBackground);
 
@@ -4484,6 +4496,8 @@ inline void LayoutObject::ClearNeedsLayoutWithoutPaintInvalidation() {
 #endif
 
   SetScrollAnchorDisablingStyleChanged(false);
+
+  SetShouldSkipLayoutCache(false);
 }
 
 inline void LayoutObject::ClearNeedsLayout() {

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

@@ -545,8 +545,7 @@ const NGLayoutResult* NGBlockNode::Layout(
   //   or the available space to our children changing.
   // - A child changed scrollbars causing our size to change (shrink-to-fit).
   //
-  // Skip this part if side-effects aren't allowed, though. Calling
-  // ClearLayoutResults() when in this state is forbidden. Also skip it if we
+  // Skip this part if side-effects aren't allowed, though. Also skip it if we
   // are resuming layout after a fragmentainer break. Changing the intrinsic
   // inline-size halfway through layout of a node doesn't make sense.
   NGBoxStrut scrollbars_after = ComputeScrollbars(constraint_space, *this);
@@ -574,7 +573,7 @@ const NGLayoutResult* NGBlockNode::Layout(
       // incorrect if we try and reuse it.
       LayoutSize old_box_size = box_->Size();
       params.previous_result = nullptr;
-      box_->ClearLayoutResults();
+      box_->SetShouldSkipLayoutCache(true);
 
 #if DCHECK_IS_ON()
       // Ensure turning on/off scrollbars only once at most, when we call
@@ -831,16 +830,11 @@ void NGBlockNode::FinishLayout(LayoutBlockFlow* block_flow,
   if (NGDisableSideEffectsScope::IsDisabled())
     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) {
-    // This would be really dangerous to do if we're not at the first fragment,
-    // though, as it would mean that we'd also clear the first successful
-    // result(s).
-    DCHECK(!IsBreakInside(break_token));
-
-    box_->ClearLayoutResults();
+    // Layout aborted, but there may be results from a previous layout lying
+    // around. They are fine to keep, but since we aborted, it means that we
+    // want to attempt layout again. Be sure to miss the cache.
+    box_->SetShouldSkipLayoutCache(true);
     return;
   }
 

third_party/blink/renderer/core/layout/ng/table/layout_ng_table_cell.cc

@@ -39,10 +39,10 @@ void LayoutNGTableCell::InvalidateLayoutResultCacheAfterMeasure() const {
   NOT_DESTROYED();
   if (LayoutBox* row = ParentBox()) {
     DCHECK(row->IsTableRow());
-    row->ClearLayoutResults();
+    row->SetShouldSkipLayoutCache(true);
     if (LayoutBox* section = row->ParentBox()) {
       DCHECK(section->IsTableSection());
-      section->ClearLayoutResults();
+      section->SetShouldSkipLayoutCache(true);
     }
   }
 }