[subresource_filter] SB throttle can send multiple speculative requests.

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
index 3479cee5c95e8e55be56e03388fc59b8d5513b37..359cfc28ac6ecda828c44e94517703f02940860f 100644
--- 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
@@ -5,20 +5,27 @@
 #include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
 
 #include <memory>
+#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/content/browser/subresource_filter_safe_browsing_client_request.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"
@@ -29,7 +36,7 @@ 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[] =
@@ -37,6 +44,12 @@ const char kMatchesPatternHistogramNameSubresourceFilterSuffix[] =
     "SubresourceFilterOnly";
 const char kNavigationChainSizeSubresourceFilterSuffix[] =
     "SubresourceFilter.PageLoad.RedirectChainLength.SubresourceFilterOnly";
+const char kSafeBrowsingNavigationDelay[] =
+    "SubresourceFilter.PageLoad.SafeBrowsingDelay";
+const char kSafeBrowsingNavigationDelayNoSpeculation[] =
+    "SubresourceFilter.PageLoad.SafeBrowsingDelay.NoRedirectSpeculation";
+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:
@@ -76,7 +89,7 @@ class MockSubresourceFilterClient
 // called by the ThrottleManager.
 class TestForwardingNavigationThrottle : public content::NavigationThrottle {
  public:
-  TestForwardingNavigationThrottle(content::NavigationHandle* handle)
+  explicit TestForwardingNavigationThrottle(content::NavigationHandle* handle)
       : content::NavigationThrottle(handle) {}
   ~TestForwardingNavigationThrottle() override {}
 
@@ -109,6 +122,8 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
 
   void SetUp() override {
     content::RenderViewHostTestHarness::SetUp();
+
+    test_io_task_runner_ = new base::TestMockTimeTaskRunner();
     scoped_feature_toggle_.reset(
         new testing::ScopedSubresourceFilterFeatureToggle(
             base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
@@ -123,6 +138,11 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
     Observe(RenderViewHostTestHarness::web_contents());
   }
 
+  void TearDown() override {
+    RunUntilIdle();
+    content::RenderViewHostTestHarness::TearDown();
+  }
+
   ContentSubresourceFilterDriverFactory* factory() {
     return ContentSubresourceFilterDriverFactory::FromWebContents(
         RenderViewHostTestHarness::web_contents());
@@ -132,30 +152,57 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
   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_io_task_runner_,
+            fake_safe_browsing_database_));
     navigation_handle->RegisterThrottleForTesting(
         base::MakeUnique<TestForwardingNavigationThrottle>(navigation_handle));
   }
 
-  void SimulateStartAndExpectProceed() {
+  content::NavigationThrottle::ThrottleCheckResult SimulateStart() {
     navigation_simulator_->Start();
-    EXPECT_EQ(content::NavigationThrottle::PROCEED,
-              navigation_simulator_->GetLastThrottleCheckResult());
+    auto result = navigation_simulator_->GetLastThrottleCheckResult();
+    if (result == content::NavigationThrottle::CANCEL)
+      navigation_simulator_.reset();
+    return result;
   }
 
-  void SimulateRedirectAndExpectProceed(const GURL& new_url) {
+  content::NavigationThrottle::ThrottleCheckResult SimulateRedirect(
+      const GURL& new_url) {
     navigation_simulator_->Redirect(new_url);
-    EXPECT_EQ(content::NavigationThrottle::PROCEED,
-              navigation_simulator_->GetLastThrottleCheckResult());
+    auto result = navigation_simulator_->GetLastThrottleCheckResult();
+    if (result == content::NavigationThrottle::CANCEL)
+      navigation_simulator_.reset();
+    return result;
   }
 
-  void SimulateCommitAndExpectProceed() {
+  content::NavigationThrottle::ThrottleCheckResult SimulateCommit() {
+    // Need to post a task to flush the IO thread because calling Commit()
+    // blocks until the throttle checks are complete.
+    // TODO(csharrison): Consider adding finer grained control to the
+    // NavigationSimulator by giving it an option to be driven by a
+    // TestMockTimeTaskRunner. Also see https://crbug.com/703346.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&base::TestMockTimeTaskRunner::RunUntilIdle,
+                              base::Unretained(test_io_task_runner_.get())));
     navigation_simulator_->Commit();
-    EXPECT_EQ(content::NavigationThrottle::PROCEED,
-              navigation_simulator_->GetLastThrottleCheckResult());
+    auto result = navigation_simulator_->GetLastThrottleCheckResult();
+    if (result == content::NavigationThrottle::CANCEL)
+      navigation_simulator_.reset();
+    return result;
+  }
+
+  void SimulateStartAndExpectProceed() {
+    EXPECT_EQ(content::NavigationThrottle::PROCEED, SimulateStart());
+  }
+
+  void SimulateRedirectAndExpectProceed(const GURL& new_url) {
+    EXPECT_EQ(content::NavigationThrottle::PROCEED, SimulateRedirect(new_url));
+  }
+
+  void SimulateCommitAndExpectProceed() {
+    EXPECT_EQ(content::NavigationThrottle::PROCEED, SimulateCommit());
   }
 
   void CreateTestNavigationForMainFrame(const GURL& first_url) {
@@ -171,20 +218,65 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
 
   void SimulateTimeout() { fake_safe_browsing_database_->SimulateTimeout(); }
 
+  void RunUntilIdle() {
+    test_io_task_runner_->RunUntilIdle();
+    base::RunLoop().RunUntilIdle();
+  }
+
   const base::HistogramTester& tester() const { return tester_; }
 
+  base::TestMockTimeTaskRunner* test_io_task_runner() const {
+    return test_io_task_runner_.get();
+  }
+
  private:
   base::FieldTrialList field_trial_list_;
   std::unique_ptr<testing::ScopedSubresourceFilterFeatureToggle>
       scoped_feature_toggle_;
+  scoped_refptr<base::TestMockTimeTaskRunner> test_io_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() {
+    std::tie(cancel_time_, result_sync_) = GetParam();
+  }
+  ~SubresourceFilterSafeBrowsingActivationThrottleTestWithCancelling()
+      override {}
+
+  void DidStartNavigation(content::NavigationHandle* handle) override {
+    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);
@@ -197,6 +289,9 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
   tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
                             0);
   tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 1);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelayNoSpeculation, 1);
+  tester().ExpectTotalCount(kSafeBrowsingCheckTime, 1);
 }
 
 TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
@@ -253,6 +348,93 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
   SimulateTimeout();
   CreateTestNavigationForMainFrame(url);
   SimulateStartAndExpectProceed();
+
+  // Flush the pending tasks on the IO thread, so the delayed task surely gets
+  // posted.
+  test_io_task_runner()->RunUntilIdle();
+
+  // Expect one delayed task, and fast forward time.
+  base::TimeDelta expected_delay =
+      SubresourceFilterSafeBrowsingClientRequest::kCheckURLTimeout;
+  EXPECT_EQ(expected_delay, test_io_task_runner()->NextPendingTaskDelay());
+  test_io_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(kSafeBrowsingNavigationDelayNoSpeculation, 1);
+  tester().ExpectTotalCount(kSafeBrowsingCheckTime, 1);
+}
+
+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);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelayNoSpeculation, 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(kSafeBrowsingNavigationDelayNoSpeculation, 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,
@@ -260,11 +442,54 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
   tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
                             0);
   tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+  tester().ExpectTimeBucketCount(kSafeBrowsingNavigationDelay,
+                                 base::TimeDelta::FromMilliseconds(0), 1);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelayNoSpeculation, 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);
 }
 
-// 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.)
+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