Remove redundant ad identification code in AdsPageLoadMetricsObserver.

Ad detection in AdsPageLoadMetricsObserver was previously done through
keeping track of ads with an override to OnSubframeNavigationEvaluated
and storing the ad frames in the set unfinished_subresource_ad_frames_,
and removal from the set when later checking if it is the frame is an
ad.

ContentSubresourceFilterThrottleManager now has an accessible
public IsFrameTaggedAsAd() method that makes ad detection within
AdsPageLoadMetricsObserver redundant.

BUG=958757

Change-Id: I0286984f81569ab35c01f92ff87303a4753b46b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1716253
Commit-Queue: Justin Miron <justinmiron@google.com>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683563}

chrome/browser/page_load_metrics/observers/ad_metrics/ads_page_load_metrics_observer.cc

@@ -16,6 +16,8 @@
 #include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
 #include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
 #include "chrome/browser/page_load_metrics/resource_tracker.h"
+#include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/core/common/common_features.h"
 #include "components/ukm/content/source_url_recorder.h"
 #include "content/public/browser/global_request_id.h"
@@ -60,8 +62,9 @@ using ResourceMimeType = AdsPageLoadMetricsObserver::ResourceMimeType;
 
 // static
 std::unique_ptr<AdsPageLoadMetricsObserver>
