This is spun of bug 1447479 where we need to associate a device with message indexes.

Oops made a mistake, I meant bug 1442133!

Comment on attachment 8961561 [details] Bug 1448165 p4 - Remove skipDeviceRegistration pref. https://reviewboard.mozilla.org/r/230416/#review235944 Looks good!

Comment on attachment 8961560 [details] Bug 1448165 p2 - Remove un-used ON_FXA_UPDATE_NOTIFICATION. https://reviewboard.mozilla.org/r/230414/#review236896

Comment on attachment 8961561 [details] Bug 1448165 p4 - Remove skipDeviceRegistration pref. https://reviewboard.mozilla.org/r/230416/#review236900 I don't understand why we need to do this - shouldn't everything end up back in place correctly next login? (and IIUC, these are the only 2 callers of resetConfigURLs, so if we really do want to do this, we should consider removing that too)

Comment on attachment 8961562 [details] Bug 1448165 p3 - Migrate deviceId and deviceRegistrationVersion under device. https://reviewboard.mozilla.org/r/230418/#review236902 This looks good in generaly, but I still need to look over how part4 and part1 interact - and I need to run - but I think I didn't see how we "migrate" the existing registration to the new format. Did I just miss it, or does that need to be added? ::: services/fxaccounts/FxAccounts.jsm:1641 (Diff revision 1) > // If you change what we send to the FxA servers during device registration, > // you'll have to bump the DEVICE_REGISTRATION_VERSION number to force older > // devices to re-register when Firefox updates > - _registerOrUpdateDevice(signedInUser) { > + async _registerOrUpdateDevice(signedInUser) { > - try { > - // Allow tests to skip device registration because: > + // Allow tests to skip device registration because: nit: Let's tweak this comment - remove (3), which probably means we can make the entire comment a single sentence.

> I don't understand why we need to do this - shouldn't everything end up back in place correctly next login? (and IIUC, these are the only 2 callers of resetConfigURLs, so if we really do want to do this, we should consider removing that too) I think I wanted to refactor FxAccounts.jsm further to call .signOut() in a few more places. I still think this is a valuable change though: signing-out shouldn't reset these config URLs in the first place imo (tcsc agrees). It also makes the signOut code a tiny bit less complex. > Did I just miss it, or does that need to be added? Check out |FxAccounts.getDeviceId| in p4, this is where the migration happens.

