Allow ad frame identification triggered by ad scripts only.

This updates the ad_page_load_metrics_observer so that it accounts for
ad frames that trigger ads without a navigation (via a script identified
as an ad via subresource filters).

Bug: 849268
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I81b9dee7e3556719e40337f2efd818f784130b78
Reviewed-on: https://chromium-review.googlesource.com/1120347
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579825}

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc

@@ -86,12 +86,14 @@ void RecordParentExistsForSubFrame(
 AdsPageLoadMetricsObserver::AdFrameData::AdFrameData(
     FrameTreeNodeId frame_tree_node_id,
     AdTypes ad_types,
-    AdOriginStatus origin_status)
+    AdOriginStatus origin_status,
+    bool frame_navigated)
     : frame_bytes(0u),
       frame_bytes_uncached(0u),
       frame_tree_node_id(frame_tree_node_id),
       ad_types(ad_types),
-      origin_status(origin_status) {}
+      origin_status(origin_status),
+      frame_navigated(frame_navigated) {}
 
 // static
 std::unique_ptr<AdsPageLoadMetricsObserver>
@@ -135,60 +137,87 @@ AdsPageLoadMetricsObserver::OnCommit(
   return CONTINUE_OBSERVING;
 }
 
-void AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
-    content::NavigationHandle* navigation_handle) {
-  // Determine if the frame is part of an existing ad, the root of a new ad,
-  // or a non-ad frame. Once a frame is labled as an ad, it is always
-  // considered an ad, even if it navigates to a non-ad page. This function
-  // labels all of a page's frames, even those that fail to commit.
-  FrameTreeNodeId frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
-  content::RenderFrameHost* parent_frame_host =
-      navigation_handle->GetParentFrame();
-
-  AdTypes ad_types = DetectAds(navigation_handle);
-
-  // If an existing subframe is navigating to an ad and was one to begin with,
-  // then keep tracking it as an ad.
-  const auto& id_and_data = ad_frames_data_.find(frame_tree_node_id);
+// Given an ad being triggered for a frame or navigation, get its AdFrameData
+// and record it into the appropriate data structures.
+void AdsPageLoadMetricsObserver::RecordAdFrameData(
+    FrameTreeNodeId ad_id,
+    AdTypes ad_types,
+    content::RenderFrameHost* ad_host,
+    bool frame_navigated) {
+  // If an existing subframe is navigating and it was an ad previously that
+  // hasn't navigated yet, then we need to update it.
+  const auto& id_and_data = ad_frames_data_.find(ad_id);
+  AdFrameData* previous_data = nullptr;
   if (id_and_data != ad_frames_data_.end() && id_and_data->second) {
-    ProcessOngoingNavigationResource(frame_tree_node_id);
-    return;
+    DCHECK(frame_navigated);
+    if (id_and_data->second->frame_navigated) {
+      // We need to update the types with any new types that triggered it.
+      id_and_data->second->ad_types |= ad_types;
+      ProcessOngoingNavigationResource(ad_id);
+      return;
+    }
+    previous_data = id_and_data->second;
   }
 
-  // Determine who the parent frame's ad ancestor is.
+  // Determine who the parent frame's ad ancestor is.  If we don't know who it
+  // is, return, such as with a frame from a previous navigation.
+  content::RenderFrameHost* parent_frame_host =
+      ad_host ? ad_host->GetParent() : nullptr;
   const auto& parent_id_and_data =
-      ad_frames_data_.find(parent_frame_host->GetFrameTreeNodeId());
-  if (parent_id_and_data == ad_frames_data_.end()) {
-    // We don't know who the parent for this frame is. One possibility is that
-    // it's a frame from a previous navigation.
-    RecordParentExistsForSubFrame(false /* parent_exists */, ad_types);
+      parent_frame_host
+          ? ad_frames_data_.find(parent_frame_host->GetFrameTreeNodeId())
+          : ad_frames_data_.end();
+  bool parent_exists = parent_id_and_data != ad_frames_data_.end();
+  RecordParentExistsForSubFrame(parent_exists, ad_types);
+  if (!parent_exists)
     return;
-  }
-  RecordParentExistsForSubFrame(true /* parent_exists */, ad_types);
-
-  AdFrameData* ad_data = parent_id_and_data->second;
 
   // This frame is not nested within an ad frame but is itself an ad.
+  AdFrameData* ad_data = parent_id_and_data->second;
   if (!ad_data && ad_types.any()) {
     AdOriginStatus origin_status = AdOriginStatus::kUnknown;
-    // NOTE: frame look-up only used for determining cross-origin status, not
-    // granting security permissions.
-    content::RenderFrameHost* ad_host = FindFrameMaybeUnsafe(navigation_handle);
     if (ad_host) {
       content::RenderFrameHost* main_host =
-          navigation_handle->GetWebContents()->GetMainFrame();
+          content::WebContents::FromRenderFrameHost(ad_host)->GetMainFrame();
+      // For ads triggered on render, their origin is their parent's origin.
+      if (!frame_navigated)
+        ad_host = ad_host->GetParent();
       origin_status = main_host->GetLastCommittedOrigin().IsSameOriginWith(
                           ad_host->GetLastCommittedOrigin())
                           ? AdOriginStatus::kSame
                           : AdOriginStatus::kCross;
     }
-    ad_frames_data_storage_.emplace_back(frame_tree_node_id, ad_types,
-                                         origin_status);
+    // If data existed already, update it and exit, otherwise, add it.
+    if (previous_data) {
+      previous_data->ad_types |= ad_types;
+      previous_data->origin_status = origin_status;
+      previous_data->frame_navigated = frame_navigated;
+      return;
+    }
+    ad_frames_data_storage_.emplace_back(ad_id, ad_types, origin_status,
+                                         frame_navigated);
     ad_data = &ad_frames_data_storage_.back();
   }
 
-  ad_frames_data_[frame_tree_node_id] = ad_data;
+  // If there was previous data, then we don't want to overwrite this frame.
+  if (!previous_data)
+    ad_frames_data_[ad_id] = ad_data;
+}
+
+// Determine if the frame is part of an existing ad, the root of a new ad, or a
+// non-ad frame. Once a frame is labeled as an ad, it is always considered an
+// ad, even if it navigates to a non-ad page. This function labels all of a
+// page's frames, even those that fail to commit.
+void AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
+    content::NavigationHandle* navigation_handle) {
+  FrameTreeNodeId frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
+  AdTypes ad_types = 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);
 
