[safe browsing] Remove stale BINHASH code.

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 246 matching lines @@
   for (size_t i = 0; i < decoded_chunks.size(); ++i) {
     ChunksToRangeString(decoded_chunks[i], &((*list_ranges)[i]));
   }
 }
 
 // Helper function to create chunk range lists for Browse related
 // lists.
 void UpdateChunkRanges(SafeBrowsingStore* store,
                        const std::vector<std::string>& listnames,
                        std::vector<SBListChunkRanges>* lists) {
+  if (!store)
+    return;
+
   DCHECK_GT(listnames.size(), 0U);
   DCHECK_LE(listnames.size(), 2U);
   std::vector<int> add_chunks;
   std::vector<int> sub_chunks;
   store->GetAddChunks(&add_chunks);
   store->GetSubChunks(&sub_chunks);
 
   // Always decode 2 ranges, even if only the first one is expected.
   // The loop below will only load as many into |lists| as |listnames|
   // indicates.
   std::vector<std::string> adds(2);
   std::vector<std::string> subs(2);
   GetChunkRanges(add_chunks, &adds);
   GetChunkRanges(sub_chunks, &subs);
 
   for (size_t i = 0; i < listnames.size(); ++i) {
     const std::string& listname = listnames[i];
     DCHECK_EQ(safe_browsing_util::GetListId(listname) % 2,
               static_cast<int>(i % 2));
     DCHECK_NE(safe_browsing_util::GetListId(listname),
               safe_browsing_util::INVALID);
     lists->push_back(SBListChunkRanges(listname));
     lists->back().adds.swap(adds[i]);
     lists->back().subs.swap(subs[i]);
   }
 }
 
