Inherit subresource filter activation from opener for certain mainframes

Currently, mainframes which do not have a committed load (e.g. due to
their initial load being aborted by a document.write() call) never have
the subresource filter activated. Further, each popup mainframe that is
same-origin to its opener (whether or not it has a committed load)
should share the activation of its opener as the opener determines its
contents including, for example, through document.write() calls.

There are two cases to consider for same-origin popups. First, a popup
with an inherited origin (e.g. an about:blank frame) should inherit the
activation of its opener; it would not otherwise be activated as its URL
wouldn't match the filter list. Second, a popup with a URL that is
handled by the network stack and is same-origin does not need to inherit
its activation as activation-rules are site-based, i.e. apply equally to
all pages with the same eTLD+1. The activation resulting from the normal
process for committed navigations will therefore be the same as the
opener's.

This cl creates a filter with the activation of the opener for
mainframes without a committed load or with an inherited origin. If the
inherited activation is disabled, no filter is created. This inheritance
behavior is similar to that of subframes with an aborted initial load
and of about:blank subframes, both of which inherit the activation of
their parent frames.

Design doc (internal-only): https://docs.google.com/document/d/12ocy5qgoMZh0Ntl6Ah3Q0GShXzRxHmSmArDy7gWGtJM/edit?usp=sharing

Bug: 1055558
Change-Id: I46d3d73d193f86323fe5b9e4c90ceba989ab7abe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2219104
Commit-Queue: Alex Turner <alexmt@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: John Delaney <johnidel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816380}

chrome/browser/subresource_filter/subresource_filter_browsertest.cc

