Safebrowsing: change gethash caching to match api 2.3 rules, fix some corner cases.

chrome/browser/safe_browsing/safe_browsing_database.cc

 // Copyright (c) 2012 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 "chrome/browser/safe_browsing/safe_browsing_database.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/bind.h"
@@ skipping 48 matching lines @@
     FILE_PATH_LITERAL(" IP Blacklist");
 
 // Filename suffix for browse store.
 // TODO(shess): "Safe Browsing Bloom Prefix Set" is full of win.
 // Unfortunately, to change the name implies lots of transition code
 // for little benefit.  If/when file formats change (say to put all
 // the data in one file), that would be a convenient point to rectify
 // this.
 const base::FilePath::CharType kBrowseDBFile[] = FILE_PATH_LITERAL(" Bloom");
 
-// The maximum staleness for a cached entry.
-const int kMaxStalenessMinutes = 45;
-
 // Maximum number of entries we allow in any of the whitelists.
 // If a whitelist on disk contains more entries then all lookups to
 // the whitelist will be considered a match.
 const size_t kMaxWhitelistSize = 5000;
 
 // If the hash of this exact expression is on a whitelist then all
 // lookups to this whitelist will be considered a match.
 const char kWhitelistKillSwitchUrl[] =
     "sb-ssl.google.com/safebrowsing/csd/killswitch";  // Don't change this!
 
@@ skipping 96 matching lines @@
       if (prefix == iter->prefix &&
           GetListIdBit(iter->chunk_id) == list_bit) {
         prefix_hits->push_back(prefix);
         found_match = true;
       }
     }
   }
   return found_match;
 }
 
-// Find the entries in |full_hashes| with prefix in |prefix_hits|, and
-// add them to |full_hits| if not expired.  "Not expired" is when
-// either |last_update| was recent enough, or the item has been
-// received recently enough.  Expired items are not deleted because a
-// future update may make them acceptable again.
-//
-// For efficiency reasons the code walks |prefix_hits| and
-// |full_hashes| in parallel, so they must be sorted by prefix.
-void GetCachedFullHashesForBrowse(const std::vector<SBPrefix>& prefix_hits,
-                                  const std::vector<SBAddFullHash>& full_hashes,
-                                  std::vector<SBFullHashResult>* full_hits,
-                                  base::Time last_update) {
-  const base::Time expire_time =
-      base::Time::Now() - base::TimeDelta::FromMinutes(kMaxStalenessMinutes);
-
-  std::vector<SBPrefix>::const_iterator piter = prefix_hits.begin();
-  std::vector<SBAddFullHash>::const_iterator hiter = full_hashes.begin();
-
-  while (piter != prefix_hits.end() && hiter != full_hashes.end()) {
-    if (*piter < hiter->full_hash.prefix) {
-      ++piter;
-    } else if (hiter->full_hash.prefix < *piter) {
-      ++hiter;
-    } else {
-      if (expire_time < last_update ||
-          expire_time.ToTimeT() < hiter->received) {
-        SBFullHashResult result;
-        const int list_bit = GetListIdBit(hiter->chunk_id);
-        DCHECK(list_bit == safe_browsing_util::MALWARE ||
-               list_bit == safe_browsing_util::PHISH);
-        const safe_browsing_util::ListType list_id =
-            static_cast<safe_browsing_util::ListType>(list_bit);
-        if (!safe_browsing_util::GetListName(list_id, &result.list_name))
-          continue;
-        result.add_chunk_id = DecodeChunkId(hiter->chunk_id);
-        result.hash = hiter->full_hash;
-        full_hits->push_back(result);
-      }
-
-      // Only increment |hiter|, |piter| might have multiple hits.
-      ++hiter;
-    }
-  }
-}
-
 // This function generates a chunk range string for |chunks|. It
 // outputs one chunk range string per list and writes it to the
 // |list_ranges| vector.  We expect |list_ranges| to already be of the
 // right size.  E.g., if |chunks| contains chunks with two different
 // list ids then |list_ranges| must contain two elements.
 void GetChunkRanges(const std::vector<int>& chunks,
                     std::vector<std::string>* list_ranges) {
   // Since there are 2 possible list ids, there must be exactly two
   // list ranges.  Even if the chunk data should only contain one
   // line, this code has to somehow handle corruption.
@@ skipping 61 matching lines @@
                               const std::string& listname,
                               std::vector<SBListChunkRanges>* lists) {
   UpdateChunkRanges(store, std::vector<std::string>(1, listname), lists);
 }
 
 // Order |SBAddFullHash| on the prefix part.  |SBAddPrefixLess()| from
 // safe_browsing_store.h orders on both chunk-id and prefix.
 bool SBAddFullHashPrefixLess(const SBAddFullHash& a, const SBAddFullHash& b) {
   return a.full_hash.prefix < b.full_hash.prefix;
 }
