Fix link highlight crash

Previously we updated fragments of link highlights during paint, and
called LocalFrameView::SetForeignLayerListNeedsUpdate() when we had
less fragments than before, but SetForeignLayerListNeedsUpdate()
should be called before paint because it deletes paint controller.

Now separate the work into a new step LinkHighlightImpl::UpdateAfterPrePaint()
to update the link highlight fragments and call
SetForeignLayerListNeedsUpdate() when the number of fragments changes.

Bug: 1021470
Change-Id: I4d81a522844e957aebcb406fb36044297872ab55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899317Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712809}

third_party/blink/renderer/core/frame/local_frame_view.cc

@@ -2453,11 +2453,9 @@ bool LocalFrameView::RunPrePaintLifecyclePhase(
     SCOPED_UMA_AND_UKM_TIMER(EnsureUkmAggregator(),
                              LocalFrameUkmAggregator::kPrePaint);
 
-    // This is before WalkTree because it may SetNeedsPaintPropertyUpdate() on
-    // layout objects.
-    GetPage()->GetLinkHighlight().UpdatePrePaint();
-
+    GetPage()->GetLinkHighlight().UpdateBeforePrePaint();
     PrePaintTreeWalk().WalkTree(*this);
+    GetPage()->GetLinkHighlight().UpdateAfterPrePaint();
   }
 
   UpdateCompositedSelectionIfNeeded();
