[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..041da9c23aaf750ac57fe9e9c242fc7c9f1b2248 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
@@ -4,21 +4,30 @@
 
 #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"
@@ -29,7 +38,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 +46,10 @@ const char kMatchesPatternHistogramNameSubresourceFilterSuffix[] =
     "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:
@@ -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_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,54 @@ 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_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.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&base::TestMockTimeTaskRunner::RunUntilIdle,
+                              base::Unretained(test_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 +215,64 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
 
   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);
@@ -253,6 +341,16 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
   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,
@@ -260,11 +358,127 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
   tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
                             0);
   tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0);
+  tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 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);
+}
+
+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);
 }
 
-// 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