1328821 - hash completion request for v4 should not depend on table freshness

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Dimi Lee [ooo Mar. 28 - Apr. 6]]

Right now when Classifier matches a completion, table freshness is used to decide if this completion is out of date or not, the reason we are doing that is because we don't have a TTL for caching.
As for V4, positive caching and negative caching is introduced[1] so we should not use table freshness as TTL.

[1] https://developers.google.com/safe-browsing/v4/caching

Comment 1

[Dimi Lee [ooo Mar. 28 - Apr. 6]]

Attachment: Bug 1328821 - hash completion request for v4 should not depend on table freshness.

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

Comment 2

[Dimi Lee [ooo Mar. 28 - Apr. 6]]

Comment on attachment 8823980 [details]
Bug 1328821 - hash completion request for v4 should not depend on table freshness.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102464/diff/1-2/

Comment 3

[Henry Chang [:hchang]]

Comment on attachment 8823980 [details]
Bug 1328821 - hash completion request for v4 should not depend on table freshness.

https://reviewboard.mozilla.org/r/102464/#review103712

Looks pretty good to me! 

Ideally I'd like to combine "freshness" and "positive/negative" cache as one concept. 
However, per offline discussion with Dimi and Thomas, combining freshness and positive/negative cache 
is not a trivial work and I don't see any immediate benefit from it. So just leave freshness
and positive/negative cache split apart.

