[FCP++] Nodes with the same size shouldn't be ignored

This CL is to fix a bug in FCP++. FCP++ is currently using a set for ranking
the nodes' size. When two node sizes are the same, the set will ignore one of
them. This bug will manifest in the following scenario:
* A and B element have the same size, which is the largest on the page.
* When the set adds A and B, it ignores B implicitly.
* A is detached from the DOM.
* LCP reports a candidate. At this time, LCP should have reported B, but as B
is not in the ranking set, it's ignored by LCP as well.

Bug: 944248
Change-Id: I55d6856ff93191494a0bd46f7043b76c2f674b7d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1534303Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644802}

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

@@ -106,7 +106,11 @@ static bool LargeImageFirst(const base::WeakPtr<ImageRecord>& a,
                             const base::WeakPtr<ImageRecord>& b) {
   DCHECK(a);
   DCHECK(b);
-  return a->first_size > b->first_size;
+  if (a->first_size != b->first_size)
+    return a->first_size > b->first_size;
+  // This make sure that two different nodes with the same |first_size| wouldn't
+  // be merged in the set.
+  return a->node_id > b->node_id;
 }
 
 ImagePaintTimingDetector::ImagePaintTimingDetector(LocalFrameView* frame_view)
@@ -355,12 +359,9 @@ void ImagePaintTimingDetector::StopRecordEntries() {
 }
 
 ImageRecord* ImagePaintTimingDetector::FindLargestPaintCandidate() {
-  return FindCandidate(size_ordered_set_);
-}
-
-ImageRecord* ImagePaintTimingDetector::FindCandidate(
-    ImageRecordSet& ordered_set) {
-  for (auto it = ordered_set.begin(); it != ordered_set.end(); ++it) {
+  DCHECK_EQ(id_record_map_.size(), size_ordered_set_.size());
+  for (auto it = size_ordered_set_.begin(); it != size_ordered_set_.end();
+       ++it) {
     if (detached_ids_.Contains((*it)->node_id))
       continue;
     DCHECK(id_record_map_.Contains((*it)->node_id));

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

@@ -90,7 +90,6 @@ class CORE_EXPORT ImagePaintTimingDetector final
   void Trace(blink::Visitor*);
 
  private:
-  ImageRecord* FindCandidate(ImageRecordSet&);
   void PopulateTraceValue(TracedValue&,
                           const ImageRecord& first_image_paint,
                           unsigned report_count) const;

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

@@ -86,6 +86,12 @@ class ImagePaintTimingDetectorTest
         .id_record_map_.size();
   }
 
+  size_t CountRankingSetRecords() {
+    return GetPaintTimingDetector()
+        .GetImagePaintTimingDetector()
+        .size_ordered_set_.size();
+  }
+
   void Analyze() {
     return GetPaintTimingDetector().GetImagePaintTimingDetector().Analyze();
   }
@@ -214,17 +220,6 @@ TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_Largest) {
 
   SetImageAndPaint("larger", 9, 9);
   UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
-  // record = FindLargestPaintCandidate();
-  // EXPECT_TRUE(record);
-  // EXPECT_EQ(record->first_size, 81ul);
-  // EXPECT_TRUE(record->loaded);
-
-  // SetImageAndPaint("medium", 7, 7);
-  // UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
-  // record = FindLargestPaintCandidate();
-  // EXPECT_TRUE(record);
-  // EXPECT_EQ(record->first_size, 81ul);
-  // EXPECT_TRUE(record->loaded);
 }
 
 TEST_F(ImagePaintTimingDetectorTest,
@@ -586,4 +581,18 @@ TEST_F(ImagePaintTimingDetectorTest, Iframe_HalfClippedByMainFrameViewport) {
   EXPECT_LT(image->first_size, 100ul);
 }
 
+TEST_F(ImagePaintTimingDetectorTest, SameSizeShouldNotBeIgnored) {
+  SetBodyInnerHTML(R"HTML(
+    <style>img { display:block }</style>
+    <img id='1'></img>
+    <img id='2'></img>
+    <img id='3'></img>
+  )HTML");
+  SetImageAndPaint("1", 5, 5);
+  SetImageAndPaint("2", 5, 5);
+  SetImageAndPaint("3", 5, 5);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  EXPECT_EQ(CountRankingSetRecords(), 3u);
+}
+
 }  // namespace blink

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

@@ -28,7 +28,13 @@ constexpr size_t kTextNodeNumberLimit = 5000;
 
 static bool LargeTextFirst(const base::WeakPtr<TextRecord>& a,
                            const base::WeakPtr<TextRecord>& b) {
-  return a->first_size > b->first_size;
+  DCHECK(a);
+  DCHECK(b);
+  if (a->first_size != b->first_size)
+    return a->first_size > b->first_size;
+  // This make sure that two different nodes with the same |first_size| wouldn't
+  // be merged in the set.
+  return a->node_id > b->node_id;
 }
 
 TextPaintTimingDetector::TextPaintTimingDetector(LocalFrameView* frame_view)
@@ -281,7 +287,7 @@ TextRecord* TextRecordsManager::FindLargestPaintCandidate() {
   // TODO(crbug/944248): An identified bug here is that the records with the
   // same size will be silently ignored by |size_ordered_set_|. This is what
   // causes these two sizes to be different.
-  DCHECK_GE(visible_node_map.size(), size_ordered_set_.size());
+  DCHECK_EQ(visible_node_map.size(), size_ordered_set_.size());
   if (!is_result_invalidated_)
     return cached_largest_paint_candidate_;
   TextRecord* new_largest_paint_candidate = nullptr;

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

@@ -46,6 +46,12 @@ class TextPaintTimingDetectorTest
                .records_manager_.detached_ids_.size();
   }
 
+  unsigned CountRankingSetSize() {
+    return GetPaintTimingDetector()
+        .GetTextPaintTimingDetector()
+        .records_manager_.size_ordered_set_.size();
+  }
+
   unsigned CountDetachedTexts() {
     return GetPaintTimingDetector()
         .GetTextPaintTimingDetector()
@@ -510,4 +516,15 @@ TEST_F(TextPaintTimingDetectorTest, Iframe_ClippedByViewport) {
   EXPECT_EQ(CountPendingSwapTime(GetChildFrameView()), 0u);
 }
 
+TEST_F(TextPaintTimingDetectorTest, SameSizeShouldNotBeIgnored) {
+  SetBodyInnerHTML(R"HTML(
+    <div>text</div>
+    <div>text</div>
+    <div>text</div>
+    <div>text</div>
+  )HTML");
+  UpdateAllLifecyclePhasesAndSimulateSwapTime();
+  EXPECT_EQ(CountRankingSetSize(), 4u);
+}
+
 }  // namespace blink