Closed Bug 1329558 Opened 4 years ago Closed 4 years ago

Implement Minimum wait duration for V4 gethash

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: dimi, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m5)

Attachments

(1 file, 1 obsolete file)

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.
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" ?
Flags: needinfo?(hchang)
offline checked with henry, this is already implemented for update.
So the only thing left here is to also support it in gethash
Flags: needinfo?(hchang)
Whiteboard: #sbv4-m5
Priority: -- → P2
Summary: Implement Minimum wait duration for V4 → Implement Minimum wait duration for V4 gethash
Blocks: 1276826
No longer blocks: safebrowsingv4
Blocks: 1329808
No longer blocks: 1276826
Assignee: nobody → 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.
Attached patch WIP (obsolete) — Splinter Review
Status: NEW → ASSIGNED
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
Attachment #8827836 - Attachment is obsolete: true
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.
Attachment #8834757 - Flags: review?(francois) → review-
Thanks for your review
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.
Attachment #8834757 - Flags: review?(francois) → review-
Comment on attachment 8834757 [details]
Bug 1329558 - Implement Minimum wait duration for V4 gethash

https://reviewboard.mozilla.org/r/110600/#review112300
Attachment #8834757 - Flags: review?(francois) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3cd262da4ec1
Implement Minimum wait duration for V4 gethash r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3cd262da4ec1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.