+  RecordAdFrameData(frame_tree_node_id, ad_types, ad_host,
+                    /*frame_navigated=*/true);
   ProcessOngoingNavigationResource(frame_tree_node_id);
 }
 
@@ -229,6 +258,15 @@ void AdsPageLoadMetricsObserver::OnSubframeNavigationEvaluated(
   }
 }
 
+void AdsPageLoadMetricsObserver::OnAdSubframeDetected(
+    content::RenderFrameHost* render_frame_host) {
+  AdTypes ad_types;
+  ad_types.set(AD_TYPE_SUBRESOURCE_FILTER);
+  FrameTreeNodeId frame_tree_node_id = render_frame_host->GetFrameTreeNodeId();
+  RecordAdFrameData(frame_tree_node_id, ad_types, render_frame_host,
+                    /*frame_navigated=*/false);
+}
+
 void AdsPageLoadMetricsObserver::OnSubresourceFilterGoingAway() {
   subresource_observer_.RemoveAll();
 }

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h

@@ -58,6 +58,10 @@ class AdsPageLoadMetricsObserver
                         bool started_in_foreground) override;
   ObservePolicy OnCommit(content::NavigationHandle* navigation_handle,
                          ukm::SourceId source_id) override;
+  void RecordAdFrameData(FrameTreeNodeId ad_id,
+                         AdTypes ad_types,
+                         content::RenderFrameHost* ad_host,
+                         bool frame_navigated);
   void OnDidFinishSubFrameNavigation(
       content::NavigationHandle* navigation_handle) override;
   ObservePolicy FlushMetricsOnAppEnterBackground(
@@ -72,12 +76,14 @@ class AdsPageLoadMetricsObserver
   struct AdFrameData {
     AdFrameData(FrameTreeNodeId frame_tree_node_id,
                 AdTypes ad_types,
-                AdOriginStatus origin_status);
+                AdOriginStatus origin_status,
+                bool frame_navigated);
     size_t frame_bytes;
     size_t frame_bytes_uncached;
     const FrameTreeNodeId frame_tree_node_id;
     AdTypes ad_types;
     AdOriginStatus origin_status;
+    bool frame_navigated;
   };
 
   // subresource_filter::SubresourceFilterObserver:
@@ -85,6 +91,8 @@ class AdsPageLoadMetricsObserver
       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

chrome/browser/subresource_filter/ad_tagging_browsertest.cc

@@ -6,6 +6,8 @@
 
 #include "base/callback.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h"
 #include "chrome/browser/subresource_filter/subresource_filter_browser_test_harness.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -129,7 +131,6 @@ content::RenderFrameHost* AdTaggingBrowserTest::CreateDocWrittenFrameImpl(
       "%s('%s', '%s');",
       ad_script ? "createDocWrittenAdFrame" : "createDocWrittenFrame",
       name.c_str(), GetURL("").spec().c_str());
-
   content::TestNavigationObserver navigation_observer(GetWebContents(), 1);
   EXPECT_TRUE(content::ExecuteScript(rfh, script));
   navigation_observer.Wait();
@@ -158,39 +159,136 @@ IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest, FramesByURL) {
 
   // Main frame.
   ui_test_utils::NavigateToURL(browser(), GetURL("frame_factory.html"));
-  EXPECT_FALSE(
-      observer.GetIsAdSubframe(GetURL("frame_factory.html")).has_value());
+  EXPECT_FALSE(observer.GetIsAdSubframe(
+      GetWebContents()->GetMainFrame()->GetFrameTreeNodeId()));
 
   // (1) Vanilla child.
-  RenderFrameHost* vanilla_child =
+  content::RenderFrameHost* vanilla_child =
       CreateSrcFrame(GetWebContents(), GetURL("frame_factory.html?1"));
-  EXPECT_FALSE(*observer.GetIsAdSubframe(GetURL("frame_factory.html?1")));
+  EXPECT_FALSE(*observer.GetIsAdSubframe(vanilla_child->GetFrameTreeNodeId()));
 
   // (2) Ad child.
   RenderFrameHost* ad_child =
       CreateSrcFrame(GetWebContents(), GetURL("frame_factory.html?2&ad=true"));
-  EXPECT_TRUE(
-      *observer.GetIsAdSubframe(GetURL("frame_factory.html?2&ad=true")));
+  EXPECT_TRUE(*observer.GetIsAdSubframe(ad_child->GetFrameTreeNodeId()));
 
   // (3) Ad child of 2.
-  CreateSrcFrame(ad_child, GetURL("frame_factory.html?sub=1&3&ad=true"));
-  EXPECT_TRUE(
-      *observer.GetIsAdSubframe(GetURL("frame_factory.html?sub=1&3&ad=true")));
+  RenderFrameHost* ad_child_2 =
+      CreateSrcFrame(ad_child, GetURL("frame_factory.html?sub=1&3&ad=true"));
+  EXPECT_TRUE(*observer.GetIsAdSubframe(ad_child_2->GetFrameTreeNodeId()));
 
   // (4) Vanilla child of 2.
-  CreateSrcFrame(ad_child, GetURL("frame_factory.html?4"));
-  EXPECT_TRUE(*observer.GetIsAdSubframe(GetURL("frame_factory.html?4")));
+  RenderFrameHost* vanilla_child_2 =
+      CreateSrcFrame(ad_child, GetURL("frame_factory.html?4"));
+  EXPECT_TRUE(*observer.GetIsAdSubframe(vanilla_child_2->GetFrameTreeNodeId()));
 
   // (5) Vanilla child of 1. This tests something subtle.
   // frame_factory.html?ad=true loads the same script that frame_factory.html
   // uses to load frames. This tests that even though the script is tagged as an
   // ad in the ad iframe, it's not considered an ad in the main frame, hence
   // it's able to create an iframe that's not labeled as an ad.
-  CreateSrcFrame(vanilla_child, GetURL("frame_factory.html?5"));
-  EXPECT_FALSE(*observer.GetIsAdSubframe(GetURL("frame_factory.html?5")));
+  RenderFrameHost* vanilla_child_3 =
+      CreateSrcFrame(vanilla_child, GetURL("frame_factory.html?5"));
+  EXPECT_FALSE(
+      *observer.GetIsAdSubframe(vanilla_child_3->GetFrameTreeNodeId()));
+}
+
+const char kSubresourceFilterOriginStatusHistogram[] =
+    "PageLoad.Clients.Ads.SubresourceFilter.FrameCounts.AdFrames.PerFrame."
+    "OriginStatus";
+const char kGoogleOriginStatusHistogram[] =
+    "PageLoad.Clients.Ads.Google.FrameCounts.AdFrames.PerFrame.OriginStatus";
+const char kAllOriginStatusHistogram[] =
+    "PageLoad.Clients.Ads.All.FrameCounts.AdFrames.PerFrame.OriginStatus";
+
+IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest, VerifySameOriginWithoutNavigate) {
+  base::HistogramTester histogram_tester;
+
+  // Main frame.
+  ui_test_utils::NavigateToURL(browser(), GetURL("frame_factory.html"));
+
+  // Ad frame via doc write.
+  CreateDocWrittenFrameFromAdScript(GetWebContents());
+
+  // Navigate away and ensure we report same origin.
+  ui_test_utils::NavigateToURL(browser(), GetURL(url::kAboutBlankURL));
+  histogram_tester.ExpectUniqueSample(
+      kAllOriginStatusHistogram,
+      AdsPageLoadMetricsObserver::AdOriginStatus::kSame, 1);
+}
+
+IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest, VerifyCrossOriginWithoutNavigate) {
+  base::HistogramTester histogram_tester;
+
+  // Main frame.
+  ui_test_utils::NavigateToURL(browser(), GetURL("frame_factory.html"));
+
+  // Regular frame that's cross origin and has a doc write ad of its own.
+  RenderFrameHost* regular_child = CreateSrcFrame(
+      GetWebContents(), embedded_test_server()->GetURL(
+                            "b.com", "/ad_tagging/frame_factory.html"));
+  CreateDocWrittenFrameFromAdScript(regular_child);
+
+  // Navigate away and ensure we report cross origin.
+  ui_test_utils::NavigateToURL(browser(), GetURL(url::kAboutBlankURL));
+  histogram_tester.ExpectUniqueSample(
+      kAllOriginStatusHistogram,
+      AdsPageLoadMetricsObserver::AdOriginStatus::kCross, 1);
 }
 
