[subresource_filter] SB throttle can send multiple speculative requests.

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

 // 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 <map>
 #include <memory>
+#include <set>
+#include <string>
+#include <tuple>
+#include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
+#include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
+#include "base/timer/mock_timer.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/content/browser/subresource_filter_safe_browsing_client.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features_test_support.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/cancelling_navigation_throttle.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
 #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/";
+char kRedirectURL[] = "http://redirect.test/";
 
 // Names of navigation chain patterns histogram.
 const char kMatchesPatternHistogramNameSubresourceFilterSuffix[] =
     "SubresourceFilter.PageLoad.RedirectChainMatchPattern."
     "SubresourceFilterOnly";
 const char kNavigationChainSizeSubresourceFilterSuffix[] =
     "SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly";
+const char kSafeBrowsingNavigationDelay[] =
+    "SubresourceFilter.SafeBrowsing.NavigationDelay";
 
 // 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
@@ skipping 16 matching lines @@
                          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 {
+    // Maintain the invariant that each client only makes a single request.
+    DCHECK(checks_.find(client) == checks_.end());
     if (simulate_timeout_)
       return false;
     if (!url_to_threat_type_.count(url))
       return true;
 
+    checks_.insert(client);
+
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
-        base::Bind(&Client::OnCheckBrowseUrlResult, base::Unretained(client),
-                   url, url_to_threat_type_[url],
-                   safe_browsing::ThreatMetadata()));
+        base::Bind(&FakeSafeBrowsingDatabaseManager::
+                       OnCheckUrlForSubresourceFilterComplete,
+                   base::Unretained(this), base::Unretained(client), url));
     return false;
   }
 
+  void OnCheckUrlForSubresourceFilterComplete(Client* client, const GURL& url) {
+    if (checks_.find(client) == checks_.end())
+      return;
+    client->OnCheckBrowseUrlResult(url, url_to_threat_type_[url],
+                                   safe_browsing::ThreatMetadata());
+  }
+
   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;
   }
 
+  void CancelCheck(Client* client) override { checks_.erase(client); }
+
  private:
   std::map<GURL, safe_browsing::SBThreatType> url_to_threat_type_;
   bool simulate_timeout_;
 
+  std::set<Client*> checks_;
+
   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);
 };
 
 // Throttle to call WillProcessResponse on the factory, which is otherwise
 // called by the ThrottleManager.
 class TestForwardingNavigationThrottle : public content::NavigationThrottle {
  public:
-  TestForwardingNavigationThrottle(content::NavigationHandle* handle)
+  explicit TestForwardingNavigationThrottle(content::NavigationHandle* handle)
       : content::NavigationThrottle(handle) {}
   ~TestForwardingNavigationThrottle() 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());
@@ skipping 23 matching lines @@
     // Note: Using NiceMock to allow uninteresting calls and suppress warnings.
     auto client =
         base::MakeUnique<::testing::NiceMock<MockSubresourceFilterClient>>();
     ContentSubresourceFilterDriverFactory::CreateForWebContents(
         RenderViewHostTestHarness::web_contents(), std::move(client));
     fake_safe_browsing_database_ = new FakeSafeBrowsingDatabaseManager();
     NavigateAndCommit(GURL("https://test.com"));
     Observe(RenderViewHostTestHarness::web_contents());
   }
 
