[subresource_filter] Do not check V4UsageStatus on Android

The V4UsageStatus is a desktop-specific check, because Android is already
shipped with V4.

Additionally, this exposed an issue with our implementation when there
is a remote db: synchronous failure (e.g. when visiting chrome:// urls)
cause our db client to cancel a non-existent request in its destructor.

This is easily fixed by marking request_complete_ in that code path.

Another small (but important!) fix is that the Android check for the
subresource filter also includes the phishing list.

Bug: 609747
Change-Id: I477361117380fcb7e14b30a677b24c5d41027e1c
Reviewed-on: https://chromium-review.googlesource.com/528393Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478772}

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

@@ -35,6 +35,26 @@
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ChromeSubresourceFilterClient);
 
+namespace {
+
+// V4 is already enabled by default on Android, do not check the V4UsageStatus
+// which performs desktop-only checks!
+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();
+#if !defined(OS_ANDROID)
+  has_supported_manager &= safe_browsing::V4FeatureList::GetV4UsageStatus() ==
+                           safe_browsing::V4FeatureList::V4UsageStatus::V4_ONLY;
+#endif
+  return has_supported_manager ? safe_browsing_service->database_manager()
+                               : nullptr;
+}
+
+}  // namespace
+
 ChromeSubresourceFilterClient::ChromeSubresourceFilterClient(
     content::WebContents* web_contents)
     : web_contents_(web_contents), did_show_ui_for_navigation_(false) {
@@ -53,23 +73,13 @@ 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();
-    scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> database_manager;
-    if (safe_browsing_service &&
-        safe_browsing_service->database_manager()->IsSupported() &&
-        safe_browsing::V4FeatureList::GetV4UsageStatus() ==
-            safe_browsing::V4FeatureList::V4UsageStatus::V4_ONLY) {
-      database_manager = safe_browsing_service->database_manager();
-    }
-
     throttles->push_back(
         base::MakeUnique<subresource_filter::
                              SubresourceFilterSafeBrowsingActivationThrottle>(
             navigation_handle, this,
             content::BrowserThread::GetTaskRunnerForThread(
                 content::BrowserThread::IO),
-            std::move(database_manager)));
+            GetDatabaseManager()));
   }
 
   auto* driver_factory =

components/safe_browsing_db/android/safe_browsing_api_handler_bridge.cc