@@ -4051,8 +4049,9 @@ void LocalFrameView::SetPaintArtifactCompositorNeedsUpdate() {
 
 void LocalFrameView::SetForeignLayerListNeedsUpdate() {
   DCHECK(!RuntimeEnabledFeatures::CompositeAfterPaintEnabled());
-  LocalFrameView* root = GetFrame().LocalFrameRoot().View();
-  if (root) {
+  DCHECK_NE(Lifecycle().GetState(), DocumentLifecycle::kInPaint);
+
+  if (LocalFrameView* root = GetFrame().LocalFrameRoot().View()) {
     // We will re-collect foreign layers in PushPaintArtifactsToCompositor().
     root->paint_controller_ = nullptr;
     if (root->paint_artifact_compositor_)

third_party/blink/renderer/core/page/link_highlight.cc

@@ -107,9 +107,14 @@ bool LinkHighlight::NeedsHighlightEffectInternal(
   return false;
 }
 
-void LinkHighlight::UpdatePrePaint() {
+void LinkHighlight::UpdateBeforePrePaint() {
   if (impl_)
-    impl_->UpdatePrePaint();
+    impl_->UpdateBeforePrePaint();
+}
+
+void LinkHighlight::UpdateAfterPrePaint() {
+  if (impl_)
+    impl_->UpdateAfterPrePaint();
 }
 
 void LinkHighlight::Paint(GraphicsContext& context) const {

third_party/blink/renderer/core/page/link_highlight.h

@@ -43,7 +43,8 @@ class CORE_EXPORT LinkHighlight final : public GarbageCollected<LinkHighlight> {
     return impl_ && NeedsHighlightEffectInternal(object);
   }
 
-  void UpdatePrePaint();
+  void UpdateBeforePrePaint();
+  void UpdateAfterPrePaint();
   void Paint(GraphicsContext&) const;
 
  private:

third_party/blink/renderer/core/paint/link_highlight_impl.cc

@@ -232,12 +232,31 @@ void LinkHighlightImpl::NotifyAnimationFinished(double, int) {
   UpdateOpacity(kStartOpacity);
 }
 
-void LinkHighlightImpl::UpdatePrePaint() {
+void LinkHighlightImpl::UpdateBeforePrePaint() {
   if (!node_ || !node_->GetLayoutObject() ||
       node_->GetLayoutObject()->GetFrameView()->ShouldThrottleRendering())
     ReleaseResources();
 }
 
+void LinkHighlightImpl::UpdateAfterPrePaint() {
+  if (!node_)
+    return;
+
+  const auto* object = node_->GetLayoutObject();
+  DCHECK(object);
+  DCHECK(!object->GetFrameView()->ShouldThrottleRendering());
+
+  size_t fragment_count = 0;
+  for (const auto* fragment = &object->FirstFragment(); fragment;
+       fragment = fragment->NextFragment())
+    ++fragment_count;
+
+  if (fragment_count != fragments_.size()) {
+    fragments_.resize(fragment_count);
+    SetPaintArtifactCompositorNeedsUpdate();
+  }
+}
+
 CompositorAnimation* LinkHighlightImpl::GetCompositorAnimation() const {
   return compositor_animation_.get();
 }
@@ -279,9 +298,7 @@ void LinkHighlightImpl::Paint(GraphicsContext& context) {
         new_path.AddRect(snapped_rect);
     }
 
-    if (index == fragments_.size())
-      fragments_.emplace_back();
-
+    DCHECK_LT(index, fragments_.size());
     auto& link_highlight_fragment = fragments_[index];
     link_highlight_fragment.SetColor(color);
 
@@ -301,12 +318,7 @@ void LinkHighlightImpl::Paint(GraphicsContext& context) {
                        bounding_rect.Location(), property_tree_state);
   }
 
-  if (index < fragments_.size()) {
-    fragments_.Shrink(index);
-    // PaintArtifactCompositor needs update for the cc::PictureLayers we just
-    // removed for the extra fragments.
-    SetPaintArtifactCompositorNeedsUpdate();
-  }
+  DCHECK_EQ(index, fragments_.size());
 }
 
 void LinkHighlightImpl::SetPaintArtifactCompositorNeedsUpdate() {

third_party/blink/renderer/core/paint/link_highlight_impl.h

@@ -72,7 +72,8 @@ class CORE_EXPORT LinkHighlightImpl final : public CompositorAnimationDelegate,
 
   const EffectPaintPropertyNode& Effect() const { return *effect_; }
 
-  void UpdatePrePaint();
+  void UpdateBeforePrePaint();
+  void UpdateAfterPrePaint();
   void Paint(GraphicsContext&);
 
   wtf_size_t FragmentCountForTesting() const { return fragments_.size(); }

third_party/blink/renderer/core/paint/link_highlight_impl_test.cc

@@ -354,6 +354,33 @@ TEST_P(LinkHighlightImplTest, MultiColumn) {
   check_layer(highlight->LayerForTesting(0));
   check_layer(highlight->LayerForTesting(1));
 
+  Element* multicol = touch_node->parentElement();
+  EXPECT_EQ(50, multicol->OffsetHeight());
+  // Make multicol shorter to create 3 total columns for touch_node.
+  multicol->setAttribute(html_names::kStyleAttr, "height: 25px");
+  UpdateAllLifecyclePhases();
+  ASSERT_EQ(&first_fragment, &touch_node->GetLayoutObject()->FirstFragment());
+  ASSERT_EQ(second_fragment, first_fragment.NextFragment());
+  const auto* third_fragment = second_fragment->NextFragment();
+  ASSERT_TRUE(third_fragment);
+  EXPECT_FALSE(third_fragment->NextFragment());
+
+  EXPECT_EQ(layer_count_before_highlight + 3, ContentLayerCount());
+  EXPECT_EQ(3u, highlight->FragmentCountForTesting());
+  check_layer(highlight->LayerForTesting(0));
+  check_layer(highlight->LayerForTesting(1));
+  check_layer(highlight->LayerForTesting(2));
+
+  // Make multicol taller to create only 1 column for touch_node.
+  multicol->setAttribute(html_names::kStyleAttr, "height: 100px");
+  UpdateAllLifecyclePhases();
+  ASSERT_EQ(&first_fragment, &touch_node->GetLayoutObject()->FirstFragment());
+  EXPECT_FALSE(first_fragment.NextFragment());
+
+  EXPECT_EQ(layer_count_before_highlight + 1, ContentLayerCount());
+  EXPECT_EQ(1u, highlight->FragmentCountForTesting());
+  check_layer(highlight->LayerForTesting(0));
+
   touch_node->remove(IGNORE_EXCEPTION_FOR_TESTING);
   UpdateAllLifecyclePhases();
   // Removing the highlight layer should drop the cc layers for highlights.

third_party/blink/renderer/core/testing/data/test_touch_link_highlight.html

@@ -29,10 +29,14 @@
     <div style="position: absolute; left: 20px; top: 260px; width: 200px;">
         <input type="text"> <!-- This will have an I-beam cursor. -->
     </div>
-    <div style="position: absolute; left: 20px; top: 300px; width: 400px; height: 50px; columns: 2">
+    <style>
+      #multicol { position: absolute; left: 20px; top: 300px; width: 400px; height: 50px; columns: 2; column-fill: auto; }
+    </style>
+    <div id="multicol">
       <a href="http://www.test.com">
-        <div style="display: inline-block; width: 200px; height: 50px">Link in column 1</div>
-        <div style="display: inline-block; width: 200px; height: 50px">Link in column 2</div>
+        <div style="display: inline-block; width: 200px; height: 25px">Link part 1</div>
+        <div style="display: inline-block; width: 200px; height: 25px">Link part 2</div>
+        <div style="display: inline-block; width: 200px; height: 25px">Link part 3</div>
       </a>
     </div>
   </body>