Last Comment Bug 692249 - Persist nextSync and numClients, use nextSync for sync after startup
: Persist nextSync and numClients, use nextSync for sync after startup
Status: RESOLVED FIXED
[verified in services][qa!][qa!:9]
: verified-beta
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on:
Blocks: 664792 692006
  Show dependency treegraph
 
Reported: 2011-10-05 13:32 PDT by Philipp von Weitershausen [:philikon]
Modified: 2012-04-11 11:07 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
v1 (29.04 KB, patch)
2011-10-13 12:01 PDT, Philipp von Weitershausen [:philikon]
rnewman: review+
Details | Diff | Review
v1.1 (28.94 KB, patch)
2011-10-13 18:01 PDT, Philipp von Weitershausen [:philikon]
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2011-10-05 13:32:33 PDT
With Instant Sync (specifically bug 664792), we unconditionally sync at start up. The idea is that starting the browser is very much like the back-from-idle notification: you'd like to get access to the data accrued on other devices.

In theory this behaviour can generate more syncs than the regular once-per-hour (or in Firefox 7, due to the backout of bug 678588, once-per-day) sync for single device users if they're restarting the browser a lot. More crucially, however, the sync after startup has no way of obeying a previously set backoff.
Comment 1 Philipp von Weitershausen [:philikon] 2011-10-13 12:01:08 PDT
Created attachment 566899 [details] [diff] [review]
v1

This brings back persisting
* nextSync
* numClients
* syncInterval
* syncThreshold

scheduleNextSync has been modified to ensure that a nextSync in the future without an active timer present will create a timer.

autoConnect now goes through scheduleNextSync and uses the interval set by nextSync. That way multi device users will still sync at start up or very soon after that and single device users will stick to the scheduled syncs.

Also added a shit-ton of trace logging to SyncScheduler and made some tests more robust.
Comment 2 Richard Newman [:rnewman] 2011-10-13 16:44:24 PDT
Comment on attachment 566899 [details] [diff] [review]
v1

Review of attachment 566899 [details] [diff] [review]:
-----------------------------------------------------------------

Just nits.

