[Backport] Security bug 1472365 and 1472366

Manual backport of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/4821128:
Avoid allocating RecordId objects in ElementTiming and LCP

RecordId objects in current code keep around references to LayoutObject
and ImageResourceContent, both GCed objects.
Turn out that most of these references are not needed and are only used
as hashmap keys that would be better served with an actual hash. The
ones that are needed don't need extensive lifetimes and can be stack
allocated.

Bug=1472365,1472366

Change-Id: I3fd77bed9899932d5bfadc2a8e6403a8e434235f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4821128
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1190644}

Change-Id: I0d8bbd2c256a6a6ebb9a07b9df17f0d4608c42b0
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/514434
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

9 files changed, 119 insertions, 53 deletions

diff --git a/chromium/third_party/blink/renderer/core/paint/BUILD.gn b/chromium/third_party/blink/renderer/core/paint/BUILD.gn
index 3db4233ba20..8c0d149465d 100644
--- a/chromium/third_party/blink/renderer/core/paint/BUILD.gn
+++ b/chromium/third_party/blink/renderer/core/paint/BUILD.gn
@@ -121,6 +121,8 @@ blink_core_sources("paint") {
     "list_item_painter.h",
     "list_marker_painter.cc",
     "list_marker_painter.h",
+    "media_record_id.cc",
+    "media_record_id.h",
     "multi_column_set_painter.cc",
     "multi_column_set_painter.h",
     "ng/ng_box_fragment_painter.cc",
diff --git a/chromium/third_party/blink/renderer/core/paint/DEPS b/chromium/third_party/blink/renderer/core/paint/DEPS
index a58190f2032..377061397a3 100644
--- a/chromium/third_party/blink/renderer/core/paint/DEPS
+++ b/chromium/third_party/blink/renderer/core/paint/DEPS
@@ -4,6 +4,8 @@ include_rules = [
     "+cc/layers/picture_layer.h",
     # For DCHECK.
     "+base/logging.h",
+    # Hash function access
+    "+base/hash/hash.h",
 ]
 
 specific_include_rules = {
diff --git a/chromium/third_party/blink/renderer/core/paint/image_element_timing.cc b/chromium/third_party/blink/renderer/core/paint/image_element_timing.cc
index 18089bb7ace..9b745590ba7 100644
--- a/chromium/third_party/blink/renderer/core/paint/image_element_timing.cc
+++ b/chromium/third_party/blink/renderer/core/paint/image_element_timing.cc
@@ -67,7 +67,7 @@ void ImageElementTiming::NotifyImageFinished(
     return;
 
   const auto& insertion_result = images_notified_.insert(
-      std::make_pair(&layout_object, cached_image), ImageInfo());
+      MediaRecordId::GenerateHash(&layout_object, cached_image), ImageInfo());
   if (insertion_result.is_new_entry)
     insertion_result.stored_value->value.load_time_ = base::TimeTicks::Now();
 }
@@ -95,7 +95,7 @@ void ImageElementTiming::NotifyImagePainted(
   if (!internal::IsExplicitlyRegisteredForTiming(layout_object))
     return;
 
-  auto it = images_notified_.find(std::make_pair(layout_object, cached_image));
+  auto it = images_notified_.find(MediaRecordId::GenerateHash(layout_object, cached_image));
   // It is possible that the pair is not in |images_notified_|. See
   // https://crbug.com/1027948
   if (it != images_notified_.end() && !it->value.is_painted_ && cached_image) {
@@ -212,7 +212,7 @@ void ImageElementTiming::NotifyBackgroundImagePainted(
 
   ImageInfo& info =
       images_notified_
-          .insert(std::make_pair(layout_object, cached_image), ImageInfo())
+          .insert(MediaRecordId::GenerateHash(layout_object, cached_image), ImageInfo())
           .stored_value->value;
   if (!info.is_painted_) {
     info.is_painted_ = true;
@@ -240,7 +240,7 @@ void ImageElementTiming::ReportImagePaintSwapTime(WebSwapResult,
 
 void ImageElementTiming::NotifyImageRemoved(const LayoutObject* layout_object,
                                             const ImageResourceContent* image) {
-  images_notified_.erase(std::make_pair(layout_object, image));
+  images_notified_.erase(MediaRecordId::GenerateHash(layout_object, image));
 }
 
 void ImageElementTiming::Trace(Visitor* visitor) const {
diff --git a/chromium/third_party/blink/renderer/core/paint/image_element_timing.h b/chromium/third_party/blink/renderer/core/paint/image_element_timing.h
index 0dbb052fe43..753ba372517 100644
--- a/chromium/third_party/blink/renderer/core/paint/image_element_timing.h
+++ b/chromium/third_party/blink/renderer/core/paint/image_element_timing.h
@@ -5,11 +5,10 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_PAINT_IMAGE_ELEMENT_TIMING_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_PAINT_IMAGE_ELEMENT_TIMING_H_
 
-#include <utility>
-
 #include "third_party/blink/public/web/web_swap_result.h"
 #include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
+#include "third_party/blink/renderer/core/paint/media_record_id.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 #include "third_party/blink/renderer/platform/supplementable.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
@@ -121,13 +120,12 @@ class CORE_EXPORT ImageElementTiming final
     base::TimeTicks load_time_;
     bool is_painted_ = false;
   };
-  typedef std::pair<const LayoutObject*, const ImageResourceContent*> RecordId;
   // Hashmap of pairs of elements, LayoutObjects (for the elements) and
   // ImageResourceContent (for the src) which correspond to either images or
   // background images whose paint has been observed. For background images,
   // only the |is_painted_| bit is used, as the timestamp needs to be tracked by
   // |background_image_timestamps_|.
-  WTF::HashMap<RecordId, ImageInfo> images_notified_;
+  WTF::HashMap<MediaRecordIdHash, ImageInfo> images_notified_;
 
   // Hashmap of background images which contain information about the load time
   // of the background image.
diff --git a/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc b/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
index b92407aedb1..3c9f99034be 100644
--- a/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
+++ b/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
@@ -166,11 +166,11 @@ void ImagePaintTimingDetector::NotifyImageRemoved(
     const ImageResourceContent* cached_image) {
   if (!is_recording_)
     return;
-  RecordId record_id = std::make_pair(&object, cached_image);
-  records_manager_.RemoveImageFinishedRecord(record_id);
-  if (!records_manager_.IsRecordedVisibleImage(record_id))
+  MediaRecordIdHash record_id_hash = MediaRecordId::GenerateHash(&object, cached_image);
+  records_manager_.RemoveImageFinishedRecord(record_id_hash);
+  if (!records_manager_.IsRecordedVisibleImage(record_id_hash))
     return;
-  records_manager_.RemoveVisibleRecord(record_id);
+  records_manager_.RemoveVisibleRecord(record_id_hash);
   need_update_timing_at_frame_end_ = true;
 }
 
@@ -224,9 +224,11 @@ void ImagePaintTimingDetector::RecordImage(
   if (records_manager_.IsRecordedInvisibleImage(object))
     return;
 
-  RecordId record_id = std::make_pair(&object, &cached_image);
+  MediaRecordId record_id(&object, &cached_image);
+  MediaRecordIdHash record_id_hash = record_id.GetHash();
+
   bool is_recorded_visible_image =
-      records_manager_.IsRecordedVisibleImage(record_id);
+      records_manager_.IsRecordedVisibleImage(record_id_hash);
   if (int depth = IgnorePaintTimingScope::IgnoreDepth()) {
     // Record the largest loaded image that is hidden due to documentElement
     // being invisible but by no other reason (i.e. IgnoreDepth() needs to be
@@ -242,9 +244,9 @@ void ImagePaintTimingDetector::RecordImage(
   }
 
   if (is_recorded_visible_image &&
-      !records_manager_.IsVisibleImageLoaded(record_id) &&
+      !records_manager_.IsVisibleImageLoaded(record_id_hash) &&
       cached_image.IsLoaded()) {
-    records_manager_.OnImageLoaded(record_id, frame_index_, style_image);
+    records_manager_.OnImageLoaded(record_id_hash, frame_index_, style_image);
     need_update_timing_at_frame_end_ = true;
     if (base::Optional<PaintTimingVisualizer>& visualizer =
             frame_view_->GetPaintTimingDetector().Visualizer()) {
@@ -272,7 +274,7 @@ void ImagePaintTimingDetector::RecordImage(
   } else {
     records_manager_.RecordVisible(record_id, rect_size);
     if (cached_image.IsLoaded()) {
-      records_manager_.OnImageLoaded(record_id, frame_index_, style_image);
+      records_manager_.OnImageLoaded(record_id_hash, frame_index_, style_image);
       need_update_timing_at_frame_end_ = true;
     }
   }
@@ -306,8 +308,7 @@ uint64_t ImagePaintTimingDetector::ComputeImageRectSize(
 void ImagePaintTimingDetector::NotifyImageFinished(
     const LayoutObject& object,
     const ImageResourceContent* cached_image) {
-  RecordId record_id = std::make_pair(&object, cached_image);
-  records_manager_.NotifyImageFinished(record_id);
+  records_manager_.NotifyImageFinished(MediaRecordId::GenerateHash(&object, cached_image));
 }
 
 void ImagePaintTimingDetector::ReportLargestIgnoredImage() {
@@ -318,13 +319,13 @@ void ImagePaintTimingDetector::ReportLargestIgnoredImage() {
 ImageRecordsManager::ImageRecordsManager(LocalFrameView* frame_view)
     : size_ordered_set_(&LargeImageFirst), frame_view_(frame_view) {}
 
-void ImageRecordsManager::OnImageLoaded(const RecordId& record_id,
+void ImageRecordsManager::OnImageLoaded(MediaRecordIdHash record_id_hash,
                                         unsigned current_frame_index,
                                         const StyleFetchedImage* style_image) {
-  base::WeakPtr<ImageRecord> record = FindVisibleRecord(record_id);
+  base::WeakPtr<ImageRecord> record = FindVisibleRecord(record_id_hash);
   DCHECK(record);
   if (!style_image) {
-    record->load_time = image_finished_times_.at(record_id);
+    record->load_time = image_finished_times_.at(record_id_hash);
     DCHECK(!record->load_time.is_null());
   } else {
     Document* document = frame_view_->GetFrame().GetDocument();
@@ -348,10 +349,9 @@ void ImageRecordsManager::ReportLargestIgnoredImage(
     largest_ignored_image_.reset();
     return;
   }
-  RecordId record_id = std::make_pair(node->GetLayoutObject(),
-                                      largest_ignored_image_->cached_image);
+  MediaRecordId record_id(node->GetLayoutObject(), largest_ignored_image_->cached_image);
   size_ordered_set_.insert(record);
-  visible_images_.insert(record_id, std::move(largest_ignored_image_));
+  visible_images_.insert(record_id.GetHash(), std::move(largest_ignored_image_));
   OnImageLoadedInternal(record, current_frame_index);
 }
 
@@ -363,22 +363,22 @@ void ImageRecordsManager::OnImageLoadedInternal(
 }
 
 void ImageRecordsManager::MaybeUpdateLargestIgnoredImage(
-    const RecordId& record_id,
+    const MediaRecordId& record_id,
     const uint64_t& visual_size) {
   if (visual_size && (!largest_ignored_image_ ||
                       visual_size > largest_ignored_image_->first_size)) {
     largest_ignored_image_ =
-        CreateImageRecord(*record_id.first, record_id.second, visual_size);
+        CreateImageRecord(*record_id.GetLayoutObject(), record_id.GetImageResourceContent(), visual_size);
     largest_ignored_image_->load_time = base::TimeTicks::Now();
   }
 }
 
-void ImageRecordsManager::RecordVisible(const RecordId& record_id,
+void ImageRecordsManager::RecordVisible(const MediaRecordId& record_id,
                                         const uint64_t& visual_size) {
   std::unique_ptr<ImageRecord> record =
-      CreateImageRecord(*record_id.first, record_id.second, visual_size);
+      CreateImageRecord(*record_id.GetLayoutObject(), record_id.GetImageResourceContent(), visual_size);
   size_ordered_set_.insert(record->AsWeakPtr());
-  visible_images_.insert(record_id, std::move(record));
+  visible_images_.insert(record_id.GetHash(), std::move(record));
 }
 
 std::unique_ptr<ImageRecord> ImageRecordsManager::CreateImageRecord(
diff --git a/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.h b/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
index e1188dbf35c..41bc9e99433 100644
--- a/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
+++ b/chromium/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
@@ -13,6 +13,7 @@
 #include "third_party/blink/public/web/web_widget_client.h"
 #include "third_party/blink/renderer/core/dom/dom_node_ids.h"
 #include "third_party/blink/renderer/core/loader/resource/image_resource_content.h"
+#include "third_party/blink/renderer/core/paint/media_record_id.h"
 #include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
@@ -53,8 +54,6 @@ class ImageRecord : public base::SupportsWeakPtr<ImageRecord> {
   bool loaded = false;
 };
 
-typedef std::pair<const LayoutObject*, const ImageResourceContent*> RecordId;
-
 // |ImageRecordsManager| is the manager of all of the images that Largest Image
 // Paint cares about. Note that an image does not necessarily correspond to a
 // node; it can also be one of the background images attached to a node.
@@ -80,13 +79,13 @@ class CORE_EXPORT ImageRecordsManager {
     invisible_images_.erase(&object);
   }
 
-  inline void RemoveImageFinishedRecord(const RecordId& record_id) {
-    image_finished_times_.erase(record_id);
+  inline void RemoveImageFinishedRecord(MediaRecordIdHash record_id_hash) {
+    image_finished_times_.erase(record_id_hash);
   }
 
-  inline void RemoveVisibleRecord(const RecordId& record_id) {
+  inline void RemoveVisibleRecord(MediaRecordIdHash record_id_hash) {
     base::WeakPtr<ImageRecord> record =
-        visible_images_.find(record_id)->value->AsWeakPtr();
+        visible_images_.find(record_id_hash)->value->AsWeakPtr();
     if (!record->paint_time.is_null()) {
       DCHECK_GT(record->first_size, 0u);
       if (record->first_size > largest_removed_image_size_) {
@@ -100,7 +99,7 @@ class CORE_EXPORT ImageRecordsManager {
       }
     }
     size_ordered_set_.erase(record);
-    visible_images_.erase(record_id);
+    visible_images_.erase(record_id_hash);
     // Leave out |images_queued_for_paint_time_| intentionally because the null
     // record will be removed in |AssignPaintTimeToRegisteredQueuedRecords|.
   }
@@ -108,29 +107,29 @@ class CORE_EXPORT ImageRecordsManager {
   inline void RecordInvisible(const LayoutObject& object) {
     invisible_images_.insert(&object);
   }
-  void RecordVisible(const RecordId& record_id, const uint64_t& visual_size);
-  bool IsRecordedVisibleImage(const RecordId& record_id) const {
-    return visible_images_.Contains(record_id);
+  void RecordVisible(const MediaRecordId& record_id, const uint64_t& visual_size);
+  bool IsRecordedVisibleImage(const MediaRecordIdHash record_id_hash) const {
+    return visible_images_.Contains(record_id_hash);
   }
   bool IsRecordedInvisibleImage(const LayoutObject& object) const {
     return invisible_images_.Contains(&object);
   }
 
-  void NotifyImageFinished(const RecordId& record_id) {
+  void NotifyImageFinished(MediaRecordIdHash record_id_hash) {
     // TODO(npm): Ideally NotifyImageFinished() would only be called when the
     // record has not yet been inserted in |image_finished_times_| but that's
     // not currently the case. If we plumb some information from
     // ImageResourceContent we may be able to ensure that this call does not
     // require the Contains() check, which would save time.
-    if (!image_finished_times_.Contains(record_id))
-      image_finished_times_.insert(record_id, base::TimeTicks::Now());
+    if (!image_finished_times_.Contains(record_id_hash))
+      image_finished_times_.insert(record_id_hash, base::TimeTicks::Now());
   }
 
-  inline bool IsVisibleImageLoaded(const RecordId& record_id) const {
-    DCHECK(visible_images_.Contains(record_id));
-    return visible_images_.at(record_id)->loaded;
+  inline bool IsVisibleImageLoaded(MediaRecordIdHash record_id_hash) const {
+    DCHECK(visible_images_.Contains(record_id_hash));
+    return visible_images_.at(record_id_hash)->loaded;
   }
-  void OnImageLoaded(const RecordId&,
+  void OnImageLoaded(MediaRecordIdHash,
                      unsigned current_frame_index,
                      const StyleFetchedImage*);
   void OnImageLoadedInternal(base::WeakPtr<ImageRecord>&,
@@ -139,7 +138,7 @@ class CORE_EXPORT ImageRecordsManager {
   // Receives a candidate image painted under opacity 0 but without nested
   // opacity. May update |largest_ignored_image_| if the new candidate has a
   // larger size.
-  void MaybeUpdateLargestIgnoredImage(const RecordId&,
+  void MaybeUpdateLargestIgnoredImage(const MediaRecordId&,
                                       const uint64_t& visual_size);
   void ReportLargestIgnoredImage(unsigned current_frame_index);
 
@@ -175,9 +174,9 @@ class CORE_EXPORT ImageRecordsManager {
  private:
   // Find the image record of an visible image.
   inline base::WeakPtr<ImageRecord> FindVisibleRecord(
-      const RecordId& record_id) const {
-    DCHECK(visible_images_.Contains(record_id));
-    return visible_images_.find(record_id)->value->AsWeakPtr();
+      const MediaRecordIdHash record_id_hash) const {
+    DCHECK(visible_images_.Contains(record_id_hash));
+    return visible_images_.find(record_id_hash)->value->AsWeakPtr();
   }
   std::unique_ptr<ImageRecord> CreateImageRecord(
       const LayoutObject& object,
@@ -192,7 +191,7 @@ class CORE_EXPORT ImageRecordsManager {
     record->loaded = true;
   }
 
-  HashMap<RecordId, std::unique_ptr<ImageRecord>> visible_images_;
+  HashMap<MediaRecordIdHash, std::unique_ptr<ImageRecord>> visible_images_;
   HashSet<const LayoutObject*> invisible_images_;
 
   // This stores the image records, which are ordered by size.
@@ -202,7 +201,7 @@ class CORE_EXPORT ImageRecordsManager {
   Deque<base::WeakPtr<ImageRecord>> images_queued_for_paint_time_;
   // Map containing timestamps of when LayoutObject::ImageNotifyFinished is
   // first called.
-  HashMap<RecordId, base::TimeTicks> image_finished_times_;
+  HashMap<MediaRecordIdHash, base::TimeTicks> image_finished_times_;
 
   Member<LocalFrameView> frame_view_;
 
diff --git a/chromium/third_party/blink/renderer/core/paint/media_record_id.cc b/chromium/third_party/blink/renderer/core/paint/media_record_id.cc
new file mode 100644
index 00000000000..5a0fc1e3c04
--- /dev/null
+++ b/chromium/third_party/blink/renderer/core/paint/media_record_id.cc
@@ -0,0 +1,27 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/paint/media_record_id.h"
+
+#include "base/hash/hash.h"
+
+namespace blink {
+
+MediaRecordId::MediaRecordId(const LayoutObject* layout,
+                             const ImageResourceContent* content)
+    : layout_object_(layout),
+      image_resource_content_(content),
+      hash_(GenerateHash(layout, image_resource_content_)) {}
+
+// This hash is used as a key where previously MediaRecordId was used directly.
+// That helps us avoid storing references to the GCed LayoutObject and
+// MediaTiming, as that can be unsafe when using regular WTF containers. It also
+// helps us avoid needlessly allocating MediaRecordId on the heap.
+MediaRecordIdHash MediaRecordId::GenerateHash(const LayoutObject* layout,
+                                              const ImageResourceContent* content) {
+  return base::HashInts(reinterpret_cast<MediaRecordIdHash>(layout),
+                        reinterpret_cast<MediaRecordIdHash>(content));
+}
+
+}  // namespace blink
diff --git a/chromium/third_party/blink/renderer/core/paint/media_record_id.h b/chromium/third_party/blink/renderer/core/paint/media_record_id.h
new file mode 100644
index 00000000000..ae8df161ba1
--- /dev/null
+++ b/chromium/third_party/blink/renderer/core/paint/media_record_id.h
@@ -0,0 +1,37 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_PAINT_MEDIA_RECORD_ID_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_PAINT_MEDIA_RECORD_ID_H_
+
+#include "third_party/blink/renderer/core/core_export.h"
+#include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
+
+namespace blink {
+class LayoutObject;
+class ImageResourceContent;
+
+using MediaRecordIdHash = size_t;
+
+class MediaRecordId {
+  STACK_ALLOCATED();
+
+ public:
+  static MediaRecordIdHash CORE_EXPORT GenerateHash(const LayoutObject* layout,
+                                                    const ImageResourceContent* content);
+
+  MediaRecordId(const LayoutObject* layout, const ImageResourceContent* content);
+
+  MediaRecordIdHash GetHash() const { return hash_; }
+  const LayoutObject* GetLayoutObject() const { return layout_object_; }
+  const ImageResourceContent* GetImageResourceContent() const { return image_resource_content_; }
+
+ private:
+  const LayoutObject* const layout_object_;
+  const ImageResourceContent* const image_resource_content_;
+  const MediaRecordIdHash hash_;
+};
+
+}  // namespace blink
+#endif
diff --git a/chromium/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py b/chromium/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
index fe8b5e5d618..01a2bd09e97 100755
--- a/chromium/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
+++ b/chromium/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
@@ -36,6 +36,7 @@ _CONFIG = [
             'base::CreateSequencedTaskRunner',
             'base::DefaultTickClock',
             'base::ElapsedTimer',
+            'base::HashInts',
             'base::JobDelegate',
             'base::JobHandle',
             'base::PostJob',