From bd1eb06d4408dcd1f978d290e578b826cd54a7e9 Mon Sep 17 00:00:00 2001 From: Hao Liu Date: Wed, 31 May 2023 16:13:28 +0000 Subject: [PATCH] Set receive_header_start only for main frame document This is to fix a bug in a previous CL. https://chromium-review.googlesource.com/c/chromium/src/+/4476487. The receive_header_start_ of the PageLoadTracker object is set for each resource. We want the field to hold the value of the main frame document only. (cherry picked from commit b682b61a6f1453d998728a4d1477329a7e58d5f5) Bug: 1431906 Change-Id: Ifc76f40c1790f626b9aacbece785010c4d6e0710 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4562797 Reviewed-by: Ian Clelland Commit-Queue: Hao Liu Reviewed-by: Yoav Weiss Cr-Original-Commit-Position: refs/heads/main@{#1150323} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575844 Bot-Commit: Rubber Stamper Cr-Commit-Position: refs/branch-heads/5790@{#188} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../browser/page_load_tracker.cc | 16 ++++++++++---- .../browser/page_load_tracker.h | 2 +- .../browser/page_load_tracker_unittest.cc | 22 ++++++++++++++++++- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/components/page_load_metrics/browser/page_load_tracker.cc b/components/page_load_metrics/browser/page_load_tracker.cc index 75bf0569b96b12..2ed223e9c5b969 100644 --- a/components/page_load_metrics/browser/page_load_tracker.cc +++ b/components/page_load_metrics/browser/page_load_tracker.cc @@ -691,8 +691,15 @@ void PageLoadTracker::FlushMetricsOnAppEnterBackground() { void PageLoadTracker::OnLoadedResource( const ExtraRequestCompleteInfo& extra_request_complete_info) { - receive_headers_start_ = - extra_request_complete_info.load_timing_info->receive_headers_start; + // The main_frame_receive_headers_start_ should be only set once during a + // page load. A new page load would have a new PageLoadTracker object. + if (extra_request_complete_info.request_destination == + network::mojom::RequestDestination::kDocument && + !main_frame_receive_headers_start_.has_value()) { + main_frame_receive_headers_start_ = + extra_request_complete_info.load_timing_info->receive_headers_start; + } + for (const auto& observer : observers_) { observer->OnLoadedResource(extra_request_complete_info); } @@ -938,10 +945,11 @@ void PageLoadTracker::OnTimingChanged() { .value()); if (largest_contentful_image_changed) { - if (receive_headers_start_.has_value() && !GetNavigationStart().is_null()) { + if (main_frame_receive_headers_start_.has_value() && + !GetNavigationStart().is_null()) { RecordLargestContentfulPaintImageLoadTiming( *paint_timing->largest_contentful_paint, - receive_headers_start_.value() - GetNavigationStart()); + main_frame_receive_headers_start_.value() - GetNavigationStart()); } } diff --git a/components/page_load_metrics/browser/page_load_tracker.h b/components/page_load_metrics/browser/page_load_tracker.h index 0737d391eb48ce..75767c4387050f 100644 --- a/components/page_load_metrics/browser/page_load_tracker.h +++ b/components/page_load_metrics/browser/page_load_tracker.h @@ -572,7 +572,7 @@ class PageLoadTracker : public PageLoadMetricsUpdateDispatcher::Client, uint32_t soft_navigation_count_ = 0; - absl::optional receive_headers_start_; + absl::optional main_frame_receive_headers_start_; const internal::PageLoadTrackerPageType page_type_; diff --git a/components/page_load_metrics/browser/page_load_tracker_unittest.cc b/components/page_load_metrics/browser/page_load_tracker_unittest.cc index 7e3818e33d1bbb..d9cdd03c700b09 100644 --- a/components/page_load_metrics/browser/page_load_tracker_unittest.cc +++ b/components/page_load_metrics/browser/page_load_tracker_unittest.cc @@ -494,10 +494,30 @@ TEST_F(PageLoadTrackerTest, LargestImageIncorrectLoadTimings) { /*load_timing_info=*/ std::make_unique(load_timing_info)); - // Set document receive_headers_start. + // Set main frame document receive_headers_start. This field should be set + // only once. const auto request_id = navigation_simulator->GetGlobalRequestID(); tester()->SimulateLoadedResource(request_info, request_id); + // Simulate the invocation of PageLoadTracker::OnLoadedResource() again with + // a ttfb earlier than the image load start and a request destination that is + // not of type Document. This should not overwrite the + // receive_headers_start that is already set. + load_timing_info.receive_headers_start = + reference_time + base::Milliseconds(29); + + ExtraRequestCompleteInfo request_info1( + /*final_url=*/url::SchemeHostPort(GURL(kTestUrl)), + /*remote_endpoint=*/net::IPEndPoint(), + /*frame_tree_node_id=*/-1, /*was_cached=*/false, /*raw_body_bytes=*/0, + /*original_network_content_length=*/0, + /*request_destination=*/network::mojom::RequestDestination::kFrame, + /*net_error=*/0, + /*load_timing_info=*/ + std::make_unique(load_timing_info)); + + tester()->SimulateLoadedResource(request_info1, request_id); + // Set largest contentful paint timings. tester()->SimulateTimingUpdate(timing);