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

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

Major differences:
 1. Even if a fullhash matches one from an addchunk, still do a gethash to verify it.
 2. Cached gethash results are cleared every update and on restart.
 3. Cached gethash results have independent cache lifetimes. (This CL uses
    hardcoded cache_lifetime of 45 minutes since it still does v2.2 requests.)

BUG=357763

[mattm, 2014-04-01 05:37:26]

shess for review
bryner for fyi

This will put us in a state where chrome is still doing v2.2 requests but using
2.3 caching rules. I think this is fine.

It doesn't address the part about getting rid of any pre-existing cached
fullhashes that were saved to the database. I guess we'll have to just dump all
chunks that contain fullhashes, but it seems ok to do that in a separate CL.

This CL should also address the corner cases I mentioned in my last email.

[Scott Hess - ex-Googler, 2014-04-01 22:08:36]

Overall, I'm still thinking about whether this change maybe changes some
rationales for the full-hash lists.  Previously they were both SBAddFullHash
because it was convenient to phrase them that way.  Now the full-hash list could
probably just be SBFullHash values, in which case it could also just be sorted
on the full hash and finding the values would be more direct.

Given that the cached items are no longer folded into the storage, it may not
make sense to keep the cached items in the database at all.  I'm uncertain about
this.

On 2014/04/01 05:37:26, mattm wrote:
> This will put us in a state where chrome is still doing v2.2 requests but
using
> 2.3 caching rules. I think this is fine.

AFAICT it won't be negative in a way which v2.3 won't expect anyhow.

> It doesn't address the part about getting rid of any pre-existing cached
> fullhashes that were saved to the database. I guess we'll have to just dump
all
> chunks that contain fullhashes, but it seems ok to do that in a separate CL.

