Make subresource filter activation agnostic of in-page navigations.

The Safe Browsing Subresource Filter used to reactivate in response to
same-page navigations, which resulted in spurious histogram samples being
recorded. In some edge cases, a same-page navigation in the main-frame also
resulted in no activation in subsequently created child frames.

This CL makes sure that the logic on both browser and renderer sides is
agnostic of in-page navigations.

BUG=637415

Review-Url: https://codereview.chromium.org/2574193002
Cr-Commit-Position: refs/heads/master@{#438584}

chrome/browser/subresource_filter/subresource_filter_browsertest.cc

@@ -8,6 +8,7 @@
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/path_service.h"
+#include "base/strings/string_piece.h"
 #include "base/test/histogram_tester.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/subresource_filter/test_ruleset_publisher.h"
@@ -31,6 +32,12 @@ namespace {
 const char kSubresourceFilterPromptHistogram[] =
     "SubresourceFilter.Prompt.NumVisibility";
 
+GURL GetURLWithFragment(const GURL& url, base::StringPiece fragment) {
+  GURL::Replacements replacements;
+  replacements.SetRefStr(fragment);
+  return url.ReplaceComponents(replacements);
+}
+
 }  // namespace
 
 namespace subresource_filter {
@@ -96,15 +103,32 @@ class SubresourceFilterBrowserTest : public InProcessBrowserTest {
     return nullptr;
   }
 
-  bool WasScriptResourceLoaded(content::RenderFrameHost* rfh) {
+  bool WasParsedScriptElementLoaded(content::RenderFrameHost* rfh) {
     DCHECK(rfh);
-    bool script_resource_was_loaded;
+    bool script_resource_was_loaded = false;
     EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
         rfh, "domAutomationController.send(!!document.scriptExecuted)",
         &script_resource_was_loaded));
     return script_resource_was_loaded;
   }
 
+  bool IsDynamicScriptElementLoaded(content::RenderFrameHost* rfh) {
+    DCHECK(rfh);
+    bool script_resource_was_loaded = false;
+    EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+        rfh, "insertScriptElementAndReportSuccess()",
+        &script_resource_was_loaded));
+    return script_resource_was_loaded;
+  }
+
+  void InsertDynamicFrameWithScript() {
+    bool frame_was_loaded = false;
+    ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+        web_contents()->GetMainFrame(), "insertFrameWithScriptAndNotify()",
+        &frame_was_loaded));
+    ASSERT_TRUE(frame_was_loaded);
+  }
+
   void SetRulesetToDisallowURLsWithPathSuffix(const std::string& suffix) {
     ASSERT_NO_FATAL_FAILURE(
         test_ruleset_publisher_.SetRulesetToDisallowURLsWithPathSuffix(suffix));
@@ -122,17 +146,37 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, MainFrameActivation) {
   ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(
       "suffix-that-does-not-match-anything"));
   ui_test_utils::NavigateToURL(browser(), url);
-  EXPECT_TRUE(WasScriptResourceLoaded(web_contents()->GetMainFrame()));
+  EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
 
   ASSERT_NO_FATAL_FAILURE(
       SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
   ui_test_utils::NavigateToURL(browser(), url);
-  EXPECT_FALSE(WasScriptResourceLoaded(web_contents()->GetMainFrame()));
+  EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
 
   // The main frame document should never be filtered.
   SetRulesetToDisallowURLsWithPathSuffix("frame_with_included_script.html");
   ui_test_utils::NavigateToURL(browser(), url);
-  EXPECT_TRUE(WasScriptResourceLoaded(web_contents()->GetMainFrame()));
+  EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
+}
+
+// There should be no document-level de-/reactivation happening on the renderer
+// side as a result of an in-page navigation.
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+                       DocumentActivationOutlivesSamePageNavigation) {
+  GURL url(GetTestUrl("subresource_filter/frame_with_delayed_script.html"));
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Deactivation would already detected by the IsDynamicScriptElementLoaded
+  // line alone. To ensure no reactivation, which would muddy up recorded
+  // histograms, also set a ruleset that allows everything. If there was
+  // reactivation, then this new ruleset would be picked up, once again causing
+  // the IsDynamicScriptElementLoaded check to fail.
+  ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(
+      "suffix-that-does-not-match-anything"));
+  ui_test_utils::NavigateToURL(browser(), GetURLWithFragment(url, "ref"));
+  EXPECT_FALSE(IsDynamicScriptElementLoaded(web_contents()->GetMainFrame()));
 }
 
 IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, SubFrameActivation) {
@@ -146,11 +190,29 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, SubFrameActivation) {
   for (const char* subframe_name : kSubframeNames) {
     content::RenderFrameHost* frame = FindFrameByName(subframe_name);
     ASSERT_TRUE(frame);
-    EXPECT_FALSE(WasScriptResourceLoaded(frame));
+    EXPECT_FALSE(WasParsedScriptElementLoaded(frame));
   }
   tester.ExpectBucketCount(kSubresourceFilterPromptHistogram, true, 1);
 }
 
