[FCP++] Image Paint Timing: node removed between callback registration and

invocation

In the current implementation, we assumed that whenever callback is invoked,
there must be at least one records needing to assign swap time to itself.
However, this ignored the possibily that nodes may be deleted between a callback
is registered and invoked.

Because the assumption is violated, we should remove the dcheck.

Bug: 869924
Change-Id: Ifdfda96fd2260f85add1c2a78ad48cff22cd0dbf
Reviewed-on: https://chromium-review.googlesource.com/c/1340966
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609388}

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

@@ -282,8 +282,8 @@ void ImagePaintTimingDetector::ReportSwapTime(
     base::TimeTicks timestamp) {
   // The callback is safe from race-condition only when running on main-thread.
   DCHECK(ThreadState::Current()->IsMainThread());
-  // Not guranteed to be non-empty, because record can be removed before the
-  // callback.
+  // Not guranteed to be non-empty, because records can be removed between
+  // callback registration and invocation.
   while (records_pending_timing_.size() > 0) {
     DOMNodeId node_id = records_pending_timing_.front();
     if (!id_record_map_.Contains(node_id)) {

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

@@ -231,6 +231,52 @@ TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_IgnoreTheRemoved) {
   EXPECT_EQ(LargestPaintStoredResult(), base::TimeTicks());
 }
 
+TEST_F(ImagePaintTimingDetectorTest,
+       LargestImagePaint_NodeRemovedBetweenRegistrationAndInvocation) {
+  SetBodyInnerHTML(R"HTML(
+    <div id="parent">
+      <img id="target"></img>
+    </div>
+  )HTML");
+  SetImageAndPaint("target", 5, 5);
+  UpdateAllLifecyclePhasesForTest();
+
+  GetDocument().getElementById("parent")->RemoveChild(
+      GetDocument().getElementById("target"));
+
+  InvokeCallback();
+
+  ImageRecord* record;
+  record = FindLargestPaintCandidate();
+  EXPECT_FALSE(record);
+}
+
+TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_IgnoreReAttached) {
+  SetBodyInnerHTML(R"HTML(
+    <div id="parent">
+    </div>
+  )HTML");
+  HTMLImageElement* image = HTMLImageElement::Create(GetDocument());
+  image->setAttribute("id", "target");
+  GetDocument().getElementById("parent")->AppendChild(image);
+  SetImageAndPaint("target", 5, 5);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  ImageRecord* record;
+  record = FindLargestPaintCandidate();
+  EXPECT_TRUE(record);
+
+  GetDocument().getElementById("parent")->RemoveChild(image);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  record = FindLargestPaintCandidate();
+  EXPECT_FALSE(record);
+
+  GetDocument().getElementById("parent")->AppendChild(image);
+  SetImageAndPaint("target", 5, 5);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  record = FindLargestPaintCandidate();
+  EXPECT_TRUE(record);
+}
+
 // This test dipicts a situation when a smaller image has loaded, but a larger
 // image is loading. When we call analyze, the result will be empty because
 // we don't know when the largest image will finish loading. We wait until