@@ -32,6 +32,7 @@
 #include "chrome/browser/subresource_filter/test_ruleset_publisher.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -818,6 +819,154 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_EQ(base::ASCIIToUTF16("failed"), title_watcher.WaitAndGetTitle());
 }
 
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+                       PopupsInheritActivation_ResourcesBlocked) {
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetWithRules({testing::CreateSuffixRule("ad=true")}));
+
+  // Block disallowed resources.
+  Configuration config(subresource_filter::mojom::ActivationLevel::kEnabled,
+                       subresource_filter::ActivationScope::ALL_SITES);
+  ResetConfiguration(std::move(config));
+
+  const std::vector<std::string> test_case_scripts = {
+      // Popup to URL
+      "window.open('/subresource_filter/popup.html');",
+
+      // Popup to empty URL
+      "popupLoadsDisallowedResource('');",
+
+      // Child of popup to empty URL
+      "popupLoadsDisallowedResourceAsDescendant('');",
+
+      // Popup to about:blank URL. about:blank popups behave differently to
+      // popups with an empty URL, so we test them separately.
+      "popupLoadsDisallowedResource('about:blank');",
+
+      // Child of popup to about:blank URL
+      "popupLoadsDisallowedResourceAsDescendant('about:blank');",
+
+      // Popup with doc.write-aborted load
+      "popupLoadsDisallowedResource('http://b.com/slow?100');",
+
+      // TODO(alexmt): Enable this test case. Currently disabled as there is no
+      // guarantee that the descendant's navigation starts after the parent's
+      // navigation ends (see crbug.com/1101569).
+      // Child of popup with doc.write-aborted load
+      // "popupLoadsDisallowedResourceAsDescendant('http://b.com/slow?100');",
+
+  };
+
+  for (const auto& test_case_script : test_case_scripts) {
+    content::WebContentsAddedObserver popup_observer;
+    ui_test_utils::NavigateToURL(
+        browser(),
+        embedded_test_server()->GetURL(
+            "/subresource_filter/popup_disallowed_load_helper.html"));
+    ASSERT_TRUE(ExecJs(web_contents(), test_case_script));
+    content::TitleWatcher title_watcher(popup_observer.GetWebContents(),
+                                        base::ASCIIToUTF16("failed"));
+    title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16("loaded"));
+
+    // Check the load was blocked.
+    EXPECT_EQ(base::ASCIIToUTF16("failed"), title_watcher.WaitAndGetTitle());
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+                       PopupNavigatesBackToAboutBlank_FilterChecked) {
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetWithRules({testing::CreateSuffixRule("ad=true")}));
+
+  // Block disallowed resources.
+  Configuration config(subresource_filter::mojom::ActivationLevel::kEnabled,
+                       subresource_filter::ActivationScope::ALL_SITES);
+  ResetConfiguration(std::move(config));
+
+  content::WebContentsAddedObserver popup_observer;
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+
+  ASSERT_TRUE(ExecJs(web_contents(),
+                     content::JsReplace("popup = window.open($1, 'name1');",
+                                        embedded_test_server()->GetURL(
+                                            "b.com", "/title2.html"))));
+
+  {
+    content::TitleWatcher title_watcher(
+        popup_observer.GetWebContents(),
+        base::ASCIIToUTF16("Title Of Awesomeness"));
+    // Wait for popup to finish loading
+    EXPECT_EQ(base::ASCIIToUTF16("Title Of Awesomeness"),
+              title_watcher.WaitAndGetTitle());
+  }
+
+  ui_test_utils::NavigateToURL(
+      chrome::FindBrowserWithWebContents(popup_observer.GetWebContents()),
+      GURL("about:blank"));
+
+  ASSERT_TRUE(ExecJs(web_contents(), R"SCRIPT(
+    // Get reference to popup without changing its location.
+    popup = window.open('', 'name1');
+    doc = popup.document;
+    doc.open();
+    doc.write(
+      "<html><body>Rewritten. <img src='/ad_tagging/pixel.png?ad=true' " +
+      "onload='window.document.title = \"loaded\";' " +
+      "onerror='window.document.title = \"failed\";'></body></html>");
+    doc.close();
+    )SCRIPT"));
+
+  content::TitleWatcher title_watcher(popup_observer.GetWebContents(),
+                                      base::ASCIIToUTF16("failed"));
+  title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16("loaded"));
+
+  // Check the load was blocked.
+  EXPECT_EQ(base::ASCIIToUTF16("failed"), title_watcher.WaitAndGetTitle());
+}
+
+// Test that resources in a popup with an aborted initial load due to a
+// doc.write are still blocked when disallowed, even if the opener is
+// immediately closed after writing.
+IN_PROC_BROWSER_TEST_F(
+    SubresourceFilterBrowserTest,
+    PopupWithDocWriteAbortedLoadAndOpenerClosed_FilterChecked) {
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetWithRules({testing::CreateSuffixRule("ad_script.js"),
+                           testing::CreateSuffixRule("ad=true")}));
+
+  // Block disallowed resources.
+  Configuration config(subresource_filter::mojom::ActivationLevel::kEnabled,
+                       subresource_filter::ActivationScope::ALL_SITES);
+  ResetConfiguration(std::move(config));
+
+  content::WebContents* original_web_contents = web_contents();
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));
+
+  content::WebContentsAddedObserver popup_observer;
+  ASSERT_TRUE(ExecJs(original_web_contents, R"SCRIPT(
+    popup = window.open('http://b.com/slow?100');
+    window.onunload = function(e){
+      doc = popup.document;
+      doc.open();
+      doc.write(
+        "<html><body>Rewritten. <img src='/ad_tagging/pixel.png?ad=true' " +
+        "onload='window.document.title = \"loaded\";' " +
+        "onerror='window.document.title = \"failed\";'></body></html>");
+      doc.close();
+    };
+    )SCRIPT"));
+  original_web_contents->ClosePage();
+
+  content::TitleWatcher title_watcher(popup_observer.GetWebContents(),
+                                      base::ASCIIToUTF16("failed"));
+  title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16("loaded"));
+
+  // Check the load was blocked.
+  EXPECT_EQ(base::ASCIIToUTF16("failed"), title_watcher.WaitAndGetTitle());
+}
+
 // Tests checking how histograms are recorded. ---------------------------------
 
 namespace {

chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc

@@ -154,6 +154,9 @@ void SubresourceFilterContentSettingsManager::SetSiteMetadataForTesting(
 void SubresourceFilterContentSettingsManager::SetSiteMetadata(
     const GURL& url,
     std::unique_ptr<base::DictionaryValue> dict) {
+  if (url.is_empty())
+    return;
+
   // Metadata expires after kMaxPersistMetadataDuration by default. If
   // kNonRenewingExpiryTime was previously set, then we are storing ads
   // intervention metadata and should not override the expiry time that

chrome/browser/subresource_filter/subresource_filter_popup_browsertest.cc

@@ -317,8 +317,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterPopupBrowserTest,
       web_contents(), "openWindow()", &opened_window));
   EXPECT_TRUE(opened_window);
 
-  // On the new window, requests should be allowed.
-  EXPECT_FALSE(AreDisallowedRequestsBlocked());
+  // On the new window, requests should be blocked due to the popup inheriting
+  // the activation state.
+  EXPECT_TRUE(AreDisallowedRequestsBlocked());
 
   RoundTripAndVerifyLogMessages(console_observer, web_contents(),
                                 {kActivationConsoleMessage},

chrome/test/data/subresource_filter/popup.html

+<html>
+  <body>
+    <img src='/subresource_filter/pixel.png?ad=true' onload='document.title = "loaded";' onerror='document.title = "failed";'>
+  </body>
+</html>
\ No newline at end of file

chrome/test/data/subresource_filter/popup_disallowed_load_helper.html

+<html>
+  <head><script>
+    function popupLoadsDisallowedResource(popup_url) {
+      popup = window.open(popup_url);
+      doc = popup.document;
+      doc.open();
+      doc.write(
+        "<html><body>Rewritten. <img src='/ad_tagging/pixel.png?ad=true' " +
+        "onload='window.document.title = \"loaded\";' " +
+        "onerror='window.document.title = \"failed\";'></body></html>");
+      doc.close();
+    }
+    function popupLoadsDisallowedResourceAsDescendant(popup_url) {
+      popup = window.open(popup_url);
+      doc = popup.document;
+      doc.open();
+      doc.write(
+          "<html><head><script>" +
+          "function onLoadHandler() {" +
+          "  iframe = document.getElementById('child');" +
+          "  if (iframe.contentDocument) {" +
+          "     window.document.title = 'loaded';" +
+          "  } else {" +
+          "     window.document.title = 'failed';" +
+          "  }}</scr" + "ipt></head>" +
+          "<body>Rewritten. <iframe id='child' src='/empty.html?ad=true' " +
+          "onload='onLoadHandler();'></body></html>");
+      doc.close();
+    }
+  </script></head>
+</html>
\ No newline at end of file

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h

@@ -189,6 +189,16 @@ class ContentSubresourceFilterThrottleManager
   AsyncDocumentSubresourceFilter* GetParentFrameFilter(
       content::NavigationHandle* child_frame_navigation);
 
+  // Returns nullptr if the frame is not activated (and therefore has no
+  // subresource filter).
+  AsyncDocumentSubresourceFilter* GetFrameFilter(
+      content::RenderFrameHost* frame_host);
+
+  // Returns the activation state of the frame's filter. If the frame is not
+  // activated (and therefore has no subresource filter), returns base::nullopt.
+  const base::Optional<subresource_filter::mojom::ActivationState>
+  GetFrameActivationState(content::RenderFrameHost* frame_host);
+
   // Calls ShowNotification on |client_| at most once per committed,
   // non-same-page navigation in the main frame.
   void MaybeShowNotification();
@@ -206,12 +216,14 @@ class ContentSubresourceFilterThrottleManager
   void SetDocumentLoadStatistics(
       mojom::DocumentLoadStatisticsPtr statistics) override;
 
-  // Adds the navigation's RenderFrameHost to activated_frame_hosts_ if it is a
-  // special navigation which did not go through navigation throttles and its
-  // parent frame is activated as well. The filter for these frames is set
-  // to nullptr.
-  void MaybeActivateSubframeSpecialUrls(
-      content::NavigationHandle* navigation_handle);
+  // Gets a filter for the navigation from |throttle|, creates and returns a new
+  // filter, or returns |nullptr|. Also updates |frame_host_filter_map_| as
+  // appropriate. |frame_host| is provided as |navigation_handle|'s getter
+  // cannot be used when the navigation has not committed.
+  AsyncDocumentSubresourceFilter* FilterForFinishedNavigation(
+      content::NavigationHandle* navigation_handle,
+      ActivationStateComputingNavigationThrottle* throttle,
+      content::RenderFrameHost* frame_host);
 
   // For each RenderFrameHost where the last committed load has subresource
   // filtering activated, owns the corresponding AsyncDocumentSubresourceFilter.
@@ -224,12 +236,13 @@ class ContentSubresourceFilterThrottleManager
       frame_host_filter_map_;
 
   // Set of RenderFrameHosts that have had at least one committed or aborted
-  // navigation. Main frames with only aborted navigations are not included.
+  // navigation.
   std::set<content::RenderFrameHost*> navigated_frames_;
 
   // For each ongoing navigation that requires activation state computation,
   // keeps track of the throttle that is carrying out that computation, so that
   // the result can be retrieved when the navigation is ready to commit.
+  // TODO(crbug.com/1134311): Key with navigation IDs instead of raw pointers.
   std::map<content::NavigationHandle*,
            ActivationStateComputingNavigationThrottle*>
       ongoing_activation_throttles_;

components/subresource_filter/content/common/subresource_filter_utils.cc

@@ -8,7 +8,7 @@
 
 namespace subresource_filter {
 
-bool ShouldUseParentActivation(const GURL& url) {
+bool ShouldInheritActivation(const GURL& url) {
   return !content::IsURLHandledByNetworkStack(url);
 }
 

components/subresource_filter/content/common/subresource_filter_utils.h

@@ -9,10 +9,14 @@ class GURL;
 
 namespace subresource_filter {
 
-// Subframe navigations matching these URLs/schemes will not trigger
-// ReadyToCommitNavigation in the browser process, so they must be treated
-// specially to maintain activation. Should only be invoked for subframes.
-bool ShouldUseParentActivation(const GURL& url);
+// Subframe navigations and initial mainframe navigations matching these URLs/
+// schemes will not trigger ReadyToCommitNavigation in the browser process, so
+// they must be treated specially to maintain activation. Each should inherit
+// the activation of its parent in the case of a subframe and its opener in the
+// case of a mainframe. This also accounts for the ability of the parent/opener
+// to affect the frame's content more directly, e.g. through document.write(),
+// even though these URLs won't match a filter list rule by themselves.
+bool ShouldInheritActivation(const GURL& url);
 
 }  // namespace subresource_filter
 

components/subresource_filter/content/renderer/subresource_filter_agent.cc

@@ -46,6 +46,18 @@ SubresourceFilterAgent::SubresourceFilterAgent(
   DCHECK(ruleset_dealer);
   // |render_frame| can be nullptr in unit tests.
   if (render_frame) {
+    // If a mainframe has an activated opener, we activate the initial empty
+    // document, which is created before this constructor. This ensures that a
+    // popup's final document is appropriately activated, even when the the
+    // initial navigation is aborted and there are no further documents created.
+    if (render_frame->IsMainFrame() &&
+        GetInheritedActivationState(render_frame).activation_level !=
+            mojom::ActivationLevel::kDisabled) {
+      const GURL& url = GetDocumentURL();
+      DCHECK(url.is_empty());
+      DCHECK(ShouldInheritActivation(url));
+      ConstructFilter(GetInheritedActivationState(render_frame), url);
+    }
     render_frame->GetAssociatedInterfaceRegistry()->AddInterface(
         base::BindRepeating(
             &SubresourceFilterAgent::OnSubresourceFilterAgentRequest,
@@ -101,16 +113,34 @@ void SubresourceFilterAgent::SetIsAdSubframe(
   render_frame()->GetWebFrame()->SetIsAdSubframe(ad_frame_type);
 }
 
-mojom::ActivationState SubresourceFilterAgent::GetParentActivationState(
+mojom::ActivationState SubresourceFilterAgent::GetInheritedActivationState(
     content::RenderFrame* render_frame) {
-  blink::WebFrame* parent =
-      render_frame ? render_frame->GetWebFrame()->Parent() : nullptr;
-  if (parent && parent->IsWebLocalFrame()) {
-    auto* agent = SubresourceFilterAgent::Get(
-        content::RenderFrame::FromWebFrame(parent->ToWebLocalFrame()));
+  DCHECK(ShouldInheritActivation(GetDocumentURL()));
+  if (!render_frame)
+    return mojom::ActivationState();
+
+  blink::WebFrame* frame_to_inherit_from =
+      render_frame->IsMainFrame() ? render_frame->GetWebFrame()->Opener()
+                                  : render_frame->GetWebFrame()->Parent();
+
+  if (!frame_to_inherit_from || !frame_to_inherit_from->IsWebLocalFrame())
+    return mojom::ActivationState();
+
+  // TODO(crbug.com/1134740): Add an IsSameOriginWith() function to
+  // WebSecurityOrigin to avoid unnecessary conversions to url::Origin.
+  url::Origin render_frame_origin =
+      render_frame->GetWebFrame()->GetSecurityOrigin();
+  url::Origin inherited_origin = frame_to_inherit_from->GetSecurityOrigin();
+
+  // Only inherit from same-origin frames.
+  if (render_frame_origin.IsSameOriginWith(inherited_origin)) {
+    auto* agent =
+        SubresourceFilterAgent::Get(content::RenderFrame::FromWebFrame(
+            frame_to_inherit_from->ToWebLocalFrame()));
     if (agent && agent->filter_for_last_created_document_)
       return agent->filter_for_last_created_document_->activation_state();
   }
+
   return mojom::ActivationState();
 }
 
@@ -173,8 +203,7 @@ void SubresourceFilterAgent::DidCreateNewDocument() {
   const bool should_record_histograms =
       !first_document_ &&
       !(IsMainFrame() && !url.SchemeIsHTTPOrHTTPS() && !url.SchemeIsFile());
-  if (first_document_) {
-    first_document_ = false;
+  if (first_document_ && !IsMainFrame()) {
     DCHECK(!filter_for_last_created_document_);
 
     // Local subframes will first create an initial empty document (with url
@@ -191,16 +220,11 @@ void SubresourceFilterAgent::DidCreateNewDocument() {
         SendFrameIsAdSubframe();
     }
   }
-
-  // Filter may outlive us, so reset the ad tracker.
-  if (filter_for_last_created_document_)
-    filter_for_last_created_document_->set_ad_resource_tracker(nullptr);
-  filter_for_last_created_document_.reset();
+  first_document_ = false;
 
   const mojom::ActivationState activation_state =
-      (!IsMainFrame() && ShouldUseParentActivation(url))
-          ? GetParentActivationState(render_frame())
-          : activation_state_for_next_document_;
+      ShouldInheritActivation(url) ? GetInheritedActivationState(render_frame())
+                                   : activation_state_for_next_document_;
 
   ResetInfoForNextDocument();
 
@@ -208,6 +232,17 @@ void SubresourceFilterAgent::DidCreateNewDocument() {
     RecordHistogramsOnFilterCreation(activation_state);
   }
 
+  ConstructFilter(activation_state, url);
+}
+
+void SubresourceFilterAgent::ConstructFilter(
+    const mojom::ActivationState activation_state,
+    const GURL& url) {
+  // Filter may outlive us, so reset the ad tracker.
+  if (filter_for_last_created_document_)
+    filter_for_last_created_document_->set_ad_resource_tracker(nullptr);
+  filter_for_last_created_document_.reset();
+
   if (activation_state.activation_level == mojom::ActivationLevel::kDisabled ||
       !ruleset_dealer_->IsRulesetFileAvailable())
     return;

components/subresource_filter/content/renderer/subresource_filter_agent.h

@@ -88,15 +88,22 @@ class SubresourceFilterAgent
       blink::mojom::AdFrameType ad_frame_type) override;
 
  private:
-  // Assumes that the parent will be in a local frame relative to this one, upon
-  // construction.
-  virtual mojom::ActivationState GetParentActivationState(
+  // Returns the activation state for the |render_frame| to inherit. Main frames
+  // inherit from their opener frames, and subframes inherit from their parent
+  // frames. Assumes that the parent/opener is in a local frame relative to this
+  // one, upon construction. This function is virtual (and not static) to ease
+  // testing.
+  // TODO(crbug.com/1134747): Modify the test harness to allow this to be static
+  virtual mojom::ActivationState GetInheritedActivationState(
       content::RenderFrame* render_frame);
 
   void RecordHistogramsOnFilterCreation(
       const mojom::ActivationState& activation_state);
   void ResetInfoForNextDocument();
 
+  void ConstructFilter(const mojom::ActivationState activation_state,
+                       const GURL& url);
+
   mojom::SubresourceFilterHost* GetSubresourceFilterHost();
 
   void OnSubresourceFilterAgentRequest(

components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc

@@ -82,10 +82,10 @@ class SubresourceFilterAgentUnderTest : public SubresourceFilterAgent {
     return std::move(last_injected_filter_);
   }
 
-  void SetParentActivationState(mojom::ActivationLevel level) {
+  void SetInheritedActivationState(mojom::ActivationLevel level) {
     mojom::ActivationState state;
     state.activation_level = level;
-    parent_activation_state_ = state;
+    inherited_activation_state_ = state;
   }
 
   void SimulateNonInitialLoad() { SetFirstDocument(false); }
@@ -93,15 +93,15 @@ class SubresourceFilterAgentUnderTest : public SubresourceFilterAgent {
   using SubresourceFilterAgent::ActivateForNextCommittedLoad;
 
  private:
-  mojom::ActivationState GetParentActivationState(
+  mojom::ActivationState GetInheritedActivationState(
       content::RenderFrame*) override {
-    return parent_activation_state_;
+    return inherited_activation_state_;
   }
 
   std::unique_ptr<blink::WebDocumentSubresourceFilter> last_injected_filter_;
   bool is_ad_subframe_ = false;
   bool is_main_frame_ = true;
-  mojom::ActivationState parent_activation_state_;
+  mojom::ActivationState inherited_activation_state_;
 
   DISALLOW_COPY_AND_ASSIGN(SubresourceFilterAgentUnderTest);
 };
@@ -568,9 +568,25 @@ TEST_F(SubresourceFilterAgentTest,
       SetTestRulesetToDisallowURLsWithPathSuffix("somethingNotMatched"));
 
   agent()->SetIsMainFrame(false);
-  agent()->SetParentActivationState(mojom::ActivationLevel::kEnabled);
+  agent()->SetInheritedActivationState(mojom::ActivationLevel::kEnabled);
+
+  EXPECT_CALL(*agent(), GetDocumentURL())
+      .WillOnce(::testing::Return(GURL("about:blank")));
+  EXPECT_CALL(*agent(), OnSetSubresourceFilterForCurrentDocumentCalled());
+  StartLoadAndSetActivationState(mojom::ActivationLevel::kEnabled);
+
+  ExpectNoSubresourceFilterGetsInjected();
+  agent_as_rfo()->DidFailProvisionalLoad();
+}
+
+TEST_F(SubresourceFilterAgentTest,
+       FailedInitialMainFrameLoad_FilterInjectedOnInitialDocumentCreation) {
+  ASSERT_NO_FATAL_FAILURE(
+      SetTestRulesetToDisallowURLsWithPathSuffix("somethingNotMatched"));
+
+  agent()->SetIsMainFrame(true);
+  agent()->SetInheritedActivationState(mojom::ActivationLevel::kEnabled);
 
-  // ExpectSubresourceFilterGetsInjected();
   EXPECT_CALL(*agent(), GetDocumentURL())
       .WillOnce(::testing::Return(GURL("about:blank")));
   EXPECT_CALL(*agent(), OnSetSubresourceFilterForCurrentDocumentCalled());
@@ -614,6 +630,7 @@ TEST_F(SubresourceFilterAgentTest, DryRun_FrameAlreadyTaggedAsAd) {
 
 TEST_F(SubresourceFilterAgentTest, DryRun_SendsFrameIsAdSubframe) {
   agent()->SetIsAdSubframe();
+  agent()->SetIsMainFrame(false);
   ExpectSendFrameIsAdSubframe();
 
   // Call DidCreateNewDocument twice and verify that SendFrameIsAdSubframe is