1119578 - Manually test that Sync engine selection does the right thing after FxA migration

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Drew Willcoxon :adw]

> markh: https://www.dropbox.com/s/qm3qkvmjnl3h5f3/Registration%20Form.pdf is the closest I can find - it shows the checkbox but is wrong about "the next step is datatype selection"
> markh: datatype selection is after migration once sync is setup for FxA
> markh: and browserid_identity already has support for that (as the same basic thing happens on normal fxa sync setup)
> markh: the distinction for migration is "checkbox should default to checked if user has some engines already unselected, and default to unchecked if all engines are already enabled"
> markh: so the test would be that line above, plus:
> markh: after migration ensure that dialog does indeed popup, has the correct "defaults", and changing the defaults does the right thing
> markh: Some of the complexity is that about 7 prefs need to be changed to hit the fxa staging server
> markh: and at least one of those prefs is reset after migration, meaning it needs to be changed multiple times
> adw: are you imagining an automated test or manual testing?
> markh: manual - I don't think we have the capability of reasonable sync automated testing

Comment 1

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

Adding some relevant pointers - bug 1098694 in particular has information about the staging server and when it should hit prod

Comment 2

[Drew Willcoxon :adw]

This doesn't quite work right.  I tried various cases several times, and when "Choose what to sync" was checked, I usually did not get the customize.xul modal dialog after migration completed.  "Usually" -- the first time I tried with "Choose what to sync" checked, I did get the dialog as expected.  Every other time, I didn't.  So there may be some race condition.

First, what does work right:  The "Choose what to sync" checkbox on the signup page is checked correctly depending on whether I had previously unselected any engines, the data of the "login" command received by aboutaccounts.js correctly contains a customizeSync bool, onLogin in aboutaccounts.js correctly sets the showCustomizationDialog pref, and needsCustomization() in browserid_identity.js correctly checks the pref.

The problem is that once I click the verification email and Firefox sees that I've been verified, a whole bunch of services.sync.* prefs get set, or cleared maybe.  At the same time, browserid_identity.js is going through the initializeWithCurrentIdentity flow.  In every case after I started instrumenting the flow, services.sync.ui.showCustomizationDialog was cleared before browserid_identity.js examined it.  I'm guessing that the one time where I got the customize.xul dialog as expected, it just so happened that showCustomizationDialog was cleared after browserid_identity.js examined it.  Or maybe the whole bunch of prefs wasn't set/cleared that time, maybe because I used an old profile and/or and old Sync account.  That was before I started instrumenting things and used new profiles and Sync accounts.

I'm guessing the whole bunch of prefs being set/cleared is due to the prefs engine kicking in?  I'll investigate more and file follow-up bugs as necessary.

Comment 3

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #2)
> I'm guessing the whole bunch of prefs being set/cleared is due to the prefs
> engine kicking in?

Oh, no.  The bunch of prefs being set/cleared are things like services.sync.clients.lastRecordUpload, things that look like appropriate metadata.  Not just a wholesale set/clearing of the sync.* branch.  And I actually unchecked the prefs engine anyway.

showCustomizationDialog is cleared by Weave.Service.startOver: http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/service.js#909

Which is called by the migrator in _promiseCurrentUserState: http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/FxaMigrator.jsm#238

So that's the race?

Comment 4

[Drew Willcoxon :adw]

startOver both clears all the Sync prefs and triggers initializeWithCurrentIdentity, and the latter happens *serially* after the former -- no race condition at all.  At least that's what I'm seeing.  initializeWithCurrentIdentity has multiple callers, but I'm seeing it on startOver's stack when startOver accesses Status._authManager: http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/service.js#931  (The _authManager getter then calls BrowserIDManager.initialize, which calls initializeWithCurrentIdentity.)

Notice that that happens after the Svc.Prefs.resetBranch("") several lines above.

So is this even a migration bug?

Comment 5

[Drew Willcoxon :adw]

> markh: obvious and easy answer would be to change that pref name
> adw: so that resetPrefs doesn't reset it?
> markh: yeah
> ...
> markh: something like services.sync-setup... or something?
> ...
> markh: renaming the pref will expose and edge case though - if they login
>        before verifying, then exit their browser, then the browser updates to
>        the new version on restart, they will not get the dialog
> markh: but I think we can live with that
> ...
> markh: I don't think there's an ordering problem really - it's just that the
>        existing code was only considering the flow for first time use when sync
>        can't have previously been configured
> ...
> markh: It's simply that we never considered that .startOver() would be called
>        *after* an FxA user was logged in
> markh: before this migration, that would "reset" sync and there's be no user
>        logged in
> markh: so it would end up as the normal first-time-use flow
> adw: when would that pref ever be true then?
> markh: on that first time flow - ie, when a user signs up for sync - there's no
>        .startOver() in that case as Sync was never previously configured
> markh: ie, on a new profile and you sign in to sync, there's no .startOver()
>        call made
> markh: previously, .startOver() was called when sync was configured for legacy
>        account and you did "unlink"
> markh: then you ended up with a fully reset sync and no FxA user logged in
> markh: then you did the "Create FxA user" flow and things work fine
> markh: this is changing things such that we are resetting sync *after* an FxA
>        user is logged in

Comment 6

[Drew Willcoxon :adw]

Closing this bug as I tested Sync engine selection after migration, with the result that it does not work when "Choose what to sync" is checked, and it needs to be fixed.  I filed bug 1124956 to fix it.