1309161 - Update freshness of completion in getHash

Status: RESOLVED DUPLICATE of bug 1333328

(Toolkit :: Safe Browsing, defect, P2)

Description

[Thomas Nguyen (:tnguyen)]

During the test of data freshness, the freshness of completion was not updated after getHash.
For example there's a case that we get full hash from getHash (but not in update), then next time matching, the completion appears to be stale.

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1309161 - Update freshness of completion in getHash.

Review commit: https://reviewboard.mozilla.org/r/84818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84818/

Comment 2

[François Marier [:francois]]

Comment on attachment 8799675 [details]
Bug 1309161 - Update freshness of completion in getHash.

https://reviewboard.mozilla.org/r/84818/#review83948

I'm not sure that this patch is updating the right freshness value. Also, it seems to be missing a few files.

::: toolkit/components/url-classifier/Classifier.cpp:1042
(Diff revision 1)
>    lookupCache->DumpCache();
>  #endif
>  
> +  int64_t now = (PR_Now() / PR_USEC_PER_SEC);
> +  LOG(("Updated completion age from getHash %s\n", table.get()));
> +  mTableFreshness.Put(table, now);

Doesn't that mark the whole table as fresh?

If we get the completions for a given prefix, it means that we can update the freshness of that prefix.

However, we shouldn't update the freshness of the other prefixes that we didn't ask for. They could be expired by now.

::: toolkit/components/url-classifier/tests/mochitest/chrome.ini:8
(Diff revision 1)
>  support-files =
>    allowlistAnnotatedFrame.html
>    classifiedAnnotatedFrame.html
>    classifiedAnnotatedPBFrame.html
>    bug_1281083.html
> +  gethash.sjs

Most of these files are missing from the patch.

Comment 3

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8799675 [details]
Bug 1309161 - Update freshness of completion in getHash.

https://reviewboard.mozilla.org/r/84818/#review83948

> Most of these files are missing from the patch.

These file exist and are being use by mochitest plain. I just add them to chrome test, dont need rewrite them

Comment 4

[François Marier [:francois]]

Comment on attachment 8799675 [details]
Bug 1309161 - Update freshness of completion in getHash.

https://reviewboard.mozilla.org/r/84818/#review83948

> These file exist and are being use by mochitest plain. I just add them to chrome test, dont need rewrite them

Oh, you're right, I completely missed that! I apologize.

Comment 5

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8799675 [details]
Bug 1309161 - Update freshness of completion in getHash.

https://reviewboard.mozilla.org/r/84818/#review83948

> Doesn't that mark the whole table as fresh?
> 
> If we get the completions for a given prefix, it means that we can update the freshness of that prefix.
> 
> However, we shouldn't update the freshness of the other prefixes that we didn't ask for. They could be expired by now.

You are right, should not do that.
The possile solution here is that we should add and update the age of completion in lookupCache (each time adding completion to cache in Update, or GetHash).

Comment 6

[Thomas Nguyen (:tnguyen)]

