Don't allow prefs to cause Sync to run too often

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

If you adjust scheduler.activeInterval to 0, Sync will run continuously (you may need to set, eg, idleInterval etc too to make it *truly* run continuously, but you get the idea). This seems bad :) Our telemetry ping is showing that over ~7 days, 2 Nightly users have submitted > 1M pings each.

IMO we should enforce a minimum value for each of the prefs that control how often we Sync. It should be possible for a user to choose a longer delay - and possibly even a slightly smaller delay, but not a delay so small it impacts the browser, the telemetry back-end and the sync back end.

Thoughts on a reasonable strategy here?
As a straw man, I'm going to mozreview a simple patch that will enforce a minimum of 75% of the default value for all intervals. Obviously the percentage is somewhat arbitrary but it seems like a reasonable compromise between "ppl want to Sync more often" and "don't set us on fire".
If we really wanted to be careful in the face of custom builds, we could also hard-code a min value of 60s - the smallest interval we have as a default is 90, which the patch would limit to 67.5. It seems quite unlikely a custom build would be insane enough to default those prefs to very low values though...
Comment on attachment 8773611 [details]
Bug 1288584 - don't allow prefs to specify a sync interval that's less than 60 seconds.

I'd keep it even simpler and just use 60s.

Perhaps this should be in info/configuration?
Attachment #8773611 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #4)
> Comment on attachment 8773611 [details]
> Bug 1288584 - don't allow prefs to specify a sync interval that's less than
> 75% of the default value.
> 
> I'd keep it even simpler and just use 60s.

The reason I went for 75% is that it would clamp, say, activeInterval from 10mins to 7.5mins - which is still significantly less often than 60s. My thinking is that allowing prefs to cause sync to run once per minute is still allowing significant traffic we would be better without.

What do people think?

Also, since this bug was opened we identified another bug that is likely to account for most of the "syncing all the time" behaviour we are seeing - while it still seems bad to me that preferences allow us to DDOS ourselves, I have no evidence this is a problem in practice. Do we actually want to bother with this at all? My gut says "yes", but my head says "you are just inventing this problem" :)
Assignee: nobody → markh
(In reply to Mark Hammond [:markh] from comment #5)

> The reason I went for 75% is that it would clamp, say, activeInterval from
> 10mins to 7.5mins - which is still significantly less often than 60s. My
> thinking is that allowing prefs to cause sync to run once per minute is
> still allowing significant traffic we would be better without.

My reasoning:

* 60s is already way better than 0.5s; multi-device users already sync one or other device every 150s or less!
* Most users will not have futzed with these prefs, so this won't apply to them anyway.
* If you really do want to sync every two minutes for some reason, this will prevent you from doing so.
* I'm worried about involving prefs reads and mathematics in safety-related stuff like this.

That is, if our goal is a last-ditch sanity rate limit, it's better if it's bulletproof-simple and the lowest feasible value.


> I have no evidence this is a problem in practice. Do we actually want to
> bother with this at all? My gut says "yes", but my head says "you are just
> inventing this problem" :)

Hit the mole while it is visible! Yes!
Status: NEW → ASSIGNED
Sold! I'll push a 60s version.
Comment on attachment 8773611 [details]
Bug 1288584 - don't allow prefs to specify a sync interval that's less than 60 seconds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66344/diff/1-2/
Attachment #8773611 - Attachment description: Bug 1288584 - don't allow prefs to specify a sync interval that's less than 75% of the default value. → Bug 1288584 - don't allow prefs to specify a sync interval that's less than 60 seconds.
Attachment #8773611 - Flags: review+ → review?(rnewman)
Comment on attachment 8773611 [details]
Bug 1288584 - don't allow prefs to specify a sync interval that's less than 60 seconds.

Carrying r+ forward
Attachment #8773611 - Flags: review?(rnewman) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/fx-team/rev/329789f1d87f
don't allow prefs to specify a sync interval that's less than 60 seconds. r=rnewman
Duplicate of this bug: 1031379
https://hg.mozilla.org/mozilla-central/rev/329789f1d87f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.