1365499 - Remove duplicated entries V4 in HashCompleter

Status: RESOLVED INVALID

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[Thomas Nguyen (:tnguyen)]

In V4, we should removed duplicated hash prefix entries in HashCompleter before propagating the hash prefixes to server.
One case that we may have duplicated entries is:
- in V4 we found 2 prefixes matches : ABABABAB and ABABABABEF, then decide to send only 4 bytes to server. Then the prefix and its noise will be duplicated
We did filter duplicated prefixes in V2 and should be do the same in V4

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: Remove duplicated entries V4 in HashCompleter

MozReview-Commit-ID: Fu6fJpVufJw

Comment 2

[François Marier [:francois]]

Comment on attachment 8868419 [details] [diff] [review]
Remove duplicated entries V4 in HashCompleter

Review of attachment 8868419 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ +471,5 @@
>  
>      // Build the "distinct" prefix array.
>      let prefixSet = new Set();
> +    this._requests.forEach(r => {
> +      let encodedParitalHash = btoa(r.partialHash);

typo: "Parital"

Comment 3

[Dimi Lee [:dimi]]

Comment on attachment 8868419 [details] [diff] [review]
Remove duplicated entries V4 in HashCompleter

Review of attachment 8868419 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ +476,5 @@
> +      // Do not need to propagate duplicated prefixes to server
> +      if (!prefixSet.has(encodedParitalHash)) {
> +        prefixSet.add(encodedParitalHash);
> +      }
> +    });

I found this is not necessary because prefixSet is already a "Set"

Comment 4

[Thomas Nguyen (:tnguyen)]

You are right, thanks you Dimi. Closed this.