[subresource_filter] Remove "pass-through" mode on throttle

This CL makes sure the safe browsing throttle always has a
database manager that can make checks. For the (extremely rare) case
where the manager is not supported on the platform, checks are
completely synchronously as SAFE.

A follow-up will remove DatabaseManager::CanCheckSubresourceFilter
from safe browsing internals (resolving the TODO here).

This CL changes when a throttle can be created, since we won't
create one unless there is a SafeBrowsing service. Most platforms
should have one though.

TBR=jkarlin@chromium.org

Change-Id: Ic4c2d174af8980d088507976ff903ee0bf9dff16
Reviewed-on: https://chromium-review.googlesource.com/1067152
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560259}

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

@@ -37,21 +37,6 @@
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ChromeSubresourceFilterClient);
 
-namespace {
-
-scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> GetDatabaseManager() {
-  safe_browsing::SafeBrowsingService* safe_browsing_service =
-      g_browser_process->safe_browsing_service();
-  bool has_supported_manager =
-      safe_browsing_service &&
-      safe_browsing_service->database_manager()->IsSupported() &&
-      safe_browsing_service->database_manager()->CanCheckSubresourceFilter();
-  return has_supported_manager ? safe_browsing_service->database_manager()
-                               : nullptr;
-}
-
-}  // namespace
-
 ChromeSubresourceFilterClient::ChromeSubresourceFilterClient(
     content::WebContents* web_contents)
     : web_contents_(web_contents) {
@@ -69,14 +54,16 @@ ChromeSubresourceFilterClient::~ChromeSubresourceFilterClient() {}
 void ChromeSubresourceFilterClient::MaybeAppendNavigationThrottles(
     content::NavigationHandle* navigation_handle,
     std::vector<std::unique_ptr<content::NavigationThrottle>>* throttles) {
-  if (navigation_handle->IsInMainFrame()) {
+  safe_browsing::SafeBrowsingService* safe_browsing_service =
+      g_browser_process->safe_browsing_service();
+  if (navigation_handle->IsInMainFrame() && safe_browsing_service) {
     throttles->push_back(
         std::make_unique<subresource_filter::
                              SubresourceFilterSafeBrowsingActivationThrottle>(
             navigation_handle, this,
             content::BrowserThread::GetTaskRunnerForThread(
                 content::BrowserThread::IO),
-            GetDatabaseManager()));
+            safe_browsing_service->database_manager()));
   }
 
   auto* driver_factory =

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

@@ -59,21 +59,17 @@ SubresourceFilterSafeBrowsingActivationThrottle::
             database_manager)
     : NavigationThrottle(handle),
       io_task_runner_(std::move(io_task_runner)),