-// Test that a subframe with a non-ad url but loaded by ad script is an ad
+// Ad script creates a frame and navigates it cross origin.
+IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest,
+                       VerifyCrossOriginWithImmediateNavigate) {
+  base::HistogramTester histogram_tester;
+
+  // Create the main frame and cross origin subframe from an ad script.
+  // This triggers both subresource_filter and Google ad detection.
+  ui_test_utils::NavigateToURL(browser(), GetURL("frame_factory.html"));
+  CreateSrcFrameFromAdScript(GetWebContents(),
+                             embedded_test_server()->GetURL(
+                                 "b.com", "/ads_observer/same_origin_ad.html"));
+
+  // Navigate away and ensure we report cross origin.
+  ui_test_utils::NavigateToURL(browser(), GetURL(url::kAboutBlankURL));
+  histogram_tester.ExpectUniqueSample(
+      kGoogleOriginStatusHistogram,
+      AdsPageLoadMetricsObserver::AdOriginStatus::kCross, 1);
+  histogram_tester.ExpectUniqueSample(
+      kSubresourceFilterOriginStatusHistogram,
+      AdsPageLoadMetricsObserver::AdOriginStatus::kCross, 1);
+  histogram_tester.ExpectUniqueSample(
+      kAllOriginStatusHistogram,
+      AdsPageLoadMetricsObserver::AdOriginStatus::kCross, 1);
+}
+
+// Ad script creates a frame and navigates it same origin.
+// It is then renavigated cross origin.
+IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest,
+                       VerifySameOriginWithCrossOriginRenavigate) {
+  base::HistogramTester histogram_tester;
+
+  // Create the main frame and same origin subframe from an ad script.
+  // This triggers the subresource_filter ad detection.
+  ui_test_utils::NavigateToURL(browser(), GetURL("frame_factory.html"));
+  RenderFrameHost* ad_child = CreateSrcFrameFromAdScript(
+      GetWebContents(), GetURL("frame_factory.html"));
+
+  // Navigate the subframe to a cross origin site.
+  NavigateFrame(ad_child, embedded_test_server()->GetURL(
+                              "b.com", "/ad_tagging/frame_factory.html"));
+
+  // Navigate away and ensure we report same origin.
+  ui_test_utils::NavigateToURL(browser(), GetURL(url::kAboutBlankURL));
+  histogram_tester.ExpectTotalCount(kGoogleOriginStatusHistogram, 0);
+  histogram_tester.ExpectUniqueSample(
+      kSubresourceFilterOriginStatusHistogram,
+      AdsPageLoadMetricsObserver::AdOriginStatus::kSame, 1);
+  histogram_tester.ExpectUniqueSample(
+      kAllOriginStatusHistogram,
+      AdsPageLoadMetricsObserver::AdOriginStatus::kSame, 1);
+}
+
+// Test that a subframe with a non-ad url but loaded by ad script is an ad.
 IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest, FrameLoadedByAdScript) {
   TestSubresourceFilterObserver observer(web_contents());
 
@@ -198,8 +296,9 @@ IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest, FrameLoadedByAdScript) {
   ui_test_utils::NavigateToURL(browser(), GetURL("frame_factory.html"));
 
   // Child frame created by ad script.
-  CreateSrcFrameFromAdScript(GetWebContents(), GetURL("frame_factory.html?1"));
-  EXPECT_TRUE(*observer.GetIsAdSubframe(GetURL("frame_factory.html?1")));
+  RenderFrameHost* ad_child = CreateSrcFrameFromAdScript(
+      GetWebContents(), GetURL("frame_factory.html?1"));
+  EXPECT_TRUE(*observer.GetIsAdSubframe(ad_child->GetFrameTreeNodeId()));
 }
 
 // Test that same-origin doc.write created iframes are tagged as ads.
