[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>

[engedy, 2017/04/25 22:03:53]

nit: map, set, string all look unused.

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

 #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/test/test_mock_time_task_runner.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/fake_safe_browsing_database_manager.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.PageLoad.SafeBrowsingDelay";
+const char kSafeBrowsingCheckTime[] =
+    "SubresourceFilter.SafeBrowsing.CheckTime";
 
 // 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 19 matching lines @@
   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 12 matching lines @@
 class SubresourceFilterSafeBrowsingActivationThrottleTest
     : public content::RenderViewHostTestHarness,
       public content::WebContentsObserver {
  public:
   SubresourceFilterSafeBrowsingActivationThrottleTest()
       : field_trial_list_(nullptr) {}
   ~SubresourceFilterSafeBrowsingActivationThrottleTest() override {}
 
   void SetUp() override {
     content::RenderViewHostTestHarness::SetUp();
+
+    test_task_runner_ = new base::TestMockTimeTaskRunner();
     scoped_feature_toggle_.reset(
         new testing::ScopedSubresourceFilterFeatureToggle(
             base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
             kActivationScopeActivationList, kActivationListSubresourceFilter));
     // 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;
     navigation_handle->RegisterThrottleForTesting(
         base::MakeUnique<SubresourceFilterSafeBrowsingActivationThrottle>(
-            navigation_handle, fake_safe_browsing_database_));
+            navigation_handle, test_task_runner_,
+            fake_safe_browsing_database_));
     navigation_handle->RegisterThrottleForTesting(
         base::MakeUnique<TestForwardingNavigationThrottle>(navigation_handle));
   }
 
+  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() {
+    // Need to post a task to flush the IO thread because calling Commit()
+    // blocks until the throttle checks are complete.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&base::TestMockTimeTaskRunner::RunUntilIdle,
+                              base::Unretained(test_task_runner_.get())));
+    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() {

[engedy, 2017/04/25 22:03:54]

idea: As an alternative, have you considered using
ScopedMockTimeMessageLoopTaskRunner and passing in the same task runner for both
IO and UI threads? Would that allow us to somehow set a timeout on the FakeSBDB,
and have a test where we start deferring, but do not end up timing out? It looks
like this is not currently tested, but seems to be the case that would happen in
practice most often. 

[Charlie Harrison, 2017/04/26 14:30:25]

I don't think this is tenable because the NavigationSimulator uses RunLoop()
internally to wait for task completion, which is incompatible with
ScopedMockTimeMessageLoopTaskRunner.

Since the commit() code path posts a task to flush the IO thread run loop, we
still get in a situation where the defer path is taken and exercised without a
timeout (though I agree it would be nice to have finer grained control).

Let me add a TODO here to consider augmenting these tests + the
NavigationSimulator infrastructure to take a MockTimeTaskRunner as input, and
giving users control over it. The design will take some thought though, and may
block on crbug.com/703346

[engedy, 2017/04/26 14:56:39]

SGTM. Thanks for digging up that bug! :)

+    test_task_runner_->RunUntilIdle();

[engedy, 2017/04/25 22:03:54]

nit: s//test_io_task_runner/?

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+    base::RunLoop().RunUntilIdle();
+  }
+
   const base::HistogramTester& tester() const { return tester_; }
 
+  base::TestMockTimeTaskRunner* test_task_runner() const {
+    return test_task_runner_.get();
+  }
+
  private:
   base::FieldTrialList field_trial_list_;
   std::unique_ptr<testing::ScopedSubresourceFilterFeatureToggle>
       scoped_feature_toggle_;
+  scoped_refptr<base::TestMockTimeTaskRunner> test_task_runner_;
   std::unique_ptr<content::NavigationSimulator> navigation_simulator_;
   scoped_refptr<FakeSafeBrowsingDatabaseManager> fake_safe_browsing_database_;
   base::HistogramTester tester_;
-  content::NavigationHandle* navigation_handle_;
 
   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();

[engedy, 2017/04/25 22:03:53]

nit: Not very familiar with parameterized tests. Is it too early to do this in
the ctor?

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+    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,

[engedy, 2017/04/25 22:03:54]

Let's verify timing histograms here too!

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

                             0);
   tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
 }
 
 TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
        ListMatched_Activation) {
   const GURL url(kURL);
   ConfigureAsSubresourceFilterOnlyURL(url);
   CreateTestNavigationForMainFrame(url);
   SimulateStartAndExpectProceed();
@@ skipping 38 matching lines @@
   tester().ExpectUniqueSample(kNavigationChainSizeSubresourceFilterSuffix, 2,
                               1);
 }
 
 TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
        ListNotMatchedAndTimeout_NoActivation) {
   const GURL url(kURL);
   SimulateTimeout();
   CreateTestNavigationForMainFrame(url);
   SimulateStartAndExpectProceed();
+
+  // Flush the pending tasks on the IO thread, so the delayed task surely gets
+  // posted.
+  test_task_runner()->RunUntilIdle();
+
+  // Expect one delayed task, and fast forward time.
+  base::TimeDelta expected_delay =
+      SubresourceFilterSafeBrowsingClientRequest::kCheckURLTimeout;
+  EXPECT_EQ(expected_delay, test_task_runner()->NextPendingTaskDelay());
+  test_task_runner()->FastForwardBy(expected_delay);
   SimulateCommitAndExpectProceed();
   EXPECT_EQ(ContentSubresourceFilterDriverFactory::ActivationDecision::
                 ACTIVATION_LIST_NOT_MATCHED,
             factory()->GetActivationDecisionForLastCommittedPageLoad());
   tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
                             0);
   tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 1);
+  tester().ExpectTotalCount(kSafeBrowsingCheckTime, 1);
 }
 
-// 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);
+  tester().ExpectTotalCount(kSafeBrowsingCheckTime, 2);
+}
+
+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