One option to dumping the chunks entirely might be simply ignore full hashes
with a non-zero received value (meaning "all of those from before your change
rolls out").  This will result in more gethash requests, of course, but the
current data is not trustworthy.

AFAICT, the existing items are treated as valid if there has been a
recent-enough update, which effectively means that they are mostly treated as
valid regardless of whether they are actually valid.  So another option would be
to keep treating them as valid for now, until everything gets to stable, then in
the next milestone switch to not accepting anything with a non-zero received
time (by then the excess gethash requests will probably be very low volume).

Mostly I want to avoid having to write anything which has to traverse the entire
file to figure something out.  After an update we can review the full-hashes
list for chunks to delete during the next update, but somehow that needs to be
accomplished.  They could be injected into the delete list and omitted from the
server request on the next update, but I'm not entirely certain that would work
if the server then sent new chunks down.  It could be done with a local
delete-only update then a retry, but that doesn't seem that great, either.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.cc (left):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.cc:345:
sb_service_->protocol_manager()->last_update());
I can't quite tell if last_update() is necessary any longer.  AFAICT it's used
for histograms and for SafeBrowsingServerTest.  The latter case seems pretty
weak to me.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/protocol_parser.cc (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/protocol_parser.cc:70: // caching rules.
I'd rephrase to "Ignore cmd_parts[1] (add_chunk_id) ...".

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:654: // search key be the
same type as the elements in the collection.
My impression is that this is because while you can use a comparator between
different types for lower_bound() or upper_bound(), the positions of the types
are such that you can't use the same comparator for both unless the parameter
types match.  Which is how you get to this point, I think.

You could use lower_bound() to find the first element, then compare the prefix
in the outer if(), then iterate while not to the end and prefix is equal.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:687: // prefix.
I think maybe this is backwards.  If there is a valid cached result, then there
is no need to check the full-hash list.  If there is no cached result, then the
full-hash list should be checked.  If there is a hit there.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1138:
browse_gethash_cache_.clear();
I think this should be cleared when the update is committed, rather than when it
is initiated.  That extends coverage over the period the update is happening.

More contentiously, I think maybe it should be cleared when the update succeeds.
 Probably before the change_detected_ test, though I think that could be argued
either way.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.h:334: // |full_hashes| must
be sorted.
Perhaps describe this as a helper for ContainsBrowseUrl.  Also reference the
sort required for full_hashes.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_store_file.h (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_store_file.h:154: // Store updates in
file store and return |add_full_hashes_result|.
I see I didn't update the comment - should mention that kept prefixes will be
added to builder and kept full hashes to that vector.

[Scott Hess - ex-Googler, 2014-04-01 22:46:04]

BTW - there is a big piece of this which is basically "Drop add_chunk_id from
results, rename pending->cached, and don't push pending full hashes into update"
which I don't have any questions about at all.  In case you want to break that
off and land it earlier.

[mattm, 2014-04-03 01:33:48]

On 2014/04/01 22:08:36, shess wrote:
> Overall, I'm still thinking about whether this change maybe changes some
> rationales for the full-hash lists.  Previously they were both SBAddFullHash
> because it was convenient to phrase them that way.  Now the full-hash list
could
> probably just be SBFullHash values, in which case it could also just be sorted
> on the full hash and finding the values would be more direct.

Isn't the chunk id necessary for handling adddel responses? Or is that
maintained somewhere else too?

> Given that the cached items are no longer folded into the storage, it may not
> make sense to keep the cached items in the database at all.  I'm uncertain
about
> this.

By "in the database" you mean in the SafeBrowsingDatabase class? I guess it
could be moved up to the SafeBrowsingDatabaseManager. One issue is it'd have to
do the BrowseFullHashesToCheck in the database manager instead, I guess that
isn't a huge problem but it would make the ContainsBrowseUrl method inconsistent
with the other methods on SafeBrowsingDatabase (also would need to update a
bunch of tests).


> On 2014/04/01 05:37:26, mattm wrote:
> > This will put us in a state where chrome is still doing v2.2 requests but
> using
> > 2.3 caching rules. I think this is fine.
> 
> AFAICT it won't be negative in a way which v2.3 won't expect anyhow.
> 
> > It doesn't address the part about getting rid of any pre-existing cached
> > fullhashes that were saved to the database. I guess we'll have to just dump
> all
> > chunks that contain fullhashes, but it seems ok to do that in a separate CL.
> 
> One option to dumping the chunks entirely might be simply ignore full hashes
> with a non-zero received value (meaning "all of those from before your change
> rolls out").  This will result in more gethash requests, of course, but the
> current data is not trustworthy.
> 
> AFAICT, the existing items are treated as valid if there has been a
> recent-enough update, which effectively means that they are mostly treated as
> valid regardless of whether they are actually valid.  So another option would
be
> to keep treating them as valid for now, until everything gets to stable, then
in
> the next milestone switch to not accepting anything with a non-zero received
> time (by then the excess gethash requests will probably be very low volume).
> 
> Mostly I want to avoid having to write anything which has to traverse the
entire
> file to figure something out.  After an update we can review the full-hashes
> list for chunks to delete during the next update, but somehow that needs to be
> accomplished.  They could be injected into the delete list and omitted from
the
> server request on the next update, but I'm not entirely certain that would
work
> if the server then sent new chunks down.  It could be done with a local
> delete-only update then a retry, but that doesn't seem that great, either.

Hm, I do like the ignore non-zero received value option (either one), if Brian
is okay with that.

[mattm, 2014-04-03 01:38:12]

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.cc (left):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.cc:345:
sb_service_->protocol_manager()->last_update());
On 2014/04/01 22:08:36, shess wrote:
> I can't quite tell if last_update() is necessary any longer.  AFAICT it's used
> for histograms and for SafeBrowsingServerTest.  The latter case seems pretty
> weak to me.

Perhaps that could be done another way, yeah. I can think about that later.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/protocol_parser.cc (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/protocol_parser.cc:70: // caching rules.
On 2014/04/01 22:08:36, shess wrote:
> I'd rephrase to "Ignore cmd_parts[1] (add_chunk_id) ...".

Done.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:654: // search key be the
same type as the elements in the collection.
On 2014/04/01 22:08:36, shess wrote:
> My impression is that this is because while you can use a comparator between
> different types for lower_bound() or upper_bound(), the positions of the types
> are such that you can't use the same comparator for both unless the parameter
> types match.  Which is how you get to this point, I think.

Yeah. I had tried to hack by defining both orderings of args for the comparison
function but that doesn't compile.

> You could use lower_bound() to find the first element, then compare the prefix
> in the outer if(), then iterate while not to the end and prefix is equal.

Hm, yeah, just using lower_bound is a good idea. It's iterating from there
anyway so getting the upper_bound beforehand is unnecessary. Done.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:687: // prefix.
On 2014/04/01 22:08:36, shess wrote:
> I think maybe this is backwards.  If there is a valid cached result, then
there
> is no need to check the full-hash list.  If there is no cached result, then
the
> full-hash list should be checked.  If there is a hit there.

Ah, good point. Could even check the cache before checking the prefix set. Done.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1138:
browse_gethash_cache_.clear();
On 2014/04/01 22:08:36, shess wrote:
> I think this should be cleared when the update is committed, rather than when
it
> is initiated.  That extends coverage over the period the update is happening.

The spec says "Clients must clear cached full-length hashes each time they
*send* an update request." but maybe I'm interpreting that overly literally. I'd
be happy to move it to UpdatedFinished if Brian agrees.

> More contentiously, I think maybe it should be cleared when the update
succeeds.
>  Probably before the change_detected_ test, though I think that could be
argued
> either way.

The spec says it should be cleared whether the update succeeds or not.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.h:334: // |full_hashes| must
be sorted.
On 2014/04/01 22:08:36, shess wrote:
> Perhaps describe this as a helper for ContainsBrowseUrl.

Done.

> Also reference the sort required for full_hashes.

Changed the wording to make it more obvious.

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_store_file.h (right):

https://codereview.chromium.org/220493003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_store_file.h:154: // Store updates in
file store and return |add_full_hashes_result|.
On 2014/04/01 22:08:36, shess wrote:
> I see I didn't update the comment - should mention that kept prefixes will be
> added to builder and kept full hashes to that vector.

Done.

[mattm, 2014-04-03 01:44:38]

On 2014/04/01 22:46:04, shess wrote:
> BTW - there is a big piece of this which is basically "Drop add_chunk_id from
> results, rename pending->cached, and don't push pending full hashes into
update"
> which I don't have any questions about at all.  In case you want to break that
> off and land it earlier.

I could try to split it up if that would be easier to review.. otherwise just
getting the incremental step committed doesn't seem too useful on its own.

[Scott Hess - ex-Googler, 2014-04-03 18:24:10]

On 2014/04/03 01:33:48, mattm wrote:
> On 2014/04/01 22:08:36, shess wrote:
> > Overall, I'm still thinking about whether this change maybe changes some
> > rationales for the full-hash lists.  Previously they were both SBAddFullHash
> > because it was convenient to phrase them that way.  Now the full-hash list
> could
> > probably just be SBFullHash values, in which case it could also just be
sorted
> > on the full hash and finding the values would be more direct.
> 
> Isn't the chunk id necessary for handling adddel responses? Or is that
> maintained somewhere else too?

Previously the pending structure collected items which were later persisted to
disk, so those items needed the chunk-id.  But not those items are transient, so
there's no reason for them to retain the chunk-id.  The full hashes read from
disk and stored in memory never needed the chunk-id, since they were never
written back to disk.  They only had it for convenience to match the pending
structure.

[I think.  At the least, I can't see any reason why the full_browse_hashes_
structure any longer needs to store chunk-id or when-received information.]

> > Given that the cached items are no longer folded into the storage, it may
not
> > make sense to keep the cached items in the database at all.  I'm uncertain
> about
> > this.
> 
> By "in the database" you mean in the SafeBrowsingDatabase class? I guess it
> could be moved up to the SafeBrowsingDatabaseManager. One issue is it'd have
to
> do the BrowseFullHashesToCheck in the database manager instead, I guess that
> isn't a huge problem but it would make the ContainsBrowseUrl method
inconsistent
> with the other methods on SafeBrowsingDatabase (also would need to update a
> bunch of tests).

I guess it's just something to keep in mind in terms of what would be preferred
for the long term.  It might be six of one and half a dozen of the other.

> Hm, I do like the ignore non-zero received value option (either one), if Brian
> is okay with that.

I'll dump those thoughts into a bug and cc people for discussion.

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

My initial thought with the cache was that it might allow the system to get away
with all of the database rigamarole - but now that I think more about it,
probably not.  The contains check actually constructs the set of prefixes, which
is a problem, and even if someone else did that, there would still be prefixes
which wouldn't be in the cache because they weren't even in the database.  Maybe
it would work if PrefixSet and the full_hashes list were also lifted out of the
database, which has a certain sense to it, but is probably a huge project.

I think the overall check implements the right thing.  I was confused for a bit
by the change from returning the full-hash matches, but I think it makes sense
in context.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.h:295: const base::TimeDelta&
cache_lifetime);
AFAICT this function is no longer used anywhere.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/protocol_manager.cc:249: // TODO(cbentzel): Should
cache_lifetime be set to 0 here?
Is there a tracking thing for this?  It seems like a form of negative caching -
if the response is invalid in the service for structural reasons, we wouldn't
want to pound the server fetching additional invalid responses, so instead we
cache a negative hit for the lifetime.  BUT, if the response is invalid, we also
don't want to trust the parsed lifetime info.  So maybe there should be a fixed
term for caching such things.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:662:
browse_gethash_cache_.erase(citer);
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?

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:679: if
(browse_prefix_set_->Exists(prefix)) {
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;
  }
}

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (left):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1011:
EXPECT_TRUE(database_->prefix_miss_cache_.empty());
EXPECT_TRUE(database_->hash_cache_.empty()); might be a reasonable alternative.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1002:
hash_cache->end());
I think EXPECT_TRUE(hash_cache->count(key)) would be reasonable here and below. 
Or count EQ 1 or GT 0.  Also since you dereference find(), you should make the
check an ASSERT.