@@ -212,19 +311,12 @@ IN_PROC_BROWSER_TEST_F(AdTaggingBrowserTest, SameOriginFrameTagging) {
   // (1) Vanilla child.
   content::RenderFrameHost* vanilla_frame =
       CreateDocWrittenFrame(GetWebContents());
-
-  // Navigate the child to a vanilla site to trigger an observer event. If the
-  // first navigation was considered an ad, the second should be as well.
-  // TOOD(jkarlin): The extra navigations in this test aren't necessary once
-  // https://crbug.com/849268 is fixed.
-  NavigateFrame(vanilla_frame, GetURL("frame_factory.html?1"));
-  EXPECT_FALSE(*observer.GetIsAdSubframe(GetURL("frame_factory.html?1")));
+  EXPECT_FALSE(observer.GetIsAdSubframe(vanilla_frame->GetFrameTreeNodeId()));
 
   // (2) Ad child.
   content::RenderFrameHost* ad_frame =
       CreateDocWrittenFrameFromAdScript(GetWebContents());
-  NavigateFrame(ad_frame, GetURL("frame_factory.html?2"));
-  EXPECT_TRUE(*observer.GetIsAdSubframe(GetURL("frame_factory.html?2")));
+  EXPECT_TRUE(*observer.GetIsAdSubframe(ad_frame->GetFrameTreeNodeId()));
 }
 
 }  // namespace

chrome/browser/subresource_filter/subresource_filter_unittest.cc

