Move full hash caching logic to v4_get_hash_protocol_manager

components/safe_browsing_db/database_manager.cc

Index: components/safe_browsing_db/database_manager.cc
diff --git a/components/safe_browsing_db/database_manager.cc b/components/safe_browsing_db/database_manager.cc
index 973beedce7607aa94d6f826afa23969ef823aec3..e0034f03ac7fdbe6650d4b6eece9ea27aa3063cf 100644
--- a/components/safe_browsing_db/database_manager.cc
+++ b/components/safe_browsing_db/database_manager.cc
@@ -12,66 +12,12 @@
 
 using content::BrowserThread;
 
-namespace {
-
-// Enumerate full hash cache hits/misses for histogramming purposes.
-// DO NOT CHANGE THE ORDERING OF THESE VALUES.
-enum V4FullHashCacheResultType {
-  // Full hashes for which there is no cache hit.
-  FULL_HASH_CACHE_MISS = 0,
-
-  // Full hashes with a cache hit.
-  FULL_HASH_CACHE_HIT = 1,
-
-  // Full hashes with a negative cache hit.
-  FULL_HASH_NEGATIVE_CACHE_HIT = 2,
-
-  // Memory space for histograms is determined by the max. ALWAYS
-  // ADD NEW VALUES BEFORE THIS ONE.
-  FULL_HASH_CACHE_RESULT_MAX
-};
-
-// Enumerate GetHash hits/misses for histogramming purposes. DO NOT CHANGE THE
-// ORDERING OF THESE VALUES.
-enum V4GetHashCheckResultType {
-  // Successful responses which returned no full hashes.
-  GET_HASH_CHECK_EMPTY = 0,
-
-  // Successful responses for which one or more of the full hashes matched.
-  GET_HASH_CHECK_HIT = 1,
-
-  // Successful responses which weren't empty but have no matches.
-  GET_HASH_CHECK_MISS = 2,
-
-  // Memory space for histograms is determined by the max. ALWAYS
-  // ADD NEW VALUES BEFORE THIS ONE.
-  GET_HASH_CHECK_RESULT_MAX
-};
-
-// Record a full hash cache hit result.
-void RecordV4FullHashCacheResult(
-    V4FullHashCacheResultType result_type) {
-  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4FullHashCacheResult", result_type,
-                            FULL_HASH_CACHE_RESULT_MAX);
-}
-
-// Record a GetHash hit result.
-void RecordV4GetHashCheckResult(
-    V4GetHashCheckResultType result_type) {
-  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4GetHashCheckResult", result_type,
-                            GET_HASH_CHECK_RESULT_MAX);
-}
-
-}  // namespace
-
 namespace safe_browsing {
 
-SafeBrowsingDatabaseManager::SafeBrowsingDatabaseManager()
-    : v4_get_hash_protocol_manager_(NULL) {
-}
+SafeBrowsingDatabaseManager::SafeBrowsingDatabaseManager() {}
 
 SafeBrowsingDatabaseManager::~SafeBrowsingDatabaseManager() {
-  DCHECK(v4_get_hash_protocol_manager_ == NULL);
+  DCHECK(!v4_get_hash_protocol_manager_);
 }
 
 void SafeBrowsingDatabaseManager::StartOnIOThread(
@@ -79,8 +25,9 @@ void SafeBrowsingDatabaseManager::StartOnIOThread(
     const V4ProtocolConfig& config) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  base::hash_set<UpdateListIdentifier> stores_to_look({GetChromeUrlApiId()});

[Nathan Parker, 2016/09/09 21:26:20]

Maybe add a TODO to pull in the list of all stores.  Once you do that, it'll
start exercising more of the caching logic (multiple stores fetched, only one is
actively used).

[vakh (use Gerrit instead), 2016/09/09 23:25:17]

This database manager only cares about this store. The V4LDBM is the one that
will care about all other stores.

   v4_get_hash_protocol_manager_ = V4GetHashProtocolManager::Create(
-      request_context_getter, config);
+      request_context_getter, stores_to_look, config);
 }
 
 // |shutdown| not used. Destroys the v4 protocol managers. This may be called
