Revert "Reland "Trigger heavy ad intervention when an unloaded heavy ad navigates""

This reverts commit 0c891022.

Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1109599#c10

Original change's description:
> Reland "Trigger heavy ad intervention when an unloaded heavy ad navigates"
>
> This is a reland of 312a57a1
>
> Original change's description:
> > Trigger heavy ad intervention when an unloaded heavy ad navigates
> >
> > Bug: 1099014
> > Change-Id: I98a2d0e548a48fd93618fbae1c1d458f1dbd58a4
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2269740
> > Reviewed-by: John Delaney <johnidel@chromium.org>
> > Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#791488}
>
> Bug: 1099014
> Change-Id: I0894d1f43127e3776e9f71bd35dcb723a1778ceb
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2356164
> Reviewed-by: John Delaney <johnidel@chromium.org>
> Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#801381}

Bug: 1099014, 1109599
Change-Id: Ia73fb4cf681524d2ecd2604622c765ac22a21049
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2381254Reviewed-by: John Delaney <johnidel@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802682}

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

@@ -258,11 +258,9 @@ AdsPageLoadMetricsObserver::OnCommit(
       navigation_handle->GetReloadType() != content::ReloadType::NONE;
 
   aggregate_frame_data_->UpdateForNavigation(
-      navigation_handle->GetRenderFrameHost(), true /* frame_navigated */,
-      true /* record_frame_metrics */);
+      navigation_handle->GetRenderFrameHost(), true /* frame_navigated */);
   main_frame_data_->UpdateForNavigation(navigation_handle->GetRenderFrameHost(),
-                                        true /* frame_navigated */,
-                                        true /* record_frame_metrics */);
+                                        true /* frame_navigated */);
 
   // The main frame is never considered an ad.
   ad_frames_data_[navigation_handle->GetFrameTreeNodeId()] =