+bool SBAddFullHashSBPrefixLess(const SBAddFullHash& a, SBPrefix b) {
+  return a.full_hash.prefix < b;
+}
 
 // This code always checks for non-zero file size.  This helper makes
 // that less verbose.
 int64 GetFileSizeOrZero(const base::FilePath& file_path) {
   int64 size_64;
   if (!base::GetFileSize(file_path, &size_64))
     return 0;
   return size_64;
 }
 
-// Used to order whitelist storage in memory.
-bool SBFullHashLess(const SBFullHash& a, const SBFullHash& b) {
-  return memcmp(a.full_hash, b.full_hash, sizeof(a.full_hash)) < 0;
-}
-
 }  // namespace
 
 // The default SafeBrowsingDatabaseFactory.
 class SafeBrowsingDatabaseFactoryImpl : public SafeBrowsingDatabaseFactory {
  public:
   virtual SafeBrowsingDatabase* CreateSafeBrowsingDatabase(
       bool enable_download_protection,
       bool enable_client_side_whitelist,
       bool enable_download_whitelist,
       bool enable_extension_blacklist,
@@ skipping 185 matching lines @@
                  base::Unretained(this)));
   DVLOG(1) << "Init browse store: " << browse_filename_.value();
 
   {
     // NOTE: There is no need to grab the lock in this function, since
     // until it returns, there are no pointers to this class on other
     // threads.  Then again, that means there is no possibility of
     // contention on the lock...
     base::AutoLock locked(lookup_lock_);
     full_browse_hashes_.clear();
-    pending_browse_hashes_.clear();
+    browse_gethash_cache_.clear();
     LoadPrefixSet();
   }
 
   if (download_store_.get()) {
     download_filename_ = DownloadDBFilename(filename_base);
     download_store_->Init(
         download_filename_,
         base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
                    base::Unretained(this)));
     DVLOG(1) << "Init download store: " << download_filename_.value();
@@ skipping 103 matching lines @@
   // Delete files on disk.
   // TODO(shess): Hard to see where one might want to delete without a
   // reset.  Perhaps inline |Delete()|?
   if (!Delete())
     return false;
 
   // Reset objects in memory.
   {
     base::AutoLock locked(lookup_lock_);
     full_browse_hashes_.clear();
-    pending_browse_hashes_.clear();
-    prefix_miss_cache_.clear();
+    browse_gethash_cache_.clear();
     browse_prefix_set_.reset();
     side_effect_free_whitelist_prefix_set_.reset();
     ip_blacklist_.clear();
   }
   // Wants to acquire the lock itself.
   WhitelistEverything(&csd_whitelist_);
   WhitelistEverything(&download_whitelist_);
   return true;
 }
 