::: toolkit/components/url-classifier/Classifier.cpp:486
(Diff revision 2)
> -        int64_t age;
> -        bool found = mTableFreshness.Get(cache->TableName(), &age);
> -        if (!found) {
> -          age = 24 * 60 * 60; // just a large number
> -        } else {
> -          int64_t now = (PR_Now() / PR_USEC_PER_SEC);
> +        // For V2, there is no TTL for caching, so we use table freshness to
> +        // decide if matching a completion should trigger a gethash request or not.
> +        // For V4, this is done by Positive Caching & Negative Caching mechanism.
> +        bool confirmed = false;
> +        if (fromCache) {
> +          cache->IsHashEntryConfirmed(lookupHash, mTableFreshness,

Is there any reason or concern that we can't merge |IsHashEntryConfirmed| to |Has|? It seems to me |fromCache| is just an intermediary info that Classifier does not need to know. Why don't we have 
|Has| return |confirmed| directly?

One of the reasons might be to reduce the number of arguments we have to pass to |Has|. (otherwise we have pass mTableFreshness and aFreshnessGuarantee to |Has|)

Anyways both works for me just would like to know if I am understanding correctly :)

::: toolkit/components/url-classifier/LookupCache.cpp:463
(Diff revision 2)
> +                                    bool* aConfirmed)
> +{
> +  int64_t age;
> +  bool found = aTableFreshness.Get(mTableName, &age);
> +  if (!found) {
> +    age = 24 * 60 * 60; // just a large number

It seems to be copied from the old code, but what do you think of using just UINT_MAX or return false here?

Comment 4

[Dimi Lee [ooo Mar. 28 - Apr. 6]]

Thanks for review!

(In reply to Henry Chang [:henry][:hchang] from comment #3)
> 
> ::: toolkit/components/url-classifier/Classifier.cpp:486
> (Diff revision 2)
> > -        int64_t age;
> > -        bool found = mTableFreshness.Get(cache->TableName(), &age);
> > -        if (!found) {
> > -          age = 24 * 60 * 60; // just a large number
> > -        } else {
> > -          int64_t now = (PR_Now() / PR_USEC_PER_SEC);
> > +        // For V2, there is no TTL for caching, so we use table freshness to
> > +        // decide if matching a completion should trigger a gethash request or not.
> > +        // For V4, this is done by Positive Caching & Negative Caching mechanism.
> > +        bool confirmed = false;
> > +        if (fromCache) {
> > +          cache->IsHashEntryConfirmed(lookupHash, mTableFreshness,
> 
> Is there any reason or concern that we can't merge |IsHashEntryConfirmed| to
> |Has|? It seems to me |fromCache| is just an intermediary info that
> Classifier does not need to know. Why don't we have 
> |Has| return |confirmed| directly?
> 
> One of the reasons might be to reduce the number of arguments we have to
> pass to |Has|. (otherwise we have pass mTableFreshness and
> aFreshnessGuarantee to |Has|)
> 
> Anyways both works for me just would like to know if I am understanding
> correctly :)
> 

You are right, the reason I add "IsHashEntryConfirmed" is too reduce the number of arguments passed to |Has|.
Another reason is that we have testcase to test |Has| and i don't want to make |Has| to have "expiration" logic in it.

> ::: toolkit/components/url-classifier/LookupCache.cpp:463
> (Diff revision 2)
> > +                                    bool* aConfirmed)
> > +{
> > +  int64_t age;
> > +  bool found = aTableFreshness.Get(mTableName, &age);
> > +  if (!found) {
> > +    age = 24 * 60 * 60; // just a large number
> 
> It seems to be copied from the old code, but what do you think of using just
> UINT_MAX or return false here?

I'll set UINT_MAX, thanks for suggestion.

Comment 5

[Dimi Lee [ooo Mar. 28 - Apr. 6]]

Comment on attachment 8823980 [details]
Bug 1328821 - hash completion request for v4 should not depend on table freshness.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102464/diff/2-3/

Comment 6

[François Marier [:francois]]

Comment on attachment 8823980 [details]
Bug 1328821 - hash completion request for v4 should not depend on table freshness.

https://reviewboard.mozilla.org/r/102464/#review105036

Looks good. Just a few small things before giving you an r+.

Feel free to remove the "for reviewer only" bits too in your next revision.

::: toolkit/components/url-classifier/LookupCache.h:73
(Diff revision 3)
>  
>    nsCString mTableName;
>  
>    uint32_t mPartialHashLength;
> +
> +  // True if the lookup result is confirmed as an unsafe url.

I would suggest "True as long as this lookup hasn't expired"

::: toolkit/components/url-classifier/LookupCache.cpp:460
(Diff revision 3)
> +LookupCacheV2::IsHashEntryConfirmed(const Completion& aEntry,
> +                                    const TableFreshnessMap& aTableFreshness,
> +                                    uint32_t aFreshnessGuarantee,
> +                                    bool* aConfirmed)
> +{
> +  int64_t age;

Let's add a comment to be explicit:

    int64_t age; // in seconds

::: toolkit/components/url-classifier/LookupCache.cpp:468
(Diff revision 3)
> +    *aConfirmed = false;
> +  } else {
> +    int64_t now = (PR_Now() / PR_USEC_PER_SEC);
> +
> +    // Considered completion as unsafe if its table is up-to-date.
> +    *aConfirmed = (now - age) < aFreshnessGuarantee;

Let's put an assert here which ensures that `age <= now`.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:105
(Diff revision 3)
>      uint32_t prefix = aCompletion.ToUint32();
>      LOG(("Probe in V4 %s: %X, found %d, complete %d", mTableName.get(),
> -          prefix, *aHas, *aComplete));
> +          prefix, *aHas, length == COMPLETE_SIZE));
>    }
>  
> +  // Note. !!!

We could add `// TODO : Bug 1311935 - Implement v4 caching` in here too.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:155
(Diff revision 3)
> +                                    uint32_t aFreshnessGuarantee,
> +                                    bool* aConfirmed)
> +{
> +  // TODO : Bug 1311935 - Implement v4 caching
> +
> +  *aConfirmed = false;

Maybe we should return `true` until this is implemented?

It would prevent us sending any extra gethash requests.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1007
(Diff revision 3)
>          if (NS_SUCCEEDED(rv)) {
>            mPendingCompletions++;
>          }
>        } else {
>          // For tables with no hash completer, a complete hash match is
>          // good enough, we'll consider it fresh, even if it hasn't been updated

Let's use the word "valid" instead of "fresh" since we're no longer relying on table freshness.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1008
(Diff revision 3)
>            mPendingCompletions++;
>          }
>        } else {
>          // For tables with no hash completer, a complete hash match is
>          // good enough, we'll consider it fresh, even if it hasn't been updated
>          // in 45 minutes.

We may as well remove the hard-coded "45 minutes" here and say "even if it hasn't been updated in a long time".

Comment 7

[Dimi Lee [ooo Mar. 28 - Apr. 6]]

Comment on attachment 8823980 [details]
Bug 1328821 - hash completion request for v4 should not depend on table freshness.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102464/diff/3-4/

Comment 8

[François Marier [:francois]]

Comment on attachment 8823980 [details]
Bug 1328821 - hash completion request for v4 should not depend on table freshness.

https://reviewboard.mozilla.org/r/102464/#review105822

Comment 9

[François Marier [:francois]]

Let's wait until bug 1331534 has landed before landing this completion fixed.

That will ensure that regardless of caching, we won't be honouring V4 matches.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5561d7805979
hash completion request for v4 should not depend on table freshness. r=francois,henry

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5561d7805979