1273398 - Implement RequestBackoff for Safe Browsing v4

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Henry Chang [:hchang]]

The backoff on request failure for v4 is slightly different from v2. We need to implement RequestBackoff (in request-backoff.js) when v4 is enabled.

Comment 1

[François Marier [:francois]]

(In reply to Henry Chang [:henry] from comment #0)
> The backoff on request failure for v4 is slightly different from v2. We need
> to implement RequestBackoff (in request-backoff.js) when v4 is enabled.

Is the backoff algorithm in V4 a lot different?

It might be useful to compare the backoff in V2 and V4 and summarize the differences. If they're not too different, we could ask Google whether they are OK with us using the V4 backoff in our V2 code too. That way we'd have only one backoff implementation.

Comment 2

[Henry Chang [:hchang]]

I digested the backoff spec in [1] and it seems they would be only different on the first error and the maximum waiting time:

       v2                 v4
----------------------------------
1    1 min             15~30 min
2    30~60  min        30~60 min
3    60~120 min        60~120 min
4    120~240 min       120~240 min
5    240~480 min       240~480 min
6    480 min           480~960 min 
7    480 min           960~1440 min
8    480 min           1440 min
9    ...               ...

I couldn't tell if they make a lot difference so maybe only google can answer this question. Do you know who is the googler that we used to contact? Thanks!

[1] https://docs.google.com/document/d/149TrX2FRMLgfchQJ-cTto6yLlPYlJVjF5QTDulERRTs/edit#

Comment 3

[Henry Chang [:hchang]]

The RequestBackoff can be seamlessly configured to v4 spec. See patch Part 2 in Bug 1264885. Probably we only need Bug 1264885.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/request-backoff.js

Comment 4

[François Marier [:francois]]

Thanks for the table Henry, that's super helpful!

I've got a meeting with Google in a few days and I will ask if they're OK with us using the V4 backoff for V2 as well. I assume they will be fine with that because it will put even less load on their servers.

Sticking to a single backoff algorithm would have a few benefits:

- only one thing to test
- we can land it ahead of time on V2 and make sure it works before the switch to V4
- less moving parts in one of the most critical parts of the Safe Browsing code

Comment 5

[Henry Chang [:hchang]]

Attachment: Part 1: listmanager v4 backoff

Comment 6

[Henry Chang [:hchang]]

Attachment: Part 2: hashcompleter v4 backoff

Comment 7

[Henry Chang [:hchang]]

We decided to use v4 backoff parameters for both v2 and v4 according to [1]. So, just made patches to change to v4 backoff settings for listmanager and hashcompleter.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1264885#c6

Comment 8

[François Marier [:francois]]

Comment on attachment 8760148 [details] [diff] [review]
Part 2: hashcompleter v4 backoff

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

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ +196,5 @@
>        // after they are dispatched.
>        var jslib = Cc["@mozilla.org/url-classifier/jslib;1"]
>                    .getService().wrappedJSObject;
> +
> +      // We use v4 request backoff both v2 and v4. See listmanager.js

You could just say "See bug 1273398."

Comment 9

[François Marier [:francois]]

Comment on attachment 8760147 [details] [diff] [review]
Part 1: listmanager v4 backoff

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

This approach works for me.

An alternative that might reduce code duplication would be to move the default parameters into RequestBackoff directly. That way we would only have one copy of the V4 backoff numbers and we could potentially add a test for it.

The test would look like the table you have in here, checking that the code matches what the spec says.

Anyways, up to you if you want to improve this code further or if you'd rather leave it the way it is.

::: toolkit/components/url-classifier/content/listmanager.js
@@ +114,5 @@
>    // Keep track of all of our update URLs.
>    if (!this.needsUpdate_[updateUrl]) {
>      this.needsUpdate_[updateUrl] = {};
> +
> +    // Following is the backoff table for v2 and v4, which shows the

Instead of documenting this decision here, we could also simply refer to the bug with "Using the V4 backoff algorithm for both V2 and V4. See bug 1273398."

Comment 10

[Henry Chang [:hchang]]

Attachment: Part 1: Wrap to RequestBackoffV4

Comment 11

[Henry Chang [:hchang]]

Attachment: Part 2: list manager to use RequestBackoffV4

Comment 12

[Henry Chang [:hchang]]

Attachment: Part 3: Hashcompleter to use RequestBakcoffV4

Comment 13

[Henry Chang [:hchang]]

Thanks for review and the alternative approach is indeed much better. I've re-attached patches so could you have review again? I just created a wrapper (RequestBackoffV4) and it is to be used by listmanager and hashcompleter.

(In reply to François Marier [:francois] from comment #9)
> Comment on attachment 8760147 [details] [diff] [review]
> Part 1: listmanager v4 backoff
> 
> Review of attachment 8760147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This approach works for me.
> 
> An alternative that might reduce code duplication would be to move the
> default parameters into RequestBackoff directly. That way we would only have
> one copy of the V4 backoff numbers and we could potentially add a test for
> it.
> 
> The test would look like the table you have in here, checking that the code
> matches what the spec says.
> 
> Anyways, up to you if you want to improve this code further or if you'd
> rather leave it the way it is.
>

Comment 14

[François Marier [:francois]]

I think this is ready to land, but if you could do a manual test first that would be great because it's a pretty important part of the code.

Comment 15

[Henry Chang [:hchang]]

The following is how I test the patches:

1) Modify 'browser.safebrowsing.provider.google.updateURL' to 'https://safebrowsing.google.com/safebrowsing-whatever', which would return 404 for whatever request we make.
2) Turn on 'browser.safebrowsing.debug'
3) Start firefox from scratch with the above changes applied.
4) See if 

"listmanager: 16:30:45 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;"

is printed with the expected timestamps.

(the log is printed by [1])

[1] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/toolkit/components/url-classifier/content/listmanager.js#409

Comment 16

[Henry Chang [:hchang]]

listmanager: 15:07:44 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;
listmanager: 15:35:25 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;
listmanager: 16:30:45 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;
listmanager: 18:21:27 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;
listmanager: 22:02:50 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;
listmanager: 05:25:36 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;
listmanager: 20:11:09 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;
listmanager: 20:11:09 GMT+0800 (CST): makeUpdateRequestForEntry_: request goog-phish-shavar;

The retry interval is

~28 min
~55 min
~111 min
~221 min
~443 min
~886 min
~24 hr
~24 hr

Just exactly what we expect!

Comment 17

[Ethan Tseng [:ethan]]

(In reply to Henry Chang [:henry][:hchang] from comment #16)
> Just exactly what we expect!

Good job!  :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4f819766df6d
Part 1: Wrap RequestBackoff to a v4-specific one. r=francois.
https://hg.mozilla.org/integration/fx-team/rev/05c73c2ad630
Part 2: Modify listmanager to use RequestBackoffV4. r=francois.
https://hg.mozilla.org/integration/fx-team/rev/01a3b0eeee75
Part 3: Modify "hash completer" to use RequestBackoffV4. r=francois.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f819766df6d
https://hg.mozilla.org/mozilla-central/rev/05c73c2ad630
https://hg.mozilla.org/mozilla-central/rev/01a3b0eeee75