@@ -48,6 +48,8 @@ int SBThreatTypeToJavaThreatType(const SBThreatType& sb_threat_type) {
       return safe_browsing::JAVA_THREAT_TYPE_POTENTIALLY_HARMFUL_APPLICATION;
     case SB_THREAT_TYPE_URL_UNWANTED:
       return safe_browsing::JAVA_THREAT_TYPE_UNWANTED_SOFTWARE;
+    case SB_THREAT_TYPE_SUBRESOURCE_FILTER:
+      return safe_browsing::JAVA_THREAT_TYPE_SUBRESOURCE_FILTER;
     default:
       NOTREACHED();
       return 0;

components/safe_browsing_db/database_manager.h

@@ -152,12 +152,11 @@ class SafeBrowsingDatabaseManager
   // to callback in |client|.
   virtual bool CheckResourceUrl(const GURL& url, Client* client) = 0;
 
-  // Called on the IO thread to check if the given url belongs to the
-  // subresource filter list. If the url doesn't belong to the list, the check
-  // happens synchronously, otherwise it returns false, and "client" is called
-  // asynchronously with the result when it is ready.
-  // Currently supported only on desktop. Returns TRUE if the list is not yet
-  // available.
+  // Called on the IO thread to check if the given url belongs to a list the
+  // subresource cares about. If the url doesn't belong to any such list and the
+  // check can happen synchronously, returns true. Otherwise it returns false,
+  // and "client" is called asynchronously with the result when it is ready.
+  // Returns true if the list is not yet available.
   virtual bool CheckUrlForSubresourceFilter(const GURL& url,
                                             Client* client) = 0;
 

components/safe_browsing_db/remote_database_manager.cc

@@ -249,7 +249,7 @@ bool RemoteSafeBrowsingDatabaseManager::CheckUrlForSubresourceFilter(
   DCHECK(api_handler) << "SafeBrowsingApiHandler was never constructed";
   api_handler->StartURLCheck(
       base::Bind(&ClientRequest::OnRequestDoneWeak, req->GetWeakPtr()), url,
-      {SB_THREAT_TYPE_SUBRESOURCE_FILTER});
+      {SB_THREAT_TYPE_SUBRESOURCE_FILTER, SB_THREAT_TYPE_URL_PHISHING});
 
   LogPendingChecks(current_requests_.size());
   current_requests_.push_back(req.release());

components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc

@@ -10,8 +10,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
-FakeSafeBrowsingDatabaseManager::FakeSafeBrowsingDatabaseManager()
-    : simulate_timeout_(false) {}
+FakeSafeBrowsingDatabaseManager::FakeSafeBrowsingDatabaseManager() {}
 
 void FakeSafeBrowsingDatabaseManager::AddBlacklistedUrl(
     const GURL& url,
@@ -38,15 +37,15 @@ FakeSafeBrowsingDatabaseManager::~FakeSafeBrowsingDatabaseManager() {}
 bool FakeSafeBrowsingDatabaseManager::CheckUrlForSubresourceFilter(
     const GURL& url,
     Client* client) {
-  if (simulate_timeout_)
-    return false;
-  if (!url_to_threat_type_.count(url))
+  if (synchronous_failure_ && !url_to_threat_type_.count(url))
     return true;
 
   // Enforce the invariant that a client will not send multiple requests, with
   // the subresource filter client implementation.
   DCHECK(checks_.find(client) == checks_.end());
   checks_.insert(client);
+  if (simulate_timeout_)
+    return false;
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&FakeSafeBrowsingDatabaseManager::
@@ -62,9 +61,15 @@ void FakeSafeBrowsingDatabaseManager::OnCheckUrlForSubresourceFilterComplete(
   if (checks_.find(client) == checks_.end())
     return;
   safe_browsing::ThreatMetadata metadata;
-  metadata.threat_pattern_type = url_to_threat_type_[url].second;
+  safe_browsing::SBThreatType threat_type =
+      safe_browsing::SBThreatType::SB_THREAT_TYPE_SAFE;
+  auto it = url_to_threat_type_.find(url);
+  if (it != url_to_threat_type_.end()) {
+    threat_type = it->second.first;
+    metadata.threat_pattern_type = it->second.second;
+  }
+  client->OnCheckBrowseUrlResult(url, threat_type, metadata);
 
-  client->OnCheckBrowseUrlResult(url, url_to_threat_type_[url].first, metadata);
   // Erase the client when a check is complete. Otherwise, it's possible
   // subsequent clients that share an address with this one will DCHECK in
   // CheckUrlForSubresourceFilter.
@@ -83,7 +88,8 @@ bool FakeSafeBrowsingDatabaseManager::ChecksAreAlwaysAsync() const {
   return false;
 }
 void FakeSafeBrowsingDatabaseManager::CancelCheck(Client* client) {
-  checks_.erase(client);
+  size_t erased = checks_.erase(client);
+  DCHECK_EQ(erased, 1u);
 }
 bool FakeSafeBrowsingDatabaseManager::CanCheckResourceType(
     content::ResourceType /* resource_type */) const {

components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h

@@ -30,6 +30,10 @@ class FakeSafeBrowsingDatabaseManager
 
   void SimulateTimeout();
 
+  // If set, will synchronously fail from CheckUrlForSubresourceFilter rather
+  // than posting a task to fail if the URL does not match the blacklist.
+  void set_synchronous_failure() { synchronous_failure_ = true; }
+
  protected:
   ~FakeSafeBrowsingDatabaseManager() override;
 
@@ -53,7 +57,8 @@ class FakeSafeBrowsingDatabaseManager
       GURL,
       std::pair<safe_browsing::SBThreatType, safe_browsing::ThreatPatternType>>
       url_to_threat_type_;
-  bool simulate_timeout_;
+  bool simulate_timeout_ = false;
+  bool synchronous_failure_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingDatabaseManager);
 };

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

@@ -303,7 +303,9 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
                                                     metadata);
   }
 
-  void SimulateTimeout() { fake_safe_browsing_database_->SimulateTimeout(); }
+  FakeSafeBrowsingDatabaseManager* fake_safe_browsing_database() {
+    return fake_safe_browsing_database_.get();
+  }
 
   void ClearAllBlacklistedUrls() {
     fake_safe_browsing_database_->RemoveAllBlacklistedUrls();
@@ -755,6 +757,18 @@ TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, ActivationList) {
   }
 }
 
+// Regression test for an issue where synchronous failure from the SB database
+// caused a double cancel. This is DCHECKed in the fake database.
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
+       SynchronousResponse) {
+  const GURL url(kURL);
+  fake_safe_browsing_database()->set_synchronous_failure();
+  SimulateStartAndExpectProceed(url);
+  SimulateCommitAndExpectProceed();
+  tester().ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix,
+                            0);
+}
+
 TEST_P(SubresourceFilterSafeBrowsingActivationThrottleScopeTest,
        ActivateForScopeType) {
   const ActivationScopeTestData& test_data = GetParam();
@@ -898,7 +912,7 @@ TEST_P(SubresourceFilterSafeBrowsingActivationThrottleParamTest,
   const ActivationListTestData& test_data = GetParam();
   const GURL url(kURL);
   const std::string suffix(GetSuffixForList(test_data.activation_list_type));
-  SimulateTimeout();
+  fake_safe_browsing_database()->SimulateTimeout();
   SimulateStartAndExpectProceed(url);
 
   // Flush the pending tasks on the IO thread, so the delayed task surely gets

components/subresource_filter/content/browser/subresource_filter_safe_browsing_client_request.cc

@@ -48,6 +48,7 @@ void SubresourceFilterSafeBrowsingClientRequest::Start() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   start_time_ = base::TimeTicks::Now();
   if (database_manager_->CheckUrlForSubresourceFilter(url_, this)) {
+    request_completed_ = true;
     SendCheckResultToClient(false /* served_from_network */,
                             safe_browsing::SB_THREAT_TYPE_SAFE,
                             safe_browsing::ThreatMetadata());