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

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 1

[Mark Hammond [:markh] [:mhammond]]

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".

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1288584 - don't allow prefs to specify a sync interval that's less than 60 seconds.

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

Comment 3

[Mark Hammond [:markh] [:mhammond]]

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 4

[Richard Newman [:rnewman]]

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?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(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" :)

Comment 6

[Richard Newman [:rnewman]]

(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!

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Sold! I'll push a 60s version.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

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/

Comment 9

[Mark Hammond [:markh] [:mhammond]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/329789f1d87f