1311933 - Add telemetry to measure if completion match type is the same for v2 and v4

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Dimi Lee [:dimi]]

When we get a full match on V2, it should be the same type (i.e. malware, phishing or unwanted) as in V4.

In the case of multiple matches being returned in V2 or V4:
if the number of different threat types is different, then it's a potential problem
especially when the error page that we show on V2 v. V4 would be different given the priority of the threat types.

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: WIP patch : Add telemetry to measure if completion match type is the same for v2 and v4

MozReview-Commit-ID: UuSaZJXdrw

Comment 2

[Dimi Lee [:dimi]]

Hi Francois,
I plan to use bit flags to record the information if the threat type is different.
Following is the idea, could you help check if you agree with this approach ? thanks!

eSameType   = 0x00
eV2Phishing = 0x01,
eV2Malware  = 0x02,
eV2Unwated  = 0x04,
eV4Phising  = 0x08,
eV4Malware  = 0x10,
eV4Unwanted = 0x20,

If threat type is the same, no matter it matches single or multiple threat type, the value recorded to telemetry will be 0. Otherwise, the value will be set according to the bit flags above.
For example, if v2 matches phising and unwanted, v4 matches malware and unwanted, the value will be the OR result of eV2Phishing, eV2Unwated, eV4Malware and eV4Unwanted.

Comment 3

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #2)
> eSameType   = 0x00
> eV2Phishing = 0x01,
> eV2Malware  = 0x02,
> eV2Unwated  = 0x04,
> eV4Phising  = 0x08,
> eV4Malware  = 0x10,
> eV4Unwanted = 0x20,
>
> If threat type is the same, no matter it matches single or multiple threat
> type, the value recorded to telemetry will be 0. Otherwise, the value will
> be set according to the bit flags above.
> For example, if v2 matches phising and unwanted, v4 matches malware and
> unwanted, the value will be the OR result of eV2Phishing, eV2Unwated,
> eV4Malware and eV4Unwanted.

That sounds like a great idea.

It's more complicated than what I originally had in mind, but it will also be more useful if we find discrepancies in the data we receive from Google.

Comment 4

[Dimi Lee [:dimi]]

Attachment: WIP Patch

Comment 5

[Dimi Lee [:dimi]]

I will move this bug to milestone 6 because it requires some logic implemented in SB caching v4(get threat type of each fullhash returned in fullhash response).

Comment 6

[Dimi Lee [:dimi]]

Attachment: WIP patch

This is a WIP patch based on bug 1311935.

Comment 7

[Dimi Lee [:dimi]]

Attachment: Bug 1311933 - P1. Use integer as the key of safebrowsing cache.

In Bug 1323953, we always send 4-bytes prefix for completion and the prefix is also
used as the key to store cache result from gethash request.
Since it is always 4-bytes, we could convert it to integer for simplicity.

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

Comment 8

[Dimi Lee [:dimi]]

Attachment: Bug 1311933 - P2. Add telemetry to measure if completion match type is the same for v2 and v4.

When full match is found in both v2 and v4, the threat types returned should also be the same.
If threat types are different, the telemetry record this by setting a bit flags which indicates
what threat types are being returned.

If threat types are the same, this telemetry will record 0.

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

Comment 9

[Dimi Lee [:dimi]]

Hi Francois,
Part1 of this patch is required because "prefix" field will be also used by CacheResultV2 so I move it to the base class.
"prefix" field is not necessary an integer but I think it will be better than use nsCString.

Comment 10

[François Marier [:francois]]

Comment on attachment 8856903 [details]
Bug 1311933 - P1. Use integer as the key of safebrowsing cache.

https://reviewboard.mozilla.org/r/128824/#review131646

Comment 11

[François Marier [:francois]]

Comment on attachment 8856904 [details]
Bug 1311933 - P2. Add telemetry to measure if completion match type is the same for v2 and v4.

https://reviewboard.mozilla.org/r/128826/#review131652

datareview+ and r+

::: toolkit/components/telemetry/Histograms.json:4388
(Diff revision 1)
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",
> +    "n_values": 64,
> +    "bug_numbers": [1311933],
> +    "description": "Whether or not an URL lookup matches the same threat type for v2 and v4. 0 if v2 and v4 match the same threat types, otherwise this value is a bit flags indicates what threat types are matched for the lookup(in 0x01 = v2 phising, 0x02 = v2 malware, 0x04 = v2 unwanted, 0x08 = v4 phising, 0x10 = v4 malware, 0x20 = v4 unwanted)"

typo: "phishing"

::: toolkit/components/telemetry/Histograms.json:4388
(Diff revision 1)
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",
> +    "n_values": 64,
> +    "bug_numbers": [1311933],
> +    "description": "Whether or not an URL lookup matches the same threat type for v2 and v4. 0 if v2 and v4 match the same threat types, otherwise this value is a bit flags indicates what threat types are matched for the lookup(in 0x01 = v2 phising, 0x02 = v2 malware, 0x04 = v2 unwanted, 0x08 = v4 phising, 0x10 = v4 malware, 0x20 = v4 unwanted)"

"Whether or not a full URL match is of the same threat type on V2 and V4 (0 = same, otherwise the value is a bit flag where 0x01 = V2 phishing, 0x02 = V2 malware..."

::: toolkit/components/url-classifier/SBTelemetryUtils.h:35
(Diff revision 1)
> +};
> +MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(MatchResult)
> +
> +enum class MatchThreatType : uint8_t
> +{
> +  eMatch            = 0x00,

I would suggest `eIdentical` to avoid the confusion with `eNoMatch` above.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1375
(Diff revision 1)
> +      for (LookupResult& l : *(mResults.get())) {
> +        if (l.hash.fixedLengthPrefix != c->prefix) {
> +          continue;
> +        }
> +
> +        // We don't care results are not confirmed.

or simply `// Ignore unconfirmed results`

Comment 12

[Dimi Lee [:dimi]]

Comment on attachment 8856903 [details]
Bug 1311933 - P1. Use integer as the key of safebrowsing cache.

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

Comment 13

[Dimi Lee [:dimi]]

Comment on attachment 8856904 [details]
Bug 1311933 - P2. Add telemetry to measure if completion match type is the same for v2 and v4.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/444f6cb17c45
P1. Use integer as the key of safebrowsing cache. r=francois
https://hg.mozilla.org/integration/autoland/rev/aa409a18a2fe
P2. Add telemetry to measure if completion match type is the same for v2 and v4. r=francois

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/444f6cb17c45
https://hg.mozilla.org/mozilla-central/rev/aa409a18a2fe