Closed Bug 1264885 Opened 4 years ago Closed 3 years ago

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

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: francois, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(1 file, 6 obsolete files)

This will involve defining what the new prefs will look like for V4 providers, but it will not include actually implementing V4 updates.
Depends on: 1037555
Depends on: 1110891
Depends on: 1273395
Attachment #8753295 - Attachment is obsolete: true
Attachment #8753793 - Attachment is obsolete: true
Attachment #8754259 - Attachment description: Part 1: Make RequestBackoff parameters pver-aware → Part 2: Make RequestBackoff parameters pver-aware
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.
Attachment #8754259 - Flags: feedback?(francois)
Attachment #8754259 - Attachment is obsolete: true
Attachment #8754259 - Flags: feedback?(francois)
Attachment #8754259 - Attachment is obsolete: false
No longer depends on: 1110891
(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 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.
Attachment #8754259 - Flags: review-
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.
Attachment #8754259 - Attachment is obsolete: true
Attachment #8754240 - Attachment is obsolete: true
Whiteboard: #sbv4-m0
Attachment #8770077 - Attachment is obsolete: true
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 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.
Attachment #8770878 - Flags: review+
Manual testing and try looks good. Flagged checkin-needed

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c64c6048bc0b&selectedJob=24012223
Keywords: checkin-needed
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
Flags: needinfo?(hchang)
Keywords: checkin-needed
Attachment #8770878 - Attachment is obsolete: true
Flags: needinfo?(hchang)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5230005e91
Use the table name to decide how to build update request. r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ad5230005e91
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.