@@ -115,7 +115,7 @@ TEST_F(SubresourceFilterTest, SimpleAllowedLoad_WithObserver) {
   SimulateNavigateAndCommit(GURL(allowed_url), subframe);
   EXPECT_EQ(subresource_filter::LoadPolicy::ALLOW,
             *observer.GetSubframeLoadPolicy(allowed_url));
-  EXPECT_FALSE(*observer.GetIsAdSubframe(allowed_url));
+  EXPECT_FALSE(*observer.GetIsAdSubframe(subframe->GetFrameTreeNodeId()));
 }
 
 TEST_F(SubresourceFilterTest, SimpleDisallowedLoad_WithObserver) {
@@ -129,10 +129,13 @@ TEST_F(SubresourceFilterTest, SimpleDisallowedLoad_WithObserver) {
             observer.GetPageActivation(url).value());
 
   GURL disallowed_url(SubresourceFilterTest::kDefaultDisallowedUrl);
-  EXPECT_FALSE(CreateAndNavigateDisallowedSubframe(main_rfh()));
+  auto* subframe =
+      content::RenderFrameHostTester::For(main_rfh())->AppendChild("subframe");
+  EXPECT_FALSE(
+      SimulateNavigateAndCommit(GURL(kDefaultDisallowedUrl), subframe));
   EXPECT_EQ(subresource_filter::LoadPolicy::DISALLOW,
             *observer.GetSubframeLoadPolicy(disallowed_url));
-  EXPECT_TRUE(*observer.GetIsAdSubframe(disallowed_url));
+  EXPECT_TRUE(*observer.GetIsAdSubframe(subframe->GetFrameTreeNodeId()));
 }
 
 TEST_F(SubresourceFilterTest, RefreshMetadataOnActivation) {

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc

@@ -400,6 +400,8 @@ void ContentSubresourceFilterThrottleManager::OnFrameIsAdSubframe(
   DCHECK(render_frame_host);
 
   ad_frames_.insert(render_frame_host);
+  SubresourceFilterObserverManager::FromWebContents(web_contents())
+      ->NotifyAdSubframeDetected(render_frame_host);
 }
 
 void ContentSubresourceFilterThrottleManager::MaybeActivateSubframeSpecialUrls(

components/subresource_filter/content/browser/subresource_filter_observer.h

@@ -12,6 +12,7 @@
 
 namespace content {
 class NavigationHandle;
+class RenderFrameHost;
 }  // namespace content
 
 namespace subresource_filter {
@@ -54,6 +55,9 @@ class SubresourceFilterObserver {
       content::NavigationHandle* navigation_handle,
       LoadPolicy load_policy,
       bool is_ad_subframe) {}
+
+  virtual void OnAdSubframeDetected(
+      content::RenderFrameHost* render_frame_host) {}
 };
 
 }  // namespace subresource_filter

components/subresource_filter/content/browser/subresource_filter_observer_manager.cc

@@ -51,4 +51,10 @@ void SubresourceFilterObserverManager::NotifySubframeNavigationEvaluated(
                                            is_ad_subframe);
 }
 
+void SubresourceFilterObserverManager::NotifyAdSubframeDetected(
+    content::RenderFrameHost* render_frame_host) {
+  for (auto& observer : observers_)
+    observer.OnAdSubframeDetected(render_frame_host);
+}
+
 }  // namespace subresource_filter

components/subresource_filter/content/browser/subresource_filter_observer_manager.h

@@ -14,6 +14,7 @@
 
 namespace content {
 class NavigationHandle;
+class RenderFrameHost;
 class WebContents;
 }  // namespace content
 
@@ -54,6 +55,10 @@ class SubresourceFilterObserverManager
       LoadPolicy load_policy,
       bool is_ad_subframe);
 
+  // Called in TODO to notify observers that an ad frame has been detected
+  // with the associated RenderFrameHost.
+  void NotifyAdSubframeDetected(content::RenderFrameHost* render_frame_host);
+
  private:
   base::ObserverList<SubresourceFilterObserver> observers_;
   DISALLOW_COPY_AND_ASSIGN(SubresourceFilterObserverManager);