Also, honestly, you're checking that the cache ended up with the items you
passed, so I'm not entirely sure that it couldn't instead be:
  EXPECT_EQ(hash_cache->size(), prefix_misses.size());
  ASSERT_TRUE(hash_cache->count(prefix_misses[0]));
  EXPECT_TRUE(hash_cache->find(prefix_misses[0])->second.full_hashes.empty());
...

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1701:
EXPECT_TRUE(database_->UpdateStarted(&lists));
Suggest ASSERT_TRUE(), as probably if this line fails, various lines below will
DCHECK or something.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:2001: // since
the DB containts a cached hit with matching prefix but not
"contains"

[Scott Hess - ex-Googler, 2014-04-03 22:42:05]

BTW, I apologize for the existence of safe_browsing_database_unittest.cc.

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

updated.

Patch set 5 addresses CL review comments.

Patch set 6 addresses the comments on the latest email thread. It seems to work,
though I haven't thought about what the changes in InsertAdd/InsertSub will do
when presented with an existing database...

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.h:295: const base::TimeDelta&
cache_lifetime);
On 2014/04/03 21:44:03, shess wrote:
> AFAICT this function is no longer used anywhere.

Removed.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/protocol_manager.cc:249: // TODO(cbentzel): Should
cache_lifetime be set to 0 here?
On 2014/04/03 21:44:03, shess wrote:
> Is there a tracking thing for this?  It seems like a form of negative caching
-
> if the response is invalid in the service for structural reasons, we wouldn't
> want to pound the server fetching additional invalid responses, so instead we
> cache a negative hit for the lifetime.  BUT, if the response is invalid, we
also
> don't want to trust the parsed lifetime info.  So maybe there should be a
fixed
> term for caching such things.

