PageLoadMetrics: Notify parents of inner frames' navigation and deletion

This patch forwards inner frame's navigation and deletion events to
parents at PageLoadTracker layer regardless of each observer's policy.
While forwarding events, ones for main frame are converted as ones for
sub-frames. This PageLoadTracker level forwarding is needed to
preprocess information to extract per-outermost page events such as
page lifecycle events.

Details are:
- Route inner page's page deletion to the outermost page.
  + Dispatch SubFrameDeleted event also to the parent tracker at
    PageLoadTracker layer.
  + Dispatch RenderFrameDeleted event as a sub-frame deletion to
    the parent tracker at PageLoadTracker layer.
  + Omit forwarding relevant events at the observer layer.
- Route inner page's navigation to the outermost page.
  + Dispatch DidFinishSubFrameNavigation event also to the parent
    tracker at PageLoadTracker layer.
  + Dispatch Commit, DidCommitSameDocumentNavigation, and
    DidInternalNavigationAbort evenst as finishing a sub-frame
    navigation to the parent tracker at PageLoadTracker layer.
  + Omit forwarding relevant events at the observer layer.

Also, there is a bug that the primary tracker is set as a parent
tracker on prerendering page. This patch fixes the issue to make
some subresource filter related tests pass.

Change-Id: Iff7bb81e9ff43e697fbab2d17b20fdc8d097a1dd
Bug: 1301880
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3578508
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#992328}

components/page_load_metrics/browser/metrics_web_contents_observer.cc

@@ -280,6 +280,7 @@ void MetricsWebContentsObserver::WillStartNavigationRequestImpl(
 
   // Prepare ukm::SourceId that is based on outermost page's navigation ID.
   ukm::SourceId source_id = ukm::kInvalidSourceId;
+  base::WeakPtr<PageLoadTracker> parent_tracker;
   if (navigation_handle->IsInPrimaryMainFrame() ||
       navigation_handle->IsInPrerenderedMainFrame()) {
     // Primary and Prerender pages use own page's navigation ID.
@@ -289,8 +290,10 @@ void MetricsWebContentsObserver::WillStartNavigationRequestImpl(
              content::FrameType::kFencedFrameRoot) {
     // For FencedFrames, use the primary page's ukm::SourceId. `primary_page_`
     // can be nullptr if the main frame is in data URL or so.
-    if (primary_page_)
+    if (primary_page_) {
       source_id = primary_page_->GetPageUkmSourceId();
+      parent_tracker = primary_page_->GetWeakPtr();
+    }
   } else {
     NOTREACHED();
   }
@@ -304,9 +307,7 @@ void MetricsWebContentsObserver::WillStartNavigationRequestImpl(
       std::make_unique<PageLoadTracker>(
           in_foreground, embedder_interface_.get(), currently_committed_url,
           !has_navigated_, navigation_handle, user_initiated_info, source_id,
-          (navigation_handle->IsInPrimaryMainFrame() || !primary_page_)
-              ? nullptr
-              : primary_page_->GetWeakPtr())));
+          parent_tracker)));
   DCHECK(insertion_result.second)
       << "provisional_loads_ already contains NavigationHandle.";
   for (auto& observer : lifecycle_observers_)

components/page_load_metrics/browser/page_load_metrics_forward_observer.cc

@@ -73,42 +73,32 @@ PageLoadMetricsForwardObserver::OnRedirect(
   return CONTINUE_OBSERVING;
 }
 
+// OnCommit and OnDidInternalNavigationAbort are handled at PageLoadTracker
+// to forward events as a sub-frame navigation regardless of each observer's
+// policy.
 PageLoadMetricsObserver::ObservePolicy PageLoadMetricsForwardObserver::OnCommit(
     content::NavigationHandle* navigation_handle) {
-  if (!parent_observer_ ||
-      parent_observer_->OnCommit(navigation_handle) == STOP_OBSERVING) {
-    return STOP_OBSERVING;
-  }
   return CONTINUE_OBSERVING;
 }
 
 void PageLoadMetricsForwardObserver::OnDidInternalNavigationAbort(
-    content::NavigationHandle* navigation_handle) {
-  if (!parent_observer_)
-    return;
-  parent_observer_->OnDidInternalNavigationAbort(navigation_handle);
-}
+    content::NavigationHandle* navigation_handle) {}
 
+// ReadyToCommitNextNavigation is an event only for main frames. As main
+// frame events are converted to sub-frames events on forwarding, this event
+// is just masked here.
 void PageLoadMetricsForwardObserver::ReadyToCommitNextNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (!parent_observer_)