@@ -89,19 +36,15 @@ void SafeBrowsingDatabaseManager::StartOnIOThread(
 void SafeBrowsingDatabaseManager::StopOnIOThread(bool shutdown) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // This cancels all in-flight GetHash requests.
-  if (v4_get_hash_protocol_manager_) {
-    delete v4_get_hash_protocol_manager_;
-    v4_get_hash_protocol_manager_ = NULL;
-  }
+  v4_get_hash_protocol_manager_.reset();
 
   // Delete pending checks, calling back any clients with empty metadata.
-  for (auto* check : api_checks_) {
+  for (const SafeBrowsingApiCheck* check : api_checks_) {
     if (check->client()) {
       check->client()->
           OnCheckApiBlacklistUrlResult(check->url(), ThreatMetadata());
     }
   }
-  base::STLDeleteElements(&api_checks_);
 }
 
 SafeBrowsingDatabaseManager::ApiCheckSet::iterator
@@ -120,7 +63,6 @@ bool SafeBrowsingDatabaseManager::CancelApiCheck(Client* client) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   ApiCheckSet::iterator it = FindClientApiCheck(client);
   if (it != api_checks_.end()) {
-    delete *it;
     api_checks_.erase(it);
     return true;
   }
@@ -141,236 +83,35 @@ bool SafeBrowsingDatabaseManager::CheckApiBlacklistUrl(const GURL& url,
   // There can only be one in-progress check for the same client at a time.
   DCHECK(FindClientApiCheck(client) == api_checks_.end());
 
-  // Compute a list of hashes for this url.
-  std::vector<SBFullHash> full_hashes;
-  UrlToFullHashes(url, false, &full_hashes);
-  if (full_hashes.empty())
-    return true;
-
-  // First check the cache.
-
-  // Used to determine cache expiration.
-  base::Time now = base::Time::Now();
-
-  std::vector<SBFullHashResult> cached_results;
-  std::vector<SBPrefix> prefixes_needing_reqs;
-  GetFullHashCachedResults(SB_THREAT_TYPE_API_ABUSE,
-      full_hashes, now, &prefixes_needing_reqs, &cached_results);
-
-  if (prefixes_needing_reqs.empty() && cached_results.empty())
-    return true;
-
-  SafeBrowsingApiCheck* check =
-      new SafeBrowsingApiCheck(url, prefixes_needing_reqs, full_hashes,
-                               cached_results, client);
-  api_checks_.insert(check);
-
-  if (prefixes_needing_reqs.empty()) {
-    check->set_start_time(base::TimeTicks::Now());
-    // We can call the callback immediately if no prefixes require a request.
-    // The |full_hash_results| representing the results fromt eh SB server will
-    // be empty.
-    std::vector<SBFullHashResult> full_hash_results;
-    HandleGetHashesWithApisResults(check, full_hash_results, base::Time());
-    return false;
-  }
-
-  v4_get_hash_protocol_manager_->GetFullHashesWithApis(prefixes_needing_reqs,
-      base::Bind(&SafeBrowsingDatabaseManager::HandleGetHashesWithApisResults,
-                 base::Unretained(this), check));
+  std::unique_ptr<SafeBrowsingApiCheck> check(
+      new SafeBrowsingApiCheck(url, client));
+  api_checks_.insert(check.get());
+  v4_get_hash_protocol_manager_->GetFullHashesWithApis(
+      url, base::Bind(&SafeBrowsingDatabaseManager::OnThreatMetadataResponse,
+                      base::Unretained(this), base::Passed(std::move(check))));
 
   return false;
 }
 
-void SafeBrowsingDatabaseManager::GetFullHashCachedResults(
-    const SBThreatType& threat_type,
-    const std::vector<SBFullHash>& full_hashes,
-    base::Time now,
-    std::vector<SBPrefix>* prefixes_needing_reqs,
-    std::vector<SBFullHashResult>* cached_results) {
-  DCHECK(prefixes_needing_reqs);
-  prefixes_needing_reqs->clear();
-  DCHECK(cached_results);
-  cached_results->clear();
-
-  // Caching behavior is documented here:
-  // https://developers.google.com/safe-browsing/v4/caching#about-caching
-  //
-  // The cache operates as follows:
-  // Lookup:
-  //     Case 1: The prefix is in the cache.
-  //         Case a: The full hash is in the cache.
-  //             Case i : The positive full hash result has not expired.
-  //                      The result is unsafe and we do not need to send a new
-  //                      request.
-  //             Case ii: The positive full hash result has expired.
-  //                      We need to send a request for full hashes.
-  //         Case b: The full hash is not in the cache.
-  //             Case i : The negative cache entry has not expired.
-  //                      The result is still safe and we do not need to send a
-  //                      new request.
-  //             Case ii: The negative cache entry has expired.
-  //                      We need to send a request for full hashes.
-  //     Case 2: The prefix is not in the cache.
-  //             We need to send a request for full hashes.
-  //
-  // Eviction:
-  //   SBCachedFullHashResult entries can be removed from the cache only when
-  //   the negative cache expire time and the cache expire time of all full
-  //   hash results for that prefix have expired.
-  //   Individual full hash results can be removed from the prefix's
-  //   cache entry if they expire AND their expire time is after the negative
-  //   cache expire time.
-  for (const SBFullHash& full_hash : full_hashes) {
-    auto entry = v4_full_hash_cache_[threat_type].find(full_hash.prefix);
-    if (entry != v4_full_hash_cache_[threat_type].end()) {
-      // Case 1.
-      SBCachedFullHashResult& cache_result = entry->second;
-
-      const SBFullHashResult* found_full_hash = nullptr;
-      size_t matched_idx = 0;
-      for (const SBFullHashResult& hash_result : cache_result.full_hashes) {
-        if (SBFullHashEqual(full_hash, hash_result.hash)) {
-          found_full_hash = &hash_result;
-          break;
-        }
-        ++matched_idx;
-      }
-
-      if (found_full_hash) {
-        // Case a.
-        if (found_full_hash->cache_expire_after > now) {
-          // Case i.
-          cached_results->push_back(*found_full_hash);
-          RecordV4FullHashCacheResult(FULL_HASH_CACHE_HIT);
-        } else {
-          // Case ii.
-          prefixes_needing_reqs->push_back(full_hash.prefix);
-          RecordV4FullHashCacheResult(FULL_HASH_CACHE_MISS);
-          // If the negative cache expire time has passed, evict this full hash
-          // result from the cache.
-          if (cache_result.expire_after <= now) {
-            cache_result.full_hashes.erase(
-                cache_result.full_hashes.begin() + matched_idx);
-            // If there are no more full hashes, we can evict the entire entry.
-            if (cache_result.full_hashes.empty()) {
-              v4_full_hash_cache_[threat_type].erase(entry);
-            }
-          }
-        }
-      } else {
-        // Case b.
-        if (cache_result.expire_after > now) {
-          // Case i.
-          RecordV4FullHashCacheResult(FULL_HASH_NEGATIVE_CACHE_HIT);
-        } else {
-          // Case ii.
-          prefixes_needing_reqs->push_back(full_hash.prefix);
-          RecordV4FullHashCacheResult(FULL_HASH_CACHE_MISS);
-        }
-      }
-    } else {
-      // Case 2.
-      prefixes_needing_reqs->push_back(full_hash.prefix);
-      RecordV4FullHashCacheResult(FULL_HASH_CACHE_MISS);
-    }
-  }
-
-  // Multiple full hashes could share a prefix, remove duplicates.
-  // TODO(kcarattini): Make |prefixes_needing_reqs| a set.
-  std::sort(prefixes_needing_reqs->begin(), prefixes_needing_reqs->end());
-  prefixes_needing_reqs->erase(std::unique(prefixes_needing_reqs->begin(),
-      prefixes_needing_reqs->end()), prefixes_needing_reqs->end());
-}
-
-void SafeBrowsingDatabaseManager::HandleGetHashesWithApisResults(
-    SafeBrowsingApiCheck* check,
-    const std::vector<SBFullHashResult>& full_hash_results,
-    const base::Time& negative_cache_expire) {
+void SafeBrowsingDatabaseManager::OnThreatMetadataResponse(
+    std::unique_ptr<SafeBrowsingApiCheck> check,
+    const ThreatMetadata& md) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(check);
 
-  // Record the network time.
-  if (!check->start_time().is_null()) {
-    UMA_HISTOGRAM_LONG_TIMES("SafeBrowsing.GetV4HashNetwork",
-                             base::TimeTicks::Now() - check->start_time());
-  }
-
-  // If the time is uninitialized, don't cache the results.
-  if (!negative_cache_expire.is_null()) {
-    // Cache the results.
-    // Create or reset all cached results for this prefix.
-    for (const SBPrefix& prefix : check->prefixes()) {
-      v4_full_hash_cache_[SB_THREAT_TYPE_API_ABUSE][prefix] =
-          SBCachedFullHashResult(negative_cache_expire);
-    }
-    // Insert any full hash hits. Note that there may be one, multiple, or no
-    // full hashes for any given prefix.
-    for (const SBFullHashResult& result : full_hash_results) {
-      v4_full_hash_cache_[SB_THREAT_TYPE_API_ABUSE][result.hash.prefix].
-          full_hashes.push_back(result);
-    }
-  }
-
   // If the check is not in |api_checks_| then the request was cancelled by the
   // client.
-  ApiCheckSet::iterator it = api_checks_.find(check);
+  ApiCheckSet::iterator it = api_checks_.find(check.get());
   if (it == api_checks_.end())
     return;
 
-  ThreatMetadata md;
-  // Merge the metadata from all matching results.
-  // Note: A full hash may have a result in both the cached results (from
-  // its own cache lookup) and in the server results (if another full hash
-  // with the same prefix needed to request results from the server). In this
-  // unlikely case, the two results' metadata will be merged.
-  bool get_hash_hit =
-      PopulateApiMetadataResult(full_hash_results, check->full_hashes(), &md);
-  PopulateApiMetadataResult(check->cached_results(), check->full_hashes(), &md);
-
-  if (get_hash_hit) {
-    RecordV4GetHashCheckResult(GET_HASH_CHECK_HIT);
-  } else if (full_hash_results.empty()) {
-    RecordV4GetHashCheckResult(GET_HASH_CHECK_EMPTY);
-  } else {
-    RecordV4GetHashCheckResult(GET_HASH_CHECK_MISS);
-  }
-
   check->client()->OnCheckApiBlacklistUrlResult(check->url(), md);
-  api_checks_.erase(it);
-  delete check;
-}
-
-// TODO(kcarattini): This is O(N^2). Look at improving performance by
-// using a map, sorting or doing binary search etc..
-bool SafeBrowsingDatabaseManager::PopulateApiMetadataResult(
-    const std::vector<SBFullHashResult>& results,
-    const std::vector<SBFullHash>& full_hashes,
-    ThreatMetadata* md) {
-  DCHECK(md);
-  bool hit = false;
-  for (const SBFullHashResult& result : results) {
-    for (const SBFullHash& full_hash : full_hashes) {
-      if (SBFullHashEqual(full_hash, result.hash)) {
-        md->api_permissions.insert(result.metadata.api_permissions.begin(),
-                                   result.metadata.api_permissions.end());
-        hit = true;
-        break;
-      }
-    }
-  }
-  return hit;
 }
 
 SafeBrowsingDatabaseManager::SafeBrowsingApiCheck::SafeBrowsingApiCheck(
     const GURL& url,
-    const std::vector<SBPrefix>& prefixes,
-    const std::vector<SBFullHash>& full_hashes,
-    const std::vector<SBFullHashResult>& cached_results,
     Client* client)
-    : url_(url), prefixes_(prefixes), full_hashes_(full_hashes),
-      cached_results_(cached_results), client_(client) {
-}
+    : url_(url), client_(client) {}
 
 SafeBrowsingDatabaseManager::SafeBrowsingApiCheck::~SafeBrowsingApiCheck() {
 }