[FCP++] TextPaint: inconsistency for node removal while pending swap time

In the current implementation, it's possible to create inconsistency while
a node waiting to be assigned with a swap time is removed. When this happens,
the detector still creates a record of the removed text, but it shouldn't, as
the created record could become the candidate metric result.

The cause is that when a record is assigned with a swap time, there is no check
of whether the node has been removed.

To fix this issue, we should mark the node as deleted when the node is removed
from the dom tree, and add the checking of validity when we assign the swap time
to the record.

The CL implements this fix.

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

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

@@ -117,6 +117,10 @@ void TextPaintTimingDetector::OnPrePaintFinished() {
 }
 
 void TextPaintTimingDetector::NotifyNodeRemoved(DOMNodeId node_id) {
+  for (TextRecord& record : texts_to_record_swap_time_) {
+    if (record.node_id == node_id)
+      record.node_id = kInvalidDOMNodeId;
+  }
   if (recorded_text_node_ids_.find(node_id) == recorded_text_node_ids_.end())
     return;
   // We assume that removed nodes' id would not be recycled, and it's expensive
@@ -161,6 +165,8 @@ void TextPaintTimingDetector::ReportSwapTime(
   // that only one or zero callback will be called after one OnPrePaintFinished.
   DCHECK_GT(texts_to_record_swap_time_.size(), 0UL);
   for (TextRecord& record : texts_to_record_swap_time_) {
+    if (record.node_id == kInvalidDOMNodeId)
+      continue;
     record.first_paint_time = timestamp;
     recorded_text_node_ids_.insert(record.node_id);
     largest_text_heap_.push(std::make_unique<TextRecord>(record));

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

@@ -24,6 +24,19 @@ class TextPaintTimingDetectorTest
     return GetFrameView().GetPaintTimingDetector();
   }
 
+  unsigned CountRecords() {
+    return GetPaintTimingDetector()
+        .GetTextPaintTimingDetector()
+        .recorded_text_node_ids_.size();
+  }
+
+  void InvokeCallback() {
+    TextPaintTimingDetector& detector =
+        GetPaintTimingDetector().GetTextPaintTimingDetector();
+    detector.ReportSwapTime(WebLayerTreeView::SwapResult::kDidSwap,
+                            CurrentTimeTicks());
+  }
+
   TimeTicks LargestPaintStoredResult() {
     return GetPaintTimingDetector()
         .GetTextPaintTimingDetector()
@@ -75,6 +88,19 @@ TEST_F(TextPaintTimingDetectorTest, LargestTextPaint_OneText) {
   EXPECT_EQ(record->text, "The only text");
 }
 
+TEST_F(TextPaintTimingDetectorTest, NodeRemovedBeforeAssigningSwapTime) {
+  SetBodyInnerHTML(R"HTML(
+    <div id="parent">
+      <div id="remove">The only text</div>
+    </div>
+  )HTML");
+  UpdateAllLifecyclePhasesForTest();
+  GetDocument().getElementById("parent")->RemoveChild(
+      GetDocument().getElementById("remove"));
+  InvokeCallback();
+  EXPECT_EQ(CountRecords(), 0u);
+}
+
 TEST_F(TextPaintTimingDetectorTest, LargestTextPaint_LargestText) {
   SetBodyInnerHTML(R"HTML(
     <div>medium text</div>