[FCP++] Text: Replace node_id with LayoutObject* - part 1

FCP++ is currently using node_id to track a layout object. But
we should use LayoutObject* instead to track the nodes. The replacement
will be more efficient and avoid the generation of node id.

After we use layout object* in text record, we start to get a risk of
UAF. UAF happens when we stop recording, block the LayoutObject
removal hook by is_recording_, and use LayoutObject* in
PopulateTraceValue. The fix to it is to clear up everything

We also noticed a risk of using |this| in two callback functions, but
it proved to be not a real risk in crbug.com/976357

|cached_largest_paint_candidate_| was raw pointer. It's better to be
a WeakPtr so no risk of becoming a dead pointer.

Note that this CL is mainly to replace node_id with LayoutObject* in
TextPaintTimingDetector's sets & maps. A subsequent patch will be made
to replace node with LayoutObject in the control logic of
TextPaintTimingDetector. And do the same thing for
ImagePaintTimingDetector.

Bug:967837

Change-Id: Iba0496cfca3f5056724c3267ec10226913d4e70e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1649467
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671360}

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

@@ -3156,7 +3156,7 @@ void LayoutObject::WillBeRemovedFromTree() {
   if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled() ||
       RuntimeEnabledFeatures::ElementTimingEnabled(&GetDocument())) {
     if (LocalFrameView* frame_view = GetFrameView()) {
-      frame_view->GetPaintTimingDetector().NotifyNodeRemoved(*this);
+      frame_view->GetPaintTimingDetector().LayoutObjectWillBeDestroyed(*this);
     }
   }
 }

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

@@ -153,7 +153,7 @@ void ImagePaintTimingDetector::OnPaintFinished() {
   RegisterNotifySwapTime();
 }
 
