S: GetThreatSeverity should handle the case of CheckResourceUrl.

components/safe_browsing_db/v4_local_database_manager_unittest.cc

Index: components/safe_browsing_db/v4_local_database_manager_unittest.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager_unittest.cc b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
index 541b14ab3bfe7f0020a323152b3deeb1161f6df1..f7617474e7dd2dfbc922abb4f44d4a4b26c53843 100644
--- a/components/safe_browsing_db/v4_local_database_manager_unittest.cc
+++ b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "components/safe_browsing_db/v4_local_database_manager.h"
+#include "base/base64.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
@@ -9,7 +11,6 @@
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/safe_browsing_db/v4_database.h"
-#include "components/safe_browsing_db/v4_local_database_manager.h"
 #include "components/safe_browsing_db/v4_test_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "crypto/sha2.h"
@@ -20,7 +21,7 @@ namespace safe_browsing {
 
 namespace {
 
-typedef base::Callback<void()> NullCallback;

[Scott Hess - ex-Googler, 2017/02/07 23:37:51]

For future reference, base::Closure is exactly this.

[vakh (use Gerrit instead), 2017/02/07 23:51:22]

Acknowledged.

+typedef std::vector<FullHashInfo> FullHashInfos;
 
 // Utility function for populating hashes.
 FullHash HashForUrl(const GURL& url) {
@@ -36,39 +37,49 @@ class FakeGetHashProtocolManager : public V4GetHashProtocolManager {
   FakeGetHashProtocolManager(
       net::URLRequestContextGetter* request_context_getter,
       const StoresToCheck& stores_to_check,
-      const V4ProtocolConfig& config)
+      const V4ProtocolConfig& config,
+      const FullHashInfos& full_hash_infos)
       : V4GetHashProtocolManager(request_context_getter,
                                  stores_to_check,
-                                 config) {}
+                                 config),
+        full_hash_infos_(full_hash_infos) {}
 
   void GetFullHashes(const FullHashToStoreAndHashPrefixesMap&,
                      FullHashCallback callback) override {
-    std::vector<FullHashInfo> full_hash_infos;
-
     // Async, since the real manager might use a fetcher.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, full_hash_infos));
+        FROM_HERE, base::Bind(callback, full_hash_infos_));
   }
+
+ private:
+  FullHashInfos full_hash_infos_;
 };
 
 class FakeGetHashProtocolManagerFactory
     : public V4GetHashProtocolManagerFactory {
  public:
+  FakeGetHashProtocolManagerFactory(const FullHashInfos& full_hash_infos)
+      : full_hash_infos_(full_hash_infos) {}
+
   std::unique_ptr<V4GetHashProtocolManager> CreateProtocolManager(
       net::URLRequestContextGetter* request_context_getter,
       const StoresToCheck& stores_to_check,
       const V4ProtocolConfig& config) override {
     return base::MakeUnique<FakeGetHashProtocolManager>(
-        request_context_getter, stores_to_check, config);
+        request_context_getter, stores_to_check, config, full_hash_infos_);
   }
