No longer assume subframes will have two documents created

Currently, the SubresourceFilterAgent assumes that frames will always
have two documents created unless that frame has an aborted initial
load. While this is current behavior in Chrome, it is contrary to the
spec, which specifies that frames with an unset src should also only
have one document created. (See crbug.com/778318 for fixing the bug.)
If Chrome correctly implemented the spec, under the current
implementation, no filter would ever be created for subframes with an
unset src, even though they can still fetch resources.

Accordingly, this change considers every created subframe document for
subresource filter creation. Initial empty documents that previously
never had filters will instead inherit their parent's activation state,
i.e. they will have a filter created unless their parent's activation
level is kDisabled. This change also allows us to remove the special
case for aborted initial loads introduced by crrev.com/c/2064474.

Bug: 1067021
Change-Id: I06a1fbb9550e0d83cddeefc6f56ebbf9a457cb71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2137511
Commit-Queue: Alex Turner <alexmt@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759308}

components/subresource_filter/content/renderer/subresource_filter_agent.cc

@@ -165,52 +165,49 @@ void SubresourceFilterAgent::OnDestruct() {
 }
 
 void SubresourceFilterAgent::DidCreateNewDocument() {
-  // TODO(csharrison): Use WebURL and WebSecurityOrigin for efficiency here and
-  // in MaybeCreateFilterForDocument, which requires changes to the unit tests.
+  // TODO(csharrison): Use WebURL and WebSecurityOrigin for efficiency here,
+  // which requires changes to the unit tests.
   const GURL& url = GetDocumentURL();
+  // Do not pollute the histograms with the empty main frame documents and
+  // initial empty documents, even though some will not have a second document.
+  const bool should_record_histograms =
+      !first_document_ &&
+      !(IsMainFrame() && !url.SchemeIsHTTPOrHTTPS() && !url.SchemeIsFile());
   if (first_document_) {
     first_document_ = false;
     DCHECK(!filter_for_last_created_document_);
 
     // Local subframes will first create an initial empty document (with url
     // kAboutBlankURL) no matter what the src is set to (or if it is not set).
-    // There is then a second document created with url equal to the set src (or
-    // kAboutBlankURL if unset). See DidFailProvisionalLoad for handling a
-    // failure in the load of this second document. Frames created by the
-    // browser initialize the LocalFrame before creating RenderFrameObservers,
-    // so the initial empty document isn't observed. We only care about local
-    // subframes.
+    // Then, if the src is set (including to kAboutBlankURL), there is a second
+    // document created with url equal to the set src. Due to a bug, currently
+    // a second document (with url kAboutBlankURL) is created even if the src is
+    // unset (see crbug.com/778318), but we should not rely on this erroneous
+    // behavior. Frames created by the browser initialize the LocalFrame before
+    // creating RenderFrameObservers, so the initial empty document isn't
+    // observed. We only care about local subframes.
     if (url == url::kAboutBlankURL) {
       if (IsAdSubframe())
         SendFrameIsAdSubframe();
-      return;
     }
   }
 
-  MaybeCreateFilterForDocument(ShouldUseParentActivation(url));
-}
-
-void SubresourceFilterAgent::MaybeCreateFilterForDocument(
-    bool should_use_parent_activation) {
   // 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();
 
-  const GURL& url = GetDocumentURL();
-
   const mojom::ActivationState activation_state =
-      (!IsMainFrame() && should_use_parent_activation)
+      (!IsMainFrame() && ShouldUseParentActivation(url))
           ? GetParentActivationState(render_frame())
           : activation_state_for_next_document_;
 
   ResetInfoForNextDocument();
 
-  // Do not pollute the histograms for empty main frame documents.
-  if (IsMainFrame() && !url.SchemeIsHTTPOrHTTPS() && !url.SchemeIsFile())
-    return;
+  if (should_record_histograms) {
+    RecordHistogramsOnFilterCreation(activation_state);
+  }
 
-  RecordHistogramsOnFilterCreation(activation_state);
   if (activation_state.activation_level == mojom::ActivationLevel::kDisabled ||
       !ruleset_dealer_->IsRulesetFileAvailable())
     return;
@@ -235,17 +232,6 @@ void SubresourceFilterAgent::MaybeCreateFilterForDocument(
 void SubresourceFilterAgent::DidFailProvisionalLoad() {
   // TODO(engedy): Add a test with `frame-ancestor` violation to exercise this.
   ResetInfoForNextDocument();
-
-  // This is necessary to account for when an initial frame load is aborted,
-  // for example due to a document.write or window.stop. We skip this if
-  // |filter_for_last_created_document_|, maintaining the prior behavior when a
-  // filter has already been created. We also skip if there is no document
-  // loader as it may have been detached due to a cross-origin navigation.
-  if (!filter_for_last_created_document_ && HasDocumentLoader()) {
-    // Use the parent as a committed load has not occurred so an activation
-    // state won't have been sent.
-    MaybeCreateFilterForDocument(true);
-  }
 }
 
 void SubresourceFilterAgent::DidFinishLoad() {

components/subresource_filter/content/renderer/subresource_filter_agent.h

@@ -58,9 +58,7 @@ class SubresourceFilterAgent
   virtual bool HasDocumentLoader();
 
   // Injects the provided subresource |filter| into the DocumentLoader
-  // orchestrating the most recently created document. If this method is called
-  // as the result of a new document element instead of a new document (e.g.
-  // due to a document.write), we reuse the previous DocumentLoader.
+  // orchestrating the most recently created document.
   virtual void SetSubresourceFilterForCurrentDocument(
       std::unique_ptr<blink::WebDocumentSubresourceFilter> filter);
 
@@ -80,6 +78,10 @@ class SubresourceFilterAgent
   virtual bool IsAdSubframe();
   virtual void SetIsAdSubframe(blink::mojom::AdFrameType ad_frame_type);
 
+  void SetFirstDocument(bool first_document) {
+    first_document_ = first_document;
+  }
+
   // mojom::SubresourceFilterAgent:
   void ActivateForNextCommittedLoad(
       mojom::ActivationStatePtr activation_state,
@@ -107,8 +109,6 @@ class SubresourceFilterAgent
   void DidFinishLoad() override;
   void WillCreateWorkerFetchContext(blink::WebWorkerFetchContext*) override;
 
-  void MaybeCreateFilterForDocument(bool should_use_parent_activation);
-
   // Owned by the ChromeContentRendererClient and outlives us.
   UnverifiedRulesetDealer* ruleset_dealer_;
 

components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc

@@ -88,6 +88,8 @@ class SubresourceFilterAgentUnderTest : public SubresourceFilterAgent {
     parent_activation_state_ = state;
   }
 
+  void SimulateNonInitialLoad() { SetFirstDocument(false); }
+
   using SubresourceFilterAgent::ActivateForNextCommittedLoad;
 
  private:
@@ -177,7 +179,7 @@ class SubresourceFilterAgentTest : public ::testing::Test {
   void FinishLoad() { agent_as_rfo()->DidFinishLoad(); }
 
   void ExpectSubresourceFilterGetsInjected() {
-    EXPECT_CALL(*agent(), GetDocumentURL()).Times(::testing::Between(1, 2));
+    EXPECT_CALL(*agent(), GetDocumentURL());
     EXPECT_CALL(*agent(), OnSetSubresourceFilterForCurrentDocumentCalled());
   }
 
@@ -240,6 +242,8 @@ class SubresourceFilterAgentTest : public ::testing::Test {
 };
 
 TEST_F(SubresourceFilterAgentTest, RulesetUnset_RulesetNotAvailable) {
+  // To record histograms, we need a non-initial load.
+  agent()->SimulateNonInitialLoad();
   base::HistogramTester histogram_tester;
   // Do not set ruleset.
   ExpectNoSubresourceFilterGetsInjected();
@@ -255,6 +259,8 @@ TEST_F(SubresourceFilterAgentTest, RulesetUnset_RulesetNotAvailable) {
 }
 
 TEST_F(SubresourceFilterAgentTest, DisabledByDefault_NoFilterIsInjected) {
+  // To record histograms, we need a non-initial load.
+  agent()->SimulateNonInitialLoad();
   base::HistogramTester histogram_tester;
   ASSERT_NO_FATAL_FAILURE(
       SetTestRulesetToDisallowURLsWithPathSuffix(kTestBothURLsPathSuffix));
@@ -297,6 +303,8 @@ TEST_F(SubresourceFilterAgentTest, Disabled_NoFilterIsInjected) {
 
 TEST_F(SubresourceFilterAgentTest,
        EnabledButRulesetUnavailable_NoFilterIsInjected) {
+  // To record histograms, we need a non-initial load.
+  agent()->SimulateNonInitialLoad();
   base::HistogramTester histogram_tester;
   ExpectNoSubresourceFilterGetsInjected();
   StartLoadAndSetActivationState(mojom::ActivationLevel::kEnabled);
@@ -315,6 +323,8 @@ TEST_F(SubresourceFilterAgentTest,
 // TODO(csharrison): Refactor these unit tests so it is easier to test with
 // real backing RenderFrames.
 TEST_F(SubresourceFilterAgentTest, EmptyDocumentLoad_NoFilterIsInjected) {
+  // To record histograms, we need a non-initial load.
+  agent()->SimulateNonInitialLoad();
   base::HistogramTester histogram_tester;
   ExpectNoSubresourceFilterGetsInjected();
   EXPECT_CALL(*agent(), GetDocumentURL())
@@ -329,6 +339,8 @@ TEST_F(SubresourceFilterAgentTest, EmptyDocumentLoad_NoFilterIsInjected) {
 }
 
 TEST_F(SubresourceFilterAgentTest, Enabled_FilteringIsInEffectForOneLoad) {
+  // To record histograms, we need a non-initial load.
+  agent()->SimulateNonInitialLoad();
   base::HistogramTester histogram_tester;
   ASSERT_NO_FATAL_FAILURE(
       SetTestRulesetToDisallowURLsWithPathSuffix(kTestFirstURLPathSuffix));
@@ -372,6 +384,8 @@ TEST_F(SubresourceFilterAgentTest, Enabled_FilteringIsInEffectForOneLoad) {
 
 TEST_F(SubresourceFilterAgentTest, Enabled_HistogramSamplesOverTwoLoads) {
   for (const bool measure_performance : {false, true}) {
+    // To record histograms, we need a non-initial load.
+    agent()->SimulateNonInitialLoad();
     base::HistogramTester histogram_tester;
     ASSERT_NO_FATAL_FAILURE(
         SetTestRulesetToDisallowURLsWithPathSuffix(kTestFirstURLPathSuffix));
@@ -470,6 +484,8 @@ TEST_F(SubresourceFilterAgentTest,
 }
 
 TEST_F(SubresourceFilterAgentTest, DryRun_ResourcesAreEvaluatedButNotFiltered) {
+  // To record histograms, we need a non-initial load.
+  agent()->SimulateNonInitialLoad();
   base::HistogramTester histogram_tester;
   ASSERT_NO_FATAL_FAILURE(
       SetTestRulesetToDisallowURLsWithPathSuffix(kTestFirstURLPathSuffix));
@@ -547,19 +563,21 @@ TEST_F(SubresourceFilterAgentTest,
   filter->ReportDisallowedLoad();
 }
 
-TEST_F(SubresourceFilterAgentTest, FailedInitialLoad_FilterInjectedOnFailure) {
+TEST_F(SubresourceFilterAgentTest,
+       FailedInitialLoad_FilterInjectedOnInitialDocumentCreation) {
   ASSERT_NO_FATAL_FAILURE(
       SetTestRulesetToDisallowURLsWithPathSuffix("somethingNotMatched"));
 
   agent()->SetIsMainFrame(false);
   agent()->SetParentActivationState(mojom::ActivationLevel::kEnabled);
 
-  ExpectNoSubresourceFilterGetsInjected();
+  // ExpectSubresourceFilterGetsInjected();
   EXPECT_CALL(*agent(), GetDocumentURL())
-      .WillRepeatedly(::testing::Return(GURL("about:blank")));
+      .WillOnce(::testing::Return(GURL("about:blank")));
+  EXPECT_CALL(*agent(), OnSetSubresourceFilterForCurrentDocumentCalled());
   StartLoadAndSetActivationState(mojom::ActivationLevel::kEnabled);
 
-  ExpectSubresourceFilterGetsInjected();
+  ExpectNoSubresourceFilterGetsInjected();
   agent_as_rfo()->DidFailProvisionalLoad();
 }
 
@@ -605,7 +623,7 @@ TEST_F(SubresourceFilterAgentTest, DryRun_SendsFrameIsAdSubframe) {
       .WillOnce(::testing::Return(GURL("about:blank")));
   agent_as_rfo()->DidCreateNewDocument();
   EXPECT_CALL(*agent(), GetDocumentURL())
-      .WillRepeatedly(::testing::Return(GURL("about:blank")));
+      .WillOnce(::testing::Return(GURL("about:blank")));
   agent_as_rfo()->DidCreateNewDocument();
 }