1019985 - Module to drive FxA migration process

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

This bug is to create an FxA module which will drive the migration process.  It will hook up to the UX components so the current state can be determined, and take the actions necessary for the final migration.

The dependent bugs are for the more complex components of this module.

Comment 1

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 2

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

Attachment: 0001-Bug-1019985-WIP-sync-migrator.patch

A WIP, proof-of-concept of what such a module might look like.  Has comments with the bug numbers of dependent bugs.

Comment 3

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

Attachment: 0006-Bug-1019985-WIP-sync-migrator.patch

This is a far more functional version.  Sadly though it still depends in particular on bug 1019408 and bug 1017433 - but even without those parts it *should* notice the EOL notification and indicate a migration is necessary - it is just that it will fail trying to push a full migration through (as it will not be able to find a "sentinel" and will be unable to block sync from restarting)

Drew, requesting a "light" feedback pass for now (but obviously make it as thorough as you like!)

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8524140 [details] [diff] [review]
0006-Bug-1019985-WIP-sync-migrator.patch

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

::: services/sync/modules/FxaMigrator.jsm
@@ +49,5 @@
> +  }
> +  // we want this.state to be STATE_COMPLETE on initialization if appropriate.
> +  // otherwise the caller will need to use the promise
> +  // XXX - do we need to ensure we aren't blocked??????????
> +  this.state = WeaveService.fxAccountsEnabled ? this.STATE_COMPLETE : null;

There should be some initial non-null state.

@@ +67,5 @@
> +  // "complete" also reflects "no migration necessary" - ie, we will be in the
> +  // "complete" state when Sync is configured for FxA.
> +  STATE_COMPLETE: "complete",
> +
> +  state: null,

Surely both `state` and promiseCurrentState shouldn't be "public".  `state` seems private, so... _state?