(In reply to Edouard Oger [:eoger] from comment #14) > > I don't understand why we need to do this - shouldn't everything end up back in place correctly next login? (and IIUC, these are the only 2 callers of resetConfigURLs, so if we really do want to do this, we should consider removing that too) > > I think I wanted to refactor FxAccounts.jsm further to call .signOut() in a > few more places. > I still think this is a valuable change though: signing-out shouldn't reset > these config URLs in the first place imo (tcsc agrees). It also makes the > signOut code a tiny bit less complex. See bug 1249520 comment 9 for the reason we drop this config at signout. The tl;dr is that if IIUC, your patch will mean it is effectively impossible for Firefox to pickup a different config from the server if that config changes - signing out and back in again isn't an ideal solution to that, but at least it's *something*.

Comment on attachment 8961559 [details] Bug 1448165 p1 - Simplify and factorize FxAccounts signout mechanism. https://reviewboard.mozilla.org/r/230412/#review237382 ::: services/fxaccounts/FxAccounts.jsm:770 (Diff revision 2) > + if (!tokenInfos) { > + return Promise.resolve(); > + } > // let's just destroy them all in parallel... > let promises = []; > for (let tokenInfo of Object.values(tokenInfos || {})) { you can probably remove the `|| {}` here?

Comment on attachment 8961559 [details] Bug 1448165 p1 - Simplify and factorize FxAccounts signout mechanism. https://reviewboard.mozilla.org/r/230412/#review237390 ::: services/fxaccounts/FxAccounts.jsm:820 (Diff revision 2) > - > - const options = { service: "sync" }; > - > - if (deviceId) { > - log.debug("destroying device, session and unsubscribing from FxA push"); > - return this.deleteDeviceRegistration(sessionToken, deviceId); > + log.error("Could not unsubscribe from push.", err); > + } > + if (sessionToken) { > + log.debug("Destroying session and device."); > + try { > + await this.fxAccountsClient.signOut(sessionToken, {service: "sync"}); Actually, looking at bug 1227527 comment 15, it's not clear this is deleting the device entirely (ie, it seems like it might just be signing out of the "sync" service) - please make sure you check with someone from FxA about this change (ie, it may be that we want to hit /account/device/destroy rather than /session/destroy to completely kill everything.

Comment on attachment 8961562 [details] Bug 1448165 p3 - Migrate deviceId and deviceRegistrationVersion under device. https://reviewboard.mozilla.org/r/230418/#review237388 These are a great improvement, thanks! ::: services/fxaccounts/FxAccounts.jsm:670 (Diff revision 2) > - .then(data => { > - if (data) { > - if (!data.deviceId || !data.deviceRegistrationVersion || > - data.deviceRegistrationVersion < this.DEVICE_REGISTRATION_VERSION) { > - // There is no device id or the device registration is outdated. > + if (!data) { > + // Without a signed-in user, there can be no device id. > + return null; > + } > + // Try migrating first. Remove this in Firefox 65+. > + if (data.deviceId) { oops - sorry I missed this, but I think we should add a test for this. ::: services/fxaccounts/FxAccounts.jsm:1644 (Diff revision 2) > // you'll have to bump the DEVICE_REGISTRATION_VERSION number to force older > // devices to re-register when Firefox updates > - _registerOrUpdateDevice(signedInUser) { > - try { > - // Allow tests to skip device registration because: > - // 1. It makes remote requests to the auth server. > + async _registerOrUpdateDevice(signedInUser) { > + // Allow tests to skip device registration because it makes remote requests > + // to the auth server and _getDeviceName does not work from xpcshell. > + if (Services.prefs.getBoolPref("identity.fxaccounts.skipDeviceRegistration", false)) { I'm not too bothered, but maybe we should rename this pref to something that indicates it is a test-only pref, as IIUC, setting it in Firefox itself will mean we don't signout correctly. OTOH though, it has no default value, so people aren't going to accidentally find it, so it's easy to argue it's not worthwhile. Your call.

Comment on attachment 8961561 [details] Bug 1448165 p4 - Remove skipDeviceRegistration pref. https://reviewboard.mozilla.org/r/230416/#review237392 IMO we need to consider how a user will get config updates before landing this part.

(In reply to Mark Hammond [:markh] from comment #15) > See bug 1249520 comment 9 for the reason we drop this config at signout. The > tl;dr is that if IIUC, your patch will mean it is effectively impossible for > Firefox to pickup a different config from the server if that config changes > - signing out and back in again isn't an ideal solution to that, but at > least it's *something*. Makes sense, thanks for finding this out! I'll drop the commit. (In reply to Mark Hammond [:markh] from comment #17) > Actually, looking at bug 1227527 comment 15, it's not clear this is deleting > the device entirely (ie, it seems like it might just be signing out of the > "sync" service) - please make sure you check with someone from FxA about > this change (ie, it may be that we want to hit /account/device/destroy > rather than /session/destroy to completely kill everything. I've confirmed with the FxA team that the behavior of /session/destroy has been changed in the past to remove the device associated with the session token.

Comment on attachment 8961561 [details] Bug 1448165 p4 - Remove skipDeviceRegistration pref. https://reviewboard.mozilla.org/r/230416/#review237762 Awesome!

Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b06f03dfb623 p1 - Simplify and factorize FxAccounts signout mechanism. r=markh https://hg.mozilla.org/integration/autoland/rev/ae3f2df2b2ee p2 - Remove un-used ON_FXA_UPDATE_NOTIFICATION. r=markh https://hg.mozilla.org/integration/autoland/rev/5489b1559fc9 p3 - Migrate deviceId and deviceRegistrationVersion under device. r=markh https://hg.mozilla.org/integration/autoland/rev/f7e80091988f p4 - Remove skipDeviceRegistration pref. r=markh,tcsc

https://hg.mozilla.org/mozilla-central/rev/b06f03dfb623 https://hg.mozilla.org/mozilla-central/rev/ae3f2df2b2ee https://hg.mozilla.org/mozilla-central/rev/5489b1559fc9 https://hg.mozilla.org/mozilla-central/rev/f7e80091988f