Unittests for the Safe Browsing Subresource filter navigation throttle

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

Index: components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc
diff --git a/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc b/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e244327a14e52a6be4e682e933bcde3567b82a4b
--- /dev/null
+++ b/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc
@@ -0,0 +1,313 @@
+// Copyright 2017 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 "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
+
+#include <memory>
+
+#include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
+#include "base/test/histogram_tester.h"
+#include "components/safe_browsing_db/test_database_manager.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
+#include "components/subresource_filter/content/browser/subresource_filter_client.h"
+#include "components/subresource_filter/core/browser/subresource_filter_features.h"

[engedy, 2017/04/06 13:50:38]

nit: Do we use this?

[melandory, 2017/04/07 09:09:44]

Yes, for feature setup

+#include "components/subresource_filter/core/browser/subresource_filter_features_test_support.h"
+#include "components/subresource_filter/core/common/activation_level.h"

[engedy, 2017/04/06 13:50:38]

nit: I think 17--20 are not used.

[melandory, 2017/04/07 09:09:44]

Done.

+#include "components/subresource_filter/core/common/activation_list.h"
+#include "components/subresource_filter/core/common/activation_state.h"
+#include "components/subresource_filter/core/common/test_ruleset_creator.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_renderer_host.h"
+#include "testing/gmock/include/gmock/gmock-generated-matchers.h"

[engedy, 2017/04/06 13:50:38]

nit: I think this is included by the one below.

[melandory, 2017/04/07 09:09:44]

Done.

+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace subresource_filter {
+
+namespace {
+
+char kURL[] = "http://example.test/";
+char kRedirectURL[] = "http://foo.test/";
+
+// Names of navigation chain patterns histogram.
+const char kMatchesPatternHistogramNameSubresourceFilterSuffix[] =
+    "SubresourceFilter.PageLoad.RedirectChainMatchPattern."
+    "SubresourceFilterOnly";
+const char kNavigationChainSizeSubresourceFilterSuffix[] =
+    "SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly";
+
+// Human readable representation of expected redirect chain match patterns.
+// The explanations for the buckets given for the following redirect chain:
+// A->B->C->D, where A is initial URL and D is a final URL.
+enum RedirectChainMatchPattern {
+  EMPTY,             // No histograms were recorded.
+  F0M0L1,            // D is a Safe Browsing match.
+  F0M1L0,            // B or C, or both are Safe Browsing matches.
+  F0M1L1,            // B or C, or both and D are Safe Browsing matches.
+  F1M0L0,            // A is Safe Browsing match
+  F1M0L1,            // A and D are Safe Browsing matches.
+  F1M1L0,            // B and/or C and A are Safe Browsing matches.
+  F1M1L1,            // B and/or C and A and D are Safe Browsing matches.
+  NO_REDIRECTS_HIT,  // Redirect chain consists of single URL, aka no redirects
+                     // has happened, and this URL was a Safe Browsing hit.
+  NUM_HIT_PATTERNS,
+};
+
+// Database manager that allows any URL to be configured as blacklisted for
+// testing.
+class FakeSafeBrowsingDatabaseManager
+    : public safe_browsing::TestSafeBrowsingDatabaseManager {
+ public:
+  FakeSafeBrowsingDatabaseManager() : simulate_timeout_(false) {}
+
+  void AddBlacklistedUrl(const GURL& url,
+                         safe_browsing::SBThreatType threat_type) {
+    url_to_threat_type_[url] = threat_type;
+  }
+
+  void SimulateTimeout() { simulate_timeout_ = true; }
+
+ protected:
+  ~FakeSafeBrowsingDatabaseManager() override {}
+
+  bool CheckUrlForSubresourceFilter(const GURL& url, Client* client) override {
+    if (simulate_timeout_)
+      return false;
+    if (!url_to_threat_type_.count(url))
+      return true;
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&Client::OnCheckBrowseUrlResult, base::Unretained(client),
+                   url, url_to_threat_type_[url],
+                   safe_browsing::ThreatMetadata()));
+    return false;
+  }
+
+  bool CheckResourceUrl(const GURL& url, Client* client) override {
+    return true;
+  }
+
+  bool IsSupported() const override { return true; }
+  bool ChecksAreAlwaysAsync() const override { return false; }
+  bool CanCheckResourceType(
+      content::ResourceType /* resource_type */) const override {
+    return true;
+  }
+
+  safe_browsing::ThreatSource GetThreatSource() const override {
+    return safe_browsing::ThreatSource::LOCAL_PVER4;
+  }
+
+  bool CheckExtensionIDs(const std::set<std::string>& extension_ids,
+                         Client* client) override {
+    return true;
+  }
+
+ private:
+  std::map<GURL, safe_browsing::SBThreatType> url_to_threat_type_;
+  bool simulate_timeout_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingDatabaseManager);
+};
+
+class MockSubresourceFilterClient
+    : public subresource_filter::SubresourceFilterClient {
+ public:
+  MockSubresourceFilterClient() {}
+
+  ~MockSubresourceFilterClient() override = default;
+
+  MOCK_METHOD1(ToggleNotificationVisibility, void(bool));
+  MOCK_METHOD1(IsWhitelistedByContentSettings, bool(const GURL&));
+  MOCK_METHOD1(WhitelistByContentSettings, void(const GURL&));
+  MOCK_METHOD0(GetRulesetDealer, VerifiedRulesetDealer::Handle*());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockSubresourceFilterClient);
+};
+
+// Used to simulate  WillProcessResonse for the factory.