@@ +107,5 @@
> +      this.log.info("Migration state: '${state}' => '${newState}'",
> +                    {state: this.state, newState: newState});
> +      if (newState != this.state) {
> +        this.state = newState;
> +        Services.obs.notifyObservers(null, OBSERVER_STATE_CHANGE_TOPIC, null);

It's very strange to me that getting the current state (via this promiseCurrentState) may have the publicly visible side effect of updating the state!

@@ +115,5 @@
> +
> +    // If we have no sync user, or are already using an FxA account we must
> +    // be done.
> +    if (WeaveService.fxAccountsEnabled) {
> +      Weave.Service.scheduler.unblockSync();

Similarly, it's strange that getting the state may have the external side effect of blocking or unblocking sync.  I wouldn't expect it to have any external side effects at all.

@@ +170,5 @@
> +  // method will trigger any UI necessary to move the state forward.  If not
> +  // specified the state progression will stop at the state where the UI
> +  // is required.
> +  step: Task.async(function* (win = null) {
> +    // we keep stepping until we no longer change state.

If that's the case "step" is a bad name.  "Step" means you take one step, not many.

But I'm not sure this method makes sense.  Who's calling it?  For instance, in my bug, in the EOL case, the sees a menu panel item that tells them they need an FxA account, so they click it.  It would be weirdly indirect and non-obvious for the click listener to call FxAMigrator.step() instead of simply opening about:accounts.

It seems to me that a call site that would call step() should just do thing that step() would have done.  Add helper methods here if they'd be useful to consumers, sure.  But as far as I can tell there's no single "crank" that would need to call a method like step() repeatedly.

Comment 5

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

(In reply to Drew Willcoxon :adw from comment #4)

> There should be some initial non-null state.

I think that's fine, so long as promiseCurrentState() can't return it.  (And yeah, .state should be ._state - or better still, I think it can actually be removed entirely)

> It's very strange to me that getting the current state (via this
> promiseCurrentState) may have the publicly visible side effect of updating
> the state!

promiseCurrentState will *determine* the current state.  But yeah, if we get rid of .state/._state, I think this objection is moot.

> 
> @@ +115,5 @@
> > +
> > +    // If we have no sync user, or are already using an FxA account we must
> > +    // be done.
> > +    if (WeaveService.fxAccountsEnabled) {
> > +      Weave.Service.scheduler.unblockSync();
> 
> Similarly, it's strange that getting the state may have the external side
> effect of blocking or unblocking sync.  I wouldn't expect it to have any
> external side effects at all.

That's really a safety thing - if something has gone wrong and we find we are complete but sync is blocked, we will be totally screwed - the fact we are reporting complete means the consumer is never again going to ask us to do anything.

This is somewhat mitigated by the fact sync will unblock itself after 2 days, but I'd rather have an observable side-effect than a blocked sync after migration.

> @@ +170,5 @@
> > +  // method will trigger any UI necessary to move the state forward.  If not
> > +  // specified the state progression will stop at the state where the UI
> > +  // is required.
> > +  step: Task.async(function* (win = null) {
> > +    // we keep stepping until we no longer change state.
> 
> If that's the case "step" is a bad name.  "Step" means you take one step,
> not many.

I'm open to better names - promiseAdvanceState()?

> But I'm not sure this method makes sense.  Who's calling it?  For instance,
> in my bug, in the EOL case, the sees a menu panel item that tells them they
> need an FxA account, so they click it.  It would be weirdly indirect and
> non-obvious for the click listener to call FxAMigrator.step() instead of
> simply opening about:accounts.

I don't agree - this means that *every* UI element needs to know what action to take in each of the states, which is duplication and error prone.  The button can instead just call .step() safe in the knowledge the right things will happen if possible.

> It seems to me that a call site that would call step() should just do thing
> that step() would have done.  Add helper methods here if they'd be useful to
> consumers, sure.  But as far as I can tell there's no single "crank" that
> would need to call a method like step() repeatedly.

It's that single button.  Eg, if we startup and the state is WAITING_VERIFIED_USER, that button will still be clickable and guide the user to take some action.

It sounds to me like you are asking for, eg, the "sync prefs" pane to have a single button, but with a large switch/case statement enumerating all the states and taking action - and for this large block to be duplicated every place we have such an element.  I think it's better for the button to call .step() and have the state transition logic in one place.

Comment 6

[Drew Willcoxon :adw]

(In reply to Mark Hammond [:markh] from comment #5)
> > There should be some initial non-null state.
> 
> I think that's fine, so long as promiseCurrentState() can't return it.

I don't get that, because promiseCurrentState is public... returning a non-null initial state to consumers is exactly what I was thinking of.

> promiseCurrentState will *determine* the current state.  But yeah, if we get
> rid of .state/._state, I think this objection is moot.

That still sounds weird.  The way you're describing promiseCurrentState, it sounds like a combination of "return me the current state" and "advance to some other state".  It's like a combination getter and step().

Am I thinking about it wrong?  There needs to be some way for consumers to simply get the current state, no?  I thought that way was promiseCurrentState.  And it would be weird for them to be able to change the current state by getting it?

> That's really a safety thing - if something has gone wrong and we find we
> are complete but sync is blocked

What does "sync is blocked" mean?

> I don't agree - this means that *every* UI element needs to know what action
> to take in each of the states, which is duplication and error prone.  The
> button can instead just call .step() safe in the knowledge the right things
> will happen if possible.

But it seems like every UI element needs to know what to do in a certain set of states anyway, because they have to draw/update themselves appropriately.  Like, my EOL menu panel item needs to have a certain label in the EOL state(s).  Surely we won't be centralizing the logic for determining that label in this migrator JSM.  I'd imagine it working by the menu item getting the current state from the JSM and then setting its label appropriately.  Then, based on that state, it would know what to do when the user clicks it.

I understand what you're saying about duplicating the logic that determines what has to happen to transition out of state X.  That's a good point.  I'd be very comfortable with FxAMigrator.transitionFromState(state), instead of step().  It's just weird that a consumer would get the current state, update itself accordingly, and then call a method to change the state that is not a function of that state.

Also, please keep in mind that I don't know a lot about Sync or FxA or migration right now.  What I'm saying might not make sense to someone who knows how it all works, but I hope my newb perspective has some value.

Comment 7

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

Attachment: 0006-Bug-1019985-WIP-sync-migrator.patch

Here's a new version that (hopefully) reflects what Drew and I discussed.  It's much simpler :)

* The only exposed states are states where the user must take action.  This means there are only 2 such states - "waiting for fxa user" and "waiting for them to be verified".

* This state can be null when no user action is necessary - this means that either no migration is necessary, or that the migration module is waiting for something out of the user's control (eg, sync to complete, a network to be available so we can send the sentinel, etc)

In an effort to avoid this module from ever being loaded in the cases when no migration is necessary, Weave.js imports this as it is loading and finds a legacy sync user is configured.  What consumers of this module should do is:

* Add an observer for "fxa-migration:state-changed".
* Send a notification "fxa-migration:state-request" - if a migration is necessary, it will respond with "fxa-migration:state-changed" informing of the state.  If the module's not (yet) loaded, nothing happens.
* When "fxa-migration:state-changed" is received, it is "safe" to import the module and start using it.

See also the WIP patch for bug 1098661, which does exactly this.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8525089 [details] [diff] [review]
0006-Bug-1019985-WIP-sync-migrator.patch

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

Thanks for explaining things yesterday, Mark.  This new consumer interface is great.  Having consumers broadcast a notification to talk to the module to avoid unnecessarily importing it is clever.

::: services/sync/modules/FxaMigrator.jsm
@@ +115,5 @@
> +  // about verification)
> +  // This is called by our observer notifications - so if there is already
> +  // a promise in-flight, it's possible we will miss something important - so
> +  // we wait for the in-flight one to complete then fire another.
> +  _promiseNextUserState(forceObserver = false) {

It's hard for me to reason about this method and think of how it might go wrong.  I think it could be simplified if we just picture a queue of promises, and always have this._nextUserStatePromise be non-null.  So in the Migrator ctor:

  function Migrator() {
    this._nextUserStatePromise = Promise.resolve();
  }

And then _promiseNextUserState would simply do:

  return this._nextUserStatePromise = this._nextUserStatePromise.then(
    this.__promiseNextUserState,
    this.__promiseNextUserState
  );

Then rename these methods to something like:

   _promiseNextUserState -> _queueCurrentUserStateFetch
  __promiseNextUserState -> _promiseCurrentUserState

"Fetch" isn't the right word; more like "determination", but that's a mouthful.  "Current" seems better than "next" because the job is to find the state we're currently at, transitioning to it if necessary.

@@ +175,5 @@
> +    try {
> +      fxauser = yield fxAccounts.getSignedInUser();
> +    } catch (ex) {
> +      // There might be a bit of a race here if the user is set while we
> +      // are checking.  This *does* happen in tests, but could for-real -

Hmm, I don't understand this part.  If the user signs in while getSignedInUser is in progress, ideally getSignedInUser would see that and not finish until the signin completes.  Otherwise you can't rely on getSignedInUser at all.  On the other hand, you can't solve the halting problem.

Anyhow, that's tangential to this patch I think. :-)

@@ +232,5 @@
> +  }),
> +
> +  // A helper for the UI to try and move to the next state.
> +  takeUserAction: Task.async(function* (win) {
> +    // XXX - is it ok to use _state here?  Can't see why not!

Me either, seems OK.

::: services/sync/tests/unit/test_fxa_migration.js
@@ +86,5 @@
> +function mockFxaCertificateFetch(fxa) {
> +
> +}
> +
> +add_task(function *() {

Might be good to break up this big task into smaller ones, at least so that in the log we can see, for example, "Entering test: noSyncUser", "Entering test: noEOL", etc.

Also, whoa, will I have to do all this stuff to set up Sync and FxA when testing the UI, too?  Can any of it be factored out into reusable chunks?

Comment 9

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

Attachment: 0006-Bug-1019985-Create-module-to-drive-sync-migration-pr.patch

(In reply to Drew Willcoxon :adw from comment #8)

> @@ +115,5 @@
> > +  // about verification)
> > +  // This is called by our observer notifications - so if there is already
> > +  // a promise in-flight, it's possible we will miss something important - so
> > +  // we wait for the in-flight one to complete then fire another.
> > +  _promiseNextUserState(forceObserver = false) {
> 
> It's hard for me to reason about this method and think of how it might go
> wrong.  I think it could be simplified if we just picture a queue of
> promises, and always have this._nextUserStatePromise be non-null.  So in the
> Migrator ctor:

It's not actually a queue (ie, if we get 3 notifications in a row, we don't need to execute _promiseNextUserState() 3 times).  The intent is just "if a notification comes in while _promiseNextUserState() is yet to complete, we need to ensure it starts again to get the real current state."  Even if we get 10 notifications while a single invocation is running we only need to schedule 1 more.

Eg, consider that the promise-based code is executing and it finds the FxA user is unverified - so it is taking the "user not verified" path - but *during* that we get the "user if verified" notification.  The promise will resolve with the "user needs verification" state and nothing is going to cause it to advance to reflect that the user actually is verified.

>   function Migrator() {
>     this._nextUserStatePromise = Promise.resolve();
>   }
> 
> And then _promiseNextUserState would simply do:
> 
>   return this._nextUserStatePromise = this._nextUserStatePromise.then(
>     this.__promiseNextUserState,
>     this.__promiseNextUserState
>   );
> 
> Then rename these methods to something like:
> 
>    _promiseNextUserState -> _queueCurrentUserStateFetch
>   __promiseNextUserState -> _promiseCurrentUserState

That is more intractable to my eyes and I think will cause a "queue" (as you imply) - but as above, it's not a queue - it's not "run once per notification" but "if we are running now, ensure run once again when complete".

Does that make things clearer?

> 
> "Fetch" isn't the right word; more like "determination", but that's a
> mouthful.  "Current" seems better than "next" because the job is to find the
> state we're currently at, transitioning to it if necessary.

Yeah, good idea - I've replaced "next" with "current" in that context.

> Hmm, I don't understand this part.  If the user signs in while
> getSignedInUser is in progress, ideally getSignedInUser would see that and
> not finish until the signin completes.  Otherwise you can't rely on
> getSignedInUser at all.  On the other hand, you can't solve the halting
> problem.

Yeah, that really was a test specific problem due to how we simulated a verified user (so the comment was wrong) but with the current version it doesn't hit that anyway, so I've removed it.

> Might be good to break up this big task into smaller ones, at least so that
> in the log we can see, for example, "Entering test: noSyncUser", "Entering
> test: noEOL", etc.

I see your point, but TBH I'm not a big fan of tests that use multiple tasks which can't be run stand-alone (ie, where one task depends on the exact order of the previously defined tasks) - some dump() calls might help though.

I'll have another look at those tests though and see what I can do.

> Also, whoa, will I have to do all this stuff to set up Sync and FxA when
> testing the UI, too?  Can any of it be factored out into reusable chunks?

As mentioned in the other bug, I don't think we should have the browser-chrome tests configure and sync.  That said though, I'm pretty sure I can remove alot of that boilerplate code from the test.  I'll look at that later.

This new version drops the takeUserAction() function and replaces it with 3 new functions we've identified we need.  I don't think the resendVerificationMail() function is working correctly yet :(  I've updated bug 1098661 to use these new functions, so the latest patch there is probably of interest.

Not specifically requesting feedback here, but it's still welcome :)

Comment 10

[Drew Willcoxon :adw]

(In reply to Mark Hammond [:markh] from comment #9)
> It's not actually a queue (ie, if we get 3 notifications in a row, we don't
> need to execute _promiseNextUserState() 3 times).  The intent is just "if a
> notification comes in while _promiseNextUserState() is yet to complete, we
> need to ensure it starts again to get the real current state."  Even if we
> get 10 notifications while a single invocation is running we only need to
> schedule 1 more.

Gotcha, but you're effectively creating a queue anyway by doing this:

return this._currentUserStatePromise.then(startNew, startNew);

Am I wrong?  If you get three notifications in a row, you'll create this._currentUserStatePromise on the first one, and then queue up two startNew's on the next two by chaining them to this._currentUserStatePromise.  When the first's promise is resolved, the second notification's startNew is called, and it'll see this._currentUserStatePromise is null because the first's set it to null.  So it'll call __promiseCurrentUserState (double underscore) again and create a new this._currentUserStatePromise.  And then the third notification's startNew isn't even gated on the second's promise; it's gated on the original this._currentUserStatePromise.  So it's free to start before the second promise is even resolved, making it possible to hit your throw("impossible!?").

That's what I mean about this method being hard to reason about and imagine the ways it could mess up.

To do what you want to do (which I'm not opposed to, but I think the safe-queueing plan I outlined is simple conceptually and implementation-wise), it seems like all you need is a _receivedAnotherNotification bool.  Set it to true if _currentUserStatePromise.  Then when _currentUserStatePromise resolves, if _receivedAnotherNotification, start again.

> This new version drops the takeUserAction() function and replaces it with 3
> new functions we've identified we need.

I see, so I'd call a couple of these from my EOL menu panel patch, which answers a question or two I had in bug 1026342 I think.

Comment 11

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

Attachment: 0007-Bug-1019985-Create-module-to-drive-sync-migration-pr.patch

(In reply to Drew Willcoxon :adw from comment #10)

> Gotcha, but you're effectively creating a queue anyway by doing this:

*sigh* - yes, you are correct - sorry about that and thanks for not taking my word for it :)

I ended up going with your original idea.  I also tweaked some of the logging initialization and cleaned up the test a little.

Comment 12

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

Attachment: 0013-sync-migrator-module-part-2-add-resent-verification-.patch

As Drew and I discussed, we need to show a confirmation when the "resend verification mail" buttons are clicked.  Currently a simple dialog is implemented in the sync preferences panes, but that's not callable from where we need it for migration.

This patch moves the function to nsBrowserGlue.js, which isn't ideal.  Note that it also adds a "wrappedJSObject" attribute to the browser glue so we don't need to go via xpconnect and have |jsval| in the IDL.

I did consider creating a new module - say, FxaUI.jsm, but that seems like overkill - this is the only function I can see that would be in that.  Maybe some functions for opening about:accounts could also go there, but that seems relatively low value.

I moved the strings into browser.properties - it might make sense to rename SyncMigration.properties from bug 1026342 and put the strings there.

Drew, what are your thoughts on this?

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8528773 [details] [diff] [review]
0013-sync-migrator-module-part-2-add-resent-verification-.patch

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

Yikes, exposing wrappedJSObject is always a red flag, but exposing it on something as old and central as nsBrowserGlue worries me.  The combination of co-opting nsBrowserGlue to show an FxA UI and then subsequently needing to expose wrappedJSObject on it makes me think that that's not the right way to do it.

Is it common for code in services/sync to assume it's running on desktop Firefox?  I would presume not.  So services/sync/modules/FxaMigrator.jsm should probably not call into Cc["@mozilla.org/browser/browserglue;1"] since that's not on mobile, right?  Or at least not assume it exists?

Hmmm, or I guess this is one reason to use XPCOM components?  When code in one part of the tree needs to invoke something in another part that may not even exist?  In that light, using an XPCOM component like nsBrowserGlue would be the right thing to do.  You just shouldn't assume the component exists.  I still don't like using nsBrowserGlue to show FxA UI.  Ideally there'd be an FxA UI component.  It could even implement nsIPromptService or nsIPrompt so you wouldn't have to expose wrappedJSObject or write new IDL.  It would implement only the method(s) you need, like alert.  But maybe that's going overboard.

Regardless of where this prompt code lives, I think it does make sense to consolidate front-end FxA code in one place -- opening about:accounts, showing this prompt, and anything else in common that the pref pane and browser-fxaccounts.js and anybody else needs to do.  We were just talking yesterday about how the pref pane is doing the same kinds of things we're doing during migration.

> I moved the strings into browser.properties - it might make sense to rename
> SyncMigration.properties from bug 1026342 and put the strings there.

See, this is why I originally wanted to name that file accounts.properties, because it might end up needing FxA strings that are independent of migration, like your strings are here. :-)

Comment 14

[Richard Newman [:rnewman]]

(In reply to Drew Willcoxon :adw from comment #13)

Concur with your review, and an answer:

> Is it common for code in services/sync to assume it's running on desktop
> Firefox?  I would presume not.

It is only used on desktop (and, historically, Metro). We pay some attention to supporting other desktop browsers (e.g., SeaMonkey, theoretically Thunderbird), but never mobile.

Comment 15

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

Shuffling dependencies around so we can get this landed before those other bugs.

Comment 16

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

Attachment: 0001-Bug-1019985-Create-module-to-drive-sync-migration-pr.patch

This addresses most review comments and gracefully handles the lack of a sentinel and block/unblock functionality

Comment 17

[Drew Willcoxon :adw]

Comment on attachment 8532306 [details] [diff] [review]
0001-Bug-1019985-Create-module-to-drive-sync-migration-pr.patch

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

::: services/sync/modules/FxaMigrator.jsm
@@ +76,5 @@
> +Migrator.prototype = {
> +  log: Log.repository.getLogger("Sync.SyncMigration"),
> +
> +  // What user action is necessary to push the migration forward?
> +  // null: there is nothing to do.  Note that a null state implies either

It would be clearer to say something like "A null state means there is nothing to do" than "null: there is nothing to do."

@@ +80,5 @@
> +  // null: there is nothing to do.  Note that a null state implies either
> +  // (a) no migration is necessary or (b) that the migrator module is waiting
> +  // for something outside of the user's control - eg, sync to complete, the
> +  // migration sentinel to be uploaded, etc.  In most cases the wait will be
> +  // short, but egde cases (eg, no network, sync bugs that prevent it stopping

Nit: egde -> edge

@@ +107,5 @@
> +      default:
> +        // some other observer that may affect our state has fired, so update.
> +        this._promiseCurrentUserState().then(
> +          () => this.log.debug("update state from observer " + topic + " complete")
> +        ).then(null, err => {

I think you can do .catch(err => {}), a little more succinct than .then(null, err => {}).

@@ +123,5 @@
> +  // This is called by our observer notifications - so if there is already
> +  // a promise in-flight, it's possible we will miss something important - so
> +  // we wait for the in-flight one to complete then fire another (ie, this
> +  // is effectively a queue of promises)
> +  _promiseCurrentUserState(forceObserver = false) {

I still think this method and the double underscore one should be renamed.  At least the double underscore one.  The double underscores are kind of a red flag that the name is bad, and it makes it easy to mistake one for the other.  I'd still recommend something like:

 _promiseCurrentUserState -> _queueCurrentUserState
__promiseCurrentUserState -> _promiseCurrentUserState

@@ +137,5 @@
> +  },
> +
> +  __promiseCurrentUserState: Task.async(function* (forceObserver) {
> +    this.log.trace("starting _promiseCurrentUserState");
> +    let _update = newState =>  {

Nit: _update doesn't need an underscore in its name.

Nit: Please remove the extra space between the => and {.

@@ +196,5 @@
> +
> +    // Have we already written the sentinel?
> +    if (!(yield this._haveSynchedMigrationSentinel())) {
> +      this.log.info("writing the migration sentinel")
> +      yield this._setSyncMigrationSentinel();

Hmm, it seems inefficient, in a pretty abstract sense, to have to yield once to check have-synced and then again to sync.  I'd expect to have to yield just once, so something like: yield this._setSyncMigrationPanelIfNecessary().

@@ +286,5 @@
> +      return sentinel.email;
> +    }
> +    // No previous migrations, so check the existing account name.
> +    let account = Weave.Service.identity.account;
> +    if (account && account.indexOf("@") != -1) {

Nit: account.contains("@")

::: services/sync/tests/unit/head_http_server.js
@@ +514,5 @@
>     *
>     * Allows the test to inspect the request. Hooks should be careful not to
>     * modify or change state of the request or they may impact future processing.
> +   * The response is also passed so the callback can set headers etc - but care
> +   * must also be taken with this.

Nit: Why is it that care must be taken?  "But care must also be taken not to..."

::: services/sync/tests/unit/test_fxa_migration.js
@@ +46,5 @@
> +function configureLegacySync() {
> +  let engine = new RotaryEngine(Service);
> +  engine.enabled = true;
> +  Svc.Prefs.set("registerEngines", engine.name);
> +  Svc.Prefs.set("log.logger.engine.rotary", "Trace");

Hmm, should this be added to initTestLogging?  I don't know a lot about Sync logging or tests so maybe that doesn't make sense.

@@ +83,5 @@
> +function configureFxa() {
> +  Services.prefs.setCharPref("identity.fxaccounts.auth.uri", "http://localhost");
> +}
> +
> +add_task(function *() {

Please name this function even though it's the only one.  Otherwise the log shows something like "Entering test: ".

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 8532306 [details] [diff] [review]
0001-Bug-1019985-Create-module-to-drive-sync-migration-pr.patch

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

::: services/sync/modules/FxaMigrator.jsm
@@ +203,5 @@
> +    // Must be ready to perform the actual migration.
> +    this.log.info("Performing final sync migration steps");
> +    // Do the actual migration.
> +    let startOverComplete = new Promise((resolve, reject) => {
> +      Services.obs.addObserver(observe = () => {

JavaScript Warning: "ReferenceError: assignment to undeclared variable observe" {file: "resource://services-sync/FxaMigrator.jsm" line: 219}

Comment 19

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

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

(In reply to Drew Willcoxon :adw from comment #18)
> JavaScript Warning: "ReferenceError: assignment to undeclared variable
> observe" {file: "resource://services-sync/FxaMigrator.jsm" line: 219}

Oops - noticed that too late :(  Followup:

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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad283fac781c
https://hg.mozilla.org/mozilla-central/rev/95925d77a314