-// Helper for deleting chunks left over from obsolete lists.
-void DeleteChunksFromStore(SafeBrowsingStore* store, int listid){
-  std::vector<int> add_chunks;
-  size_t adds_deleted = 0;
-  store->GetAddChunks(&add_chunks);
-  for (std::vector<int>::const_iterator iter = add_chunks.begin();
-       iter != add_chunks.end(); ++iter) {
-    if (GetListIdBit(*iter) == GetListIdBit(listid)) {
-      adds_deleted++;
-      store->DeleteAddChunk(*iter);
-    }
-  }
-  if (adds_deleted > 0)
-    UMA_HISTOGRAM_COUNTS("SB2.DownloadBinhashAddsDeleted", adds_deleted);
+void UpdateChunkRangesForLists(SafeBrowsingStore* store,
+                               const std::string& listname0,
+                               const std::string& listname1,
+                               std::vector<SBListChunkRanges>* lists) {
+  std::vector<std::string> listnames;
+  listnames.push_back(listname0);
+  listnames.push_back(listname1);
+  UpdateChunkRanges(store, listnames, lists);
+}
 
-  std::vector<int> sub_chunks;
-  size_t subs_deleted = 0;
-  store->GetSubChunks(&sub_chunks);
-  for (std::vector<int>::const_iterator iter = sub_chunks.begin();
-       iter != sub_chunks.end(); ++iter) {
-    if (GetListIdBit(*iter) == GetListIdBit(listid)) {
-      subs_deleted++;
-      store->DeleteSubChunk(*iter);
-    }
-  }
-  if (subs_deleted > 0)
-    UMA_HISTOGRAM_COUNTS("SB2.DownloadBinhashSubsDeleted", subs_deleted);
+void UpdateChunkRangesForList(SafeBrowsingStore* store,
+                              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;
 }
 
 // This code always checks for non-zero file size.  This helper makes
 // that less verbose.
@@ skipping 112 matching lines @@
 // static
 base::FilePath SafeBrowsingDatabase::IpBlacklistDBFilename(
     const base::FilePath& db_filename) {
   return base::FilePath(db_filename.value() + kIPBlacklistDBFile);
 }
 
 SafeBrowsingStore* SafeBrowsingDatabaseNew::GetStore(const int list_id) {
   if (list_id == safe_browsing_util::PHISH ||
       list_id == safe_browsing_util::MALWARE) {
     return browse_store_.get();
-  } else if (list_id == safe_browsing_util::BINURL ||
-             list_id == safe_browsing_util::BINHASH) {
+  } else if (list_id == safe_browsing_util::BINURL) {
     return download_store_.get();
   } else if (list_id == safe_browsing_util::CSDWHITELIST) {
     return csd_whitelist_store_.get();
   } else if (list_id == safe_browsing_util::DOWNLOADWHITELIST) {
     return download_whitelist_store_.get();
   } else if (list_id == safe_browsing_util::EXTENSIONBLACKLIST) {
     return extension_blacklist_store_.get();
   } else if (list_id == safe_browsing_util::SIDEEFFECTFREEWHITELIST) {
     return side_effect_free_whitelist_store_.get();
   } else if (list_id == safe_browsing_util::IPBLACKLIST) {
@@ skipping 275 matching lines @@
     return false;
 
   std::vector<SBPrefix> prefixes;
   GetDownloadUrlPrefixes(urls, &prefixes);
   return MatchAddPrefixes(download_store_.get(),
                           safe_browsing_util::BINURL % 2,
                           prefixes,
                           prefix_hits);
 }
 
-bool SafeBrowsingDatabaseNew::ContainsDownloadHashPrefix(
-    const SBPrefix& prefix) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
-
-  // Ignore this check when download store is not available.
-  if (!download_store_.get())
-    return false;
-
-  std::vector<SBPrefix> prefix_hits;
-  return MatchAddPrefixes(download_store_.get(),
-                          safe_browsing_util::BINHASH % 2,
-                          std::vector<SBPrefix>(1, prefix),
-                          &prefix_hits);
-}
-
 bool SafeBrowsingDatabaseNew::ContainsCsdWhitelistedUrl(const GURL& url) {
   // This method is theoretically thread-safe but we expect all calls to
   // originate from the IO thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   std::vector<SBFullHash> full_hashes;
   BrowseFullHashesToCheck(url, true, &full_hashes);
   return ContainsWhitelistedHashes(csd_whitelist_, full_hashes);
 }
 
 bool SafeBrowsingDatabaseNew::ContainsDownloadWhitelistedUrl(const GURL& url) {
@@ skipping 381 matching lines @@
     HandleCorruptDatabase();
     return false;
   }
 
   if (ip_blacklist_store_ && !ip_blacklist_store_->BeginUpdate()) {
     RecordFailure(FAILURE_IP_BLACKLIST_UPDATE_BEGIN);
     HandleCorruptDatabase();
     return false;
   }
 
-  std::vector<std::string> browse_listnames;
-  browse_listnames.push_back(safe_browsing_util::kMalwareList);
-  browse_listnames.push_back(safe_browsing_util::kPhishingList);
-  UpdateChunkRanges(browse_store_.get(), browse_listnames, lists);
+  UpdateChunkRangesForLists(browse_store_.get(),
+                            safe_browsing_util::kMalwareList,
+                            safe_browsing_util::kPhishingList,
+                            lists);
 
-  if (download_store_.get()) {
-    // This store used to contain kBinHashList in addition to
-    // kBinUrlList.  Strip the stale data before generating the chunk
-    // ranges to request.  UpdateChunkRanges() will traverse the chunk
-    // list, so this is very cheap if there are no kBinHashList chunks.
-    const int listid =
-        safe_browsing_util::GetListId(safe_browsing_util::kBinHashList);
-    DeleteChunksFromStore(download_store_.get(), listid);
+  // NOTE(shess): |download_store_| used to contain kBinHashList, which has been
+  // deprecated.  Code to delete the list from the store shows no histogram hits
+  // at all, so it has been removed.  Everything _should_ be resilient to extra

[mattm, 2014/02/19 23:47:09]

Looks like the histograms just don't show on the dashboard since there was no
description added to histograms.xml for them. If you wanted to be extra safe you
could add the descriptions, wait for the dashboard to be updated, and check the
results.  But since the removal code has been in there for many versions now I'm
fine with just killing it.

[Scott Hess - ex-Googler, 2014/02/20 19:13:20]

I updated histograms.xml, and now the dashboard shows that we have odd users. 
~15k/day cases where something is deleted.  I have no idea what those users are
doing, it seems unlikely to me that there are this many people per day who have
not launched Chrome for 18 months, but today is the day!  But if there are,
well, there's not really all that much we could do that will impact them.  I
think it's more likely that they have something funky going on outside of
Chrome, like a pre-built profile (say for a testing VM), but I can't think of
any reason to keep the code in that case, either.

I will change the comment, though.

[It just occurred to me that if a user has some sort of corruption which
prevents the update from succeeding, then they might hit this histogram every 45
minutes or so.  I don't know what would cause that, but that could cut the
incidence down to as few as 500 users/day.]

+  // data of that sort.
+  UpdateChunkRangesForList(download_store_.get(),
+                           safe_browsing_util::kBinUrlList, lists);
 
-    // The above marks the chunks for deletion, but they are not
-    // actually deleted until the database is rewritten.  The
-    // following code removes the kBinHashList part of the request
-    // before continuing so that UpdateChunkRanges() doesn't break.
-    std::vector<std::string> download_listnames;
-    download_listnames.push_back(safe_browsing_util::kBinUrlList);
-    download_listnames.push_back(safe_browsing_util::kBinHashList);
-    UpdateChunkRanges(download_store_.get(), download_listnames, lists);
-    DCHECK_EQ(lists->back().name,
-              std::string(safe_browsing_util::kBinHashList));
-    lists->pop_back();
+  UpdateChunkRangesForList(csd_whitelist_store_.get(),
+                           safe_browsing_util::kCsdWhiteList, lists);
 
-    // TODO(shess): This problem could also be handled in
-    // BeginUpdate() by detecting the chunks to delete and rewriting
-    // the database before it's used.  When I implemented that, it
-    // felt brittle, it might be easier to just wait for some future
-    // format change.
-  }
+  UpdateChunkRangesForList(download_whitelist_store_.get(),
+                           safe_browsing_util::kDownloadWhiteList, lists);
 
-  if (csd_whitelist_store_.get()) {
-    std::vector<std::string> csd_whitelist_listnames;
-    csd_whitelist_listnames.push_back(safe_browsing_util::kCsdWhiteList);
-    UpdateChunkRanges(csd_whitelist_store_.get(),
-                      csd_whitelist_listnames, lists);
-  }
+  UpdateChunkRangesForList(extension_blacklist_store_.get(),
+                           safe_browsing_util::kExtensionBlacklist, lists);
 
-  if (download_whitelist_store_.get()) {
-    std::vector<std::string> download_whitelist_listnames;
-    download_whitelist_listnames.push_back(
-        safe_browsing_util::kDownloadWhiteList);
-    UpdateChunkRanges(download_whitelist_store_.get(),
-                      download_whitelist_listnames, lists);
-  }
+  UpdateChunkRangesForList(side_effect_free_whitelist_store_.get(),
+                           safe_browsing_util::kSideEffectFreeWhitelist, lists);
 
-  if (extension_blacklist_store_) {
-    UpdateChunkRanges(
-        extension_blacklist_store_.get(),
-        std::vector<std::string>(1, safe_browsing_util::kExtensionBlacklist),
-        lists);
-  }
-
-  if (side_effect_free_whitelist_store_) {
-    UpdateChunkRanges(
-        side_effect_free_whitelist_store_.get(),
-        std::vector<std::string>(
-            1, safe_browsing_util::kSideEffectFreeWhitelist),
-        lists);
-  }
-
-  if (ip_blacklist_store_) {
-    UpdateChunkRanges(
-        ip_blacklist_store_.get(),
-        std::vector<std::string>(1, safe_browsing_util::kIPBlacklist),
-        lists);
-  }
+  UpdateChunkRangesForList(ip_blacklist_store_.get(),
+                           safe_browsing_util::kIPBlacklist, lists);
 
   corruption_detected_ = false;
   change_detected_ = false;
   return true;
 }
 
 void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
   DCHECK_EQ(creation_loop_, base::MessageLoop::current());
 
   // The update may have failed due to corrupt storage (for instance,
@@ skipping 539 matching lines @@
 }
 
 bool SafeBrowsingDatabaseNew::IsMalwareIPMatchKillSwitchOn() {
   SBFullHash malware_kill_switch;
   crypto::SHA256HashString(kMalwareIPKillSwitchUrl, &malware_kill_switch,
                            sizeof(malware_kill_switch));
   std::vector<SBFullHash> full_hashes;
   full_hashes.push_back(malware_kill_switch);
   return ContainsWhitelistedHashes(csd_whitelist_, full_hashes);
 }