Closed Bug 668309 Opened 10 years ago Closed 10 years ago

Sync on non-idle not functioning correctly

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: emtwo, Assigned: philikon)

References

Details

(Whiteboard: [verified in services])

Attachments

(4 files, 1 obsolete file)

* 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.
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.
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
(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.
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.
Blocks: 664792
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)
* syncIfMPUnlocked had an extra underscore in a reference to it.
Attachment #543021 - Attachment is patch: true
Attachment #543021 - Flags: review+
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 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 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+
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?]
TPS passed.
(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!
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.
STRs for QA: See bug 664792 comment 34, particularly the first and last point.
Whiteboard: [fixed in services][qa?] → [fixed in services]
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]
Status: RESOLVED → VERIFIED
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.