-AdsPageLoadMetricsObserver::CreateIfNeeded() {
-  if (!base::FeatureList::IsEnabled(subresource_filter::kAdTagging))
+AdsPageLoadMetricsObserver::CreateIfNeeded(content::WebContents* web_contents) {
+  if (!base::FeatureList::IsEnabled(subresource_filter::kAdTagging) ||
+      !ChromeSubresourceFilterClient::FromWebContents(web_contents))
     return nullptr;
   return std::make_unique<AdsPageLoadMetricsObserver>();
 }
@@ -247,16 +250,24 @@ void AdsPageLoadMetricsObserver::ReadyToCommitNextNavigation(
 void AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
     content::NavigationHandle* navigation_handle,
     const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  // If the AdsPageLoadMetricsObserver is created, this does not return nullptr.
+  auto* client = ChromeSubresourceFilterClient::FromWebContents(
+      navigation_handle->GetWebContents());
+  // AdsPageLoadMetricsObserver is not created unless there is a
+  // ChromeSubresourceFilterClient
+  DCHECK(client);
   FrameTreeNodeId frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
-  bool is_adframe = DetectAds(navigation_handle);
 
   // NOTE: Frame look-up only used for determining cross-origin status, not
   // granting security permissions.
-  content::RenderFrameHost* ad_host = FindFrameMaybeUnsafe(navigation_handle);
+  content::RenderFrameHost* frame_host =
+      FindFrameMaybeUnsafe(navigation_handle);
 
-  RecordAdFrameData(frame_tree_node_id, is_adframe, ad_host,
+  bool is_adframe = client->GetThrottleManager()->IsFrameTaggedAsAd(frame_host);
+
+  RecordAdFrameData(frame_tree_node_id, is_adframe, frame_host,
                     /*frame_navigated=*/true);
-  ProcessOngoingNavigationResource(ad_host);
+  ProcessOngoingNavigationResource(frame_host);
 }
 
 void AdsPageLoadMetricsObserver::FrameReceivedFirstUserActivation(
@@ -305,20 +316,6 @@ void AdsPageLoadMetricsObserver::OnResourceDataUseObserved(
   }
 }
 
-void AdsPageLoadMetricsObserver::OnSubframeNavigationEvaluated(
-    content::NavigationHandle* navigation_handle,
-    subresource_filter::LoadPolicy load_policy,
-    bool is_ad_subframe) {
-  // We don't track DISALLOW frames because their resources won't be loaded
-  // and therefore would provide bad histogram data. Note that WOULD_DISALLOW
-  // is only seen in dry runs.
-  if (is_ad_subframe &&
-      load_policy != subresource_filter::LoadPolicy::DISALLOW) {
-    unfinished_subresource_ad_frames_.insert(
-        navigation_handle->GetFrameTreeNodeId());
-  }
-}
-
 void AdsPageLoadMetricsObserver::OnPageInteractive(
     const page_load_metrics::mojom::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
@@ -417,16 +414,6 @@ void AdsPageLoadMetricsObserver::OnSubresourceFilterGoingAway() {
   subresource_observer_.RemoveAll();
 }
 
-bool AdsPageLoadMetricsObserver::DetectSubresourceFilterAd(
-    FrameTreeNodeId frame_tree_node_id) {
-  return unfinished_subresource_ad_frames_.erase(frame_tree_node_id);
-}
-
-bool AdsPageLoadMetricsObserver::DetectAds(
-    content::NavigationHandle* navigation_handle) {
-  return DetectSubresourceFilterAd(navigation_handle->GetFrameTreeNodeId());
-}
-
 int AdsPageLoadMetricsObserver::GetUnaccountedAdBytes(
     int process_id,
     const page_load_metrics::mojom::ResourceDataUpdatePtr& resource) const {

chrome/browser/page_load_metrics/observers/ad_metrics/ads_page_load_metrics_observer.h

@@ -31,7 +31,8 @@ class AdsPageLoadMetricsObserver
 
   // Returns a new AdsPageLoadMetricObserver. If the feature is disabled it
   // returns nullptr.
-  static std::unique_ptr<AdsPageLoadMetricsObserver> CreateIfNeeded();
+  static std::unique_ptr<AdsPageLoadMetricsObserver> CreateIfNeeded(
+      content::WebContents* web_contents);
 
   // For a given subframe, returns whether or not the subframe's url would be
   // considering same origin to the main frame's url. |use_parent_origin|
@@ -103,23 +104,10 @@ class AdsPageLoadMetricsObserver
 
  private:
   // subresource_filter::SubresourceFilterObserver:
-  void OnSubframeNavigationEvaluated(
-      content::NavigationHandle* navigation_handle,
-      subresource_filter::LoadPolicy load_policy,
-      bool is_ad_subframe) override;
   void OnAdSubframeDetected(
       content::RenderFrameHost* render_frame_host) override;
   void OnSubresourceFilterGoingAway() override;
 
-  // Determines if the URL of a frame matches the SubresourceFilter block
-  // list. Should only be called once per frame navigation.
-  bool DetectSubresourceFilterAd(FrameTreeNodeId frame_tree_node_id);
-
-  // This should only be called once per frame navigation, as the
-  // SubresourceFilter detector clears its state about detected frames after
-  // each call in order to free up memory.
-  bool DetectAds(content::NavigationHandle* navigation_handle);
-
   // Gets the number of bytes that we may have not attributed to ad
   // resources due to the resource being reported as an ad late.
   int GetUnaccountedAdBytes(
@@ -166,11 +154,6 @@ class AdsPageLoadMetricsObserver
   // |ad_frames_data_storage_|.
   std::map<FrameTreeNodeId, std::list<FrameData>::iterator> ad_frames_data_;
 
-  // The set of frames that have yet to finish but that the SubresourceFilter
-  // has reported are ads. Once DetectSubresourceFilterAd is called the id is
-  // removed from the set.
-  std::set<FrameTreeNodeId> unfinished_subresource_ad_frames_;
-
   // When the observer receives report of a document resource loading for a
   // sub-frame before the sub-frame commit occurs, hold onto the resource
   // request info (delay it) until the sub-frame commits.

chrome/browser/page_load_metrics/observers/ad_metrics/ads_page_load_metrics_observer_browsertest.cc

@@ -1082,3 +1082,37 @@ IN_PROC_BROWSER_TEST_F(AdsPageLoadMetricsObserverBrowserTest,
   EXPECT_EQ(1, samples.front().count);
   EXPECT_LE(min_percent, samples.front().min);
 }
+
+IN_PROC_BROWSER_TEST_F(AdsPageLoadMetricsObserverBrowserTest,
+                       DisallowedAdFrames_NotMeasured) {
+  base::HistogramTester histogram_tester;
+  ukm::TestAutoSetUkmRecorder ukm_recorder;
+
+  ResetConfiguration(subresource_filter::Configuration(
+      subresource_filter::mojom::ActivationLevel::kEnabled,
+      subresource_filter::ActivationScope::ALL_SITES));
+
+  // cross_site_iframe_factory loads URLs like:
+  // http://b.com:40919/cross_site_iframe_factory.html?b()
+  SetRulesetToDisallowURLsWithPathSuffix("b()");
+  const GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b,b,c,d)"));
+
+  auto waiter = CreatePageLoadMetricsTestWaiter();
+  ui_test_utils::NavigateToURL(browser(), main_url);
+
+  // One favicon resource and 2 resources for frames a,c,d
+  waiter->AddPageExpectation(
+      page_load_metrics::PageLoadMetricsTestWaiter::TimingField::kLoadEvent);
+  waiter->Wait();
+
+  // Navigate away to force the histogram recording.
+  ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
+
+  // Check that adframes are not included in UKM's or UMA metrics.
+  auto entries =
+      ukm_recorder.GetEntriesByName(ukm::builders::AdFrameLoad::kEntryName);
+  EXPECT_EQ(0u, entries.size());
+  histogram_tester.ExpectTotalCount(
+      "PageLoad.Clients.Ads.Bytes.AdFrames.Aggregate.Total", 0);
+}

chrome/browser/page_load_metrics/page_load_metrics_initialize.cc

@@ -128,7 +128,7 @@ void PageLoadMetricsEmbedder::RegisterObservers(
     tracker->AddObserver(
         std::make_unique<DataSaverSiteBreakdownMetricsObserver>());
     std::unique_ptr<AdsPageLoadMetricsObserver> ads_observer =
-        AdsPageLoadMetricsObserver::CreateIfNeeded();
+        AdsPageLoadMetricsObserver::CreateIfNeeded(tracker->GetWebContents());
     if (ads_observer)
       tracker->AddObserver(std::move(ads_observer));