Didn't find one, so I filed http://crbug.com/360232

Also added a histogram for it (and for the urlrequest errors too)

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:662:
browse_gethash_cache_.erase(citer);
On 2014/04/03 21:44:03, shess wrote:
> 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?

Done.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:679: if
(browse_prefix_set_->Exists(prefix)) {
On 2014/04/03 21:44:03, shess wrote:
> 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;
>   }
> }

Done.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (left):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1011:
EXPECT_TRUE(database_->prefix_miss_cache_.empty());
On 2014/04/03 21:44:03, shess wrote:
> EXPECT_TRUE(database_->hash_cache_.empty()); might be a reasonable
alternative.

PopulateDatabaseForCacheTest inserts some more things into the cache so it
doesn't work with the single cache (for both misses and hits), but I moved the
check to other place where it works.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1002:
hash_cache->end());
On 2014/04/03 21:44:03, shess wrote:
> I think EXPECT_TRUE(hash_cache->count(key)) would be reasonable here and
below. 
> Or count EQ 1 or GT 0.  Also since you dereference find(), you should make the
> check an ASSERT.
> 
> Also, honestly, you're checking that the cache ended up with the items you
> passed, so I'm not entirely sure that it couldn't instead be:
>   EXPECT_EQ(hash_cache->size(), prefix_misses.size());
>   ASSERT_TRUE(hash_cache->count(prefix_misses[0]));
>   EXPECT_TRUE(hash_cache->find(prefix_misses[0])->second.full_hashes.empty());
> ...

Done.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1701:
EXPECT_TRUE(database_->UpdateStarted(&lists));
On 2014/04/03 21:44:03, shess wrote:
> Suggest ASSERT_TRUE(), as probably if this line fails, various lines below
will
> DCHECK or something.

Done.

https://codereview.chromium.org/220493003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:2001: // since
the DB containts a cached hit with matching prefix but not
On 2014/04/03 21:44:03, shess wrote:
> "contains"

Done.

[Scott Hess - ex-Googler, 2014-04-05 00:28:28]

On 2014/04/04 23:58:49, mattm wrote:
> updated.

Just FYI, my brain has reached saturation for the day, and tomorrow looks kind
of busy.  I'll think hard about reviewing by Monday before I forget all the
important pieces.

[Scott Hess - ex-Googler, 2014-04-07 17:32:02]

https://codereview.chromium.org/220493003/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/220493003/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:683: }
I think we need to iron out the thread we're having with Brian about whether
this is correct.  IMHO if the server sent a prefix with an associated fullhash,
then we definitely need to check the fullhash in this case.  But maybe the
server never ever would send that?  I don't think it's obvious.

Also, this change implies that all existing databases are broken and will need
to be resynced.  Which also is a problem.