Delete stale chunks from safe-browsing downloads store.

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 205 matching lines @@
     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);
+
+  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);
+}
+
 // 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;
 }
 
 // As compared to the bloom filter, PrefixSet should have these
 // properties:
 // - Any bloom filter miss should be a prefix set miss.
 // - Any prefix set hit should be a bloom filter hit.
@@ skipping 822 matching lines @@
     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);
 
   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);

[mattm, 2012/08/07 02:29:25]

So, regarding the comments about this here: 
http://codereview.chromium.org/9138003/diff2/10001:4002/chrome/browser/safe_b...
 have you decided it's fine to just do the call every time through instead of
checking?  

[Scott Hess - ex-Googler, 2012/08/07 16:36:11]

Per the comment below, DeleteChunksFromStore() just registers the chunks to be
deleted at update time, so they'll still be present when UpdateChunkRanges()
runs.  I experimented with actually deleting them now, but at this point the
prefix data is not loaded, so it's annoying.  Re-writing the store would be
best, but would require handling a bunch of potential errors (what if the
rewrite fails?).  Modifying the store to strip data at read may have unintended
consequences (I think it purposely doesn't strip chunks until after subs and
adds have cancelled out).

Long term, removing chunks from the TOC lists might be reasonable, I'll have to
think on that.  If the writing code verified that prefixes belonged to valid
chunks, then the deletion caches could be removed, and the file wouldn't contain
unreachable data.  That might have performance implications if not done right.

Anyhoo ... the chunk list is a smallish set of integers which should easily fit
in cache, and UpdateChunkRanges() will traverse it a few cycles later, so just
keeping the code simple seemed safest.

+
+    // 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));
-    // Remove kBinHashList entry so that we do not request updates for it from
-    // the server.  The existing data will still be retained by
-    // SafeBrowsingStoreFile::DoUpdate.
-    // TODO(mattm): write some code to remove the kBinHashList data from the
-    // file?
     lists->pop_back();
+
+    // 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.
   }
 
   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);
   }
 
   if (download_whitelist_store_.get()) {
@@ skipping 371 matching lines @@
   if (std::binary_search(new_whitelist.begin(), new_whitelist.end(),
                          kill_switch)) {
     // The kill switch is whitelisted hence we whitelist all URLs.
     WhitelistEverything(whitelist);
   } else {
     base::AutoLock locked(lookup_lock_);
     whitelist->second = false;
     whitelist->first.swap(new_whitelist);
   }
 }