[engedy, 2017/04/06 13:50:38]

phrasing nit:

// Throttle to call WillProcessResponse on the factory, which is otherwise
called by the ThrottleManager.

[melandory, 2017/04/07 09:09:44]

Done.

+class ForwardingNavigationThrottle : public content::NavigationThrottle {

[engedy, 2017/04/06 13:50:38]

naming nit: TestForwardingNavigationThrottle

[melandory, 2017/04/07 09:09:44]

Done.

+ public:
+  ForwardingNavigationThrottle(content::NavigationHandle* handle)
+      : content::NavigationThrottle(handle) {}
+  ~ForwardingNavigationThrottle() override {}
+
+  // content::NavigationThrottle:
+  content::NavigationThrottle::ThrottleCheckResult WillProcessResponse()
+      override {
+    content::WebContents* web_contents = navigation_handle()->GetWebContents();
+    ContentSubresourceFilterDriverFactory* factory =
+        ContentSubresourceFilterDriverFactory::FromWebContents(web_contents);
+    factory->WillProcessResponse(navigation_handle());
+    return content::NavigationThrottle::PROCEED;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ForwardingNavigationThrottle);
+};
+
+}  //  namespace
+
+class SubresourceFilterSafeBrowsingActivationThrottleTest
+    : public content::RenderViewHostTestHarness,
+      public content::WebContentsObserver {
+ public:
+  SubresourceFilterSafeBrowsingActivationThrottleTest()
+      : field_trial_list_(nullptr) {}
+  ~SubresourceFilterSafeBrowsingActivationThrottleTest() override {}
+
+  void SetUp() override {
+    content::RenderViewHostTestHarness::SetUp();
+    scoped_feature_toggle_.reset(
+        new testing::ScopedSubresourceFilterFeatureToggle(
+            base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
+            kActivationScopeActivationList, kActivationListSubresourceFilter));
+    auto client = base::MakeUnique<MockSubresourceFilterClient>();
+    ContentSubresourceFilterDriverFactory::CreateForWebContents(
+        RenderViewHostTestHarness::web_contents(), std::move(client));
+    fake_safe_browsing_database_ = new FakeSafeBrowsingDatabaseManager();
+    NavigateAndCommit(GURL(kURL));

[engedy, 2017/04/06 13:50:38]

Let's navigate to some other URL here, it's unclear if navigating to this same
URL again later influences the results.

(I think this navigation could be just removed?)

[melandory, 2017/04/07 09:09:44]

Done.

+    Observe(RenderViewHostTestHarness::web_contents());
+  }
+
+  ContentSubresourceFilterDriverFactory* factory() {
+    return ContentSubresourceFilterDriverFactory::FromWebContents(
+        RenderViewHostTestHarness::web_contents());
+  }
+
+  // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    ASSERT_TRUE(navigation_handle->IsInMainFrame());
+    navigation_handle->RegisterThrottleForTesting(
+        base::MakeUnique<SubresourceFilterSafeBrowsingActivationThrottle>(
+            navigation_handle, fake_safe_browsing_database_));
+    navigation_handle->RegisterThrottleForTesting(
+        base::MakeUnique<ForwardingNavigationThrottle>(navigation_handle));
+  }
+
+  void SimulateStartAndExpectProceed() {
+    navigation_simulator_->Start();
+    EXPECT_EQ(content::NavigationThrottle::PROCEED,
+              navigation_simulator_->GetLastThrottleCheckResult());
+  }
+
+  void SimulateRedirectAndExpectProceed(const GURL& new_url) {
+    navigation_simulator_->Redirect(new_url);
+    EXPECT_EQ(content::NavigationThrottle::PROCEED,
+              navigation_simulator_->GetLastThrottleCheckResult());
+  }
+
+  void SimulateCommitAndExpectProceed() {
+    navigation_simulator_->Commit();
+    EXPECT_EQ(content::NavigationThrottle::PROCEED,
+              navigation_simulator_->GetLastThrottleCheckResult());
+  }
+
+  void CreateTestNavigationForMainFrame(const GURL& first_url) {
+    navigation_simulator_ =
+        content::NavigationSimulator::CreateRendererInitiated(first_url,
+                                                              main_rfh());
+  }
+
+  void ConfigureAsSubresourceFilterOnlyURL(const GURL& url) {
+    fake_safe_browsing_database_->AddBlacklistedUrl(
+        url, safe_browsing::SB_THREAT_TYPE_SUBRESOURCE_FILTER);
+  }
+
+  void SimulateTimeout() { fake_safe_browsing_database_->SimulateTimeout(); }
+
+  const base::HistogramTester& tester() const { return tester_; }
+
+ private:
+  base::FieldTrialList field_trial_list_;
+  std::unique_ptr<content::NavigationSimulator> navigation_simulator_;
+  scoped_refptr<FakeSafeBrowsingDatabaseManager> fake_safe_browsing_database_;
+  std::unique_ptr<testing::ScopedSubresourceFilterFeatureToggle>

