Refactor subresource filter activation to support PlzNavigate cleanly.

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/test/histogram_tester.h"
 #include "components/safe_browsing_db/util.h"
 #include "components/subresource_filter/content/browser/content_subresource_filter_driver.h"
@@ skipping 125 matching lines @@
 };
 
 class MockSubresourceFilterDriver : public ContentSubresourceFilterDriver {
  public:
   explicit MockSubresourceFilterDriver(
       content::RenderFrameHost* render_frame_host)
       : ContentSubresourceFilterDriver(render_frame_host) {}
 
   ~MockSubresourceFilterDriver() override = default;
 
-  MOCK_METHOD3(ActivateForProvisionalLoad,
-               void(ActivationLevel, const GURL&, bool));
+  MOCK_METHOD2(ActivateForNextCommittedLoad, void(ActivationLevel, bool));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockSubresourceFilterDriver);
 };
 
 class MockSubresourceFilterClient : public SubresourceFilterClient {
  public:
   MockSubresourceFilterClient() {}
 
   ~MockSubresourceFilterClient() override = default;
@@ skipping 41 matching lines @@
     return ContentSubresourceFilterDriverFactory::FromWebContents(
         web_contents());
   }
 
   MockSubresourceFilterClient* client() { return client_; }
   MockSubresourceFilterDriver* driver() { return driver_; }
 
   MockSubresourceFilterDriver* subframe_driver() { return subframe_driver_; }
   content::RenderFrameHost* subframe_rfh() { return subframe_rfh_; }
 
+  void SimulateNavigationCommit(content::RenderFrameHost* rfh,
+                                const GURL& url) {
+    // RenderFrameHostTester currently only simulates a call to
+    // ReadyToCommitNavigation when browser-side-navigation is enabled.

[clamy, 2017/02/09 10:32:06]

Can you add a TODO to remove this function when crbug.com/688393 is fixed?
Thanks!

[engedy, 2017/02/09 13:42:01]

Done.

+    if (!content::IsBrowserSideNavigationEnabled()) {
+      factory()->ReadyToCommitNavigationInternal(rfh, url,
+                                                 false /* failed_navigation */);
+    }
+    content::RenderFrameHostTester::For(rfh)->SimulateNavigationCommit(url);
+  }
+
   void BlacklistURLWithRedirectsNavigateAndCommit(
       const std::vector<bool>& blacklisted_urls,
       const std::vector<GURL>& navigation_chain,
       safe_browsing::SBThreatType threat_type,
       safe_browsing::ThreatPatternType threat_type_metadata,
       RedirectChainMatchPattern expected_pattern,
       bool expected_activation) {
     base::HistogramTester tester;
     EXPECT_CALL(*client(), ToggleNotificationVisibility(false)).Times(1);
     content::RenderFrameHostTester* rfh_tester =
         content::RenderFrameHostTester::For(main_rfh());
 
     rfh_tester->SimulateNavigationStart(navigation_chain.front());
     if (blacklisted_urls.front()) {
       factory()->OnMainResourceMatchedSafeBrowsingBlacklist(
           navigation_chain.front(), navigation_chain, threat_type,
           threat_type_metadata);
     }
     ::testing::Mock::VerifyAndClearExpectations(client());
 
     for (size_t i = 1; i < navigation_chain.size(); ++i) {
       const GURL url = navigation_chain[i];
       if (i < blacklisted_urls.size() && blacklisted_urls[i]) {
         factory()->OnMainResourceMatchedSafeBrowsingBlacklist(
             url, navigation_chain, threat_type, threat_type_metadata);
       }
       rfh_tester->SimulateRedirect(url);
     }
-    EXPECT_CALL(*driver(),
-                ActivateForProvisionalLoad(::testing::_, ::testing::_,
-                                           expected_measure_performance()))
+
+    EXPECT_CALL(*driver(), ActivateForNextCommittedLoad(
+                               ::testing::_, expected_measure_performance()))
         .Times(expected_activation);
-    if (!content::IsBrowserSideNavigationEnabled()) {
-      factory()->ReadyToCommitNavigationInternal(main_rfh(),
-                                                 navigation_chain.back());
-    }
-
-    rfh_tester->SimulateNavigationCommit(navigation_chain.back());
+    SimulateNavigationCommit(main_rfh(), navigation_chain.back());
     ::testing::Mock::VerifyAndClearExpectations(driver());
 
     if (expected_pattern != EMPTY) {
       EXPECT_THAT(tester.GetAllSamples(kMatchesPatternHistogramName),
                   ::testing::ElementsAre(base::Bucket(expected_pattern, 1)));
       EXPECT_THAT(
           tester.GetAllSamples(kNavigationChainSize),
           ::testing::ElementsAre(base::Bucket(navigation_chain.size(), 1)));
 
     } else {
       EXPECT_THAT(tester.GetAllSamples(kMatchesPatternHistogramName),
                   ::testing::IsEmpty());
       EXPECT_THAT(tester.GetAllSamples(kNavigationChainSize),
                   ::testing::IsEmpty());
     }
   }
 
   void NavigateAndCommitSubframe(const GURL& url, bool expected_activation) {
     EXPECT_CALL(*subframe_driver(),
-                ActivateForProvisionalLoad(::testing::_, ::testing::_,
-                                           expected_measure_performance()))
+                ActivateForNextCommittedLoad(::testing::_,
+                                             expected_measure_performance()))
         .Times(expected_activation);
     EXPECT_CALL(*client(), ToggleNotificationVisibility(::testing::_)).Times(0);
 