-    return;
-  parent_observer_->ReadyToCommitNextNavigation(navigation_handle);
-}
+    content::NavigationHandle* navigation_handle) {}
 
+// OnDidFinishSubFrameNavigation is handled at PageLoadTracker to forward
+// events regardless of each observer's policy.
 void PageLoadMetricsForwardObserver::OnDidFinishSubFrameNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (!parent_observer_)
-    return;
-  parent_observer_->OnDidFinishSubFrameNavigation(navigation_handle);
-}
+    content::NavigationHandle* navigation_handle) {}
 
+// OnCommitSameDocumentNavigation is handled at PageLoadTracker to forward
+// events as a sub-frame navigation regardless of each observer's policy.
 void PageLoadMetricsForwardObserver::OnCommitSameDocumentNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (!parent_observer_)
-    return;
-  parent_observer_->OnCommitSameDocumentNavigation(navigation_handle);
-}
+    content::NavigationHandle* navigation_handle) {}
 
 PageLoadMetricsObserver::ObservePolicy PageLoadMetricsForwardObserver::OnHidden(
     const mojom::PageLoadTiming& timing) {
@@ -401,17 +391,13 @@ void PageLoadMetricsForwardObserver::FrameSizeChanged(
   parent_observer_->FrameSizeChanged(render_frame_host, frame_size);
 }
 
+// OnRenderFrameDeleted and OnSubFrameDeleted are handled at PageLoadTracker
+// to forward events as sub-frames deletion regardless of each observer's
+// policy.
 void PageLoadMetricsForwardObserver::OnRenderFrameDeleted(
-    content::RenderFrameHost* render_frame_host) {
-  if (!parent_observer_)
-    return;
-  parent_observer_->OnRenderFrameDeleted(render_frame_host);
-}
+    content::RenderFrameHost* render_frame_host) {}
 
 void PageLoadMetricsForwardObserver::OnSubFrameDeleted(int frame_tree_node_id) {
-  if (!parent_observer_)
-    return;
-  parent_observer_->OnSubFrameDeleted(frame_tree_node_id);
 }
 
 void PageLoadMetricsForwardObserver::OnCookiesRead(

components/page_load_metrics/browser/page_load_metrics_forward_observer_unittest.cc

@@ -23,7 +23,7 @@ const char kTestUrl[] = "https://a.test/";
 struct PageLoadMetricsObserverEvents final {
   bool was_started = false;
   bool was_fenced_frames_started = false;
-  size_t commit_count = 0;
+  size_t event_count = 0;
 };
 
 class TestPageLoadMetricsObserver final : public PageLoadMetricsObserver {
@@ -57,16 +57,16 @@ class TestPageLoadMetricsObserver final : public PageLoadMetricsObserver {
     return FORWARD_OBSERVING;
   }
 
-  ObservePolicy OnCommit(
-      content::NavigationHandle* navigation_handle) override {
-    // TestPageLoadMetricsObserver will be instantiated for the Primary page
-    // and a FencedFrames page. As instance for a FencedFrames page will be
-    // destructed after `OnFencedFramesStart` and `OnCommit` will not be
-    // invoked for the instance. Instead, PageLoadMetricsForwardObserver routes
-    // the event to the Primary page's observer.
+  ObservePolicy ShouldObserveMimeType(
+      const std::string& mime_type) const override {
+    // TestPageLoadMetricsObserver will be instantiated for the Primary page and
+    // a FencedFrames page. As instance for a FencedFrames page will be
+    // destructed after `OnFencedFramesStart` and `ShouldObserverMimeType` will
+    // not be invoked for the instance. Instead, PageLoadMetricsForwardObserver
+    // routes the event to the Primary page's observer.
     EXPECT_FALSE(is_in_fenced_frames_);
     if (!is_in_fenced_frames_)
-      events_->commit_count++;
+      events_->event_count++;
     return CONTINUE_OBSERVING;
   }
 
@@ -122,10 +122,9 @@ TEST_F(PageLoadMetricsForwardObserverTest, Basic) {
   // Check observer behaviors.
   EXPECT_TRUE(GetEvents().was_started);
   EXPECT_TRUE(GetEvents().was_fenced_frames_started);
-  // OnCommit will be invoked twice in the primary page observer, once is for
-  // its own navigation, the other is forwarded one for the navigation in the
-  // FencedFrames' root.
-  EXPECT_EQ(2u, GetEvents().commit_count);
+  // The event will be invoked twice in the primary page observer, once is for
+  // its own, the other is forwarded one from the FencedFrames' page.
+  EXPECT_EQ(2u, GetEvents().event_count);
 
   // Check metrics.
   tester()->histogram_tester().ExpectBucketCount(

components/page_load_metrics/browser/page_load_tracker.cc

@@ -372,6 +372,10 @@ void PageLoadTracker::PageShown() {
 }
 
 void PageLoadTracker::SubFrameDeleted(int frame_tree_node_id) {
+  if (parent_tracker_) {
+    // Notify the parent of inner subframe deletions.
+    parent_tracker_->SubFrameDeleted(frame_tree_node_id);
+  }
   metrics_update_dispatcher_.OnSubFrameDeleted(frame_tree_node_id);
   largest_contentful_paint_handler_.OnSubFrameDeleted(frame_tree_node_id);
   for (const auto& observer : observers_) {
@@ -380,6 +384,12 @@ void PageLoadTracker::SubFrameDeleted(int frame_tree_node_id) {
 }
 
 void PageLoadTracker::RenderFrameDeleted(content::RenderFrameHost* rfh) {
+  if (parent_tracker_) {
+    // Notify the parent of the inner main frame deletion as a sub-frame
+    // deletion.
+    parent_tracker_->SubFrameDeleted(rfh->GetFrameTreeNodeId());
+  }
+
   for (const auto& observer : observers_) {
     observer->OnRenderFrameDeleted(rfh);
   }
@@ -392,6 +402,12 @@ void PageLoadTracker::WillProcessNavigationResponse(
 }
 
 void PageLoadTracker::Commit(content::NavigationHandle* navigation_handle) {
+  if (parent_tracker_) {
+    // Notify the parent of the inner main frame navigation as a sub-frame
+    // navigation.
+    parent_tracker_->DidFinishSubFrameNavigation(navigation_handle);
+  }
+
   did_commit_ = true;
   url_ = navigation_handle->GetURL();
   // Some transitions (like CLIENT_REDIRECT) are only known at commit time.
@@ -430,27 +446,47 @@ void PageLoadTracker::DidActivatePrerenderedPage(
 
 void PageLoadTracker::DidCommitSameDocumentNavigation(
     content::NavigationHandle* navigation_handle) {
+  if (parent_tracker_) {
+    // Notify the parent of the inner main frame navigation as a sub-frame
+    // navigation.
+    parent_tracker_->DidFinishSubFrameNavigation(navigation_handle);
+  }
+
   for (const auto& observer : observers_) {
     observer->OnCommitSameDocumentNavigation(navigation_handle);
   }
 }
 
 void PageLoadTracker::DidInternalNavigationAbort(
     content::NavigationHandle* navigation_handle) {
+  if (parent_tracker_) {
+    // Notify the parent of the inner main frame navigation as a sub-frame
+    // navigation.
+    parent_tracker_->DidFinishSubFrameNavigation(navigation_handle);
+  }
+
   for (const auto& observer : observers_) {
     observer->OnDidInternalNavigationAbort(navigation_handle);
   }
 }
 
 void PageLoadTracker::ReadyToCommitNavigation(
     content::NavigationHandle* navigation_handle) {
+  // Don't notify the parent as inner main frame's events are converted to
+  // sub-frames events for the parent, but this event is only for the main
+  // frame.
+
   for (const auto& observer : observers_) {
     observer->ReadyToCommitNextNavigation(navigation_handle);
   }
 }
 
 void PageLoadTracker::DidFinishSubFrameNavigation(
     content::NavigationHandle* navigation_handle) {
+  if (parent_tracker_) {
+    // Notify the parent of inner frame navigations.
+    parent_tracker_->DidFinishSubFrameNavigation(navigation_handle);
+  }
   metrics_update_dispatcher_.DidFinishSubFrameNavigation(navigation_handle);
   largest_contentful_paint_handler_.OnDidFinishSubFrameNavigation(
       navigation_handle, navigation_start_);

components/page_load_metrics/browser/page_load_tracker.h

@@ -503,7 +503,7 @@ class PageLoadTracker : public PageLoadMetricsUpdateDispatcher::Client,
   page_load_metrics::LargestContentfulPaintHandler
       experimental_largest_contentful_paint_handler_;
 
-  base::WeakPtr<PageLoadTracker> parent_tracker_;
+  const base::WeakPtr<PageLoadTracker> parent_tracker_;
 
   base::WeakPtrFactory<PageLoadTracker> weak_factory_{this};
 };

components/page_load_metrics/browser/page_load_tracker_unittest.cc

@@ -25,6 +25,8 @@ struct PageLoadMetricsObserverEvents final {
   bool was_fenced_frames_started = false;
   bool was_prerender_started = false;
   bool was_committed = false;
+  bool was_sub_frame_deleted = false;
+  size_t sub_frame_navigation_count = 0;
 };
 
 class TestPageLoadMetricsObserver final : public PageLoadMetricsObserver {
@@ -62,6 +64,13 @@ class TestPageLoadMetricsObserver final : public PageLoadMetricsObserver {
     events_->was_committed = true;
     return CONTINUE_OBSERVING;
   }
+  void OnDidFinishSubFrameNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    events_->sub_frame_navigation_count++;
+  }
+  void OnSubFrameDeleted(int frame_tree_node_id) override {
+    events_->was_sub_frame_deleted = true;
+  }
 
   bool stop_on_prerender_ = false;
   bool stop_on_fenced_frames_ = false;
@@ -105,13 +114,16 @@ class PageLoadTrackerTest : public PageLoadMetricsObserverContentTestHarness {
     if (tracker->GetUrl() != target_url_)
       return;
 
+    DCHECK(!is_observer_passed_);
     tracker->AddObserver(std::unique_ptr<PageLoadMetricsObserver>(observer_));
+    is_observer_passed_ = true;
   }
 
   base::flat_map<std::string, ukm::SourceId> ukm_source_ids_;
 
   PageLoadMetricsObserverEvents events_;
   raw_ptr<TestPageLoadMetricsObserver> observer_;
+  bool is_observer_passed_ = false;
 
   base::test::ScopedFeatureList scoped_feature_list_;
   GURL target_url_;
@@ -142,6 +154,53 @@ TEST_F(PageLoadTrackerTest, PrimaryPageType) {
   EXPECT_TRUE(GetEvents().was_ready_to_commit_next_navigation);
 }
 
+TEST_F(PageLoadTrackerTest, EventForwarding) {
+  // Target URL to monitor the tracker via the test observer.
+  SetTargetUrl(kTestUrl);
+  StopObservingOnFencedFrames();
+
+  // Navigate in.
+  NavigateAndCommit(GURL(kTestUrl));
+
+  // Add a fenced frame.
+  content::RenderFrameHost* fenced_frame_root =
+      content::RenderFrameHostTester::For(web_contents()->GetMainFrame())
+          ->AppendFencedFrame();
+  {
+    const char kFencedFramesUrl[] = "https://a.test/fenced_frames";
+    auto simulator = content::NavigationSimulator::CreateForFencedFrame(
+        GURL(kFencedFramesUrl), fenced_frame_root);
+    ASSERT_NE(nullptr, simulator);
+    simulator->Commit();
+  }
+
+  // Check observer behaviors.
+  // The navigation and frame deletion in the FencedFrames should be observed as
+  // sub-frame events.
+  EXPECT_TRUE(GetEvents().was_started);
+  EXPECT_FALSE(GetEvents().was_fenced_frames_started);
+  EXPECT_FALSE(GetEvents().was_prerender_started);
+  EXPECT_TRUE(GetEvents().was_committed);
+  EXPECT_FALSE(GetEvents().was_sub_frame_deleted);
+  EXPECT_EQ(1u, GetEvents().sub_frame_navigation_count);
+
+  // Navigate out.
+  {
+    const char kFencedFramesNavigationUrl[] = "https://b.test/fenced_frames";
+    auto simulator = content::NavigationSimulator::CreateForFencedFrame(
+        GURL(kFencedFramesNavigationUrl), fenced_frame_root);
+    ASSERT_NE(nullptr, simulator);
+    simulator->Commit();
+  }
+
+  // Check observer behaviors again after the render frame's deletion.
+  // TODO(https://crbug.com/1301880): RenderFrameDeleted() doesn't seem called
+  // and following check fails. Revisit this issue later to clarify the
+  // expectations.
+  // EXPECT_TRUE(GetEvents().was_sub_frame_deleted);
+  EXPECT_EQ(2u, GetEvents().sub_frame_navigation_count);
+}
+
 // TODO(https://crbug.com/1312096): Enable the test on Android.
 #if BUILDFLAG(IS_ANDROID)
 #define MAYBE_PrerenderPageType DISABLED_PrerenderPageType