Not sure quite what's up here, but if I go through the signup flow, then verify email from a desktop computer, the handset gets logged out, and *not* set to verified login state. This is definitely just a nice to have, because I can easily log back in and see the verified state. Logging it without more detail as a placeholder for now

Note that, if I log back in after being logged out, the phone does show that the account is in verified state. Funky.

Some shell output: ------------------- A coding exception was thrown in a Promise resolution callback. Full message: TypeError: currentState.whenKeysReadyDeferred is null See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise Full stack: FxAccountsInternal.prototype.getKeys/<@resource://gre/modules/FxAccounts.jsm:499:7 Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:11 this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:586:7 ------------------

I simply checked for a null keyFetchToken in getKeys() before calling fetchAndUnwrapKeys(), and removed the corresponding test in the latter and its unit test. fetchAndUnwrapKeys() is only called in that one place.

Comment on attachment 8397342 [details] [diff] [review] 983445-check_for_token.patch Review of attachment 8397342 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I don't think it's necessary to remove the logout protection in fetchAndUnwrap keys, but I leave it up to you to remove it if you prefer. ::: services/fxaccounts/FxAccounts.jsm @@ +41,5 @@ > "whenVerified" > ]; > > +let log = Log.repository.getLogger("FirefoxAccounts"); > +log.warn(' abcdefg\n'); The logger is exported by FxAccountsCommon.js already. You can enable debugging by preffing 'identity.fxaccounts.loglevel' to 'DEBUG' @@ -510,5 @@ > - if (!keyFetchToken) { > - yield this.signOut(); > - return null; > - } > - I'm sort of on the fence about removing this. Having the |if (data.keyFetchToken)| guard in getKeys() makes perfect sense. And I know there are no other callers of this function. But removing the logout protection makes me feel that it's losing some future-proofing.

Great, thanks Jed. I replaced the null-token check and cleaned up the logging.

Cleaned up, test restored and refocused, missing reject() handler restored.