components/subresource_filter/content/browser/subresource_filter_observer_test_utils.cc

@@ -7,6 +7,7 @@
 #include "base/logging.h"
 #include "components/subresource_filter/core/common/activation_state.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_frame_host.h"
 
 namespace subresource_filter {
 
@@ -40,7 +41,13 @@ void TestSubresourceFilterObserver::OnSubframeNavigationEvaluated(
     LoadPolicy load_policy,
     bool is_ad_subframe) {
   subframe_load_evaluations_[navigation_handle->GetURL()] = load_policy;
-  ad_subframe_evaluations_[navigation_handle->GetURL()] = is_ad_subframe;
+  ad_subframe_evaluations_[navigation_handle->GetFrameTreeNodeId()] =
+      is_ad_subframe;
+}
+
+void TestSubresourceFilterObserver::OnAdSubframeDetected(
+    content::RenderFrameHost* render_frame_host) {
+  ad_subframe_evaluations_[render_frame_host->GetFrameTreeNodeId()] = true;
 }
 
 void TestSubresourceFilterObserver::DidFinishNavigation(
@@ -71,8 +78,8 @@ TestSubresourceFilterObserver::GetPageActivation(const GURL& url) const {
 }
 
 base::Optional<bool> TestSubresourceFilterObserver::GetIsAdSubframe(
-    const GURL& url) const {
-  auto it = ad_subframe_evaluations_.find(url);
+    int frame_tree_node_id) const {
+  auto it = ad_subframe_evaluations_.find(frame_tree_node_id);
   if (it != ad_subframe_evaluations_.end())
     return it->second;
   return base::Optional<bool>();

components/subresource_filter/content/browser/subresource_filter_observer_test_utils.h

@@ -45,6 +45,8 @@ class TestSubresourceFilterObserver : public SubresourceFilterObserver,
       content::NavigationHandle* navigation_handle,
       LoadPolicy load_policy,
       bool is_ad_subframe) override;
+  void OnAdSubframeDetected(
+      content::RenderFrameHost* render_frame_host) override;
 
   // content::WebContentsObserver
   void DidFinishNavigation(
@@ -52,7 +54,7 @@ class TestSubresourceFilterObserver : public SubresourceFilterObserver,
 
   base::Optional<ActivationLevel> GetPageActivation(const GURL& url) const;
   base::Optional<LoadPolicy> GetSubframeLoadPolicy(const GURL& url) const;
-  base::Optional<bool> GetIsAdSubframe(const GURL& url) const;
+  base::Optional<bool> GetIsAdSubframe(int frame_tree_node_id) const;
   base::Optional<ActivationLevel> GetPageActivationForLastCommittedLoad() const;
 
   using SafeBrowsingCheck =
@@ -62,7 +64,7 @@ class TestSubresourceFilterObserver : public SubresourceFilterObserver,
 
  private:
   std::map<GURL, LoadPolicy> subframe_load_evaluations_;
-  std::map<GURL, bool> ad_subframe_evaluations_;
+  std::map<int, bool> ad_subframe_evaluations_;
 
   std::map<GURL, ActivationLevel> page_activations_;
   std::map<GURL, SafeBrowsingCheck> safe_browsing_checks_;

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -146,6 +146,10 @@
 -AdsPageLoadMetricsObserverBrowserTest.OriginStatusMetricCross
 -AdsPageLoadMetricsObserverBrowserTest.DocOverwritesNavigation
 -AdsPageLoadMetricsObserverBrowserTest.SubresourceFilter
+-AdTaggingBrowserTest.VerifyCrossOriginWithoutNavigate
+-AdTaggingBrowserTest.VerifySameOriginWithoutNavigate
+-AdTaggingBrowserTest.VerifyCrossOriginWithImmediateNavigate
+-AdTaggingBrowserTest.VerifySameOriginWithCrossOriginRenavigate
 -PageLoadMetricsBrowserTest.LoadingMetrics
 -PageLoadMetricsBrowserTest.LoadingMetricsFailed
 -PageLoadMetricsBrowserTest.PayloadSize