+// The page-level activation state on the browser-side should not be reset when
+// a same-page navigation starts in the main frame. Verify this by dynamically
+// inserting a subframe afterwards, and still expecting activation.
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+                       PageLevelActivationOutlivesSamePageNavigation) {
+  GURL url(GetTestUrl("subresource_filter/frame_set.html"));
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  ui_test_utils::NavigateToURL(browser(), GetURLWithFragment(url, "ref"));
+
+  ASSERT_NO_FATAL_FAILURE(InsertDynamicFrameWithScript());
+  content::RenderFrameHost* dynamic_frame = FindFrameByName("dynamic");
+  ASSERT_TRUE(dynamic_frame);
+  EXPECT_FALSE(WasParsedScriptElementLoaded(dynamic_frame));
+}
+
 IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
                        PRE_MainFrameActivationOnStartup) {
   SetRulesetToDisallowURLsWithPathSuffix("included_script.js");
@@ -162,7 +224,7 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   // Verify that the ruleset persisted in the previous session is used for this
   // page load right after start-up.
   ui_test_utils::NavigateToURL(browser(), url);
-  EXPECT_FALSE(WasScriptResourceLoaded(web_contents()->GetMainFrame()));
+  EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
 }
 
 IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
@@ -172,7 +234,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   GURL url(GetTestUrl("subresource_filter/frame_set.html"));
   base::HistogramTester tester;
   ui_test_utils::NavigateToURL(browser(), url);
-  EXPECT_TRUE(ExecuteScript(FindFrameByName("three"), "runny()"));
+  tester.ExpectBucketCount(kSubresourceFilterPromptHistogram, true, 1);
+  // Check that the bubble is not shown again for this navigation.
+  EXPECT_FALSE(IsDynamicScriptElementLoaded(FindFrameByName("three")));
   tester.ExpectBucketCount(kSubresourceFilterPromptHistogram, true, 1);
   // Check that bubble is shown for new navigation.
   ui_test_utils::NavigateToURL(browser(), url);

chrome/test/data/subresource_filter/frame_set.html

 <html>
+  <head>
+  <script type="text/javascript">
+     function insertFrameWithScriptAndNotify() {
+       var frame = document.createElement("iframe");
+       frame.name = "dynamic";
+       frame.src = "frame_with_included_script.html";
+       frame.onload = () => window.domAutomationController.send(true)
+       frame.onerror = () => window.domAutomationController.send(false)
+       document.body.appendChild(frame);
+     }
+  </script>
+  </head>
   <body>
     <iframe name="one" src="frame_with_included_script.html"></iframe>
     <iframe name="two" src="frame_with_included_script.html"></iframe>

chrome/test/data/subresource_filter/frame_with_delayed_script.html

 <html>
   <head>
   <script type="text/javascript">
-     function runny() {
-          var s = document.createElement("script");
-          s.type = "text/javascript"
-          s.src = "included_script.js";
-          // Makes sure that the test would non-flakily fail if the subresource
-          // is allowed to load.
-          s.async = false;
-          document.head.appendChild(s);
-        }
+     function insertScriptElementAndReportSuccess() {
+       var s = document.createElement("script");
+       s.type = "text/javascript"
+       s.src = "included_script.js";
+       s.onload = () => { window.domAutomationController.send(true); }
+       s.onerror = () => { window.domAutomationController.send(false); }
+       document.head.appendChild(s);
+     }
   </script>
   </head>
 </html>

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