@@ -399,8 +397,7 @@ void AdsPageLoadMetricsObserver::UpdateAdFrameData(
 
   if (should_create_new_frame_data) {
     if (previous_data) {
-      previous_data->UpdateForNavigation(ad_host, frame_navigated,
-                                         true /* record_frame_metrics */);
+      previous_data->UpdateForNavigation(ad_host, frame_navigated);
       return;
     }
     if (base::FeatureList::IsEnabled(features::kV8PerAdFrameMemoryMonitoring) &&
@@ -421,8 +418,7 @@ void AdsPageLoadMetricsObserver::UpdateAdFrameData(
         heavy_ad_threshold_noise_provider_->GetNetworkThresholdNoiseForFrame());
     ad_data_iterator = --ad_frames_data_storage_.end();
     ad_data = &*ad_data_iterator;
-    ad_data->UpdateForNavigation(ad_host, frame_navigated,
-                                 true /* record_frame_metrics */);
+    ad_data->UpdateForNavigation(ad_host, frame_navigated);
   }
 
   // Maybe update frame depth based on the new ad frames distance to the ad
@@ -465,33 +461,6 @@ void AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
   content::RenderFrameHost* frame_host =
       FindFrameMaybeUnsafe(navigation_handle);
 
-  // We want to reset the FrameData for an ad after heavy ads fires, so that we
-  // can trigger on subsequent navigations if the page tries to serve another ad
-  // in the frame.
-  if (navigation_handle->IsErrorPage() &&
-      navigation_handle->GetNetErrorCode() == net::ERR_BLOCKED_BY_CLIENT &&
-      navigation_handle->HasCommitted()) {
-    const auto& id_and_data = ad_frames_data_.find(frame_tree_node_id);
-    if (id_and_data != ad_frames_data_.end() &&
-        id_and_data->second != ad_frames_data_storage_.end() &&
-        id_and_data->second->heavy_ad_status_with_policy() !=
-            FrameData::HeavyAdStatus::kNone) {
-      RecordPerFrameMetrics(*id_and_data->second,
-                            GetDelegate().GetPageUkmSourceId());
-      ad_frames_data_storage_.erase(id_and_data->second);
-      ad_frames_data_.erase(id_and_data);
-      ad_frames_data_storage_.emplace_back(
-          frame_tree_node_id, heavy_ad_threshold_noise_provider_
-                                  ->GetNetworkThresholdNoiseForFrame());
-      auto ad_data_iterator = --ad_frames_data_storage_.end();
-      FrameData* ad_data = &*ad_data_iterator;
-      ad_frames_data_[frame_tree_node_id] = ad_data_iterator;
-      ad_data->UpdateForNavigation(frame_host, true /*frame_navigated=*/,
-                                   false /*record_frame_metrics=*/);
-      return;
-    }
-  }
-
   bool is_adframe = client->GetThrottleManager()->IsFrameTaggedAsAd(frame_host);
 
   // TODO(https://crbug.com): The following block is a hack to ignore certain

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

@@ -1672,31 +1672,15 @@ IN_PROC_BROWSER_TEST_F(AdsPageLoadMetricsObserverResourceBrowserTest,
   EXPECT_EQ(4u, console_observer.messages().size());
 }
 
-class NoHostThresholdHeavyAdsBrowserTest
-    : public AdsPageLoadMetricsObserverResourceBrowserTest {
- public:
-  NoHostThresholdHeavyAdsBrowserTest() {
-    scoped_feature_list_.InitAndEnableFeatureWithParameters(
-        features::kHeavyAdPrivacyMitigations, {{"host-threshold", "100"}});
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-};
-
 // Verifies that the frame is navigated to the intervention page when a
 // heavy ad intervention triggers.
-IN_PROC_BROWSER_TEST_F(NoHostThresholdHeavyAdsBrowserTest,
+IN_PROC_BROWSER_TEST_F(AdsPageLoadMetricsObserverResourceBrowserTest,
                        HeavyAdInterventionEnabled_ErrorPageLoaded) {
   base::HistogramTester histogram_tester;
   auto incomplete_resource_response =
       std::make_unique<net::test_server::ControllableHttpResponse>(
           embedded_test_server(), "/ads_observer/incomplete_resource.js",
           true /*relative_url_is_prefix*/);
-  auto incomplete_resource_response2 =
-      std::make_unique<net::test_server::ControllableHttpResponse>(
-          embedded_test_server(), "/ads_observer/incomplete_resource2.js",
-          true /*relative_url_is_prefix*/);
   ASSERT_TRUE(embedded_test_server()->Start());
 
   // Create a navigation observer that will watch for the intervention to
@@ -1706,6 +1690,7 @@ IN_PROC_BROWSER_TEST_F(NoHostThresholdHeavyAdsBrowserTest,
   content::TestNavigationObserver error_observer(web_contents,
                                                  net::ERR_BLOCKED_BY_CLIENT);
 
+  auto waiter = CreateAdsPageLoadMetricsTestWaiter();
   GURL url = embedded_test_server()->GetURL(
       "/ads_observer/ad_with_incomplete_resource.html");
   ui_test_utils::NavigateToURL(browser(), url);
@@ -1727,26 +1712,6 @@ IN_PROC_BROWSER_TEST_F(NoHostThresholdHeavyAdsBrowserTest,
   histogram_tester.ExpectBucketCount(
       "Blink.UseCounter.Features",
       blink::mojom::WebFeature::kHeavyAdIntervention, 1);
-
-  content::TestNavigationObserver error_observer2(web_contents,
-                                                  net::ERR_BLOCKED_BY_CLIENT);
-
-  // Test that subsequent navigations to the error page can still trigger heavy
-  // ads (crbug.com/1099014).
-  EXPECT_TRUE(content::ExecJs(web_contents, "advanceSrcdoc();",
-                              content::EXECUTE_SCRIPT_NO_USER_GESTURE));
-
-  // Load a resource large enough to trigger the intervention.
-  LoadLargeResource(incomplete_resource_response2.get(),
-                    kMaxHeavyAdNetworkSize);
-
-  error_observer2.WaitForNavigationFinished();
-
-  // We can't check whether the navigation didn't occur because the error page
-  // load is not synchronous. Instead check that we didn't log intervention UMA
-  // that is always recorded when the intervention occurs. Both
-  // interventions should have been logged.
-  histogram_tester.ExpectTotalCount(kHeavyAdInterventionTypeHistogramId, 2);
 }
 
 class AdsPageLoadMetricsObserverResourceBrowserTestWithoutHeavyAdIntervention

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

@@ -84,10 +84,8 @@ FrameData::FrameData(FrameTreeNodeId root_frame_tree_node_id,
 FrameData::~FrameData() = default;
 
 void FrameData::UpdateForNavigation(content::RenderFrameHost* render_frame_host,
-                                    bool frame_navigated,
-                                    bool record_metrics) {
+                                    bool frame_navigated) {
   frame_navigated_ = frame_navigated;
-  record_metrics_ = record_metrics;
   if (!render_frame_host)
     return;
 
@@ -269,7 +267,7 @@ void FrameData::MaybeUpdateFrameDepth(
 }
 
 bool FrameData::ShouldRecordFrameForMetrics() const {
-  return record_metrics_ && (bytes() != 0 || !GetTotalCpuUsage().is_zero());
+  return bytes() != 0 || !GetTotalCpuUsage().is_zero();
 }
 
 void FrameData::RecordAdFrameLoadUkmEvent(ukm::SourceId source_id) const {

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

@@ -156,8 +156,7 @@ class FrameData {
 
   // Update the metadata of this frame if it is being navigated.
   void UpdateForNavigation(content::RenderFrameHost* render_frame_host,
-                           bool frame_navigated,
-                           bool record_frame_metrics);
+                           bool frame_navigated);
 
   // Updates the number of bytes loaded in the frame given a resource load.
   void ProcessResourceLoadInFrame(
@@ -406,12 +405,6 @@ class FrameData {
   // Number of bytes of noise that should be added to the network threshold.
   const int heavy_ad_network_threshold_noise_;
 
-  // |record_metrics| indicates whether metrics should be logged for this frame.
-  // This may be false in cases where we are tracking a frame, but do not want
-  // to log metrics until a subsequent navigation, e.g. if a frame is currently
-  // navigated to the heavy ads error page.
-  bool record_metrics_ = true;
-
   DISALLOW_COPY_AND_ASSIGN(FrameData);
 };
 

chrome/test/data/ads_observer/ad_with_incomplete_resource.html

@@ -6,10 +6,6 @@
   let adIframe = createAdIframe();
   // "incomplete_resource.js" should never be a complete resource load.
   adIframe.srcdoc = "<link rel='preload' href='incomplete_resource.js' as='script'>";
-
-  function advanceSrcdoc() {
-    adIframe.srcdoc =  "<link rel='preload' href='incomplete_resource2.js' as='script'>";
-  }
 </script>
 
 </body>