Closed Bug 692249 Opened 13 years ago Closed 13 years ago

Persist nextSync and numClients, use nextSync for sync after startup

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
firefox8 --- fixed
firefox9 --- fixed

People

(Reporter: philikon, Assigned: philikon)

References

Details

(Keywords: verified-beta, Whiteboard: [verified in services][qa!][qa!:9])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #566899 - Flags: review?(rnewman)
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.
Attachment #566899 - Flags: review?(rnewman) → review+
Attached patch v1.1Splinter Review
Addressed nits
Attachment #566899 - Attachment is obsolete: true
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.
Attachment #566990 - Flags: approval-mozilla-beta?
Attachment #566990 - Flags: approval-mozilla-aurora?
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.
Whiteboard: [fixed in services]
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
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.
(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?
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.
Whiteboard: [fixed in services] → [verified in services]
Attachment #566990 - Flags: approval-mozilla-beta?
Attachment #566990 - Flags: approval-mozilla-beta+
Attachment #566990 - Flags: approval-mozilla-aurora?
Attachment #566990 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/cc0be790f8c6
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Target Milestone: mozilla8 → mozilla10
Whiteboard: [verified in services] → [verified in services][qa+]
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)
Keywords: verified-beta
Whiteboard: [verified in services][qa+] → [verified in services][qa+][qa!:9]
Whiteboard: [verified in services][qa+][qa!:9] → [verified in services][qa!][qa!:9]
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.

Attachment

General

Created:
Updated:
Size: