Closed
Bug 668309
Opened 13 years ago
Closed 13 years ago
Sync on non-idle not functioning correctly
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
VERIFIED
FIXED
mozilla7
People
(Reporter: emtwo, Assigned: philikon)
References
Details
(Whiteboard: [verified in services])
Attachments
(4 files, 1 obsolete file)
7.18 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
27.42 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
3.66 KB,
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
Details | Diff | Splinter Review |
* Cursor movements & clicking the back/forward buttons do not seem to be registering as non-idle events (these can be done for a very long time but a sync will never happen) * If less than 5 minutes has passed since the last sync then tab switches, opening new tabs, and clicking on links, do nothing (as expected) but any time after 5 minutes these actions will trigger a sync. If these actions are done in less than 5 minutes since the last sync, and then no actions are performed afterwards, the next expected sync will still not happen.
Comment 1•13 years ago
|
||
STR's 1) force a sync (note time of sync) 2) do some basic browser stuff, run a search, click a results link, go back/forward in that tab 3) wait at least five minutes expected results: 5 minute non-idle sync should trigger tested results: no sync is triggered. As Marina mentioned, if after five minutes you do something like switch tabs, a sync is triggered.
Comment 2•13 years ago
|
||
from https://bugzilla.mozilla.org/show_bug.cgi?id=664792#c34 * On a machine that's active (non-idle), Sync should sync: - every 5 minutes if there was nothing downloaded in the last sync - every 1 minute if the last sync downloaded new items The one minute timer when sync downloaded new items also does not work.
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to comment #1) > 1) force a sync (note time of sync) > 2) do some basic browser stuff, run a search, click a results link, go > back/forward in that tab > 3) wait at least five minutes Please define "wait". Do nothing with the machine? Continue to use the machine but not the browser? > As Marina mentioned, if after five minutes you do something like switch > tabs, a sync is triggered. Well, yeah, if you do something after 5 minutes of *inactivity*, the "back" observer will fire.
Comment 4•13 years ago
|
||
do nothing, you can also be doing minor stuff in the browser, like back/fwd. same failure either way. Step two is just there to make sure your active at some point during the 5 minute timer.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #542945 -
Flags: review?(rnewman)
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #542947 -
Flags: review?(rnewman)
Assignee | ||
Comment 7•13 years ago
|
||
This is a wall paper fix that should hopefully take us back to the old behaviour (before bug 664792), by making both the active and the idle interval 5 minutes. This means we're effectively backing out a part of Instant Sync, but overall it should ensure that we're not regressing in terms of sync intervals.
Attachment #542956 -
Flags: review?(rnewman)
Reporter | ||
Comment 8•13 years ago
|
||
* syncIfMPUnlocked had an extra underscore in a reference to it.
Reporter | ||
Updated•13 years ago
|
Attachment #543021 -
Attachment is patch: true
Assignee | ||
Updated•13 years ago
|
Attachment #543021 -
Flags: review+
Assignee | ||
Comment 9•13 years ago
|
||
Comment on attachment 542956 [details] [diff] [review] Part 3 (v1): Set active = idle interval Obsoleting Part 3 as we figured out the underlying problem and provided the bugfix patch in its place. This is good to go, pending reviews for Part 1 + 2.
Attachment #542956 -
Attachment is obsolete: true
Attachment #542956 -
Flags: review?(rnewman)
Comment 10•13 years ago
|
||
Comment on attachment 542945 [details] [diff] [review] Part 1 (v1): Some gratuitious logging Review of attachment 542945 [details] [diff] [review]: ----------------------------------------------------------------- I'm not convinced that all of these log statements are what you intend. I will amend this patch and land it; if I mis-interpreted, you can fix and amend later. They're only logging statements, so it's not a big deal. ::: services/sync/modules/policies.js @@ +58,5 @@ > */ > syncTimer: null, > > setDefaults: function setDefaults() { > + this._log.trace("Setting values to defaults."); Setting *which* values? @@ +118,5 @@ > let sync_interval; > this._syncErrors = 0; > > if (Status.sync == NO_SYNC_NODE_FOUND) { > + this._log.trace("Scheduling a sync at NODE_SYNC_NODE_FOUND"); Should be "at interval NO_SYNC_NODE_FOUND", no? @@ +161,5 @@ > if (numItems) > this.hasIncomingItems = true; > break; > case "idle": > + this._log.trace("We're no longer idle."); This doesn't seem like it's correct... @@ +228,5 @@ > this._log.debug("Client count: " + this.numClients + " -> " + numClients); > this.numClients = numClients; > > if (numClients <= 1) { > + this._log.trace("Adjusting syncThreshold to SINGLE_USER_THRESHOLD"); Nit: final '.' to match other log items like this. @@ +233,3 @@ > this.syncThreshold = SINGLE_USER_THRESHOLD; > + } else { > + this._log.trace("Adjusting syncThreshold to MULTI_DEVICE_THRESHOLD"); Nit: final '.' to match other log items like this.
Attachment #542945 -
Flags: review?(rnewman) → review+
Comment 11•13 years ago
|
||
Comment on attachment 542947 [details] [diff] [review] Part 2 (v1): Make intervals pref-able Review of attachment 542947 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/modules/policies.js @@ +63,5 @@ > + > + this.singleDeviceInterval = Svc.Prefs.get("scheduler.singleDeviceInterval") * 1000; > + this.idleInterval = Svc.Prefs.get("scheduler.idleInterval") * 1000; > + this.activeInterval = Svc.Prefs.get("scheduler.activeInterval") * 1000; > + this.immediateInterval = Svc.Prefs.get("scheduler.immediateInterval") * 1000; Nit: I'd prefer these lined up. Broader issue for later consideration: altering these prefs only takes effect after a restart. Do we care? @@ +187,5 @@ > }, > > adjustSyncInterval: function adjustSyncInterval() { > if (this.numClients <= 1) { > + this._log.trace("Adjusting syncInterval to singleDeviceInterval"); Nit: closing period to match other logs. ::: services/sync/services-sync.js @@ +12,5 @@ > +pref("services.sync.scheduler.singleDeviceInterval", 86400); // 1 day > +pref("services.sync.scheduler.idleInterval", 3600); // 1 hour > +pref("services.sync.scheduler.activeInterval", 300); // 5 minutes > +pref("services.sync.scheduler.immediateInterval", 60); // 1 minute > +pref("services.sync.scheduler.idleTime", 300); // 5 minute Nit: minutes.
Attachment #542947 -
Flags: review?(rnewman) → review+
Comment 12•13 years ago
|
||
Pushed to services-central with the amendments above. The third part didn't apply cleanly for some reason. 107 tests pass; I'll keep an eye open for TPS. Philipp, please check my amendments from Comment 10. http://hg.mozilla.org/services/services-central/rev/92b28e3fd1d0 http://hg.mozilla.org/services/services-central/rev/1dfd8a7df152 http://hg.mozilla.org/services/services-central/rev/57d18f1dcf10 Flagging as [qa?]. Philipp or Marina, please list any additional steps you want QA to follow, or switch that to [qa-].
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Whiteboard: [fixed in services][qa?]
Comment 13•13 years ago
|
||
TPS passed.
Assignee | ||
Comment 14•13 years ago
|
||
(In reply to comment #10) > I'm not convinced that all of these log statements are what you intend. I > will amend this patch and land it; if I mis-interpreted, you can fix and > amend later. They're only logging statements, so it's not a big deal. Err, yeah, you're right. I wrote that patch in a hurry, so yeah, thanks for fixing :) (In reply to comment #11) > > + this.singleDeviceInterval = Svc.Prefs.get("scheduler.singleDeviceInterval") * 1000; > > + this.idleInterval = Svc.Prefs.get("scheduler.idleInterval") * 1000; > > + this.activeInterval = Svc.Prefs.get("scheduler.activeInterval") * 1000; > > + this.immediateInterval = Svc.Prefs.get("scheduler.immediateInterval") * 1000; > > Nit: I'd prefer these lined up. > > Broader issue for later consideration: altering these prefs only takes > effect after a restart. Do we care? Nope. I specifically opted for this one-time solution rather than getters that would go to prefs each time (which seemed like overkill). (In reply to comment #12) > Pushed to services-central with the amendments above. The third part didn't > apply cleanly for some reason. That's weird. > http://hg.mozilla.org/services/services-central/rev/92b28e3fd1d0 > http://hg.mozilla.org/services/services-central/rev/1dfd8a7df152 > http://hg.mozilla.org/services/services-central/rev/57d18f1dcf10 Thanks, Richard!
Assignee | ||
Comment 15•13 years ago
|
||
There's a WinXP debug failure in test_syncscheduler due to a somewhat brittle test in test_scheduleNextSync(). The offending line is the last one: do_check_eq(SyncScheduler.syncTimer.delay, expectedInterval); which just can't be guaranteed. What can be guaranteed is do_check_true(SyncScheduler.syncTimer.delay <= expectedInterval); The rest of the patch is clean up, making the test a bit easier to read (getting rid of the useless initial_nextSync and syncInterval variables). Kicked off a try build: http://tbpl.mozilla.org/?tree=Try&rev=1d42be9f1456. Will land this once it goes green.
Assignee | ||
Comment 16•13 years ago
|
||
STRs for QA: See bug 664792 comment 34, particularly the first and last point.
Whiteboard: [fixed in services][qa?] → [fixed in services]
Comment 17•13 years ago
|
||
All of the intervals idle and non-idle appear to be working correctly with the builds from this morning 20110630
Whiteboard: [fixed in services] → [verified in services]
Assignee | ||
Comment 18•13 years ago
|
||
Landed the follow-up: http://hg.mozilla.org/services/services-central/rev/5f78a0ad9a4b
Assignee | ||
Comment 19•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/92b28e3fd1d0 http://hg.mozilla.org/mozilla-central/rev/1dfd8a7df152 http://hg.mozilla.org/mozilla-central/rev/57d18f1dcf10 http://hg.mozilla.org/mozilla-central/rev/5f78a0ad9a4b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•