+
+ private:
+  FullHashInfos full_hash_infos_;
 };
 
 // Use FakeGetHashProtocolManagerFactory in scope, then reset.
 class ScopedFakeGetHashProtocolManagerFactory {
  public:
-  ScopedFakeGetHashProtocolManagerFactory() {
+  ScopedFakeGetHashProtocolManagerFactory(
+      const FullHashInfos& full_hash_infos) {
     V4GetHashProtocolManager::RegisterFactory(
-        base::MakeUnique<FakeGetHashProtocolManagerFactory>());
+        base::MakeUnique<FakeGetHashProtocolManagerFactory>(full_hash_infos));
   }
   ~ScopedFakeGetHashProtocolManagerFactory() {
     V4GetHashProtocolManager::RegisterFactory(nullptr);
@@ -169,9 +180,7 @@ class TestClient : public SafeBrowsingDatabaseManager::Client {
                                 const std::string& threat_hash) override {
     DCHECK_EQ(expected_url, url);
     DCHECK_EQ(expected_sb_threat_type, threat_type);
-    // |threat_hash| is empty because GetFullHashes calls back with empty
-    // |full_hash_infos|.
-    DCHECK(threat_hash.empty());
+    DCHECK(threat_type != SB_THREAT_TYPE_SAFE || threat_hash.empty());
     on_check_resource_url_result_called_ = true;
   }
 
@@ -447,7 +456,7 @@ TEST_F(V4LocalDatabaseManagerTest, TestChecksAreQueued) {
 // Verify that a window where checks cannot be cancelled is closed.
 TEST_F(V4LocalDatabaseManagerTest, CancelPending) {
   // Setup to receive full-hash misses.
-  ScopedFakeGetHashProtocolManagerFactory pin;
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));

[Scott Hess - ex-Googler, 2017/02/07 23:37:51]

These can just be FullHashInfos(), right?  FullHashInfos({}) feels like it's
doing something tricksy.

[vakh (use Gerrit instead), 2017/02/07 23:51:22]

../../components/safe_browsing_db/v4_local_database_manager_unittest.cc:459:46:
error: parentheses were disambiguated as a function declaration
[-Werror,-Wvexing-parse]
  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos());
                                             ^~~~~~~~~~~~~~~~~

[Scott Hess - ex-Googler, 2017/02/07 23:55:41]

Reeeeeeeeeeeeally?  Wow.  I guess.  What if you didn't have a convenient
constructor to trigger using uniform initialization, though?  OK.

 
   // Reset the database manager so it picks up the replacement protocol manager.
   ResetLocalDatabaseManager();
@@ -674,7 +683,7 @@ TEST_F(V4LocalDatabaseManagerTest, TestMatchModuleWhitelist) {
 
 // This verifies the fix for race in http://crbug.com/660293
 TEST_F(V4LocalDatabaseManagerTest, TestCheckBrowseUrlWithSameClientAndCancel) {
-  ScopedFakeGetHashProtocolManagerFactory pin;
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
   // Reset the database manager so it picks up the replacement protocol manager.
   ResetLocalDatabaseManager();
   WaitForTasksOnTaskRunner();
@@ -709,7 +718,7 @@ TEST_F(V4LocalDatabaseManagerTest, TestCheckBrowseUrlWithSameClientAndCancel) {
 
 TEST_F(V4LocalDatabaseManagerTest, TestCheckResourceUrl) {
   // Setup to receive full-hash misses.
-  ScopedFakeGetHashProtocolManagerFactory pin;
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
 
   // Reset the database manager so it picks up the replacement protocol manager.
   ResetLocalDatabaseManager();
@@ -734,7 +743,7 @@ TEST_F(V4LocalDatabaseManagerTest, TestCheckResourceUrl) {
 
 TEST_F(V4LocalDatabaseManagerTest, TestSubresourceFilterCallback) {
   // Setup to receive full-hash misses.
-  ScopedFakeGetHashProtocolManagerFactory pin;
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
 
   // Reset the database manager so it picks up the replacement protocol manager.
   ResetLocalDatabaseManager();
@@ -761,6 +770,37 @@ TEST_F(V4LocalDatabaseManagerTest, TestSubresourceFilterCallback) {
   }
 }
 
+TEST_F(V4LocalDatabaseManagerTest, TestCheckResourceUrlReturnsBad) {
+  std::string base64_encoded = "ZVcaD6lke9GaaZEf07X3CpuEgMAqbpAyPw3sX/7eK9M=";
+  std::string base64_decoded;
+  base::Base64Decode(base64_encoded, &base64_decoded);
+  FullHashInfo fhi(base64_decoded, GetChromeUrlClientIncidentId(),
+                   base::Time::Time());
+
+  // Setup to receive full-hash hit.
+  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({fhi}));
+
+  // Reset the database manager so it picks up the replacement protocol manager.
+  ResetLocalDatabaseManager();
+  WaitForTasksOnTaskRunner();
+
+  // An URL and matching prefix.
+  const GURL url("http://example.com/a/");
+  const HashPrefix hash_prefix("eW\x1A\xF\xA9");
+
+  // Put a match in the db that will cause a protocol-manager request.
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetChromeUrlClientIncidentId(),
+                                       hash_prefix);
+  ReplaceV4Database(store_and_hash_prefixes, true /* stores_available */);
+
+  TestClient client(SB_THREAT_TYPE_BLACKLISTED_RESOURCE, url);
+  EXPECT_FALSE(v4_local_database_manager_->CheckResourceUrl(url, &client));
+  EXPECT_FALSE(client.on_check_resource_url_result_called_);
+  WaitForTasksOnTaskRunner();
+  EXPECT_TRUE(client.on_check_resource_url_result_called_);
+}
+
 // TODO(nparker): Add tests for
 //   CheckDownloadUrl()
 //   CheckExtensionIDs()