-    factory()->ReadyToCommitNavigationInternal(subframe_rfh(), url);
+    content::RenderFrameHostTester::For(subframe_rfh())
+        ->SimulateNavigationStart(url);
+    SimulateNavigationCommit(subframe_rfh(), url);
     ::testing::Mock::VerifyAndClearExpectations(subframe_driver());
     ::testing::Mock::VerifyAndClearExpectations(client());
   }
 
   void NavigateAndExpectActivation(
       const std::vector<bool>& blacklisted_urls,
       const std::vector<GURL>& navigation_chain,
       safe_browsing::SBThreatType threat_type,
       safe_browsing::ThreatPatternType threat_type_metadata,
       RedirectChainMatchPattern expected_pattern,
@@ skipping 16 matching lines @@
         expected_pattern, expected_activation);
   }
 
   void EmulateDidDisallowFirstSubresourceMessage() {
     factory()->OnMessageReceived(
         SubresourceFilterHostMsg_DidDisallowFirstSubresource(
             main_rfh()->GetRoutingID()),
         main_rfh());
   }
 
+  void EmulateFailedNavigationAndExpectNoActivation(const GURL& url) {
+    EXPECT_CALL(*client(), ToggleNotificationVisibility(false)).Times(1);
+    EXPECT_CALL(*driver(),
+                ActivateForNextCommittedLoad(::testing::_, ::testing::_))
+        .Times(0);
+
+    // ReadyToCommitNavigation with browser-side navigation disabled is not
+    // called in production code. It is called with browser-side navigation

[clamy, 2017/02/09 10:32:06]

I'm not sure I understand this comment. We should call ReadyToCommitNavigation
both with and without PlzNavigate in production code. The issue is with the
testing code.

[engedy, 2017/02/09 13:42:01]

What I meant to write here: for *failed* navigations, ReadyToCommitNavigation is
invoked only for PlzNavigate, as otherwise the only hook into the network stack
is the NavigationResourceThrottle, and ResourceThrottle::WillProcessResponse is
not invoked by ThrottlingResourceHandler for failures such as network errors.

Clarified the comment.

+    // enabled, in which case RenderFrameHostTester already calls it.
+    content::RenderFrameHostTester* rfh_tester =
+        content::RenderFrameHostTester::For(main_rfh());
+    rfh_tester->SimulateNavigationStart(url);
+    rfh_tester->SimulateNavigationError(url, 403);
+    ::testing::Mock::VerifyAndClearExpectations(driver());
+    ::testing::Mock::VerifyAndClearExpectations(client());
+  }
+
   void EmulateInPageNavigation(const std::vector<bool>& blacklisted_urls,
                                RedirectChainMatchPattern expected_pattern,
                                bool expected_activation) {
     // This test examines the navigation with the following sequence of events:
     //   DidStartProvisional(main, "example.com")
     //   ReadyToCommitNavigation(“example.com”)
     //   DidCommitProvisional(main, "example.com")
     //   DidStartProvisional(sub, "example.com/login")
     //   DidCommitProvisional(sub, "example.com/login")
     //   DidCommitProvisional(main, "example.com#ref")
 
     NavigateAndExpectActivation(blacklisted_urls, {GURL(kExampleUrl)},
                                 expected_pattern, expected_activation);
-    EXPECT_CALL(*driver(), ActivateForProvisionalLoad(
-                               ::testing::_, ::testing::_, ::testing::_))
+    EXPECT_CALL(*driver(),
+                ActivateForNextCommittedLoad(::testing::_, ::testing::_))
         .Times(0);
     EXPECT_CALL(*client(), ToggleNotificationVisibility(::testing::_)).Times(0);
     content::RenderFrameHostTester::For(main_rfh())
         ->SimulateNavigationCommit(GURL(kExampleUrl));
     ::testing::Mock::VerifyAndClearExpectations(driver());
     ::testing::Mock::VerifyAndClearExpectations(client());
   }
 
  private:
   static bool expected_measure_performance() {
@@ skipping 102 matching lines @@
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
       kActivationScopeActivationList,
       kActivationListSocialEngineeringAdsInterstitial,
       "1" /* performance_measurement_rate */);
   EmulateInPageNavigation({true}, NO_REDIRECTS_HIT,
                           true /* expected_activation */);
 }
 
+TEST_F(ContentSubresourceFilterDriverFactoryTest, FailedNavigation) {
+  base::FieldTrialList field_trial_list(nullptr);
+  testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
+      base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
+      kActivationScopeAllSites);
+  const GURL url(kExampleUrl);
+  NavigateAndExpectActivation({false}, {url}, EMPTY,
+                              true /* expected_activation */);
+  EmulateFailedNavigationAndExpectNoActivation(url);
+}
+
 TEST_F(ContentSubresourceFilterDriverFactoryTest, RedirectPatternTest) {
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
       kActivationScopeActivationList,
       kActivationListSocialEngineeringAdsInterstitial);
   struct RedirectRedirectChainMatchPatternTestData {
     std::vector<bool> blacklisted_urls;
     std::vector<GURL> navigation_chain;
     RedirectChainMatchPattern hit_expected_pattern;
@@ skipping 188 matching lines @@
     ActivationScopeTest,
     ContentSubresourceFilterDriverFactoryActivationScopeTest,
     ::testing::ValuesIn(kActivationScopeTestData));
 
 INSTANTIATE_TEST_CASE_P(
     ActivationLevelTest,
     ContentSubresourceFilterDriverFactoryActivationLevelTest,
     ::testing::ValuesIn(kActivationLevelTestData));
 
 }  // namespace subresource_filter