+  void TearDown() override {
+    RunUntilIdle();
+    content::RenderViewHostTestHarness::TearDown();
+  }
+
   ContentSubresourceFilterDriverFactory* factory() {
     return ContentSubresourceFilterDriverFactory::FromWebContents(
         RenderViewHostTestHarness::web_contents());
   }
 
   // content::WebContentsObserver:
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override {
     ASSERT_TRUE(navigation_handle->IsInMainFrame());
-    navigation_handle_ = navigation_handle;
+    auto mock_timer = base::MakeUnique<base::MockTimer>(
+        false /* retain_user_task */, false /* is_repeating */);
+    mock_timer_ = mock_timer.get();
     navigation_handle->RegisterThrottleForTesting(
         base::MakeUnique<SubresourceFilterSafeBrowsingActivationThrottle>(
-            navigation_handle, fake_safe_browsing_database_));
+            navigation_handle, std::move(mock_timer),
+            fake_safe_browsing_database_));
     navigation_handle->RegisterThrottleForTesting(
         base::MakeUnique<TestForwardingNavigationThrottle>(navigation_handle));
   }
 
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    mock_timer_ = nullptr;
+  }
+
+  content::NavigationThrottle::ThrottleCheckResult SimulateStart() {
+    navigation_simulator_->Start();
+    auto result = navigation_simulator_->GetLastThrottleCheckResult();
+    if (result == content::NavigationThrottle::CANCEL)
+      navigation_simulator_.reset();
+    return result;
+  }
+
+  content::NavigationThrottle::ThrottleCheckResult SimulateRedirect(
+      const GURL& new_url) {
+    navigation_simulator_->Redirect(new_url);
+    auto result = navigation_simulator_->GetLastThrottleCheckResult();
+    if (result == content::NavigationThrottle::CANCEL)
+      navigation_simulator_.reset();
+    return result;
+  }
+
+  content::NavigationThrottle::ThrottleCheckResult SimulateCommit() {
+    navigation_simulator_->Commit();
+    auto result = navigation_simulator_->GetLastThrottleCheckResult();
+    if (result == content::NavigationThrottle::CANCEL)
+      navigation_simulator_.reset();
+    return result;
+  }
+
   void SimulateStartAndExpectProceed() {
-    navigation_simulator_->Start();
-    EXPECT_EQ(content::NavigationThrottle::PROCEED,
-              navigation_simulator_->GetLastThrottleCheckResult());
+    EXPECT_EQ(content::NavigationThrottle::PROCEED, SimulateStart());
   }
 
   void SimulateRedirectAndExpectProceed(const GURL& new_url) {
-    navigation_simulator_->Redirect(new_url);
-    EXPECT_EQ(content::NavigationThrottle::PROCEED,
-              navigation_simulator_->GetLastThrottleCheckResult());
+    EXPECT_EQ(content::NavigationThrottle::PROCEED, SimulateRedirect(new_url));
   }
 
   void SimulateCommitAndExpectProceed() {
-    navigation_simulator_->Commit();
-    EXPECT_EQ(content::NavigationThrottle::PROCEED,
-              navigation_simulator_->GetLastThrottleCheckResult());
+    EXPECT_EQ(content::NavigationThrottle::PROCEED, SimulateCommit());
   }
 
   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(); }
 
+  void RunUntilIdle() {
+    base::RunLoop().RunUntilIdle();
+    auto callback = []() { base::RunLoop().RunUntilIdle(); };
+    base::RunLoop run_loop;
+    content::BrowserThread::PostTaskAndReply(content::BrowserThread::IO,
+                                             FROM_HERE, base::Bind(callback),
+                                             run_loop.QuitClosure());
+    run_loop.Run();
+    base::RunLoop().RunUntilIdle();
+  }
+
   const base::HistogramTester& tester() const { return tester_; }
 
+  base::MockTimer* mock_timer() { return mock_timer_; }

[engedy, 2017/04/20 18:56:23]

Without having read this unit test too thoroughly: Instead of mocking out the
entire timer, would it be possible to just mock out time by means of
ScopedMockTimeMessageLoopTaskRunner and passing in its MockClock? That could be
used to test timing constraint quite precisely, too.

+
  private:
   base::FieldTrialList field_trial_list_;
   std::unique_ptr<testing::ScopedSubresourceFilterFeatureToggle>
       scoped_feature_toggle_;
   std::unique_ptr<content::NavigationSimulator> navigation_simulator_;
   scoped_refptr<FakeSafeBrowsingDatabaseManager> fake_safe_browsing_database_;
   base::HistogramTester tester_;
-  content::NavigationHandle* navigation_handle_;
+
+  // Only valid during a navigation.
+  base::MockTimer* mock_timer_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(SubresourceFilterSafeBrowsingActivationThrottleTest);
 };
 
