No upper time limit on shavar polling interval

RESOLVED FIXED in Firefox 45

Status

()

defect
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mwobensmith, Assigned: francois)

Tracking

44 Branch
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted 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.
Assignee

Comment 2

4 years ago
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
Assignee

Updated

4 years ago
Blocks: 1149867
Assignee

Comment 3

4 years ago
Posted 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)
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED
Comment on attachment 8695000 [details] [diff] [review]
bug1212649.patch

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+
Assignee

Comment 5

4 years ago
Addressing gcp's comments and carrying his r+.
Attachment #8695000 - Attachment is obsolete: true
Attachment #8695608 - Flags: review+

Comment 7

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d76f652669d
Status: ASSIGNED → RESOLVED
Closed: 4 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.