[engedy, 2017/04/06 13:50:38]

nit: Could you please move this next to |field_trial_list_|?

[melandory, 2017/04/07 09:09:44]

Done.

+      scoped_feature_toggle_;
+  base::HistogramTester tester_;
+
+  DISALLOW_COPY_AND_ASSIGN(SubresourceFilterSafeBrowsingActivationThrottleTest);
+};
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, NoHitNavigation) {

[engedy, 2017/04/06 13:50:38]

nit: Could we name these test cases using the pattern `scenario_result`, e.g.,
"ListNotMatched_NoActivation", "ListMatched_Activation",
"ListMatchedAfterRedirect_...".

[melandory, 2017/04/07 09:09:44]

Done.

+  const GURL url(kURL);
+  CreateTestNavigationForMainFrame(url);
+  SimulateStartAndExpectProceed();
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(ContentSubresourceFilterDriverFactory::ActivationDecision::
+                ACTIVATION_LIST_NOT_MATCHED,
+            factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
+                            0);
+  tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+}
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, HitNavigation) {
+  const GURL url(kURL);
+  ConfigureAsSubresourceFilterOnlyURL(url);
+  CreateTestNavigationForMainFrame(url);
+  SimulateStartAndExpectProceed();
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(
+      ContentSubresourceFilterDriverFactory::ActivationDecision::ACTIVATED,
+      factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectUniqueSample(
+      kMatchesPatternHistogramNameSubresourceFilterSuffix, NO_REDIRECTS_HIT, 1);
+  tester().ExpectUniqueSample(kNavigationChainSizeSubresourceFilterSuffix, 1,
+                              1);
+}
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
+       CheckRedirectsNoHit) {
+  const GURL url(kURL);
+  CreateTestNavigationForMainFrame(url);
+  SimulateStartAndExpectProceed();
+  SimulateRedirectAndExpectProceed(GURL(kRedirectURL));
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(ContentSubresourceFilterDriverFactory::ActivationDecision::
+                ACTIVATION_LIST_NOT_MATCHED,
+            factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
+                            0);
+  tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+}
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, CheckRedirectsHit) {
+  const GURL url(kURL);
+  ConfigureAsSubresourceFilterOnlyURL(GURL(kRedirectURL));
+  CreateTestNavigationForMainFrame(url);
+  SimulateStartAndExpectProceed();
+  SimulateRedirectAndExpectProceed(GURL(kRedirectURL));
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(
+      ContentSubresourceFilterDriverFactory::ActivationDecision::ACTIVATED,
+      factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectUniqueSample(
+      kMatchesPatternHistogramNameSubresourceFilterSuffix, F0M0L1, 1);
+  tester().ExpectUniqueSample(kNavigationChainSizeSubresourceFilterSuffix, 2,
+                              1);
+}
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, SimulateTimeout) {
+  const GURL url(kURL);
+  SimulateTimeout();
+  CreateTestNavigationForMainFrame(url);
+  SimulateStartAndExpectProceed();
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(ContentSubresourceFilterDriverFactory::ActivationDecision::
+                ACTIVATION_LIST_NOT_MATCHED,
+            factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
+                            0);
+  tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+}
+
+}  // namespace subresource_filter

[engedy, 2017/04/06 13:50:38]

Could you please add one more test that destroys the Navigation along with
corresponding throttles while the SB check is pending? (To be run by ASAN bots
to ensure no use-after-free.)

[melandory, 2017/04/07 09:09:44]

As per offline discussion, it's tricky to implement before we implement
different check logic, so TODO is added.