-// TODO(lzheng): Remove matching_list, it is not used anywhere.
 bool SafeBrowsingDatabaseNew::ContainsBrowseUrl(
     const GURL& url,
-    std::string* matching_list,
     std::vector<SBPrefix>* prefix_hits,
-    std::vector<SBFullHashResult>* full_hits,
-    base::Time last_update) {
+    std::vector<SBFullHashResult>* cache_hits) {
   // Clear the results first.
-  matching_list->clear();
   prefix_hits->clear();
-  full_hits->clear();
+  cache_hits->clear();
 
   std::vector<SBFullHash> full_hashes;
   BrowseFullHashesToCheck(url, false, &full_hashes);
   if (full_hashes.empty())
     return false;
 
+  std::sort(full_hashes.begin(), full_hashes.end(), SBFullHashLess);
+
+  return ContainsBrowseUrlHashes(full_hashes, prefix_hits, cache_hits);
+}
+
+bool SafeBrowsingDatabaseNew::ContainsBrowseUrlHashes(
+    const std::vector<SBFullHash>& full_hashes,
+    std::vector<SBPrefix>* prefix_hits,
+    std::vector<SBFullHashResult>* cache_hits) {
   // This function is called on the I/O thread, prevent changes to
   // filter and caches.
   base::AutoLock locked(lookup_lock_);
 
   // |browse_prefix_set_| is empty until it is either read from disk, or the
   // first update populates it.  Bail out without a hit if not yet
   // available.
   if (!browse_prefix_set_.get())
     return false;
 
-  size_t miss_count = 0;
+  const base::Time now = base::Time::Now();
+
   for (size_t i = 0; i < full_hashes.size(); ++i) {
     const SBPrefix prefix = full_hashes[i].prefix;
+
+    // First check if there is a valid cached result for this prefix.
+    std::map<SBPrefix, SBCachedFullHashResult>::iterator citer =
+        browse_gethash_cache_.find(prefix);
+    if (citer != browse_gethash_cache_.end()) {
+      if (now > citer->second.expire_after) {
+        // If the cached entry is expired, remove it and ignore it.
+        browse_gethash_cache_.erase(citer);

[Scott Hess - ex-Googler, 2014/04/03 21:44:03]

Having one case with a continue and one without is kinda freaking me out a
little.  Would

if (now <= expire_after) {
  //...
  continue;
}

// Remove expired entries.
erase(citer);

work for you?

[mattm, 2014/04/04 23:58:49]

Done.

+      } else {
+        for (std::vector<SBFullHashResult>::const_iterator fiter =
+                 citer->second.full_hashes.begin();
+             fiter != citer->second.full_hashes.end();
+             ++fiter) {
+          if (SBFullHashEqual(full_hashes[i], fiter->hash))
+            cache_hits->push_back(*fiter);
+        }
+        // If the prefix was in the cache, don't add the prefix to
+        // prefix_hits. The result will be in cache_hits (if the fullhash
+        // matched), or not (if it was a cached miss).
+        continue;
+      }
+    }
+
+    // There was no valid cached result for the prefix, so check the database.
     if (browse_prefix_set_->Exists(prefix)) {

[Scott Hess - ex-Googler, 2014/04/03 21:44:03]

I'm wondering if the control-flow can be flattened, here.  Like maybe:

if (!Exists(prefix))
  continue;

match = lower_bound(...);
if (match == end() || match.prefix != prefix) {
  prefix_hits->push_back_if_unique(prefix);
  continue;
}

while (match != end() && match.prefix == prefix) {
  if (SBFHE(match.full_hash, full_hashes[i])) {
    prefix_hits->push_back_if_unique(prefix);
    break;
  }
}

[mattm, 2014/04/04 23:58:49]

Done.

-      prefix_hits->push_back(prefix);
-      if (prefix_miss_cache_.count(prefix) > 0)
-        ++miss_count;
+      std::vector<SBAddFullHash>::const_iterator db_fullhash_prefix_match =
+          std::lower_bound(full_browse_hashes_.begin(),
+                           full_browse_hashes_.end(),
+                           prefix,
+                           SBAddFullHashSBPrefixLess);
+      // If the database contains any fullhashes with the same prefix, then we
+      // only count it as a prefix hit if there is an exact fullhash match.
+      if (db_fullhash_prefix_match != full_browse_hashes_.end() &&
+          db_fullhash_prefix_match->full_hash.prefix == prefix) {
+        bool fullhash_match = false;
+        // If full_browse_hashes_ was sorted on the fullhash (not just the
+        // prefix), could do binary_search here, but there are unlikely to be
+        // enough prefix matches to matter.
+        while (db_fullhash_prefix_match != full_browse_hashes_.end() &&
+               db_fullhash_prefix_match->full_hash.prefix == prefix) {
+          if (SBFullHashEqual(db_fullhash_prefix_match->full_hash,
+                              full_hashes[i])) {
+            fullhash_match = true;
+            break;
+          }
+          ++db_fullhash_prefix_match;
+        }
+        if (!fullhash_match)
+          continue;
+      }
+
+      // Only add a given prefix once. Since full_hashes is sorted, we only
+      // need to check the last entry of prefix_hits.
+      if (prefix_hits->empty() || prefix_hits->back() != prefix)
+        prefix_hits->push_back(prefix);
     }
   }
 
-  // If all the prefixes are cached as 'misses', don't issue a GetHash.
-  if (miss_count == prefix_hits->size())
-    return false;
-
-  // Find the matching full-hash results.  |full_browse_hashes_| are from the
-  // database, |pending_browse_hashes_| are from GetHash requests between
-  // updates.
-  std::sort(prefix_hits->begin(), prefix_hits->end());
-
-  GetCachedFullHashesForBrowse(*prefix_hits, full_browse_hashes_,
-                               full_hits, last_update);
-  GetCachedFullHashesForBrowse(*prefix_hits, pending_browse_hashes_,
-                               full_hits, last_update);
-  return true;
+  return !prefix_hits->empty() || !cache_hits->empty();
 }
 
 bool SafeBrowsingDatabaseNew::ContainsDownloadUrl(
     const std::vector<GURL>& urls,
     std::vector<SBPrefix>* prefix_hits) {
   DCHECK_EQ(creation_loop_, base::MessageLoop::current());
 
   // Ignore this check when download checking is not enabled.
   if (!download_store_.get())
     return false;
@@ skipping 138 matching lines @@
     store->WriteAddPrefix(encoded_chunk_id, host);
   } else if (entry->IsPrefix()) {
     // Prefixes only.
     for (int i = 0; i < count; i++) {
       const SBPrefix prefix = entry->PrefixAt(i);
       STATS_COUNTER("SB.PrefixAdd", 1);
       store->WriteAddPrefix(encoded_chunk_id, prefix);
     }
   } else {
     // Prefixes and hashes.
-    const base::Time receive_time = base::Time::Now();
     for (int i = 0; i < count; ++i) {
       const SBFullHash full_hash = entry->FullHashAt(i);
       const SBPrefix prefix = full_hash.prefix;
 
       STATS_COUNTER("SB.PrefixAdd", 1);
       store->WriteAddPrefix(encoded_chunk_id, prefix);
 
       STATS_COUNTER("SB.PrefixAddFull", 1);
-      store->WriteAddHash(encoded_chunk_id, receive_time, full_hash);
+      store->WriteAddHash(encoded_chunk_id, full_hash);
     }
   }
 }
 
 // Helper to iterate over all the entries in the hosts in |chunks| and
 // add them to the store.
 void SafeBrowsingDatabaseNew::InsertAddChunks(
     const safe_browsing_util::ListType list_id,
     const SBChunkList& chunks) {
   DCHECK_EQ(creation_loop_, base::MessageLoop::current());
@@ skipping 147 matching lines @@
       if (chunk_deletes[i].is_sub_del)
         store->DeleteSubChunk(encoded_chunk_id);
       else
         store->DeleteAddChunk(encoded_chunk_id);
     }
   }
 }
 
 void SafeBrowsingDatabaseNew::CacheHashResults(
     const std::vector<SBPrefix>& prefixes,
-    const std::vector<SBFullHashResult>& full_hits) {
+    const std::vector<SBFullHashResult>& full_hits,
+    const base::TimeDelta& cache_lifetime) {
+
+  const base::Time expire_after = base::Time::Now() + cache_lifetime;
+
   // This is called on the I/O thread, lock against updates.
   base::AutoLock locked(lookup_lock_);
 
-  if (full_hits.empty()) {
-    prefix_miss_cache_.insert(prefixes.begin(), prefixes.end());
-    return;
+  // Create or reset all cached results for these prefixes.
+  for (std::vector<SBPrefix>::const_iterator i = prefixes.begin();
+       i != prefixes.end();
+       ++i) {
+    browse_gethash_cache_[*i] = SBCachedFullHashResult(expire_after);
   }
 
-  // TODO(shess): SBFullHashResult and SBAddFullHash are very similar.
-  // Refactor to make them identical.
-  const base::Time now = base::Time::Now();
-  const size_t orig_size = pending_browse_hashes_.size();
-  for (std::vector<SBFullHashResult>::const_iterator iter = full_hits.begin();
-       iter != full_hits.end(); ++iter) {
-    const int list_id = safe_browsing_util::GetListId(iter->list_name);
-    if (list_id == safe_browsing_util::MALWARE ||
-        list_id == safe_browsing_util::PHISH) {
-      int encoded_chunk_id = EncodeChunkId(iter->add_chunk_id, list_id);
-      SBAddFullHash add_full_hash(encoded_chunk_id, now, iter->hash);
-      pending_browse_hashes_.push_back(add_full_hash);
-    }
+  // Insert any fullhash hits. Note that there may be one, multiple, or no
+  // fullhashes for any given entry in |prefixes|.
+  for (std::vector<SBFullHashResult>::const_iterator i = full_hits.begin();
+       i != full_hits.end();
+       ++i) {
+    browse_gethash_cache_[i->hash.prefix].full_hashes.push_back(*i);
   }
-
-  // Sort new entries then merge with the previously-sorted entries.
-  std::vector<SBAddFullHash>::iterator
-      orig_end = pending_browse_hashes_.begin() + orig_size;
-  std::sort(orig_end, pending_browse_hashes_.end(), SBAddFullHashPrefixLess);
-  std::inplace_merge(pending_browse_hashes_.begin(),
-                     orig_end, pending_browse_hashes_.end(),
-                     SBAddFullHashPrefixLess);
 }
 
 bool SafeBrowsingDatabaseNew::UpdateStarted(
     std::vector<SBListChunkRanges>* lists) {
   DCHECK_EQ(creation_loop_, base::MessageLoop::current());
   DCHECK(lists);
 
   // If |BeginUpdate()| fails, reset the database.
   if (!browse_store_->BeginUpdate()) {
     RecordFailure(FAILURE_BROWSE_DATABASE_UPDATE_BEGIN);
@@ skipping 37 matching lines @@
   if (ip_blacklist_store_ && !ip_blacklist_store_->BeginUpdate()) {
     RecordFailure(FAILURE_IP_BLACKLIST_UPDATE_BEGIN);
     HandleCorruptDatabase();
     return false;
   }
 
   UpdateChunkRangesForLists(browse_store_.get(),
                             safe_browsing_util::kMalwareList,
                             safe_browsing_util::kPhishingList,
                             lists);
+  // Cached fullhash results must be cleared on every database update (whether
+  // successful or not.)
+  browse_gethash_cache_.clear();
 
   // NOTE(shess): |download_store_| used to contain kBinHashList, which has been
   // deprecated.  Code to delete the list from the store shows ~15k hits/day as
   // of Feb 2014, so it has been removed.  Everything _should_ be resilient to
   // extra data of that sort.
   UpdateChunkRangesForList(download_store_.get(),
                            safe_browsing_util::kBinUrlList, lists);
 
   UpdateChunkRangesForList(csd_whitelist_store_.get(),
                            safe_browsing_util::kCsdWhiteList, lists);
@@ skipping 114 matching lines @@
     UpdateIpBlacklistStore();
 }
 
 void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     SBWhitelist* whitelist) {
   if (!store)
     return;
 
-  // For the whitelists, we don't cache and save full hashes since all
-  // hashes are already full.
-  std::vector<SBAddFullHash> empty_add_hashes;
-
   // Note: |builder| will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> full_hashes;
-  if (!store->FinishUpdate(empty_add_hashes, &builder, &full_hashes)) {
+  if (!store->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_WHITELIST_DATABASE_UPDATE_FINISH);
     WhitelistEverything(whitelist);
     return;
   }
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
 
   LoadWhitelist(full_hashes, whitelist);
 }
 
 int64 SafeBrowsingDatabaseNew::UpdateHashPrefixStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     FailureType failure_type) {
-  // We don't cache and save full hashes.
-  std::vector<SBAddFullHash> empty_add_hashes;
-
   // These results are not used after this call. Simply ignore the
   // returned value after FinishUpdate(...).
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> add_full_hashes_result;
 
-  if (!store->FinishUpdate(empty_add_hashes,
-                           &builder,
-                           &add_full_hashes_result)) {
+  if (!store->FinishUpdate(&builder, &add_full_hashes_result)) {
     RecordFailure(failure_type);
   }
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
 
   return GetFileSizeOrZero(store_filename);
 }
 
 void SafeBrowsingDatabaseNew::UpdateBrowseStore() {
-  // Copy out the pending add hashes.  Copy rather than swapping in
-  // case |ContainsBrowseURL()| is called before the new filter is complete.
-  std::vector<SBAddFullHash> pending_add_hashes;
-  {
-    base::AutoLock locked(lookup_lock_);
-    pending_add_hashes.insert(pending_add_hashes.end(),
-                              pending_browse_hashes_.begin(),
-                              pending_browse_hashes_.end());
-  }
-
   // Measure the amount of IO during the filter build.
   base::IoCounters io_before, io_after;
   base::ProcessHandle handle = base::Process::Current().handle();
   scoped_ptr<base::ProcessMetrics> metric(
 #if !defined(OS_MACOSX)
       base::ProcessMetrics::CreateProcessMetrics(handle)
 #else
       // Getting stats only for the current process is enough, so NULL is fine.
       base::ProcessMetrics::CreateProcessMetrics(handle, NULL)
 #endif
   );
 
   // IoCounters are currently not supported on Mac, and may not be
   // available for Linux, so we check the result and only show IO
   // stats if they are available.
   const bool got_counters = metric->GetIOCounters(&io_before);
 
   const base::TimeTicks before = base::TimeTicks::Now();
 
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> add_full_hashes;
-  if (!browse_store_->FinishUpdate(pending_add_hashes,
-                                   &builder, &add_full_hashes)) {
+  if (!browse_store_->FinishUpdate(&builder, &add_full_hashes)) {
     RecordFailure(FAILURE_BROWSE_DATABASE_UPDATE_FINISH);
     return;
   }
   scoped_ptr<safe_browsing::PrefixSet> prefix_set(builder.GetPrefixSet());
 
   // This needs to be in sorted order by prefix for efficient access.
   std::sort(add_full_hashes.begin(), add_full_hashes.end(),
             SBAddFullHashPrefixLess);
 
   // Swap in the newly built filter and cache.
   {
     base::AutoLock locked(lookup_lock_);
     full_browse_hashes_.swap(add_full_hashes);
-
-    // TODO(shess): If |CacheHashResults()| is posted between the
-    // earlier lock and this clear, those pending hashes will be lost.
-    // It could be fixed by only removing hashes which were collected
-    // at the earlier point.  I believe that is fail-safe as-is (the
-    // hash will be fetched again).
-    pending_browse_hashes_.clear();
-    prefix_miss_cache_.clear();
     browse_prefix_set_.swap(prefix_set);
   }
 
   DVLOG(1) << "SafeBrowsingDatabaseImpl built prefix set in "
            << (base::TimeTicks::Now() - before).InMilliseconds()
            << " ms total.";
   UMA_HISTOGRAM_LONG_TIMES("SB2.BuildFilter", base::TimeTicks::Now() - before);
 
   // Persist the prefix set to disk.  Since only this thread changes
   // |browse_prefix_set_|, there is no need to lock.
@@ skipping 21 matching lines @@
   file_size = GetFileSizeOrZero(browse_filename_);
   UMA_HISTOGRAM_COUNTS("SB2.BrowseDatabaseKilobytes",
                        static_cast<int>(file_size / 1024));
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(browse_filename_);
 #endif
 }
 
 void SafeBrowsingDatabaseNew::UpdateSideEffectFreeWhitelistStore() {
-  std::vector<SBAddFullHash> empty_add_hashes;
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> add_full_hashes_result;
 
   if (!side_effect_free_whitelist_store_->FinishUpdate(
-          empty_add_hashes,
-          &builder,
-          &add_full_hashes_result)) {
+          &builder, &add_full_hashes_result)) {
     RecordFailure(FAILURE_SIDE_EFFECT_FREE_WHITELIST_UPDATE_FINISH);
     return;
   }
   scoped_ptr<safe_browsing::PrefixSet> prefix_set(builder.GetPrefixSet());
 
   // Swap in the newly built prefix set.
   {
     base::AutoLock locked(lookup_lock_);
     side_effect_free_whitelist_prefix_set_.swap(prefix_set);
   }
@@ skipping 20 matching lines @@
                        static_cast<int>(file_size / 1024));
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(side_effect_free_whitelist_filename_);
   base::mac::SetFileBackupExclusion(
       side_effect_free_whitelist_prefix_set_filename_);
 #endif
 }
 
 void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
-  // For the IP blacklist, we don't cache and save full hashes since all
-  // hashes are already full.
-  std::vector<SBAddFullHash> empty_add_hashes;
-
   // Note: prefixes will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> full_hashes;
-  if (!ip_blacklist_store_->FinishUpdate(empty_add_hashes,
-                                         &builder, &full_hashes)) {
+  if (!ip_blacklist_store_->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_IP_BLACKLIST_UPDATE_FINISH);
     LoadIpBlacklist(std::vector<SBAddFullHash>());  // Clear the list.
     return;
   }
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(ip_blacklist_filename_);
 #endif
 
   LoadIpBlacklist(full_hashes);
@@ skipping 195 matching lines @@
   base::AutoLock locked(lookup_lock_);
   ip_blacklist_.swap(new_blacklist);
 }
 
 bool SafeBrowsingDatabaseNew::IsMalwareIPMatchKillSwitchOn() {
   SBFullHash malware_kill_switch = SBFullHashForString(kMalwareIPKillSwitchUrl);
   std::vector<SBFullHash> full_hashes;
   full_hashes.push_back(malware_kill_switch);
   return ContainsWhitelistedHashes(csd_whitelist_, full_hashes);
 }