1336909 - Restrict URLCLASSIFIER_PREFIX_MATCH to profiles that have working V4

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

It's possible that URLCLASSIFIER_PREFIX_MATCH includes matches on machines where V4 is disabled (by the user) or because of a missing Google API key, thereby inflating the difference between V2 and V4.

We should try to only send that probe when we detect that both V2 and V4 are working.

Comment 1

[Dimi Lee [:dimi]]

Attachment: WIP

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1336909 - Restrict URLCLASSIFIER_PREFIX_MATCH to profiles that have working V4.

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

Comment 3

[François Marier [:francois]]

Comment on attachment 8834769 [details]
Bug 1336909 - Restrict URLCLASSIFIER_PREFIX_MATCH to profiles that have working V4.

https://reviewboard.mozilla.org/r/110612/#review111886

::: toolkit/components/url-classifier/Classifier.cpp:455
(Diff revision 1)
>      } else {
>        return NS_ERROR_FAILURE;
>      }
>    }
>  
> +  // Only record telemetry when both v2 and v4 have data.

Is it possible that we have a database but no lookup cache yet?

(i.e. because we've downloaded the list but we haven't used it in a lookup yet)

::: toolkit/components/url-classifier/LookupCache.cpp:476
(Diff revision 1)
>  }
>  
> +bool
> +LookupCacheV2::IsEmpty()
> +{
> +  bool IsEmpty;

nit: `isEmpty` (lowercase `i`)

::: toolkit/components/url-classifier/LookupCacheV4.cpp:123
(Diff revision 1)
>  }
>  
> +bool
> +LookupCacheV4::IsEmpty()
> +{
> +  bool IsEmpty;

nit: `isEmpty` (lowercase `i`)

::: toolkit/components/url-classifier/LookupCacheV4.cpp:124
(Diff revision 1)
>  
> +bool
> +LookupCacheV4::IsEmpty()
> +{
> +  bool IsEmpty;
> +  mVLPrefixSet->IsEmpty(&IsEmpty);

Do we also need to check the fixed-length prefix set here?

Comment 4

[Dimi Lee [:dimi]]

Comment on attachment 8834769 [details]
Bug 1336909 - Restrict URLCLASSIFIER_PREFIX_MATCH to profiles that have working V4.

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

Comment 5

[Dimi Lee [:dimi]]

Comment on attachment 8834769 [details]
Bug 1336909 - Restrict URLCLASSIFIER_PREFIX_MATCH to profiles that have working V4.

https://reviewboard.mozilla.org/r/110612/#review111886

> Is it possible that we have a database but no lookup cache yet?
> 
> (i.e. because we've downloaded the list but we haven't used it in a lookup yet)

lookup cache contains prefix set(database) and cache(completions).
So as long as we have data in database, we will have lookup cache.

> Do we also need to check the fixed-length prefix set here?

fixed-length prefix set is included in the IsEmpty call of VLPrefixSet.

Comment 6

[François Marier [:francois]]

Comment on attachment 8834769 [details]
Bug 1336909 - Restrict URLCLASSIFIER_PREFIX_MATCH to profiles that have working V4.

https://reviewboard.mozilla.org/r/110612/#review111892

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f0968db9234
Restrict URLCLASSIFIER_PREFIX_MATCH to profiles that have working V4. r=francois

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f0968db9234