We are handling the case that gethash should update time to live of completion in lookupcache (currently, it's set to ttl of the whole table update, mTableFreshness).
For example the below case:
- Update(set ttl) ----(over 45min, expired)---> clasifyURL match(this should trigger GetHash), and then, if there no new update, each time we find an matching URL from that time, GetHash is always called
As said in specs:
"A URL matches a full length hash obtained in a response to an HTTP Request for Full Length Hashes made within the past 45 minutes from the time a warning is to be provided, provided that such a hash has not been subsequently removed from the list (e.g. via a sub chunk)"

Hmm, after thinking about that, obviously, ttl of completion in this case does not depend on the server (V2 is not like v3 and v4, that server returns a ttl in response), but depends on the last result of matching in client(last result is matched, then update ttl, otherwise using prefix's ttl). 
The specs is a little bit confused because server does not know if the completion response is matched or not. I think v2 server does not care about ttl of completion in GetHash  but only care about ttl of prefix(or completion) in Update.
Imho, I'd prefer to update ttl of completion each time doing getHash, no matter if the url matches or not.
So we should find a way to store ttl completion (probably clone a new hashtable of ttl completions stored in LookupCache then use it to check the freshness of completion).
What do you think about that?

Comment 7

[François Marier [:francois]]

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #6)
> We are handling the case that gethash should update time to live of
> completion in lookupcache (currently, it's set to ttl of the whole table
> update, mTableFreshness).

Ok, so we're doing the second bullet under https://web.archive.org/web/20160422212049/https://developers.google.com/safe-browsing/developers_guide_v2#LookupsAgeOfDataUsage right:

"A URL matches a full length hash obtained in a response to an HTTP Request for Full Length Hashes, provided that the prefix of the matching full length hash has not been removed from the list (e.g. via a sub chunk), and further provided that the list has been successfully updated via an HTTP Request for Data (where the entire update was successfully processed) within the past 45 minutes from the time a warning is to be provided"

> For example the below case:
> - Update(set ttl) ----(over 45min, expired)---> clasifyURL match(this should
> trigger GetHash), and then, if there no new update, each time we find an
> matching URL from that time, GetHash is always called
> As said in specs:
> "A URL matches a full length hash obtained in a response to an HTTP Request
> for Full Length Hashes made within the past 45 minutes from the time a
> warning is to be provided, provided that such a hash has not been
> subsequently removed from the list (e.g. via a sub chunk)"

My reading of this is that we also need to consider a full hash valid if it got returned by gethash in the past 45 minutes.

> Hmm, after thinking about that, obviously, ttl of completion in this case
> does not depend on the server (V2 is not like v3 and v4, that server returns
> a ttl in response), but depends on the last result of matching in
> client(last result is matched, then update ttl, otherwise using prefix's
> ttl).
> The specs is a little bit confused because server does not know if the
> completion response is matched or not. I think v2 server does not care about
> ttl of completion in GetHash  but only care about ttl of prefix(or
> completion) in Update.

By "does not depend on the server", I think you mean "does not depend on list updates", which I think is not quite right. It depends on either list updates or gethash. If one of these was < 45 minutes ago, then the completion is still valid.

> So we should find a way to store ttl completion (probably clone a new
> hashtable of ttl completions stored in LookupCache then use it to check the
> freshness of completion).
> What do you think about that?

Yes, I think we need to find a way to store the ttl of gethash requests too and then use that in addition to what we currently use when determining whether or not a completion is fresh.

Comment 8

[Thomas Nguyen (:tnguyen)]

Thanks you for sharing your time for this.
> "A URL matches a full length hash obtained in a response to an HTTP Request
> for Full Length Hashes made within the past 45 minutes from the time a
> warning is to be provided, provided that such a hash has not been
> subsequently removed from the list (e.g. via a sub chunk)"

Yep, but after looking again, you will see "from the time a warning is to be provided" means, we only update ttl of gethash completion if the URL matches completion (UI shows warning).
That's what I thought oddly, or am I missing something?
If I understand correctly, I'd prefer to update ttl everytime gethash, no matter if the warning is shown

Comment 9

[François Marier [:francois]]

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #8)
> Yep, but after looking again, you will see "from the time a warning is to be
> provided" means, we only update ttl of gethash completion if the URL matches
> completion (UI shows warning).

Ah yes, I see what you mean. This seems to imply that we only keep track of the TTLs on HITs, not MISSes. That seems odd to me too.

> If I understand correctly, I'd prefer to update ttl everytime gethash, no
> matter if the warning is shown

I think it makes sense to update the TTL of the complete hash after every gethash request. We should however not update the TTL of the prefix. That should stay with the TTL of the table that we got from the last update.

Comment 10

[Dimi Lee [:dimi]]

This one should be resolved when Bug 1333328 land