@@ -159,7 +159,7 @@ ContentSubresourceFilterDriverFactory::DriverFromFrameHost(
 
 void ContentSubresourceFilterDriverFactory::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (navigation_handle->IsInMainFrame()) {
+  if (navigation_handle->IsInMainFrame() && !navigation_handle->IsSamePage()) {
     navigation_chain_.clear();
     activation_list_matches_.clear();
     navigation_chain_.push_back(navigation_handle->GetURL());
@@ -187,6 +187,7 @@ void ContentSubresourceFilterDriverFactory::RenderFrameDeleted(
 
 void ContentSubresourceFilterDriverFactory::ReadyToCommitNavigation(
     content::NavigationHandle* navigation_handle) {
+  DCHECK(!navigation_handle->IsSamePage());
   content::RenderFrameHost* render_frame_host =
       navigation_handle->GetRenderFrameHost();
   GURL url = navigation_handle->GetURL();

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc

@@ -313,7 +313,7 @@ class ContentSubresourceFilterDriverFactoryTest
     EXPECT_CALL(*driver(), ActivateForProvisionalLoad(
                                ::testing::_, ::testing::_, ::testing::_))
         .Times(0);
-    EXPECT_CALL(*client(), ToggleNotificationVisibility(::testing::_)).Times(1);
+    EXPECT_CALL(*client(), ToggleNotificationVisibility(::testing::_)).Times(0);
     content::RenderFrameHostTester::For(main_rfh())
         ->SimulateNavigationCommit(GURL(kExampleUrl));
     ::testing::Mock::VerifyAndClearExpectations(driver());

components/subresource_filter/content/renderer/subresource_filter_agent.cc

@@ -143,6 +143,9 @@ void SubresourceFilterAgent::DidStartProvisionalLoad() {
 void SubresourceFilterAgent::DidCommitProvisionalLoad(
     bool is_new_navigation,
     bool is_same_page_navigation) {
+  if (is_same_page_navigation)
+    return;
+
   RecordHistogramsOnLoadCommitted();
   if (activation_state_for_provisional_load_ != ActivationState::DISABLED &&
       ruleset_dealer_->IsRulesetAvailable()) {
@@ -159,6 +162,7 @@ void SubresourceFilterAgent::DidCommitProvisionalLoad(
     filter_for_last_committed_load_ = filter->AsWeakPtr();
     SetSubresourceFilterForCommittedLoad(std::move(filter));
   }
+
   activation_state_for_provisional_load_ = ActivationState::DISABLED;
 }
 

components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc

@@ -121,6 +121,13 @@ class SubresourceFilterAgentTest : public ::testing::Test {
         true /* is_new_navigation */, false /* is_same_page_navigation */);
   }
 
+  void PerformSamePageNavigationWithoutSettingActivationState() {
+    agent_as_rfo()->DidStartProvisionalLoad();
+    agent_as_rfo()->DidCommitProvisionalLoad(
+        true /* is_new_navigation */, true /* is_same_page_navigation */);
+    // No DidFinishLoad is called in this case.
+  }
+
   void StartLoadAndSetActivationState(ActivationState activation_state,
                                       bool measure_performance = false) {
     agent_as_rfo()->DidStartProvisionalLoad();
@@ -238,10 +245,23 @@ TEST_F(SubresourceFilterAgentTest, Enabled_FilteringIsInEffectForOneLoad) {
   ExpectLoadAllowed(kTestSecondURL, true);
   FinishLoad();
 
+  // In-page navigation should not count as a new load.
+  ExpectNoSubresourceFilterGetsInjected();
+  ExpectNoSignalAboutFirstSubresourceDisallowed();
+  PerformSamePageNavigationWithoutSettingActivationState();
+  ExpectLoadAllowed(kTestFirstURL, false);
+  ExpectLoadAllowed(kTestSecondURL, true);
+
   ExpectNoSubresourceFilterGetsInjected();
   StartLoadWithoutSettingActivationState();
   FinishLoad();
 
+  // Resource loads after the in-page navigation should not be counted toward
+  // the figures below, as they came after the original page load event.
+  histogram_tester.ExpectUniqueSample(kSubresourcesTotal, 2, 1);
+  histogram_tester.ExpectUniqueSample(kSubresourcesEvaluated, 2, 1);
+  histogram_tester.ExpectUniqueSample(kSubresourcesMatchedRules, 1, 1);
+  histogram_tester.ExpectUniqueSample(kSubresourcesDisallowed, 1, 1);
   EXPECT_THAT(histogram_tester.GetAllSamples(kDocumentLoadActivationState),
               ::testing::ElementsAre(
                   base::Bucket(static_cast<int>(ActivationState::DISABLED), 1),
@@ -249,7 +269,7 @@ TEST_F(SubresourceFilterAgentTest, Enabled_FilteringIsInEffectForOneLoad) {
   histogram_tester.ExpectUniqueSample(kDocumentLoadRulesetIsAvailable, 1, 1);
 }
 
-TEST_F(SubresourceFilterAgentTest, Enabled_HistogramSamples) {
+TEST_F(SubresourceFilterAgentTest, Enabled_HistogramSamplesOverTwoLoads) {
   for (const bool measure_performance : {false, true}) {
     base::HistogramTester histogram_tester;
     ASSERT_NO_FATAL_FAILURE(