1019408 - Provide facility to prevent starting during migration

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

As part of the sync migration, we will need to "reset" the legacy sync (ie, call service.startOver()) before re-configuring it for the FxA account.

We will want to ensure Sync isn't running when we take this step - so we need to wait for the sync to complete.

The fly in the ointment is the risk that sync doesn't complete for some users (eg, bug 1006360 and others) - so if sync has started once in the current session, this wait will be futile.  One option here would be that when we get to the migration step where we need to wait for a sync, we should ensure (via prefs?) that sync is effectively disabled.  Then the scenario for these users would be that the migration can complete the next time Firefox is started - disabling sync means that this new startup will not sync before we've had the opportunity to complete the migration.

(Yes, these users will still almost be certainly be left with a sync that still never completes, but at least it will be an FxA based sync)

Comment 1

[Richard Newman [:rnewman]]

You can test if a sync is running: look at the service lock, or listen to start notifications. (TPS actually does something similar: watch for start and finish via observers.)

You should also trivially be able to determine when it started -- record a timestamp at start.

If start is way before finish, you can determine hang.

You might consider extending the "App. quitting" check in the spinning code to allow for interruption.

Comment 2

[Mike Hoye [:mhoye]]

Flagging as diamond - Markh, would you be willing to mentor this bug?

Comment 3

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

(In reply to Mike Hoye [:mhoye] from comment #2)
> Flagging as diamond - Markh, would you be willing to mentor this bug?

I'm away for 8 weeks starting next Friday, and I expect we will want this bug worked on over that time, so that probably means I can't in practice.

Comment 4

[Mike Hoye [:mhoye]]

Thanks; Richard, how about you? Mentoring! Fame(*) and fortune(*)!

(*) - Mentoring is not guaranteed to result in fame or fortune.

Comment 5

[Richard Newman [:rnewman]]

I can make a little time, albeit probably not the attention I would like to lavish on a contributor!

Comment 6

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

Attachment: 0003-Bug-1019408-add-facility-to-block-and-unblock-new-sy.patch

As mentioned above, I think it's important to be able to prevent sync starting just in-case the user is one of the few for which sync never stops.  Such users would need to complete a migration within the first 10 seconds of the browser starting to ever complete.

This patch adds 2 new methods - blockSync() and unblockSync().  blockSync() takes a param to indicate how long to block for, although this will only be used by tests.  The default is to block for 2 days.  This is a safety valve incase we end up calling .blockSync() and never arrange to call unblockSync() - eg, if the user migrates to the point where we call .blockSync(), then fecal matter hits the fan such that the migration never gets to the point of unblocking.  In this case sync will magically restart (presumably with the legacy provider) after a couple of days.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8519714 [details] [diff] [review]
0003-Bug-1019408-add-facility-to-block-and-unblock-new-sy.patch

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

So... this looks fine, but is there a reason you didn't just specify a backoff, and use the existing backoff machinery?

Something like

  Status.backoffInterval = interval;
  Status.minimumNextSync = Date.now() + interval;

::: services/sync/modules/policies.js
@@ +509,5 @@
> +   * that it will automatically unblock.  This ensures that if things go
> +   * really pear-shaped and we never end up calling unblockSync() we haven't
> +   * completely broken the world.
> +   */
> +  blockSync: function(until = null) {

Just

  function (until) {

should do the trick. Omit the argument, and it'll default to undefined.

@@ +511,5 @@
> +   * completely broken the world.
> +   */
> +  blockSync: function(until = null) {
> +    if (!until) {
> +      until = Date.now() + DEFAULT_BLOCK_PERIOD;

This is slightly worrying thanks to clock drift, but we don't have too many options.

@@ +514,5 @@
> +    if (!until) {
> +      until = Date.now() + DEFAULT_BLOCK_PERIOD;
> +    }
> +    // until is specified in ms, but Prefs can't hold that much
> +    Svc.Prefs.set("scheduler.blocked-until", Math.floor(until / 1000));

If you want to be really safe, you should flush prefs here.

  Services.prefs.savePrefFile(null);

Otherwise a crash or force quit during migration might not have set the pref. That might be desirable; your call.

Comment 8

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

(In reply to Richard Newman [:rnewman] from comment #7)
> So... this looks fine, but is there a reason you didn't just specify a
> backoff, and use the existing backoff machinery?

Mainly as these aren't persisted over browser restarts IIUC.

Comment 9

[Richard Newman [:rnewman]]

Did you consider making the existing backoff mechanism persistent?

(Not a dig; just want to make sure we're not adding a parallel mechanism when one isn't needed.)

Comment 10

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

(In reply to Richard Newman [:rnewman] from comment #9)
> Did you consider making the existing backoff mechanism persistent?

The problem as I see it is:

* A sync is in progress
* We request sync to be blocked.
* During that sync we get a backoff header (which will typically be a number of minutes), so re-adjust the backoff to the lesser value.
* User is one for which sync never stops - so eventually they exit their browser
* User restarts, and the interval in the backoff header has expires, so sync restarts.

I guess we could mitigate this by changing the backoff logic such that Status.backoffInterval = max(Status.backoffInterval, newInterval), but that kind of change scares me a little.  Further, the fact that the current backoff interval isn't persisted has never been seen as a problem in the past - if we make this change, it will probably live forever, even though this migration requirement is relatively short-term - and for the vast majority of users, we expect the block() and unblock() to be within the same session, so nothing will actually be persisted (ie, the pref will be added and removed before shutdown)

So on balance, I see reusing the backoff mechanism as riskier - what do you think?

Comment 11

[Richard Newman [:rnewman]]

I'm happy with that analysis.

Comment 12

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

Comment on attachment 8519714 [details] [diff] [review]
0003-Bug-1019408-add-facility-to-block-and-unblock-new-sy.patch

(In reply to Richard Newman [:rnewman] from comment #11)
> I'm happy with that analysis.

Great :)

Comment 13

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

Attachment: 0002-Bug-1019408-add-facility-to-block-and-unblock-new-sy.patch

This now removes the ability for the migration module to function without this patch.

Comment 14

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

https://hg.mozilla.org/integration/fx-team/rev/a9d870df6db2

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a9d870df6db2