-void ImagePaintTimingDetector::NotifyNodeRemoved(DOMNodeId node_id) {
+void ImagePaintTimingDetector::LayoutObjectWillBeDestroyed(DOMNodeId node_id) {
   if (!is_recording_)
     return;
   // Todo: check whether it is visible background image.

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

@@ -212,7 +212,7 @@ class CORE_EXPORT ImagePaintTimingDetector final
       const ImageResourceContent& cached_image,
       const PropertyTreeState& current_paint_chunk_properties);
   void OnPaintFinished();
-  void NotifyNodeRemoved(DOMNodeId);
+  void LayoutObjectWillBeDestroyed(DOMNodeId);
   void NotifyBackgroundImageRemoved(DOMNodeId, const ImageResourceContent*);
   // After the method being called, the detector stops to record new entries and
   // node removal. But it still observe the loading status. In other words, if

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

@@ -31,11 +31,11 @@ void LargestContentfulPaintCalculator::OnLargestImageUpdated(
 }
 
 void LargestContentfulPaintCalculator::OnLargestTextUpdated(
-    const TextRecord* largest_text) {
+    base::WeakPtr<TextRecord> largest_text) {
   largest_text_.reset();
   if (largest_text) {
     largest_text_ = std::make_unique<TextRecord>(
-        kInvalidDOMNodeId, largest_text->first_size, FloatRect());
+        largest_text->node_id, largest_text->first_size, FloatRect());
     largest_text_->paint_time = largest_text->paint_time;
   }
 

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

@@ -21,7 +21,7 @@ class CORE_EXPORT LargestContentfulPaintCalculator final
 
   void OnLargestImageUpdated(const ImageRecord* largest_image);
 
-  void OnLargestTextUpdated(const TextRecord* largest_text);
+  void OnLargestTextUpdated(base::WeakPtr<TextRecord> largest_text);
 
   void Trace(blink::Visitor* visitor);
 

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

@@ -61,8 +61,10 @@ PaintTimingDetector::PaintTimingDetector(LocalFrameView* frame_view)
 void PaintTimingDetector::NotifyPaintFinished() {
   if (text_paint_timing_detector_) {
     text_paint_timing_detector_->OnPaintFinished();
-    if (text_paint_timing_detector_->FinishedReportingText())
+    if (text_paint_timing_detector_->FinishedReportingText()) {
+      text_paint_timing_detector_->StopRecordEntries();
       text_paint_timing_detector_ = nullptr;
+    }
   }
   if (image_paint_timing_detector_) {
     image_paint_timing_detector_->OnPaintFinished();
@@ -114,15 +116,17 @@ void PaintTimingDetector::NotifyImagePaint(
   }
 }
 
-void PaintTimingDetector::NotifyNodeRemoved(const LayoutObject& object) {
+void PaintTimingDetector::LayoutObjectWillBeDestroyed(
+    const LayoutObject& object) {
+  if (text_paint_timing_detector_)
+    text_paint_timing_detector_->LayoutObjectWillBeDestroyed(object);
+
   DOMNodeId node_id = DOMNodeIds::ExistingIdForNode(object.GetNode());
   if (node_id == kInvalidDOMNodeId)
     return;
 
-  if (text_paint_timing_detector_)
-    text_paint_timing_detector_->NotifyNodeRemoved(node_id);
   if (image_paint_timing_detector_)
-    image_paint_timing_detector_->NotifyNodeRemoved(node_id);
+    image_paint_timing_detector_->LayoutObjectWillBeDestroyed(node_id);
 }
 
 void PaintTimingDetector::NotifyBackgroundImageRemoved(
@@ -137,12 +141,7 @@ void PaintTimingDetector::NotifyBackgroundImageRemoved(
   }
 }
 
-void PaintTimingDetector::NotifyInputEvent(WebInputEvent::Type type) {
-  if (type == WebInputEvent::kMouseMove || type == WebInputEvent::kMouseEnter ||
-      type == WebInputEvent::kMouseLeave ||
-      WebInputEvent::IsPinchGestureEventType(type)) {
-    return;
-  }
+void PaintTimingDetector::StopRecordingIfNeeded() {
   DCHECK(frame_view_);
   if (text_paint_timing_detector_) {
     text_paint_timing_detector_->StopRecordingLargestTextPaint();
@@ -155,19 +154,19 @@ void PaintTimingDetector::NotifyInputEvent(WebInputEvent::Type type) {
     image_paint_timing_detector_->StopRecordEntries();
 }
 
+void PaintTimingDetector::NotifyInputEvent(WebInputEvent::Type type) {
+  if (type == WebInputEvent::kMouseMove || type == WebInputEvent::kMouseEnter ||
+      type == WebInputEvent::kMouseLeave ||
+      WebInputEvent::IsPinchGestureEventType(type)) {
+    return;
+  }
+  StopRecordingIfNeeded();
+}
+
 void PaintTimingDetector::NotifyScroll(ScrollType scroll_type) {
   if (scroll_type != kUserScroll && scroll_type != kCompositorScroll)
     return;
-  DCHECK(frame_view_);
-  if (text_paint_timing_detector_) {
-    text_paint_timing_detector_->StopRecordingLargestTextPaint();
-    if (!RuntimeEnabledFeatures::ElementTimingEnabled(
-            frame_view_->GetFrame().GetDocument())) {
-      text_paint_timing_detector_->StopRecordEntries();
-    }
-  }
-  if (image_paint_timing_detector_)
-    image_paint_timing_detector_->StopRecordEntries();
+  StopRecordingIfNeeded();
 }
 
 bool PaintTimingDetector::NeedToNotifyInputOrScroll() const {

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

@@ -49,7 +49,7 @@ class CORE_EXPORT PaintTimingDetector
       const PropertyTreeState& current_paint_chunk_properties);
   inline static void NotifyTextPaint(const IntRect& text_visual_rect);
 
-  void NotifyNodeRemoved(const LayoutObject&);
+  void LayoutObjectWillBeDestroyed(const LayoutObject&);
   void NotifyBackgroundImageRemoved(const LayoutObject&,
                                     const ImageResourceContent*);
   void NotifyPaintFinished();
@@ -83,6 +83,7 @@ class CORE_EXPORT PaintTimingDetector
   void Trace(Visitor* visitor);
 
  private:
+  void StopRecordingIfNeeded();
   bool HasLargestImagePaintChanged(base::TimeTicks, uint64_t size) const;
   bool HasLargestTextPaintChanged(base::TimeTicks, uint64_t size) const;
   Member<LocalFrameView> frame_view_;

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

@@ -40,9 +40,6 @@ class TextRecord : public base::SupportsWeakPtr<TextRecord> {
   FloatRect element_timing_rect_;
   // The time of the first paint after fully loaded.
   base::TimeTicks paint_time = base::TimeTicks();
-#ifndef NDEBUG
-  String text = "";
-#endif
   DISALLOW_COPY_AND_ASSIGN(TextRecord);
 };
 
@@ -56,40 +53,43 @@ class TextRecordsManager {
 
  public:
   TextRecordsManager();
-  TextRecord* FindLargestPaintCandidate();
+  base::WeakPtr<TextRecord> FindLargestPaintCandidate();
 
-  void RemoveVisibleRecord(const DOMNodeId&);
-  void RemoveInvisibleRecord(const DOMNodeId&);
-  inline void RecordInvisibleNode(const DOMNodeId& node_id) {
+  void RemoveVisibleRecord(const LayoutObject&);
+  void RemoveInvisibleRecord(const LayoutObject&);
+  inline void RecordInvisibleObject(const LayoutObject& object) {
     DCHECK(!HasTooManyNodes());
-    invisible_node_ids_.insert(node_id);
+    invisible_node_ids_.insert(&object);
   }
-  void RecordVisibleNode(const DOMNodeId&,
-                         const uint64_t& visual_size,
-                         const FloatRect& element_timing_rect,
-                         const LayoutObject&);
+  void RecordVisibleObject(const LayoutObject&,
+                           const uint64_t& visual_size,
+                           const FloatRect& element_timing_rect,
+                           DOMNodeId);
   bool NeedMeausuringPaintTime() const {
     return !texts_queued_for_paint_time_.IsEmpty();
   }
   void AssignPaintTimeToQueuedNodes(const base::TimeTicks&);
 
   bool HasTooManyNodes() const;
-  inline bool HasRecorded(const DOMNodeId& node_id) const {
-    return visible_node_map_.Contains(node_id) ||
-           invisible_node_ids_.Contains(node_id);
+  inline bool HasRecorded(const LayoutObject& object) const {
+    return visible_node_map_.Contains(&object) ||
+           invisible_node_ids_.Contains(&object);
   }
 
-  size_t CountVisibleNodes() const { return visible_node_map_.size(); }
-  size_t CountInvisibleNodes() const { return invisible_node_ids_.size(); }
+  size_t CountVisibleObjects() const { return visible_node_map_.size(); }
+  size_t CountInvisibleObjects() const { return invisible_node_ids_.size(); }
 
-  inline bool IsKnownVisibleNode(const DOMNodeId& node_id) const {
-    return visible_node_map_.Contains(node_id);
+  inline bool IsKnownVisible(const LayoutObject& object) const {
+    return visible_node_map_.Contains(&object);
   }
 
-  inline bool IsKnownInvisibleNode(const DOMNodeId& node_id) const {
-    return invisible_node_ids_.Contains(node_id);
+  inline bool IsKnownInvisible(const LayoutObject& object) const {
+    return invisible_node_ids_.Contains(&object);
   }
 
+  void CleanUp();
+
+  void CleanUpLargestContentfulPaint();
   void StopRecordingLargestTextPaint();
   bool IsRecordingLargestTextPaint() const { return is_recording_ltp_; }
 
@@ -113,14 +113,15 @@ class TextRecordsManager {
   // Timing.
   bool is_recording_ltp_ =
       RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled();
-  HashMap<DOMNodeId, std::unique_ptr<TextRecord>> visible_node_map_;
-  HashSet<DOMNodeId> invisible_node_ids_;
-  // This is used to order the nodes in |visible_node_map_| so that we can find
-  // the largest node efficiently. Note that the entries in |size_ordered_set_|
-  // and |visible_node_map_| should always be added/deleted together.
+  // Once LayoutObject* is destroyed, |visible_node_map_| and
+  // |invisible_node_ids_| must immediately clear the corresponding record from
+  // themselves.
+  HashMap<const LayoutObject*, std::unique_ptr<TextRecord>> visible_node_map_;
+  HashSet<const LayoutObject*> invisible_node_ids_;
+
   TextRecordSet size_ordered_set_;
   Deque<base::WeakPtr<TextRecord>> texts_queued_for_paint_time_;
-  TextRecord* cached_largest_paint_candidate_;
+  base::WeakPtr<TextRecord> cached_largest_paint_candidate_;
   Member<TextElementTiming> text_element_timing_;
 
   DISALLOW_COPY_AND_ASSIGN(TextRecordsManager);
@@ -156,8 +157,8 @@ class CORE_EXPORT TextPaintTimingDetector final
                             const IntRect& aggregated_visual_rect,
                             const PropertyTreeState&);
   void OnPaintFinished();
-  void NotifyNodeRemoved(DOMNodeId);
-  TextRecord* FindLargestPaintCandidate();
+  void LayoutObjectWillBeDestroyed(const LayoutObject&);
+  base::WeakPtr<TextRecord> FindLargestPaintCandidate();
   void StopRecordEntries();
   void StopRecordingLargestTextPaint();
   bool IsRecording() const { return is_recording_; }

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

@@ -42,16 +42,17 @@ class TextPaintTimingDetectorTest
 
   LocalFrameView& GetChildFrameView() { return *ChildFrame().View(); }
 
-  unsigned CountVisibleTexts() {
-    if (!GetPaintTimingDetector().GetTextPaintTimingDetector())
-      return 0u;
+  TextPaintTimingDetector* GetTextPaintTimingDetector() {
+    return GetPaintTimingDetector().GetTextPaintTimingDetector();
+  }
 
-    return GetPaintTimingDetector()
-        .GetTextPaintTimingDetector()
+  wtf_size_t CountVisibleTexts() {
+    DCHECK(GetTextPaintTimingDetector());
+    return GetTextPaintTimingDetector()
         ->records_manager_.visible_node_map_.size();
   }
 
-  unsigned CountRankingSetSize() {
+  wtf_size_t CountRankingSetSize() {
     return GetPaintTimingDetector()
         .GetTextPaintTimingDetector()
         ->records_manager_.size_ordered_set_.size();
@@ -118,14 +119,14 @@ class TextPaintTimingDetectorTest
     return div;
   }
 
-  TextRecord* TextRecordOfLargestTextPaint() {
+  base::WeakPtr<TextRecord> TextRecordOfLargestTextPaint() {
     return GetFrameView()
         .GetPaintTimingDetector()
         .GetTextPaintTimingDetector()
         ->FindLargestPaintCandidate();
   }
 
-  TextRecord* ChildFrameTextRecordOfLargestTextPaint() {
+  base::WeakPtr<TextRecord> ChildFrameTextRecordOfLargestTextPaint() {
     return GetChildFrameView()
         .GetPaintTimingDetector()
         .GetTextPaintTimingDetector()
@@ -282,19 +283,19 @@ TEST_F(TextPaintTimingDetectorTest, VisitSameNodeTwiceBeforePaintTimeIsSet) {
 
 TEST_F(TextPaintTimingDetectorTest, LargestTextPaint_ReportFirstPaintTime) {
   base::TimeTicks start_time = NowTicks();
-  AdvanceClock(base::TimeDelta::FromSecondsD(1));
+  AdvanceClock(TimeDelta::FromSecondsD(1));
   SetBodyInnerHTML(R"HTML(
   )HTML");
   Element* text = AppendDivElementToBody("text");
   UpdateAllLifecyclePhasesAndSimulateSwapTime();
-  AdvanceClock(base::TimeDelta::FromSecondsD(1));
+  AdvanceClock(TimeDelta::FromSecondsD(1));
   text->setAttribute(html_names::kStyleAttr,
                      AtomicString("position:fixed;left:30px"));
   UpdateAllLifecyclePhasesAndSimulateSwapTime();
-  AdvanceClock(base::TimeDelta::FromSecondsD(1));
-  TextRecord* record = TextRecordOfLargestTextPaint();
+  AdvanceClock(TimeDelta::FromSecondsD(1));
+  base::WeakPtr<TextRecord> record = TextRecordOfLargestTextPaint();
   EXPECT_TRUE(record);
-  EXPECT_EQ(record->paint_time, start_time + base::TimeDelta::FromSecondsD(1));
+  EXPECT_EQ(record->paint_time, start_time + TimeDelta::FromSecondsD(1));
 }
 
 TEST_F(TextPaintTimingDetectorTest,
@@ -438,11 +439,7 @@ TEST_F(TextPaintTimingDetectorTest, StopRecordingOverNodeLimit) {
   AppendDivElementToBody(WTF::String::Number(5000));
   UpdateAllLifecyclePhasesAndSimulateSwapTime();
   // Reached limit, so stopped recording and now should have 0 texts.
-  EXPECT_EQ(CountVisibleTexts(), 0u);
-
-  AppendDivElementToBody(WTF::String::Number(5001));
-  UpdateAllLifecyclePhasesAndSimulateSwapTime();
-  EXPECT_EQ(CountVisibleTexts(), 0u);
+  EXPECT_FALSE(GetTextPaintTimingDetector());
 }
 
 // This is for comparison with the ClippedByViewport test.
@@ -522,7 +519,7 @@ TEST_F(TextPaintTimingDetectorTest, Iframe) {
   UpdateAllLifecyclePhasesForTest();
   EXPECT_EQ(CountPendingSwapTime(GetChildFrameView()), 1u);
   ChildFrameSwapTimeCallBack();
-  TextRecord* text = ChildFrameTextRecordOfLargestTextPaint();
+  base::WeakPtr<TextRecord> text = ChildFrameTextRecordOfLargestTextPaint();
   EXPECT_TRUE(text);
 }