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 0a49f61053ca70ba71e791330bd52b2510f6674c..b0f134d6d30a05cccb73f1b7fcf01de53782d8dd 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,20 +4,29 @@ |
#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" |
@@ -28,7 +37,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[] = |
@@ -36,6 +45,8 @@ const char kMatchesPatternHistogramNameSubresourceFilterSuffix[] = |
"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: |
@@ -72,19 +83,30 @@ class FakeSafeBrowsingDatabaseManager |
~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; |
} |
@@ -105,10 +127,14 @@ class FakeSafeBrowsingDatabaseManager |
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); |
}; |
@@ -132,7 +158,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 {} |
@@ -174,6 +200,11 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest |
Observe(RenderViewHostTestHarness::web_contents()); |
} |
+ void TearDown() override { |
+ RunUntilIdle(); |
+ content::RenderViewHostTestHarness::TearDown(); |
+ } |
+ |
ContentSubresourceFilterDriverFactory* factory() { |
return ContentSubresourceFilterDriverFactory::FromWebContents( |
RenderViewHostTestHarness::web_contents()); |
@@ -183,30 +214,57 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest |
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 SimulateStartAndExpectProceed() { |
+ void DidFinishNavigation( |
+ content::NavigationHandle* navigation_handle) override { |
+ mock_timer_ = nullptr; |
+ } |
+ |
+ 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() { |
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) { |
@@ -222,8 +280,21 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest |
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_; } |
+ |
private: |
base::FieldTrialList field_trial_list_; |
std::unique_ptr<testing::ScopedSubresourceFilterFeatureToggle> |
@@ -231,11 +302,48 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest |
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); |
@@ -304,6 +412,18 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, |
SimulateTimeout(); |
CreateTestNavigationForMainFrame(url); |
SimulateStartAndExpectProceed(); |
+ |
+ // This call blocks the current task until the timeout occurs after a few |
Charlie Harrison
2017/04/19 23:58:24
Comment could be clarified.
|
+ // seconds. Get around this by posting a task which fires the timer. |
+ 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, |
@@ -311,11 +431,127 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, |
tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix, |
0); |
tester().ExpectTotalCount(kNavigationChainSizeSubresourceFilterSuffix, 0); |
+ tester().ExpectTotalCount(kSafeBrowsingNavigationDelay, 1); |
+ tester().ExpectTimeBucketCount(kSafeBrowsingNavigationDelay, |
+ base::TimeDelta::FromMilliseconds(0), 0); |
+} |
+ |
+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); |
} |
-// 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 |