Delete stale chunks from safe-browsing downloads store.

Delete stale chunks from safe-browsing downloads store.

Download store used to request kBinHashList, but that is now obsolete.
Older clients continue to have a substantial number of such chunks, as
evidenced by SB2.DownloadDatabaseKilobytes showing a distinct 50/50
split between a database size around 2M and another around 4M.

BUG=108130
TEST=histograms of downloads store file size stops being bimodal.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150369

[Scott Hess - ex-Googler, 2012-07-23 20:36:57]

Was cleaning up old clients and ran across this...

[Scott Hess - ex-Googler, 2012-07-25 19:35:09]

ping?  Maybe you're OOO ...

[Scott Hess - ex-Googler, 2012-07-25 19:39:47]

On 2012/07/25 19:35:09, shess wrote:
> ping?  Maybe you're OOO ...

Actually, looks like you are.  If you find this LGTY, feel free to hit the
commit-queue button for me.  I'll be OOO next week, and this is hardly
high-priority.

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

thanks, this got buried on my todo list somewhere.

One question/comment. lgtm if you think it isn't necessary to make that change.

https://chromiumcodereview.appspot.com/10806071/diff/1/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://chromiumcodereview.appspot.com/10806071/diff/1/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database.cc:1103:
DeleteChunksFromStore(download_store_.get(), listid);
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]

https://chromiumcodereview.appspot.com/10806071/diff/1/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://chromiumcodereview.appspot.com/10806071/diff/1/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database.cc:1103:
DeleteChunksFromStore(download_store_.get(), listid);
On 2012/08/07 02:29:25, mattm wrote:
> 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?  

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.

[commit-bot: I haz the power, 2012-08-07 16:37:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/10806071/1

[commit-bot: I haz the power, 2012-08-07 17:57:21]

Change committed as 150369