Closed Bug 1311933 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: #sbv4-m6)

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → tnguyen
Attachment #8811190 - Attachment description: Add telemetry to measure if completion match type is the same for v2 and v4 → WIP patch : Add telemetry to measure if completion match type is the same for v2 and v4
Whiteboard: #sbv4-m4 → #sbv4-m5
Assignee: tnguyen → hchang
Status: NEW → ASSIGNED
Assignee: hchang → dlee
Attachment #8811190 - Attachment is obsolete: true
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.
Flags: needinfo?(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.
Flags: needinfo?(francois)
Attached patch WIP Patch (obsolete) — Splinter Review
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).
Whiteboard: #sbv4-m5 → #sbv4-m6
Attachment #8839346 - Attachment is obsolete: true
Attached patch WIP patchSplinter Review
This is a WIP patch based on bug 1311935.
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 on attachment 8856903 [details]
Bug 1311933 - P1. Use integer as the key of safebrowsing cache.

https://reviewboard.mozilla.org/r/128824/#review131646
Attachment #8856903 - Flags: review?(francois) → review+
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": ["safebrowsing-telemetry@mozilla.org"],
> +    "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": ["safebrowsing-telemetry@mozilla.org"],
> +    "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`
Attachment #8856904 - Flags: review?(francois) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/444f6cb17c45
https://hg.mozilla.org/mozilla-central/rev/aa409a18a2fe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1358324
You need to log in before you can comment on or make changes to this bug.