-      // The throttle can be created without a valid database manager. If so, it
-      // becomes a pass-through throttle and should never defer.
-      database_client_(database_manager
-                           ? new SubresourceFilterSafeBrowsingClient(
-                                 std::move(database_manager),
-                                 AsWeakPtr(),
-                                 io_task_runner_,
-                                 base::ThreadTaskRunnerHandle::Get())
-                           : nullptr,
+      database_client_(new SubresourceFilterSafeBrowsingClient(
+                           std::move(database_manager),
+                           AsWeakPtr(),
+                           io_task_runner_,
+                           base::ThreadTaskRunnerHandle::Get()),
                        base::OnTaskRunnerDeleter(io_task_runner_)),
       client_(client) {
   DCHECK(handle->IsInMainFrame());
 
   CheckCurrentUrl();
-  DCHECK(!database_client_ || !check_results_.empty());
+  DCHECK(!check_results_.empty());
 }
 
 SubresourceFilterSafeBrowsingActivationThrottle::
@@ -124,15 +120,13 @@ SubresourceFilterSafeBrowsingActivationThrottle::
 content::NavigationThrottle::ThrottleCheckResult
 SubresourceFilterSafeBrowsingActivationThrottle::WillRedirectRequest() {
   CheckCurrentUrl();
-  DCHECK(!database_client_ || !check_results_.empty());
   return PROCEED;
 }
 
 content::NavigationThrottle::ThrottleCheckResult
 SubresourceFilterSafeBrowsingActivationThrottle::WillProcessResponse() {
-  DCHECK(!database_client_ || !check_results_.empty());
   // No need to defer the navigation if the check already happened.
-  if (!database_client_ || HasFinishedAllSafeBrowsingChecks()) {
+  if (HasFinishedAllSafeBrowsingChecks()) {
     NotifyResult();
     return PROCEED;
   }
@@ -169,8 +163,7 @@ void SubresourceFilterSafeBrowsingActivationThrottle::OnCheckUrlResultOnUI(
 }
 
 void SubresourceFilterSafeBrowsingActivationThrottle::CheckCurrentUrl() {
-  if (!database_client_)
-    return;
+  DCHECK(database_client_);
   check_start_times_.push_back(base::TimeTicks::Now());
   check_results_.emplace_back();
   size_t id = check_results_.size() - 1;
@@ -184,20 +177,18 @@ void SubresourceFilterSafeBrowsingActivationThrottle::NotifyResult() {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("loading"),
                "SubresourceFilterSafeBrowsingActivationThrottle::NotifyResult");
   // Compute the matched list and notify observers of the check result.
-  DCHECK(!database_client_ || !check_results_.empty());
+  DCHECK(!check_results_.empty());
   ActivationList matched_list = ActivationList::NONE;
   bool warning = false;
-  if (!check_results_.empty()) {
-    const auto& check_result = check_results_.back();
-    DCHECK(check_result.finished);
-    matched_list = GetListForThreatTypeAndMetadata(
-        check_result.threat_type, check_result.threat_metadata, &warning);
-    SubresourceFilterObserverManager::FromWebContents(
-        navigation_handle()->GetWebContents())
-        ->NotifySafeBrowsingCheckComplete(navigation_handle(),
-                                          check_result.threat_type,
-                                          check_result.threat_metadata);
-  }
+  const auto& check_result = check_results_.back();
+  DCHECK(check_result.finished);
+  matched_list = GetListForThreatTypeAndMetadata(
+      check_result.threat_type, check_result.threat_metadata, &warning);
+  SubresourceFilterObserverManager::FromWebContents(
+      navigation_handle()->GetWebContents())
+      ->NotifySafeBrowsingCheckComplete(navigation_handle(),
+                                        check_result.threat_type,
+                                        check_result.threat_metadata);
 
   Configuration matched_configuration;
   ActivationDecision activation_decision = ActivationDecision::UNKNOWN;

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

@@ -337,9 +337,6 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
     fake_safe_browsing_database_->RemoveAllBlacklistedUrls();
   }
 
-  // With a null database the throttle becomes pass-through.
-  void UsePassThroughThrottle() { fake_safe_browsing_database_ = nullptr; }
-
   void RunUntilIdle() {
     base::RunLoop().RunUntilIdle();
     test_io_task_runner_->RunUntilIdle();
@@ -473,26 +470,6 @@ class SubresourceFilterSafeBrowsingActivationThrottleScopeTest
       SubresourceFilterSafeBrowsingActivationThrottleScopeTest);
 };
 
-TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
-       PassThroughThrottle) {
-  UsePassThroughThrottle();
-  SimulateNavigateAndCommit({GURL(kURL), GURL(kRedirectURL)}, main_rfh());
-  EXPECT_EQ(ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET,
-            *observer()->GetPageActivationForLastCommittedLoad());
-
-  scoped_configuration()->ResetConfiguration(
-      Configuration(ActivationLevel::ENABLED, ActivationScope::ALL_SITES));
-  SimulateNavigateAndCommit({GURL(kURL), GURL(kRedirectURL)}, main_rfh());
-  EXPECT_EQ(ActivationDecision::ACTIVATED,
-            *observer()->GetPageActivationForLastCommittedLoad());
-
-  scoped_configuration()->ResetConfiguration(
-      Configuration(ActivationLevel::ENABLED, ActivationScope::NO_SITES));
-  SimulateNavigateAndCommit({GURL(kURL), GURL(kRedirectURL)}, main_rfh());
-  EXPECT_EQ(ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET,
-            *observer()->GetPageActivationForLastCommittedLoad());
-}
-
 TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, NoConfigs) {
   scoped_configuration()->ResetConfiguration(std::vector<Configuration>());
   SimulateNavigateAndCommit({GURL(kURL)}, main_rfh());

components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc

@@ -37,7 +37,9 @@ SubresourceFilterSafeBrowsingClient::SubresourceFilterSafeBrowsingClient(
     : database_manager_(std::move(database_manager)),
       throttle_(std::move(throttle)),
       io_task_runner_(std::move(io_task_runner)),
-      throttle_task_runner_(std::move(throttle_task_runner)) {}
+      throttle_task_runner_(std::move(throttle_task_runner)) {
+  DCHECK(database_manager_);
+}
 
 SubresourceFilterSafeBrowsingClient::~SubresourceFilterSafeBrowsingClient() {}
 

components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.cc

@@ -46,7 +46,13 @@ SubresourceFilterSafeBrowsingClientRequest::
 void SubresourceFilterSafeBrowsingClientRequest::Start(const GURL& url) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   start_time_ = base::TimeTicks::Now();
+
+  // Just return SAFE if the database is not supported.
+  // TODO(csharrison): Remove CanCheckSubresourceFilter now that V4 has fully
+  // shipped.
   bool synchronous_finish =
+      !database_manager_->IsSupported() ||
+      !database_manager_->CanCheckSubresourceFilter() ||
       database_manager_->CheckUrlForSubresourceFilter(url, this);
   if (synchronous_finish) {
     request_completed_ = true;