1329558 - Implement Minimum wait duration for V4 gethash

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Dimi Lee [:dimi]]

Minimum wait duration is described in
https://developers.google.com/safe-browsing/v4/request-frequency

This value specifies how long update or gethash request should wait until next request could be made.

Comment 1

[Dimi Lee [:dimi]]

Hi Henry,
I just checked the code and i guess we have not yet implemented this for v4 update, is it correct ?
If we haven't implement this for update, do you think we should do it before "Enable update v4 on nightly" ?

Comment 2

[Dimi Lee [:dimi]]

offline checked with henry, this is already implemented for update.
So the only thing left here is to also support it in gethash

Comment 3

[Thomas Nguyen (:tnguyen)]

I am looking at this. I think we will not care about the case that we are going to restart FF (the minimum wait duration we get in the last running FF will be reset to 0). 
Otherwise, we may need to save the last gethash time, min wait duration (probably to Prefs or database). Taking a glance at chromium code and they ignored that. The minimum wait duration normally is rather small and may not have to many effects.

Comment 4

[Thomas Nguyen (:tnguyen)]

Attachment: WIP

Comment 5

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1329558 - Implement Minimum wait duration for V4 gethash

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

Comment 6

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8834757 [details]
Bug 1329558 - Implement Minimum wait duration for V4 gethash

https://reviewboard.mozilla.org/r/110600/#review111868

::: toolkit/components/url-classifier/tests/unit/test_hashcompleter_v4.js:57
(Diff revision 1)
>  // Handles request for TEST_TABLE_DATA_V4.
>  let gHttpServV4 = null;
> -let gExpectedGetHashQueryV4 = "";
>  
> -const NEW_CLIENT_STATE = 'sta\0te';
> -const CHECKSUM = '\x30\x67\xc7\x2c\x5e\x50\x1c\x31\xe3\xfe\xca\x73\xf0\x47\xdc\x34\x1a\x95\x63\x99\xec\x70\x5e\x0a\xee\x9e\xfb\x17\xa1\x55\x35\x78';
> +const NEW_CLIENT_STATE = "sta\0te";
> +const CHECKSUM = "\x30\x67\xc7\x2c\x5e\x50\x1c\x31\xe3\xfe\xca\x73\xf0\x47\xdc\x34\x1a\x95\x63\x99\xec\x70\x5e\x0a\xee\x9e\xfb\x17\xa1\x55\x35\x78";

Nit
Eliminate warning from eslint

::: toolkit/components/url-classifier/tests/unit/test_hashcompleter_v4.js:93
(Diff revision 1)
> -  gExpectedGetHashQueryV4 = '&$req=' + request;
> +  registerHandlerGethashV4("&$req=" + request);
> -
>    let completeFinishedCnt = 0;
>  
>    gCompleter.complete("0123", TEST_TABLE_DATA_V4.gethashUrl, TEST_TABLE_DATA_V4.tableName, {
> -    completion: function (hash, table, chunkId) {
> +    completion(hash, table, chunkId) {

ditto

Comment 7

[François Marier [:francois]]

Comment on attachment 8834757 [details]
Bug 1329558 - Implement Minimum wait duration for V4 gethash

https://reviewboard.mozilla.org/r/110600/#review111914

Good patch. I've got a few comments around validation of the results from Google and making the units clearer.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:169
(Diff revision 1)
>  
>    // Whether we have been informed of a shutdown by the shutdown event.
>    this._shuttingDown = false;
>  
> +  // A map of getHash URLs to next gethash time
> +  this._nextGethashTimes = {};

This is in milliseconds, right?

Let's call this `_nextGethashTimeMs` to make the unit explicit.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:270
(Diff revision 1)
>    },
>  
>    // Returns true if we can make a request from the given url, false otherwise.
>    canMakeRequest: function(aGethashUrl) {
> -    return this._backoffs[aGethashUrl].canMakeRequest();
> +    return this._backoffs[aGethashUrl].canMakeRequest() &&
> +           Date.now() > this._nextGethashTimes[aGethashUrl];

This should probably be `>=`.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:571
(Diff revision 1)
>          log("V4 fullhash response parsed callback: " +
>              "MinWaitDuration(" + aMinWaitDuration + "), " +
>              "NegativeCacheDuration(" + aNegCacheDuration + ")");
>  
> +        this._completer._nextGethashTimes[this.gethashUrl] =
> +          Date.now() + aMinWaitDuration;

We should put a limit on how big `aMinWaitDuration` could be. Maybe a maximum of 24 hours [like for updates](http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/toolkit/components/url-classifier/content/listmanager.js#20).

Also, we should make sure `aMinWaitDuration >= 0` too in case we receive a negative number.

Comment 8

[Thomas Nguyen (:tnguyen)]

Thanks for your review

Comment 9

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8834757 [details]
Bug 1329558 - Implement Minimum wait duration for V4 gethash

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

Comment 10

[François Marier [:francois]]

Comment on attachment 8834757 [details]
Bug 1329558 - Implement Minimum wait duration for V4 gethash

https://reviewboard.mozilla.org/r/110600/#review112266

Could you also please add the following fix to your patch: [DurationToMs() incorrectly converts nanoseconds to microseconds instead of milliseconds](http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp#430-434).

We can fix it by just looking at the seconds and ignoring the nanoseconds.

We should also have the following comment above that line:

    // Seconds precision is good enough. Ignore nanoseconds like Chrome does.

After that, we should make sure the tests still pass because we might be relying on this buggy behaviour.

Comment 11

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8834757 [details]
Bug 1329558 - Implement Minimum wait duration for V4 gethash

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

Comment 12

[François Marier [:francois]]

Comment on attachment 8834757 [details]
Bug 1329558 - Implement Minimum wait duration for V4 gethash

https://reviewboard.mozilla.org/r/110600/#review112300

Comment 13

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8398be408d71&selectedJob=75865182

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3cd262da4ec1
Implement Minimum wait duration for V4 gethash r=francois

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/3cd262da4ec1