1305801 - Store V4 update data to disk

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Dimi Lee [:dimi]]

To store v4 update data, we have to
1. Support variable length PrefixSet (bug 1283007)
2. Support full update and write variable length prefixes to disk (bug 1283009)
3. Support partial update (bug 1287058)

Comment 1

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P2. Add telemetry for variable-length prefix set.

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

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P2. Add telemetry for variable-length prefix set.

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

Comment 3

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P3. Testcase for variable length prefixSet.

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

Comment 4

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P4. Store variable-length prefix to disk.

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

Comment 5

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

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

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8795477 [details]
Bug 1305801 - P2. Add telemetry for variable-length prefix set.

This patch is already r+ in bug 1283007

Comment 7

[Dimi Lee [:dimi]]

Comment on attachment 8795478 [details]
Bug 1305801 - P2. Add telemetry for variable-length prefix set.

This patch is already r+ in bug 1283007

Comment 8

[Dimi Lee [:dimi]]

Comment on attachment 8795479 [details]
Bug 1305801 - P3. Testcase for variable length prefixSet.

This patch is already r+ in bug 1283007

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 8795480 [details]
Bug 1305801 - P4. Store variable-length prefix to disk.

This patch is already r+ in bug 1283009

Comment 10

[Dimi Lee [:dimi]]

Comment on attachment 8795481 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

This patch is already r+ in bug 1287058

Comment 11

[François Marier [:francois]]

Comment on attachment 8795477 [details]
Bug 1305801 - P2. Add telemetry for variable-length prefix set.

https://reviewboard.mozilla.org/r/81510/#review80082

Carrying existing r+

Comment 12

[François Marier [:francois]]

Comment on attachment 8795478 [details]
Bug 1305801 - P2. Add telemetry for variable-length prefix set.

https://reviewboard.mozilla.org/r/81512/#review80084

::: toolkit/components/telemetry/Histograms.json:3769
(Diff revision 1)
>      "n_buckets": 15,
>      "description": "Time spent constructing PrefixSet from DB (ms)"
>    },
> +  "URLCLASSIFIER_VLPS_FILELOAD_TIME": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "56",

Let's make these 58 since Nightly is now up to 52.

::: toolkit/components/telemetry/Histograms.json:3778
(Diff revision 1)
> +    "bug_numbers": [1283007],
> +    "description": "Time spent loading Variable-Length PrefixSet from file (ms)"
> +  },
> +  "URLCLASSIFIER_VLPS_FALLOCATE_TIME": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "56",

ditto

Comment 13

[François Marier [:francois]]

Comment on attachment 8795479 [details]
Bug 1305801 - P3. Testcase for variable length prefixSet.

https://reviewboard.mozilla.org/r/81514/#review80086

Carrying previous r+

Comment 14

[François Marier [:francois]]

Comment on attachment 8795480 [details]
Bug 1305801 - P4. Store variable-length prefix to disk.

https://reviewboard.mozilla.org/r/81516/#review80092

Carrying prior r+

Comment 15

[Dimi Lee [:dimi]]

Comment on attachment 8795477 [details]
Bug 1305801 - P2. Add telemetry for variable-length prefix set.

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

Comment 16

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P1. Implement variable length PrefixSet class for Safe Browsing v4.

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

Comment 17

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P3. Testcase for variable length prefixSet.

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

Comment 18

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P4. Store variable-length prefix to disk.

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

Comment 19

[Dimi Lee [:dimi]]

Attachment: Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

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

Comment 20

[François Marier [:francois]]

Comment on attachment 8795493 [details]
Bug 1305801 - P3. Testcase for variable length prefixSet.

https://reviewboard.mozilla.org/r/81538/#review80096

Carrying prior r+

Comment 21

[François Marier [:francois]]

Comment on attachment 8795494 [details]
Bug 1305801 - P4. Store variable-length prefix to disk.

https://reviewboard.mozilla.org/r/81540/#review80098

Carrying prior r+

Comment 22

[François Marier [:francois]]

