SafeBrowsing: Implement cache eviction for API full hash results.

SafeBrowsing: Implement cache eviction for API full hash results.

BUG=561867
Committed: https://crrev.com/4973a979cd41a717aa90bd5cd65658ef0dbac705
Cr-Commit-Position: refs/heads/master@{#397570}

[kcarattini, 2016-05-26 06:36:44]

kcarattini@chromium.org changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2016-05-31 22:28:31]

lgtm

...though this depends on a CL still under review.

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:176: size_t i = 0;
how about something a bit more descriptive, like matched_idx

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:220:
std::sort(prefixes_needing_reqs->begin(), prefixes_needing_reqs->end());
an idea: prefixes_needing_req's could be a set (like your fix to api metatdata)

[Nathan Parker, 2016-05-31 23:32:04]

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:195: if
(cache_result.expire_after <= now) {
What if there are positive cache entries that haven't expired? Or is the
negative cache always longer?

Might it be necessary to have a periodic scan for expired entries, so they don't
sit around forever if they aren't revisited? I'm not familiar enough with the
pver3 caching to know if it does that.

[kcarattini, 2016-06-01 10:58:36]

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:176: size_t i = 0;
On 2016/05/31 22:28:31, Nathan Parker wrote:
> how about something a bit more descriptive, like matched_idx

Done.

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:195: if
(cache_result.expire_after <= now) {
On 2016/05/31 23:32:04, Nathan Parker wrote:
> What if there are positive cache entries that haven't expired? Or is the
> negative cache always longer?

Alex, correct me if I'm wrong, but I do not believe there is any guarantee that
the negative cache duration is longer. This code only removes the full hash in
question (that has expired), and only removes the entire cache entry if there
are no more full hashes in the entry (therefore no more positive cache entries
that haven't expired yes).

> 
> Might it be necessary to have a periodic scan for expired entries, so they
don't
> sit around forever if they aren't revisited? I'm not familiar enough with the
> pver3 caching to know if it does that.

I do not believe the pver3 code does this, so I would guess it's probably not
necessary. But I don't know. We could add UMA for the number of entries in the
cache? Something else?

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:220:
std::sort(prefixes_needing_reqs->begin(), prefixes_needing_reqs->end());
On 2016/05/31 22:28:31, Nathan Parker wrote:
> an idea: prefixes_needing_req's could be a set (like your fix to api
metatdata)

I added a TODO. Since this change will require piping through the protocol
manager as well, I don't want to bloat this cl.

[woz, 2016-06-02 15:38:25]

awoz@chromium.org changed reviewers:
+ awoz@chromium.org

[woz, 2016-06-02 15:38:26]

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):

https://codereview.chromium.org/2009133005/diff/60001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:195: if
(cache_result.expire_after <= now) {
On 2016/06/01 10:58:36, kcarattini wrote:
> On 2016/05/31 23:32:04, Nathan Parker wrote:
> > What if there are positive cache entries that haven't expired? Or is the
> > negative cache always longer?
> 
> Alex, correct me if I'm wrong, but I do not believe there is any guarantee
that
> the negative cache duration is longer. This code only removes the full hash in
> question (that has expired), and only removes the entire cache entry if there
> are no more full hashes in the entry (therefore no more positive cache entries
> that haven't expired yes).

That's right -- there's no guarantee that neg. cache duration > pos. cache
duration.

> 
> > 
> > Might it be necessary to have a periodic scan for expired entries, so they
> don't
> > sit around forever if they aren't revisited? I'm not familiar enough with
the
> > pver3 caching to know if it does that.
> 
> I do not believe the pver3 code does this, so I would guess it's probably not
> necessary. But I don't know. We could add UMA for the number of entries in the
> cache? Something else? 
>

[kcarattini, 2016-06-02 22:14:18]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2016-06-02 22:14:19]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2009133005/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-02 22:15:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009133005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009133005/120001

[commit-bot: I haz the power, 2016-06-03 00:56:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-03 00:58:10]

Description was changed from

==========
SafeBrowsing: Implement cache eviction for API full hash results.

BUG=561867
==========

to

==========
SafeBrowsing: Implement cache eviction for API full hash results.

BUG=561867

Committed: https://crrev.com/4973a979cd41a717aa90bd5cd65658ef0dbac705
Cr-Commit-Position: refs/heads/master@{#397570}
==========

[commit-bot: I haz the power, 2016-06-03 00:58:11]

Patchset 7 (id:??) landed as
https://crrev.com/4973a979cd41a717aa90bd5cd65658ef0dbac705
Cr-Commit-Position: refs/heads/master@{#397570}