+class SubresourceFilterSafeBrowsingActivationThrottleTestWithCancelling
+    : public SubresourceFilterSafeBrowsingActivationThrottleTest,
+      public ::testing::WithParamInterface<
+          std::tuple<content::CancellingNavigationThrottle::CancelTime,
+                     content::CancellingNavigationThrottle::ResultSynchrony>> {
+ public:
+  SubresourceFilterSafeBrowsingActivationThrottleTestWithCancelling() {}
+  ~SubresourceFilterSafeBrowsingActivationThrottleTestWithCancelling()
+      override {}
+
+  void DidStartNavigation(content::NavigationHandle* handle) override {
+    std::tie(cancel_time_, result_sync_) = GetParam();
+    handle->RegisterThrottleForTesting(
+        base::MakeUnique<content::CancellingNavigationThrottle>(
+            handle, cancel_time_, result_sync_));
+    SubresourceFilterSafeBrowsingActivationThrottleTest::DidStartNavigation(
+        handle);
+  }
+
+  content::CancellingNavigationThrottle::CancelTime cancel_time() {
+    return cancel_time_;
+  }
+
+  content::CancellingNavigationThrottle::ResultSynchrony result_sync() {
+    return result_sync_;
+  }
+
+ private:
+  content::CancellingNavigationThrottle::CancelTime cancel_time_;
+  content::CancellingNavigationThrottle::ResultSynchrony result_sync_;
+
+  DISALLOW_COPY_AND_ASSIGN(
+      SubresourceFilterSafeBrowsingActivationThrottleTestWithCancelling);
+};
+
 TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
        ListNotMatched_NoActivation) {
   const GURL url(kURL);
   CreateTestNavigationForMainFrame(url);
   SimulateStartAndExpectProceed();
   SimulateCommitAndExpectProceed();
   EXPECT_EQ(ContentSubresourceFilterDriverFactory::ActivationDecision::
                 ACTIVATION_LIST_NOT_MATCHED,
             factory()->GetActivationDecisionForLastCommittedPageLoad());
   tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
@@ skipping 48 matching lines @@
   tester().ExpectUniqueSample(kNavigationChainSizeSubresourceFilterSuffix, 2,
                               1);
 }
 
 TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
        ListNotMatchedAndTimeout_NoActivation) {
   const GURL url(kURL);
   SimulateTimeout();
   CreateTestNavigationForMainFrame(url);
   SimulateStartAndExpectProceed();
+
+  // The call to simulate a commit blocks the current task waiting for the
+  // navigation to proceed. Get around this by posting a task which verifies the
+  // timer is correct and fires it.
+  auto fire_timer = [](base::MockTimer* timer) {
+    EXPECT_TRUE(timer->IsRunning());
+    EXPECT_EQ(SubresourceFilterSafeBrowsingClientRequest::kCheckURLTimeout,
+              timer->GetCurrentDelay());
+    timer->Fire();
+  };
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(fire_timer, base::Unretained(mock_timer())));
   SimulateCommitAndExpectProceed();
   EXPECT_EQ(ContentSubresourceFilterDriverFactory::ActivationDecision::
                 ACTIVATION_LIST_NOT_MATCHED,
             factory()->GetActivationDecisionForLastCommittedPageLoad());
   tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
                             0);
   tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 1);
+  tester().ExpectTimeBucketCount(kSafeBrowsingNavigationDelay,
+                                 base::TimeDelta::FromMilliseconds(0), 0);
 }
 