Comment on attachment 8407183 [details] [diff] [review] 983445-check_for_token.patch Review of attachment 8407183 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks, Sam! ::: services/fxaccounts/FxAccounts.jsm @@ +505,5 @@ > */ > getKeys: function() { > let currentState = this.currentAccountState; > + return currentState.getUserAccountData().then((userData) => { > + if (!userData) { Thanks for cleaning up these variable names. We had too many return values called 'data'. @@ +514,4 @@ > } > if (!currentState.whenKeysReadyDeferred) { > currentState.whenKeysReadyDeferred = Promise.defer(); > + if (userData.keyFetchToken) { Cool. This guards the auto-logout from fetchAndUnwrapKeys, but fetchAndUnwrapKeys will still log you out if you call it without a keyfetch token for some reason. ::: services/fxaccounts/tests/xpcshell/test_accounts.js @@ +321,5 @@ > }); > }); > > fxa.setSignedInUser(user).then((user) => { > + fxa.internal.fetchAndUnwrapKeys(); Cool.

Comment on attachment 8407183 [details] [diff] [review] 983445-check_for_token.patch ># HG changeset patch ># User Sam Penrose <spenrose@mozilla.com> ># Date 1397603116 25200 ># Tue Apr 15 16:05:16 2014 -0700 ># Node ID 60e8f32a386cefd1bd071d1f2e7c37b5c6352740 ># Parent 088f4efdaba17f47384e17d4824e504feed8749d >Bug 983445 - Verification causes logout on device r=jedp > >diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm >--- a/services/fxaccounts/FxAccounts.jsm >+++ b/services/fxaccounts/FxAccounts.jsm >@@ -500,48 +500,53 @@ FxAccountsInternal.prototype = { > * kA: An encryption key from the FxA server > * kB: An encryption key derived from the user's FxA password > * verified: email verification status > * } > * or null if no user is signed in > */ > getKeys: function() { > let currentState = this.currentAccountState; >- return currentState.getUserAccountData().then((data) => { >- if (!data) { >+ return currentState.getUserAccountData().then((userData) => { >+ if (!userData) { > throw new Error("Can't get keys; User is not signed in"); > } >- if (data.kA && data.kB) { >- return data; >+ if (userData.kA && userData.kB) { >+ return userData; > } > if (!currentState.whenKeysReadyDeferred) { > currentState.whenKeysReadyDeferred = Promise.defer(); >- this.fetchAndUnwrapKeys(data.keyFetchToken).then( >- data => { >- if (!data.kA || !data.kB) { >- currentState.whenKeysReadyDeferred.reject( >- new Error("user data missing kA or kB") >- ); >- return; >+ if (userData.keyFetchToken) { >+ this.fetchAndUnwrapKeys(userData.keyFetchToken).then( >+ (dataWithKeys) => { >+ if (!dataWithKeys.kA || !dataWithKeys.kB) { >+ currentState.whenKeysReadyDeferred.reject( >+ new Error("user data missing kA or kB") >+ ); >+ return; >+ } >+ currentState.whenKeysReadyDeferred.resolve(dataWithKeys); >+ }, >+ (err) => { >+ currentState.whenKeysReadyDeferred.reject(err); > } >- currentState.whenKeysReadyDeferred.resolve(data); >- }, >- err => currentState.whenKeysReadyDeferred.reject(err) >- ); >+ ); >+ } > } > return currentState.whenKeysReadyDeferred.promise; > }).then(result => currentState.resolve(result)); > }, > > fetchAndUnwrapKeys: function(keyFetchToken) { > log.debug("fetchAndUnwrapKeys: token: " + keyFetchToken); > let currentState = this.currentAccountState; > return Task.spawn(function* task() { > // Sign out if we don't have a key fetch token. > if (!keyFetchToken) { >+ log.warn("improper fetchAndUnwrapKeys() call: token missing"); > yield this.signOut(); > return null; > } > > let {kA, wrapKB} = yield this.fetchKeys(keyFetchToken); > > let data = yield currentState.getUserAccountData(); > >diff --git a/services/fxaccounts/tests/xpcshell/test_accounts.js b/services/fxaccounts/tests/xpcshell/test_accounts.js >--- a/services/fxaccounts/tests/xpcshell/test_accounts.js >+++ b/services/fxaccounts/tests/xpcshell/test_accounts.js >@@ -300,34 +300,34 @@ add_test(function test_getKeys() { > do_test_finished(); > run_next_test(); > }); > }); > }); > }); > }); > >-// getKeys with no keyFetchToken should trigger signOut >-add_test(function test_getKeys_no_token() { >+// fetchAndUnwrapKeys with no keyFetchToken should trigger signOut >+add_test(function test_fetchAndUnwrapKeys_no_token() { > do_test_pending(); > > let fxa = new MockFxAccounts(); > let user = getTestUser("lettuce.protheroe"); > delete user.keyFetchToken > > makeObserver(ONLOGOUT_NOTIFICATION, function() { >- log.debug("test_getKeys_no_token observed logout"); >+ log.debug("test_fetchAndUnwrapKeys_no_token observed logout"); > fxa.internal.getUserAccountData().then(user => { > do_test_finished(); > run_next_test(); > }); > }); > > fxa.setSignedInUser(user).then((user) => { >- fxa.internal.getKeys(); >+ fxa.internal.fetchAndUnwrapKeys(); > }); > }); > > // Alice (User A) signs up but never verifies her email. Then Bob (User B) > // signs in with a verified email. Ensure that no sign-in events are triggered > // on Alice's behalf. In the end, Bob should be the signed-in user. > add_test(function test_overlapping_signins() { > do_test_pending();

Backed out for xpcshell bustage. Please make sure this passes on Try before re-requesting checkin. https://hg.mozilla.org/integration/fx-team/rev/711ddfbb8bbf https://tbpl.mozilla.org/php/getParsedLog.php?id=37949327&tree=Fx-Team

Mark, the test that's failing is here: https://hg.mozilla.org/mozilla-central/file/b735e618c2a8/services/sync/tests/unit/test_browserid_identity.js#l555 "BrowserIDManager correctly handles getKeys succeeding but not returning keys." It's not obvious to me how my tweaks to the promises in FxAccounts.jsm/getKeys() have either broken the code or merely messed up the mocks in the test module. Would you mind taking a look and seeing if anything jumps out at you?

To be specific, the purpose of this patch is to not call FxAccounts.fetchAndUnwrapKeys on B2G when it will never succeed because we don't have a keyFetchToken. The test above triggers getKeys() returning early by deleting kA and kB from its mock, but the mock doesn't have a keyFetchToken. So it's unclear to me if: - the test should be deleted - the test should mock fetchAndUnwrapKeys() and or/ keyFetchToken differently - the test's concurrency handling needs tweaking If you can give me some guidance on which of these areas to focus on, I'd be much obliged.

OK, per Mark's helpful guidance in IRC, I have tweaked the sync test that was breaking and also fixed an unhappy path where a promise was not getting rejected.

Here's a try push for this: https://tbpl.mozilla.org/?tree=Try&rev=1e7db711a467

I've starred all the failures and oranges. I don't see this patch breaking anything.