S: GetThreatSeverity should handle the case of CheckResourceUrl.

components/safe_browsing_db/v4_local_database_manager_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // 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"
 #include "base/run_loop.h"
 #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"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "testing/platform_test.h"
 
 namespace safe_browsing {
 
 namespace {
 
-typedef base::Callback<void()> NullCallback;
+typedef std::vector<FullHashInfo> FullHashInfos;
 
 // Utility function for populating hashes.
 FullHash HashForUrl(const GURL& url) {
   std::vector<FullHash> full_hashes;
   V4ProtocolManagerUtil::UrlToFullHashes(url, &full_hashes);
   // ASSERT_GE(full_hashes.size(), 1u);
   return full_hashes[0];
 }
 
 // Always returns misses from GetFullHashes().
 class FakeGetHashProtocolManager : public V4GetHashProtocolManager {
  public:
   FakeGetHashProtocolManager(
       net::URLRequestContextGetter* request_context_getter,
       const StoresToCheck& stores_to_check,
-      const V4ProtocolConfig& config)
+      const V4ProtocolConfig& config,
+      FullHashInfos full_hash_infos)

[Nathan Parker, 2017/02/07 22:59:58]

nit: const FullHashInfos&, here and below.  Performance isn't as critical in
tests, but a good habit.

[vakh (use Gerrit instead), 2017/02/07 23:11:21]

Done.

       : 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(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(FullHashInfos full_hash_infos) {
     V4GetHashProtocolManager::RegisterFactory(
-        base::MakeUnique<FakeGetHashProtocolManagerFactory>());
+        base::MakeUnique<FakeGetHashProtocolManagerFactory>(full_hash_infos));
   }
   ~ScopedFakeGetHashProtocolManagerFactory() {
     V4GetHashProtocolManager::RegisterFactory(nullptr);
   }
 };
 
 }  // namespace
 
 class FakeV4Database : public V4Database {
  public:
@@ skipping 80 matching lines @@
     if (manager_to_cancel_) {
       manager_to_cancel_->CancelCheck(this);
     }
   }
 
   void OnCheckResourceUrlResult(const GURL& url,
                                 SBThreatType threat_type,
                                 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());

[Nathan Parker, 2017/02/07 22:59:58]

nit: I had to think about what this was asserting.  Would this be clearer as

if (threat_type == SB_THREAT_SAFE) 
  DCHECK(threat_has.empty();

[vakh (use Gerrit instead), 2017/02/07 23:11:21]

I read it as: "Either it is not safe, or the hash is empty"

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

Well, that's also how I read it - but I still have to think about what it is
asserting.

I think what you're asserting is that when it is safe, the hash is empty?  IMHO
Nathan's phrasing says exactly that.  Similar would be:

if (!thread_hash.empty())
  DCHECK_NE(SB_THREAD_TYPE_SAFE, thread_type);

But ... it's your code, so I'm not blocking.

I will mention that DCHECK in test code has poor outcomes.  Sometimes/often you
end up with a crash without a decent backtrace.

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

Acknowledged.

Re-wrote to be more accurate and less complicated to understand.
It is all still within DCHECK though.

     on_check_resource_url_result_called_ = true;
   }
 
   SBThreatType expected_sb_threat_type;
   GURL expected_url;
   bool on_check_browse_url_result_called_;
   bool on_check_resource_url_result_called_;
   V4LocalDatabaseManager* manager_to_cancel_;
 };
 
@@ skipping 255 matching lines @@
   // The database is unavailable so the check should get queued.
   EXPECT_EQ(1ul, GetQueuedChecks().size());
 
   StopLocalDatabaseManager();
   EXPECT_TRUE(GetQueuedChecks().empty());
 }
 
 // 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({}));
 
   // 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.
@@ skipping 206 matching lines @@
   // Whitelisted.
   EXPECT_TRUE(
       v4_local_database_manager_->MatchModuleWhitelistString("chrome.dll"));
 
   EXPECT_FALSE(FakeV4LocalDatabaseManager::PerformFullHashCheckCalled(
       v4_local_database_manager_));
 }
 
 // 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();
 
   StoreAndHashPrefixes store_and_hash_prefixes;
   store_and_hash_prefixes.emplace_back(GetUrlMalwareId(),
                                        HashPrefix("sن\340\t\006_"));
   ReplaceV4Database(store_and_hash_prefixes);
 
   GURL first_url("http://example.com/a");
@@ skipping 14 matching lines @@
   WaitForTasksOnTaskRunner();
   // |on_check_browse_url_result_called_| is true only if OnCheckBrowseUrlResult
   // gets called with the |url| equal to |expected_url|, which is |second_url|
   // in
   // this test.
   EXPECT_TRUE(client.on_check_browse_url_result_called_);
 }
 
 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();
   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_SAFE, 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_);
 }
 
 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();
   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(GetUrlSubresourceFilterId(),
                                        hash_prefix);
   ReplaceV4Database(store_and_hash_prefixes, true /* stores_available */);
 
   // Test that a request flows through to the callback.
   {
     TestClient client(SB_THREAT_TYPE_SAFE, url);
     EXPECT_FALSE(
         v4_local_database_manager_->CheckUrlForSubresourceFilter(url, &client));
     EXPECT_FALSE(client.on_check_browse_url_result_called_);
     WaitForTasksOnTaskRunner();
     EXPECT_TRUE(client.on_check_browse_url_result_called_);
   }
 }
 
+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::Now() + base::TimeDelta::FromMinutes(5));

[Nathan Parker, 2017/02/07 22:59:59]

Is this time used?  Anything with a real clock can be flakey.  If we don't need
a real time, just use base::Time(0)

[vakh (use Gerrit instead), 2017/02/07 23:11:21]

Done.

+
+  // 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()

[Nathan Parker, 2017/02/07 22:59:58]

I'll just leave and arrow pointing to these...
/'\
 |
 | 
 |


;-)

[vakh (use Gerrit instead), 2017/02/07 23:11:21]

Acknowledged. Will implement very soon.

 
 }  // namespace safe_browsing