-// TODO(melandory): Once non-defering check in WillStart is implemented 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.)
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
+       ListMatchedOnStart_NoDelay) {
+  const GURL url(kURL);
+  ConfigureAsSubresourceFilterOnlyURL(url);
+  CreateTestNavigationForMainFrame(url);
+  SimulateStartAndExpectProceed();
+
+  // Get the database result back before commit.
+  RunUntilIdle();
+
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(
+      ContentSubresourceFilterDriverFactory::ActivationDecision::ACTIVATED,
+      factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectUniqueSample(
+      kMatchesPatternHistogramNameSubresourceFilterSuffix, NO_REDIRECTS_HIT, 1);
+  tester().ExpectUniqueSample(kNavigationChainSizeSubresourceFilterSuffix, 1,
+                              1);
+  tester().ExpectTimeBucketCount(kSafeBrowsingNavigationDelay,
+                                 base::TimeDelta::FromMilliseconds(0), 1);
+}
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
+       ListMatchedOnRedirect_NoDelay) {
+  const GURL url(kURL);
+  const GURL redirect_url(kRedirectURL);
+  ConfigureAsSubresourceFilterOnlyURL(redirect_url);
+  CreateTestNavigationForMainFrame(url);
+
+  SimulateStartAndExpectProceed();
+  SimulateRedirectAndExpectProceed(redirect_url);
+
+  // Get the database result back before commit.
+  RunUntilIdle();
+
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(
+      ContentSubresourceFilterDriverFactory::ActivationDecision::ACTIVATED,
+      factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectUniqueSample(
+      kMatchesPatternHistogramNameSubresourceFilterSuffix, F0M0L1, 1);
+  tester().ExpectUniqueSample(kNavigationChainSizeSubresourceFilterSuffix, 2,
+                              1);
+  tester().ExpectTimeBucketCount(kSafeBrowsingNavigationDelay,
+                                 base::TimeDelta::FromMilliseconds(0), 1);
+}
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
+       ListMatchedOnStartWithRedirect_NoActivation) {
+  const GURL url(kURL);
+  const GURL redirect_url(kRedirectURL);
+  ConfigureAsSubresourceFilterOnlyURL(url);
+  CreateTestNavigationForMainFrame(url);
+
+  // These two lines also test how the database client reacts to two requests
+  // happening one after another.
+  SimulateStartAndExpectProceed();
+  SimulateRedirectAndExpectProceed(redirect_url);
+
+  // Get the database result back before commit.
+  RunUntilIdle();
+
+  SimulateCommitAndExpectProceed();
+  EXPECT_EQ(ContentSubresourceFilterDriverFactory::ActivationDecision::
+                ACTIVATION_LIST_NOT_MATCHED,
+            factory()->GetActivationDecisionForLastCommittedPageLoad());
+  tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
+                            0);
+  tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+  tester().ExpectTimeBucketCount(kSafeBrowsingNavigationDelay,
+                                 base::TimeDelta::FromMilliseconds(0), 1);
+}
+
+TEST_P(SubresourceFilterSafeBrowsingActivationThrottleTestWithCancelling,
+       Cancel) {
+  const GURL url(kURL);
+  SCOPED_TRACE(::testing::Message() << "CancelTime: " << cancel_time()
+                                    << " ResultSynchrony: " << result_sync());
+  ConfigureAsSubresourceFilterOnlyURL(url);
+  CreateTestNavigationForMainFrame(url);
+
+  content::NavigationThrottle::ThrottleCheckResult result = SimulateStart();
+  if (cancel_time() ==
+      content::CancellingNavigationThrottle::WILL_START_REQUEST) {
+    EXPECT_EQ(content::NavigationThrottle::CANCEL, result);
+    tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 0);
+    return;
+  }
+  EXPECT_EQ(content::NavigationThrottle::PROCEED, result);
+
+  result = SimulateRedirect(GURL(kRedirectURL));
+  if (cancel_time() ==
+      content::CancellingNavigationThrottle::WILL_REDIRECT_REQUEST) {
+    EXPECT_EQ(content::NavigationThrottle::CANCEL, result);
+    tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 0);
+    return;
+  }
+  EXPECT_EQ(content::NavigationThrottle::PROCEED, result);
+
+  base::RunLoop().RunUntilIdle();
+
+  result = SimulateCommit();
+  EXPECT_EQ(content::NavigationThrottle::CANCEL, result);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 0);
+}
+
+INSTANTIATE_TEST_CASE_P(
+    CancelMethod,
+    SubresourceFilterSafeBrowsingActivationThrottleTestWithCancelling,
+    ::testing::Combine(
+        ::testing::Values(
+            content::CancellingNavigationThrottle::WILL_START_REQUEST,
+            content::CancellingNavigationThrottle::WILL_REDIRECT_REQUEST,
+            content::CancellingNavigationThrottle::WILL_PROCESS_RESPONSE),
+        ::testing::Values(
+            content::CancellingNavigationThrottle::SYNCHRONOUS,
+            content::CancellingNavigationThrottle::ASYNCHRONOUS)));
 
 }  // namespace subresource_filter