Delete stale chunks from safe-browsing downloads store.

chrome/browser/safe_browsing/safe_browsing_database.cc

Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 31cac5078dc8976ff0711add57a576f95e455414..cb70c75725e012edaeadb2f0886126c51172d6df 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -223,6 +223,35 @@ void UpdateChunkRanges(SafeBrowsingStore* store,
   }
 }
 
+// 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) {
@@ -1065,18 +1094,31 @@ bool SafeBrowsingDatabaseNew::UpdateStarted(
   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()) {