1264885 - Refactor the listmanager to add support for both V2 an V4 of the protocol

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[François Marier [:francois]]

This will involve defining what the new prefs will look like for V4 providers, but it will not include actually implementing V4 updates.

Comment 1

[Henry Chang [:hchang]]

Attachment: Part 1: Build update request based on provider's version

Comment 2

[Henry Chang [:hchang]]

Attachment: Part 1: Build update request based on provider's version

Comment 3

[Henry Chang [:hchang]]

Attachment: Part 1: Make update request pver-aware

Comment 4

[Henry Chang [:hchang]]

Attachment: Part 2: Make RequestBackoff parameters pver-aware

Comment 5

[François Marier [:francois]]

For the backoff part, let's try to see if we can just use the same algorithm for both V2 and V4 given that they are almost the same. I will ask Google.

Comment 6

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #5)
> For the backoff part, let's try to see if we can just use the same algorithm
> for both V2 and V4 given that they are almost the same. I will ask Google.

Google says that we can use the V4 algorithm in both cases. In practice the early re-tries have not been useful

So let's simplify the backoff code by migrating to the V4 backoff algorithm now for all Safe Browsing providers regardless of version.

Comment 7

[François Marier [:francois]]

Comment on attachment 8754259 [details] [diff] [review]
Part 2: Make RequestBackoff parameters pver-aware

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

As per comment 6, we can remove the V2 stuff from this patch.

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 8754259 [details] [diff] [review]
Part 2: Make RequestBackoff parameters pver-aware

Moved to Bug 1273398 since v2 backoff config will no longer be used.

Comment 9

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1264885-Use-the-table-name-to-decide-how-to-buil.patch

Comment 10

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1264885-Use-the-table-name-to-decide-how-to-buil.patch

Comment 11

[Henry Chang [:hchang]]

Attached a new patch which 

1) removes "goog4-*-proto"s from urlclassifier-phish/malwareTable to prevent the v4 stuff being used accidentally
2) removes stub function buildUpdateRequestProtobuf_()

from the previous patch.

Besides, even though v4 update/gethashURL is not actually being used, I still put the up-to-date URL to those prefs according to [1][2]. Hope they will not be change anymore...

[1] https://chromium.googlesource.com/chromium/src.git/+/f7dbf39be31d8aa9214d5d84da613508d4e06491/components/safe_browsing_db/v4_update_protocol_manager.cc#355
[2] https://chromium.googlesource.com/chromium/src.git/+/f7dbf39be31d8aa9214d5d84da613508d4e06491/components/safe_browsing_db/v4_get_hash_protocol_manager.cc#414

Comment 12

[François Marier [:francois]]

Comment on attachment 8770878 [details] [diff] [review]
0001-Bug-1264885-Use-the-table-name-to-decide-how-to-buil.patch

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

Looks good.

I don't think we need to add a test here, but please do a manual test to ensure that V2 updates still work.

Comment 13

[Henry Chang [:hchang]]

Manual testing and try looks good. Flagged checkin-needed

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c64c6048bc0b&selectedJob=24012223

Comment 14

[Carsten Book [:Tomcat]]

has problems to apply:

1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/all.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug-1264885-Use-the-table-name-to-decide-how-to-buil.patch

Comment 15

[Henry Chang [:hchang]]

Attachment: Patch for checkin (r+'ed and rebased)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5230005e91
Use the table name to decide how to build update request. r=francois

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad5230005e91