Comment on attachment 8795492 [details]
Bug 1305801 - P1. Implement variable length PrefixSet class for Safe Browsing v4.

https://reviewboard.mozilla.org/r/81536/#review80100

Carrying prior r+

Comment 23

[Dimi Lee [:dimi]]

Attachment: P5 interdiff

Hi gcp,
Since we want to land bug 1283007, bug 1283009 and bug 1257058 together, so I move those patches to this new bug. Could you help review the last part again ? Attach the interdiff(compare with the patch you have reviewed before) because there are some modifications according to francois's suggestion in Bug 1287058 Comment 25.
Thanks for help!

Comment 27

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

https://reviewboard.mozilla.org/r/81542/#review80292

Francois asked:
 - In the case of a prefix being in the add list and also in the original prefix set, we may want to issue a warning and to ignore it instead of returning an error. Perhaps with telemetry as well.

The Telemetry part wasn't addressed. I think it would be good to add a telemetry boolean (or enum for failing in the different update parts?) whenever we encounter a failure to update. If the algorithm fails somewhere in the field we won't otherwhise know.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:159
(Diff revision 1)
> +  nsDependentCSubstring smallestOldPrefix;
> +  nsDependentCSubstring smallestAddPrefix;
> +
> +  // This is used to avoid infinite loop for partial update algorithm.
> +  uint32_t index = MAX_LOOPS;
> +  for(;index > 0; index--) {

I know francois asked to make sure this loop doesn't get infinite, but these magic numbers here are way too ugly.

As I understand, every loop must process either an element from oldPSet or one from addPSet. That means that the maximum number of iterations here would be the sum of their sizes, plus or minus one due to loop boundary conditions.

So I suggest replacing the magic number here by the amount of times you expect the loop to run. If you break out of the loop due to hitting that maximum, there's been an error somewhere.

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:281
(Diff revision 1)
> +    CreateRandomSortedPrefixArray(5000, 4, 4, pArray);
> +    CreateRandomSortedPrefixArray(1000, 5, 32, pArray);
> +    RemoveIntersection(fArray, pArray);
> +    PrefixArrayToPrefixStringMap(pArray, pMap);
> +
> +    // Remove 1/5 of elemets of original prefix set.

...elements...

This is misspelled throughout the file, so do a search & replace.

Comment 28

[Dimi Lee [:dimi]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #27)
> 
> The Telemetry part wasn't addressed. I think it would be good to add a
> telemetry boolean (or enum for failing in the different update parts?)
> whenever we encounter a failure to update. If the algorithm fails somewhere
> in the field we won't otherwhise know.
> 

We discussed after that and we think maybe a warning message is good enough for now, so that's why i didn't add telemetry.
But we all agree add telemetry will be useful so i will add it in this patch.

> 
> So I suggest replacing the magic number here by the amount of times you
> expect the loop to run. If you break out of the loop due to hitting that
> maximum, there's been an error somewhere.
> 

That's a really a good idea, thank you!

Comment 29

[Dimi Lee [:dimi]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

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

Comment 30

[François Marier [:francois]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

https://reviewboard.mozilla.org/r/81542/#review80458

::: toolkit/components/telemetry/Histograms.json:3773
(Diff revision 2)
>      "alert_emails": ["[e-mail]"],
>      "expires_in_version": "58",
>      "kind": "exponential",
>      "high": 1000,
>      "n_buckets": 10,
> -    "bug_numbers": [1283007],
> +    "bug_numbers": [1305801],

I think it would be better to keep the old bug number because that's where we explained why we need to collect this data.

::: toolkit/components/telemetry/Histograms.json:3782
(Diff revision 2)
>      "alert_emails": ["[e-mail]"],
>      "expires_in_version": "58",
>      "kind": "exponential",
>      "high": 1000,
>      "n_buckets": 10,
> -    "bug_numbers": [1283007],
> +    "bug_numbers": [1305801],

ditto

::: toolkit/components/telemetry/Histograms.json:3828
(Diff revision 2)
>    },
> +  "URLCLASSIFIER_UPDATE_ERROR_TYPE": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "58",
> +    "kind": "enumerated",
> +    "n_values": 3,

It would be better to use a larger number here because we can never change the `n_values` once we add it. That way if we need to add other types of errors later, we have room to do it. Maybe use 10?

::: toolkit/components/telemetry/Histograms.json:3830
(Diff revision 2)
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "58",
> +    "kind": "enumerated",
> +    "n_values": 3,
> +    "bug_numbers": [1305801],
> +    "description": " (0=DUPLICATE PREFIX, 1=INFINITE LOOP, 2=WRONG REMOVAL INDICES)"

"An error was encountered while parsing a partial update returned by a Safe Browsing V4 server (0 = addition of an already existing prefix, 1 = parser got into an infinite loop, 2 = removal index out of bounds)"

::: toolkit/components/url-classifier/LookupCacheV4.cpp:216
(Diff revision 2)
> +                            DUPLICATE_PREFIX);
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +
> +  // We expecet index will be greater or equal to 0 because number of maximum runs will be

typo: "expect"

"number of maximum runs" => "max number of runs"

Comment 31

[Dimi Lee [:dimi]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

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

Comment 32

[Dimi Lee [:dimi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7373408bfd04

Comment 33

[Dimi Lee [:dimi]]

Comment on attachment 8795492 [details]
Bug 1305801 - P1. Implement variable length PrefixSet class for Safe Browsing v4.

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

Comment 34

[Dimi Lee [:dimi]]

Comment on attachment 8795477 [details]
Bug 1305801 - P2. Add telemetry for variable-length prefix set.

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

Comment 35

[Dimi Lee [:dimi]]

Comment on attachment 8795493 [details]
Bug 1305801 - P3. Testcase for variable length prefixSet.

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

Comment 36

[Dimi Lee [:dimi]]

Comment on attachment 8795494 [details]
Bug 1305801 - P4. Store variable-length prefix to disk.

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

Comment 37

[Dimi Lee [:dimi]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

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

Comment 38

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

https://reviewboard.mozilla.org/r/81542/#review81494

::: toolkit/components/url-classifier/LookupCacheV4.cpp:162
(Diff revisions 1 - 4)
>    nsDependentCSubstring smallestOldPrefix;
>    nsDependentCSubstring smallestAddPrefix;
>  
>    // This is used to avoid infinite loop for partial update algorithm.
> -  uint32_t index = MAX_LOOPS;
> -  for(;index > 0; index--) {
> +  int32_t index = oldPSet.Count() + addPSet.Count();
> +  for(;index >= -1; index--) {

Why not do the +1 on the previous line? (And maybe, comment where it comes from! Right now -1 is just a magic number)

It's even weirder because the error check below is also <= -1. So if the for loop check here is on the -1 condition, we're already in an error state? So why not break out immediately instead of continuing to -2?

::: toolkit/components/url-classifier/LookupCacheV4.cpp:218
(Diff revisions 1 - 4)
>      }
>    }
>  
> -  if (index <= 0) {
> -    NS_WARNING("There are too many prefxies than expected.");
> +  // We expect index will be greater or equal to 0 because max number of runs will be
> +  // the number of original prefix + add prefix.
> +  if (index <= -1) {

so it's if (index < 0)

::: toolkit/components/url-classifier/LookupCacheV4.cpp:241
(Diff revisions 1 - 4)
> +  : mCount(0)
>  {
>    for (auto iter = aMap.ConstIter(); !iter.Done(); iter.Next()) {
>      uint32_t size = iter.Key();
>      mMap.Put(size, new PrefixString(*iter.Data(), size));
> +    mCount += iter.Data()->Length() / size;

Is this not equivalent to mCount++?

::: toolkit/components/url-classifier/LookupCacheV4.cpp:251
(Diff revisions 1 - 4)
> +  : mCount(0)
>  {
>    for (auto iter = aMap.ConstIter(); !iter.Done(); iter.Next()) {
>      uint32_t size = iter.Key();
>      mMap.Put(size, new PrefixString(iter.Data()->GetPrefixString(), size));
> +    mCount += iter.Data()->GetPrefixString().Length() / size;

Same question.

::: toolkit/components/url-classifier/Classifier.cpp:979
(Diff revision 4)
>          nsCString* prefix = new nsCString(iter.Data()->GetPrefixString());
> -        prefixes.Put(iter.Key(), prefix);
> +        output->Put(iter.Key(), prefix);
>        }
>      } else {
> -      // TODO: Bug 1287058, partial update
> +      PrefixStringMap* input = nullptr;
> +      // If both prefix set is empty, this means we are doing a partial update

...if both prefix sets are...

Comment 39

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8795492 [details]
Bug 1305801 - P1. Implement variable length PrefixSet class for Safe Browsing v4.

https://reviewboard.mozilla.org/r/81536/#review81486

Comment 40

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8795493 [details]
Bug 1305801 - P3. Testcase for variable length prefixSet.

https://reviewboard.mozilla.org/r/81538/#review81490

Comment 41

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8795494 [details]
Bug 1305801 - P4. Store variable-length prefix to disk.

https://reviewboard.mozilla.org/r/81540/#review81492

Comment 42

[Dimi Lee [:dimi]]

Thanks for review!

(In reply to Gian-Carlo Pascutto [:gcp] from comment #38)
> Comment on attachment 8795495 [details]
> Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.
> 
> https://reviewboard.mozilla.org/r/81542/#review81494
> 
> ::: toolkit/components/url-classifier/LookupCacheV4.cpp:162
> (Diff revisions 1 - 4)
> >    nsDependentCSubstring smallestOldPrefix;
> >    nsDependentCSubstring smallestAddPrefix;
> >  
> >    // This is used to avoid infinite loop for partial update algorithm.
> > -  uint32_t index = MAX_LOOPS;
> > -  for(;index > 0; index--) {
> > +  int32_t index = oldPSet.Count() + addPSet.Count();
> > +  for(;index >= -1; index--) {
> 
> Why not do the +1 on the previous line? (And maybe, comment where it comes
> from! Right now -1 is just a magic number)
> 

I'll change to +1. I use -- in this patch is only because then i don't have to write 
"oldPSet.Count() + addPSet.Count()" again in error check statement.

> 
> ::: toolkit/components/url-classifier/LookupCacheV4.cpp:241
> (Diff revisions 1 - 4)
> > +  : mCount(0)
> >  {
> >    for (auto iter = aMap.ConstIter(); !iter.Done(); iter.Next()) {
> >      uint32_t size = iter.Key();
> >      mMap.Put(size, new PrefixString(*iter.Data(), size));
> > +    mCount += iter.Data()->Length() / size;
> 
> Is this not equivalent to mCount++?
> 

No, mCount calculates the total prefixes we have, and for prefixes with the same prefix size will be
stored in |iter.Data| as a large string. So string length/prefix size will be the number of prefixes for a given prefix size.

Comment 43

[Dimi Lee [:dimi]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

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

Comment 44

[Dimi Lee [:dimi]]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7373408bfd04
https://treeherder.mozilla.org/#/jobs?repo=try&revision=898398837a55

Comment 45

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #42)
> > 
> > Why not do the +1 on the previous line? (And maybe, comment where it comes
> > from! Right now -1 is just a magic number)
> > 
> 
> I'll change to +1. I use -- in this patch is only because then i don't have
> to write 
> "oldPSet.Count() + addPSet.Count()" again in error check statement.
> 

ah, sorry i found i mis-understood what you are saying first time.
I will fix this...

Comment 46

[Dimi Lee [:dimi]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81542/diff/5-6/

Comment 47

[Dimi Lee [:dimi]]

try result is in Comment 44

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Please mark the unresolved issues with Part 5 in MozReview as resolved so that this can autoland.

Comment 49

[Dimi Lee [:dimi]]

All issues are marked as fixed. Could you help land it, thanks.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14457cc4c325
Part 1: Implement variable length PrefixSet class for Safe Browsing v4. r=francois, r=gcp
https://hg.mozilla.org/integration/autoland/rev/56ceae52d847
Part 2: Add telemetry for variable-length prefix set. r=francois
https://hg.mozilla.org/integration/autoland/rev/def8da367beb
Part 3: Testcase for variable length prefixSet. r=francois, r=gcp
https://hg.mozilla.org/integration/autoland/rev/bca0e706dbc5
Part 4: Store variable-length prefix to disk. r=francois, r=gcp
https://hg.mozilla.org/integration/autoland/rev/0c95d5dec6d9
Part 5: Support SafeBrowsing v4 partial update. r=gcp

Comment 51

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/aa1675d1dcf6633e1fcc6921090ce253be2c23ff for ASan gtest heap-buffer-overflows in VariableLengthPrefixSet.VariableLengthSet a la https://treeherder.mozilla.org/logviewer.html#?job_id=4404877&repo=autoland#L7615

Comment 52

[Dimi Lee [:dimi]]

Comment on attachment 8795492 [details]
Bug 1305801 - P1. Implement variable length PrefixSet class for Safe Browsing v4.

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

Comment 53

[Dimi Lee [:dimi]]

Comment on attachment 8795477 [details]
Bug 1305801 - P2. Add telemetry for variable-length prefix set.

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

Comment 54

[Dimi Lee [:dimi]]

Comment on attachment 8795493 [details]
Bug 1305801 - P3. Testcase for variable length prefixSet.

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

Comment 55

[Dimi Lee [:dimi]]

Comment on attachment 8795494 [details]
Bug 1305801 - P4. Store variable-length prefix to disk.

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

Comment 56

[Dimi Lee [:dimi]]

Comment on attachment 8795495 [details]
Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81542/diff/6-7/

Comment 57

[Dimi Lee [:dimi]]

Fix the try error:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3432a293f98e&selectedJob=28504105

The root cause is memcmp in P1 may out of bound in certain case.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39a5fc72213d
Part 1: Implement variable length PrefixSet class for Safe Browsing v4. r=francois, r=gcp
https://hg.mozilla.org/integration/autoland/rev/4b01da3f451a
Part 2: Add telemetry for variable-length prefix set. r=francois
https://hg.mozilla.org/integration/autoland/rev/94d8f8d4e3d5
Part 3: Testcase for variable length prefixSet. r=francois, r=gcp
https://hg.mozilla.org/integration/autoland/rev/122db2c234f1
Part 4: Store variable-length prefix to disk. r=francois, r=gcp
https://hg.mozilla.org/integration/autoland/rev/2d4f6c10aacf
Part 5: Support SafeBrowsing v4 partial update. r=gcp

Comment 59

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

https://hg.mozilla.org/mozilla-central/rev/39a5fc72213d
https://hg.mozilla.org/mozilla-central/rev/4b01da3f451a
https://hg.mozilla.org/mozilla-central/rev/94d8f8d4e3d5
https://hg.mozilla.org/mozilla-central/rev/122db2c234f1
https://hg.mozilla.org/mozilla-central/rev/2d4f6c10aacf

Comment 60

[Georg Fritzsche [:gfritzsche]]

(In reply to François Marier [:francois] from comment #30)
> Comment on attachment 8795495 [details]
> Bug 1305801 - P5. Supports SafeBrowsing v4 partial update.
> 
> https://reviewboard.mozilla.org/r/81542/#review80458
> 
> ::: toolkit/components/telemetry/Histograms.json:3773
> (Diff revision 2)
> >      "alert_emails": ["[e-mail]"],
> >      "expires_in_version": "58",
> >      "kind": "exponential",
> >      "high": 1000,
> >      "n_buckets": 10,
> > -    "bug_numbers": [1283007],
> > +    "bug_numbers": [1305801],
> 
> I think it would be better to keep the old bug number because that's where
> we explained why we need to collect this data.

Or just add both in these cases - it's an array to allow for adding multiple related bugs.