Don't store UkmRecord on LocalFrameUkmAggregator

(cherry picked from commit 6e382de) Bug: 1402117 Change-Id: I6c4c8b7d8519c7ec42173a27c3c3257822d3576f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4117553 Commit-Queue: Stefan Zager <szager@chromium.org> Reviewed-by: Stephen Chenney <schenney@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1085145} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4132606 Reviewed-by: Krishna Govind <govind@chromium.org> Owners-Override: Krishna Govind <govind@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Commit-Position: refs/branch-heads/5481@{#106} Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
chromium · Jan 3, 2023 · 185ca6f · 185ca6f
1 parent 840c473
commit 185ca6f
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 53 deletions.
diff --git a/third_party/blink/renderer/core/frame/local_frame_ukm_aggregator.cc b/third_party/blink/renderer/core/frame/local_frame_ukm_aggregator.cc
@@ -122,14 +122,8 @@ void LocalFrameUkmAggregator::AbsoluteMetricRecord::reset() {
   main_frame_count = 0;
 }
 
-LocalFrameUkmAggregator::LocalFrameUkmAggregator(
-    int64_t source_id,
-    ukm::UkmRecorder* recorder,
-    bool is_for_main_frame_local_frame_root)
-    : source_id_(source_id),
-      recorder_(recorder),
-      clock_(base::DefaultTickClock::GetInstance()),
-      is_for_main_frame_(is_for_main_frame_local_frame_root) {
+LocalFrameUkmAggregator::LocalFrameUkmAggregator()
+    : clock_(base::DefaultTickClock::GetInstance()) {
   // All of these are assumed to have one entry per sub-metric.
   DCHECK_EQ(std::size(absolute_metric_records_), metrics_data().size());
   DCHECK_EQ(std::size(current_sample_.sub_metrics_counts),
@@ -193,12 +187,16 @@ LocalFrameUkmAggregator::LocalFrameUkmAggregator(
   }
 }
 
-LocalFrameUkmAggregator::~LocalFrameUkmAggregator() {
-  ReportUpdateTimeEvent();
+LocalFrameUkmAggregator::~LocalFrameUkmAggregator() = default;
+
+void LocalFrameUkmAggregator::TransmitFinalSample(int64_t source_id,
+                                                  ukm::UkmRecorder* recorder,
+                                                  bool is_for_main_frame) {
+  ReportUpdateTimeEvent(source_id, recorder);
 
   base::UmaHistogramBoolean("Blink.LocalFrameRoot.DidReachFirstContentfulPaint",
                             fcp_state_ != kBeforeFCPSignal);
-  if (is_for_main_frame_) {
+  if (is_for_main_frame) {
     base::UmaHistogramBoolean(
         "Blink.LocalFrameRoot.DidReachFirstContentfulPaint.MainFrame",
         fcp_state_ != kBeforeFCPSignal);
@@ -463,7 +461,9 @@ void LocalFrameUkmAggregator::RecordImplCompositorSample(
 void LocalFrameUkmAggregator::RecordEndOfFrameMetrics(
     base::TimeTicks start,
     base::TimeTicks end,
-    cc::ActiveFrameSequenceTrackers trackers) {
+    cc::ActiveFrameSequenceTrackers trackers,
+    int64_t source_id,
+    ukm::UkmRecorder* recorder) {
   last_frame_request_timestamp_for_test_ =
       request_timestamp_for_current_frame_.value_or(base::TimeTicks());
 
@@ -508,8 +508,8 @@ void LocalFrameUkmAggregator::RecordEndOfFrameMetrics(
   // Report the FCP metrics, if necessary, after updating the sample so that
   // the sample has been recorded for the frame that produced FCP.
   if (report_fcp_metrics) {
-    ReportPreFCPEvent();
-    ReportUpdateTimeEvent();
+    ReportPreFCPEvent(source_id, recorder);
+    ReportUpdateTimeEvent(source_id, recorder);
     // Update the state to prevent future reporting.
     fcp_state_ = kHavePassedFCP;
   }
@@ -553,7 +553,8 @@ void LocalFrameUkmAggregator::UpdateSample(
   current_sample_.trackers = trackers;
 }
 
-void LocalFrameUkmAggregator::ReportPreFCPEvent() {
+void LocalFrameUkmAggregator::ReportPreFCPEvent(int64_t source_id,
+                                                ukm::UkmRecorder* recorder) {
 #define RECORD_METRIC(name)                                         \
   {                                                                 \
     auto& absolute_record = absolute_metric_records_[k##name];      \
@@ -575,7 +576,10 @@ void LocalFrameUkmAggregator::ReportPreFCPEvent() {
         ToSample(ApplyBucket(absolute_record.pre_fcp_aggregate))); \
   }
 
-  ukm::builders::Blink_PageLoad builder(source_id_);
+  if (!recorder) {
+    return;
+  }
+  ukm::builders::Blink_PageLoad builder(source_id);
   primary_metric_.uma_aggregate_counter->Count(
       ToSample(primary_metric_.pre_fcp_aggregate));
   builder.SetMainFrame(ToSample(primary_metric_.pre_fcp_aggregate));
@@ -611,15 +615,18 @@ void LocalFrameUkmAggregator::ReportPreFCPEvent() {
   RECORD_METRIC(ParseStyleSheet);
   RECORD_METRIC(Accessibility);
 
-  builder.Record(recorder_);
+  builder.Record(recorder);
 #undef RECORD_METRIC
 #undef RECORD_BUCKETED_METRIC
 }
 
-void LocalFrameUkmAggregator::ReportUpdateTimeEvent() {
+void LocalFrameUkmAggregator::ReportUpdateTimeEvent(
+    int64_t source_id,
+    ukm::UkmRecorder* recorder) {
   // Don't report if we haven't generated any samples.
-  if (!frames_since_last_report_)
+  if (!recorder || !frames_since_last_report_) {
     return;
+  }
 
 #define RECORD_METRIC(name)                                      \
   builder.Set##name(current_sample_.sub_metrics_counts[k##name]) \
@@ -631,7 +638,7 @@ void LocalFrameUkmAggregator::ReportUpdateTimeEvent() {
       .Set##name##BeginMainFrame(                                             \
           ApplyBucket(current_sample_.sub_main_frame_counts[k##name]));
 
-  ukm::builders::Blink_UpdateTime builder(source_id_);
+  ukm::builders::Blink_UpdateTime builder(source_id);
   builder.SetMainFrame(current_sample_.primary_metric_count);
   builder.SetMainFrameIsBeforeFCP(fcp_state_ != kHavePassedFCP);
   builder.SetMainFrameReasons(current_sample_.trackers);
@@ -666,7 +673,7 @@ void LocalFrameUkmAggregator::ReportUpdateTimeEvent() {
   RECORD_METRIC(ParseStyleSheet);
   RECORD_METRIC(Accessibility);
 
-  builder.Record(recorder_);
+  builder.Record(recorder);
 #undef RECORD_METRIC
 #undef RECORD_BUCKETED_METRIC
 

diff --git a/third_party/blink/renderer/core/frame/local_frame_ukm_aggregator.h b/third_party/blink/renderer/core/frame/local_frame_ukm_aggregator.h
@@ -33,10 +33,6 @@ enum class DocumentUpdateReason;
 // SCOPED_UMA_AND_UKM_TIMER macro combined with
 // LocalFrameView::RecordEndOfFrameMetrics.
 //
-// It takes the following constructor parameters:
-// - source_id: UKM Source ID associated with the events.
-// - recorder: UkmRecorder which will handle the events
-//
 // The aggregator manages all of the UKM and UMA names for LocalFrameView.
 // It constructs and takes ownership of the UMA counters when constructed
 // itself. We do this to localize all UMA and UKM metrics in one place, so
@@ -262,9 +258,7 @@ class CORE_EXPORT LocalFrameUkmAggregator
     int64_t metric_index_ = -1;
   };
 
-  LocalFrameUkmAggregator(int64_t source_id,
-                          ukm::UkmRecorder*,
-                          bool is_for_main_frame_local_frame_root);
+  LocalFrameUkmAggregator();
   LocalFrameUkmAggregator(const LocalFrameUkmAggregator&) = delete;
   LocalFrameUkmAggregator& operator=(const LocalFrameUkmAggregator&) = delete;
   ~LocalFrameUkmAggregator();
@@ -285,7 +279,9 @@ class CORE_EXPORT LocalFrameUkmAggregator
   // trackers, telling us the reasons for requesting a BeginMainFrame.
   void RecordEndOfFrameMetrics(base::TimeTicks start,
                                base::TimeTicks end,
-                               cc::ActiveFrameSequenceTrackers trackers);
+                               cc::ActiveFrameSequenceTrackers trackers,
+                               int64_t source_id,
+                               ukm::UkmRecorder* recorder);
 
   // Record a sample for a sub-metric. This should only be used when
   // a ScopedUkmHierarchicalTimer cannot be used (such as when the timed
@@ -336,6 +332,10 @@ class CORE_EXPORT LocalFrameUkmAggregator
 
   void OnCommitRequested();
 
+  void TransmitFinalSample(int64_t source_id,
+                           ukm::UkmRecorder* recorder,
+                           bool is_for_main_frame);
+
   base::TimeTicks LastFrameRequestTimeForTest() const {
     return last_frame_request_timestamp_for_test_;
   }
@@ -375,11 +375,11 @@ class CORE_EXPORT LocalFrameUkmAggregator
   // Reports the current sample to the UKM system. Called on the first main
   // frame update after First Contentful Paint and at destruction. Also resets
   // the frame count.
-  void ReportUpdateTimeEvent();
+  void ReportUpdateTimeEvent(int64_t source_id, ukm::UkmRecorder* recorder);
 
   // Reports the Blink.PageLoad to the UKM system. Called on the first main
   // frame after First Contentful Paint.
-  void ReportPreFCPEvent();
+  void ReportPreFCPEvent(int64_t source_id, ukm::UkmRecorder* recorder);
 
   // To test event sampling. Controls whether we update the current sample
   // on the next frame, or do not. Values persist until explicitly changed.
@@ -396,9 +396,6 @@ class CORE_EXPORT LocalFrameUkmAggregator
     intersection_observer_sample_period_ = period;
   }
 
-  // UKM system data
-  const int64_t source_id_;
-  ukm::UkmRecorder* const recorder_;
   const base::TickClock* clock_;
 
   // Event and metric data
@@ -446,9 +443,6 @@ class CORE_EXPORT LocalFrameUkmAggregator
   absl::optional<base::TimeTicks> animation_request_timestamp_;
   absl::optional<base::TimeTicks> request_timestamp_for_current_frame_;
   base::TimeTicks last_frame_request_timestamp_for_test_;
-
-  // True if the local frame root that instantiated this is the main frame.
-  bool is_for_main_frame_ = false;
 };
 
 }  // namespace blink

diff --git a/third_party/blink/renderer/core/frame/local_frame_ukm_aggregator_test.cc b/third_party/blink/renderer/core/frame/local_frame_ukm_aggregator_test.cc
@@ -35,17 +35,27 @@ class LocalFrameUkmAggregatorTest : public testing::Test {
     aggregator_.reset();
   }
 
+  int64_t source_id() const { return source_id_; }
+
   LocalFrameUkmAggregator& aggregator() {
     CHECK(aggregator_);
     return *aggregator_;
   }
 
   ukm::TestUkmRecorder& recorder() { return recorder_; }
 
-  void ResetAggregator() { aggregator_.reset(); }
+  void ResetAggregator() {
+    if (aggregator_) {
+      aggregator_->TransmitFinalSample(source_id(), &recorder(),
+                                       /* is_for_main_frame */ true);
+      aggregator_.reset();
+    }
+  }
+
   void RestartAggregator() {
-    aggregator_ = base::MakeRefCounted<LocalFrameUkmAggregator>(
-        ukm::UkmRecorder::GetNewSourceID(), &recorder_, true);
+    source_id_ = ukm::UkmRecorder::GetNewSourceID();
+    aggregator_ = base::MakeRefCounted<LocalFrameUkmAggregator>();
+    // ukm::UkmRecorder::GetNewSourceID(), &recorder_, true);
     aggregator_->SetTickClockForTesting(test_task_runner_->GetMockTickClock());
   }
 
@@ -181,7 +191,8 @@ class LocalFrameUkmAggregatorTest : public testing::Test {
       test_task_runner_->FastForwardBy(
           base::Milliseconds(millisecond_per_step));
     }
-    aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers);
+    aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers,
+                                         source_id(), &recorder());
   }
 
   void SimulatePreFrame(unsigned millisecond_per_step) {
@@ -205,7 +216,8 @@ class LocalFrameUkmAggregatorTest : public testing::Test {
 
     aggregator().BeginMainFrame();
     aggregator().RecordForcedLayoutSample(reason, start_time, end_time);
-    aggregator().RecordEndOfFrameMetrics(start_time, end_time, 0);
+    aggregator().RecordEndOfFrameMetrics(start_time, end_time, 0, source_id(),
+                                         &recorder());
     ResetAggregator();
 
     EXPECT_EQ(recorder().entries_count(), expected_num_entries);
@@ -245,6 +257,7 @@ class LocalFrameUkmAggregatorTest : public testing::Test {
   }
 
  private:
+  int64_t source_id_;
   scoped_refptr<LocalFrameUkmAggregator> aggregator_;
   ukm::TestUkmRecorder recorder_;
 };
@@ -610,7 +623,8 @@ TEST_F(LocalFrameUkmAggregatorTest, IntersectionObserverSamplePeriod) {
         LocalFrameUkmAggregator::kDisplayLockIntersectionObserver);
     test_task_runner_->FastForwardBy(base::Milliseconds(1));
   }
-  aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers);
+  aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers, source_id(),
+                                       &recorder());
   histogram_tester.ExpectUniqueSample("Blink.Layout.UpdateTime.PreFCP", 1000,
                                       1);
   histogram_tester.ExpectUniqueSample(
@@ -628,7 +642,8 @@ TEST_F(LocalFrameUkmAggregatorTest, IntersectionObserverSamplePeriod) {
         LocalFrameUkmAggregator::kDisplayLockIntersectionObserver);
     test_task_runner_->FastForwardBy(base::Milliseconds(1));
   }
-  aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers);
+  aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers, source_id(),
+                                       &recorder());
   histogram_tester.ExpectUniqueSample("Blink.Layout.UpdateTime.PreFCP", 1000,
                                       2);
   histogram_tester.ExpectUniqueSample(
@@ -646,7 +661,8 @@ TEST_F(LocalFrameUkmAggregatorTest, IntersectionObserverSamplePeriod) {
         LocalFrameUkmAggregator::kDisplayLockIntersectionObserver);
     test_task_runner_->FastForwardBy(base::Milliseconds(1));
   }
-  aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers);
+  aggregator().RecordEndOfFrameMetrics(start_time, Now(), trackers, source_id(),
+                                       &recorder());
   histogram_tester.ExpectUniqueSample("Blink.Layout.UpdateTime.PreFCP", 1000,
                                       3);
   histogram_tester.ExpectUniqueSample(
@@ -714,8 +730,10 @@ class LocalFrameUkmAggregatorSimTest : public SimTest {
     // Simulate the first contentful paint in the main frame.
     document.View()->GetUkmAggregator()->BeginMainFrame();
     PaintTiming::From(GetDocument()).MarkFirstContentfulPaint();
+    Document* root_document = LocalFrameRoot().GetFrame()->GetDocument();
     document.View()->GetUkmAggregator()->RecordEndOfFrameMetrics(
-        base::TimeTicks(), base::TimeTicks() + base::Microseconds(10), 0);
+        base::TimeTicks(), base::TimeTicks() + base::Microseconds(10), 0,
+        root_document->UkmSourceID(), root_document->UkmRecorder());
 
     target1->setAttribute(html_names::kStyleAttr, "width: 60px");
     Compositor().BeginFrame();

diff --git a/third_party/blink/renderer/core/frame/local_frame_view.cc b/third_party/blink/renderer/core/frame/local_frame_view.cc
@@ -502,7 +502,16 @@ void LocalFrameView::Dispose() {
   if (owner_element && owner_element->OwnedEmbeddedContentView() == this)
     owner_element->SetEmbeddedContentView(nullptr);
 
-  ukm_aggregator_.reset();
+  if (ukm_aggregator_) {
+    LocalFrame& root_frame = GetFrame().LocalFrameRoot();
+    Document* root_document = root_frame.GetDocument();
+    if (root_document) {
+      ukm_aggregator_->TransmitFinalSample(root_document->UkmSourceID(),
+                                           root_document->UkmRecorder(),
+                                           root_frame.IsMainFrame());
+    }
+    ukm_aggregator_.reset();
+  }
   layout_shift_tracker_->Dispose();
 
 #if DCHECK_IS_ON()
@@ -4727,10 +4736,7 @@ LocalFrameUkmAggregator* LocalFrameView::GetUkmAggregator() {
   if (!local_root->ukm_aggregator_) {
     if (!local_root->frame_->GetChromeClient().IsSVGImageChromeClient()) {
       local_root->ukm_aggregator_ =
-          base::MakeRefCounted<LocalFrameUkmAggregator>(
-              local_root->frame_->GetDocument()->UkmSourceID(),
-              local_root->frame_->GetDocument()->UkmRecorder(),
-              local_root->frame_->IsMainFrame());
+          base::MakeRefCounted<LocalFrameUkmAggregator>();
     }
   }
   return local_root->ukm_aggregator_.get();

diff --git a/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc b/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
@@ -2377,13 +2377,15 @@ void WebFrameWidgetImpl::RecordEndOfFrameMetrics(
     cc::ActiveFrameSequenceTrackers trackers) {
   if (!LocalRootImpl())
     return;
-
+  Document* document = LocalRootImpl()->GetFrame()->GetDocument();
+  DCHECK(document);
   LocalRootImpl()
       ->GetFrame()
       ->View()
       ->GetUkmAggregator()
       ->RecordEndOfFrameMetrics(frame_begin_time, base::TimeTicks::Now(),
-                                trackers);
+                                trackers, document->UkmSourceID(),
+                                document->UkmRecorder());
 }
 
 void WebFrameWidgetImpl::WillHandleGestureEvent(const WebGestureEvent& event,