::: services/sync/modules/policies.js
@@ +365,5 @@
>      }
>  
>      // Ensure the interval is set to no less than the backoff.
>      if (Status.backoffInterval && interval < Status.backoffInterval) {
> +      this._log.trace("Requested interval " + Math.ceil(interval / 1000) +

Please just use interval + " msec" in all of these places instead. No point in doing two math ops only to be misleading about a value in Trace logs that users won't typically see...

js> Math.ceil(1 / 1000)
1

@@ +414,2 @@
>  
> +    this._log.debug("Starting client initiated backoff, next sync in " +

"Starting client-initiated backoff. Next sync in " +

::: services/sync/tests/unit/test_syncscheduler.js
@@ +104,5 @@
>    const THRESHOLD = 3142;
>    const SCORE = 2718;
>    const TIMESTAMP1 = 1275493471649;
>  
> +  _("The 'nextSync' attribute stores a millisecond timestamp to the nearest second.");

"rounded down to the nearest second."

@@ +107,5 @@
>  
> +  _("The 'nextSync' attribute stores a millisecond timestamp to the nearest second.");
> +  do_check_eq(SyncScheduler.nextSync, 0);
> +  SyncScheduler.nextSync = TIMESTAMP1;
> +  do_check_eq(SyncScheduler.nextSync, Math.floor(TIMESTAMP1/1000)*1000);

Spaces between operators, please.
Comment 3 Philipp von Weitershausen [:philikon] 2011-10-13 18:01:05 PDT
Created attachment 566990 [details] [diff] [review]
v1.1

Addressed nits
Comment 4 Philipp von Weitershausen [:philikon] 2011-10-13 18:02:43 PDT
Comment on attachment 566990 [details] [diff] [review]
v1.1

Requesting approval for Aurora and Beta. This change brings back some pre Firefox 7 behaviour that will help reduce load on the servers and make the client honour server-requested backoff intervals even at startup.
Comment 5 Philipp von Weitershausen [:philikon] 2011-10-15 15:49:34 PDT
https://hg.mozilla.org/services/services-central/rev/cc0be790f8c6

STRs for QA:

Instead of syncing immediately-ish after startup, Sync will now resume a previously scheduled sync timer (like it did in Firefox 6 and earlier). This should be especially apparent for single device users as they will strictly stick to a 24 hour interval. In general it will also prevent lots and lots of syncs from people who restart their browser a lot.

For smoketesting, please be sure to
* test both single and multi-device scenarios,
* test starting the browser before previously scheduled sync will occur, *as well as* after it would've occurred (in the latter case it should sync immediately after startup since it's "late" for the previously scheduled one)
* test profiles with Master Passwords. Sync should prompt once for the MP. If that's denied, it should no longer prompt, but retry to sync every 15 minutes. If the MP becomes unlocked by some other means later, Sync will automatically resume then.
Comment 6 Mihaela Velimiroviciu (:mihaelav) 2011-10-18 07:36:03 PDT
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111017 Firefox/10.0a1
Mozilla/5.0 (X11; Linux i686; rv:10.0a1) Gecko/20111017 Firefox/10.0a1

Scenario #1:
Prerequisites:
1. Sync is set up on device(s)
2. History items are available on device(s)

STR:
1. Start Browser and make sure last sync was successfull
2. Close browser
3. Change pc/device time to less than 10 minutes(for multiple device accounts)/24 hours(for single device accounts) since last sync
4. Start the browser and wait for the 10 minutes/24 hours synce last sync to pass
Result: After time passes a new sync is performed


Scenario #2:
Prerequisites:
1. Sync is set up on device(s)
2. History items are available on device(s)

STR:
1. Start Browser and make sure last sync was successfull
2. Close browser
3. Change pc/device time to more than 10 minutes(for multiple device accounts)/24 hours(for single device accounts) since last sync
4. Start the browser
Result: A new sync is performed


OSX&Android:
In scenario #1, sync is performed after browser restart even if 10mins/24 hours didn't pass since last sync.
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20111017 Firefox/10.0a1
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111017 Firefox/10.0a1 Fennec/10.0a1
Comment 7 Mihaela Velimiroviciu (:mihaelav) 2011-10-18 09:26:14 PDT
Correction on comment #6:
For multiple device accounts on OS X, sync is performed after every 90 seconds, even if no items were retrieved at last sync. Is this expected?
For single device accounts, sync is performed correctly after 24 hours.
Comment 8 Philipp von Weitershausen [:philikon] 2011-10-18 09:29:46 PDT
(In reply to Mihaela Velimiroviciu [QA] from comment #7)
> Correction on comment #6:
> For multiple device accounts on OS X, sync is performed after every 90
> seconds, even if no items were retrieved at last sync. Is this expected?

It is not. How are you verifying that no items were retrieved?
Comment 9 Tracy Walker [:tracy] 2011-10-18 13:40:19 PDT
This looks good to me with s-c builds.

I can't confirm what Mihaela was seeing.  if I do nothing on the other client.  Mac changes to activeInterval as expected.

Note: It is easy to pollute results by simply looking at a different log on a client under test.  That changes the title of the tab, which will then be uploaded on next sync, which will be applied to other client on next sync and that client will then set itself to immediateInterval.  Weee!  

Patience is king.
Comment 10 Philipp von Weitershausen [:philikon] 2011-10-18 20:14:32 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3aaf5bf21a4
Comment 11 Philipp von Weitershausen [:philikon] 2011-10-18 20:38:04 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/6120192ea12e
Comment 12 Richard Newman [:rnewman] 2011-10-21 11:51:22 PDT
https://hg.mozilla.org/mozilla-central/rev/cc0be790f8c6
Comment 13 Mihaela Velimiroviciu (:mihaelav) 2011-12-12 06:23:50 PST
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0

Verified on latest Beta (9beta5) and scheduled sync uses the 10 minutes/24h timer (if no data was retrieved at the previous successful sync)

Note You need to log in before you can comment on or make changes to this bug.