Closed Bug 1212649 Opened 7 years ago Closed 7 years ago

No upper time limit on shavar polling interval


(Toolkit :: Safe Browsing, defect)

44 Branch
Not set



Tracking Status
firefox44 --- affected
firefox45 --- verified


(Reporter: mwobensmith, Assigned: francois)




(3 files, 1 obsolete file)

Attached file n-1800000001.txt
When the initial request goes to download a list, the value specified in the response for "n:" indicates the interval (in seconds) at which to check back for a list update. This limit is supposed to be capped at some short-term value, such as 86400 (one day) or less. 

Currently it appears that there is no upper limit. 

Two error scenarios:

- Specifying 1800000001 seems to indicate an interval time of 1800000001 seconds.
- Specifying -1 seems to indicate an interval of 4294967295 seconds.

I've attached logs of stdout to show what I've seen when doing the above.
Given the recent changes to list update frequency (see bug 1175562), we should fix this bug in two ways:

1. cap the interval to 24 hours in the protocol parser
2. reset the pref to a sane value when we read a value larger than 24 hours in the list update code
Blocks: 1149867
Attached patch bug1212649.patch (obsolete) — Splinter Review
This patch does two things:

- puts a cap on the "n" values coming from the server
- ignores nextUpdateTime values that are larger than 1 day
Attachment #8695000 - Flags: review?(gpascutto)
Comment on attachment 8695000 [details] [diff] [review]

Review of attachment 8695000 [details] [diff] [review]:

Seems OK to me. The SafeBrowsing v2 spec doesn't really say there's an upper limit to the interval that the server specifies, but in case of errors, backoff etc the maximum wait interval is 8 hours, so limiting the next-update-time to 1 day looks like it should be safe in practice.

::: toolkit/components/url-classifier/content/listmanager.js
@@ +438,4 @@
>    // our delay time for requesting updates. We always use a non-repeating
>    // timer since the delay is set differently at every callback.
> +  const minDelaySec = 5 * 60;
> +  const maxDelaySec = 24 * 60 * 60;

Why not multiply by 1000 immediately? It's more consistent with all the other time constants and it simplifies the code below.
Attachment #8695000 - Flags: review?(gpascutto) → review+
Attached patch bug1212649.patchSplinter Review
Addressing gcp's comments and carrying his r+.
Attachment #8695000 - Attachment is obsolete: true
Attachment #8695608 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Verified fixed on Nightly 45, 2015-12-13. 

In addition, the logging messages indicate that the delay is either too short or too long, which is helpful for debugging.
You need to log in before you can comment on or make changes to this bug.