909967 - Firefox Account Signed-in User module

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Zachary Carter [:zaach]]

We'll need a service for setting and retrieving the logged-in user and their credentials. Credentials will include the user's email address, a sessionToken, two encryption keys (kA and kB), and a Persona assertion for authenticating with a Sync 2.0 token server.

Comment 1

[Richard Newman [:rnewman]]

Just tweaking the name to clarify that this is a client-side component, rather than a server. (We get a lot of misfiled bugs due to "Backend", so I'm pre-empting incorrect fixes!)

Comment 2

[Zachary Carter [:zaach]]

Attachment: fxaservice20130828.patch

This creates a new Firefox Accounts service with three methods: setSignedInUser, getSignedInUser, and signOut.

Comment 3

[Zachary Carter [:zaach]]

Attachment: fxaservice20130829.patch

Comment 4

[Richard Newman [:rnewman]]

Gavin asked me to leave some thoughts here, so here goes.

* Try to reuse code. E.g., saving/loading JSON to/from disk:

    https://hg.mozilla.org/mozilla-central/file/default/services/common/utils.js#l348

* No static singletons, please. Aim for test code being able to make multiple account managers, with different backing files. This will make testing everything, but particularly stuff like file version upgrades, much less painful, and is generally good practice.

* On the subject of version upgrades: please version those file contents.

* Perf: you're loading JSON every time someone asks for the signed in user. That makes sense if forces outside your control can write to that file, but if you own it, make this service a write-through cache. Better to keep one object in memory than touch the filesystem.

* There are some conceptual questions in my mind. Will this ever need to support more than one stored account (user switching)? If so, it might be worth thinking about making storage future-proof and adjusting the API to match.

* You're building a service here. You should consider making it a first-class service, a la the FHR components:

    https://hg.mozilla.org/mozilla-central/file/b818c7222968/services/datareporting/DataReportingService.js

This buys you singleton management, interface versioning, and other things, all for a little bit of boilerplate.

Comment 5

[Chris Karlof [:ckarlof]]

> This is a bigger patch on which I have opinions (use services, no singletons, laziness, design for testability, questions around multiple account support and future proofing, versioning of on-disk payloads). This would benefit from multiple reviews, so I suggest that Gavin or Tim run through it first, and have me as a second pass next week.

(I wrote this after you recent review, but I think it's still worth posting)

Richard, I agree with your comments regarding this. 

I recognize the current implementation is done via global singleton, and we'll fix that. We may need some help on the XPCOM services stuff, but we know enough to at least get started on that.

How about this:

1) Have an instantiable FxAccountsManager object (not tied to that name) that manages the currently logged in user. It's constructor will take refs to service dependencies.
2) Implement a XPCOM service that will manage a single instance of the above object.
3) Change aboutaccounts.js to fetch this instantiation via the XPCOM service interface, and we build an instantiable wrapper that takes a FxAccountsManager ref in its constructor.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Chris Karlof [:ckarlof] from comment #5)

> How about this:

Yup, exactly.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Richard Newman [:rnewman] from comment #4)
> * Try to reuse code. E.g., saving/loading JSON to/from disk:
> 
>    
> https://hg.mozilla.org/mozilla-central/file/default/services/common/utils.
> js#l348

Yeah, we should definitely use OS.File for this rather than the "old" APIs you're using.

> * No static singletons, please. Aim for test code being able to make
> multiple account managers, with different backing files. This will make
> testing everything, but particularly stuff like file version upgrades, much
> less painful, and is generally good practice.

I don't quite understand this crusade, but the suggestion sounds reasonable :)

> * You're building a service here. You should consider making it a
> first-class service, a la the FHR components:
> 
> https://hg.mozilla.org/mozilla-central/file/b818c7222968/services/
> datareporting/DataReportingService.js
> 
> This buys you singleton management, interface versioning, and other things,
> all for a little bit of boilerplate.

XPCOM seems like overkill to get that. I don't see why we need it?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 797403 [details] [diff] [review]
fxaservice20130829.patch

>diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm

>+// We concatenate the JSMs together to eliminate compartment overhead.
>+// This is a giant hack until compartment overhead is no longer an
>+// issue.
>+#define MERGED_COMPARTMENT

This shouldn't be an issue on Desktop, and I thought it wasn't really (as much of) an issue for b2g now either, after work in bug 759585 and bug 807478 and other related work. Is this a remnant from older code written when things were different, or is it really still needed for some reason?

Comment 9

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

Comment on attachment 797403 [details] [diff] [review]
fxaservice20130829.patch

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

I've no show-stopper issues with this patch, but do think it can be made simpler and reorganized to make the actual work being clearer (ie, this looks like alot of code to do not much actual work.  The fact it's not doing much is (obviously) fine as things kick off, but the LOC seems quite high for what it does.)

So f+, but IMO it's not ready for review yet.  However, I note the "interface" being exposed is simple enough (ie, f+ for that, notwithstanding my "FxAccounts vs accounts" issue) that it shouldn't block moving forward with the storage model here.

The structure of the top-level modules seems confusing to me at first glance:

* FxAccounts is basically an object with "accounts" and "storage" - FxAccounts.accounts seems somewhat strange - especially as one of the first comments in accounts.jsm talks about "Firefox Accounts".  It's not clear to me what the difference between "FxAccounts" and "accounts" are (I think the answer is "nothing".)

* The accounts module imports the storage module - it's not clear why they are split at the higher level if the lower levels depend on it.  Eg, we have "FxAccounts" with "accounts" and "storage", but "accounts" *also* references "storage".  Why not just have "storage" directly on "accounts"?  (It would make more sense to me if the intent was "pluggable" storage, so accounts was passed a storage instance - but I doubt that's necessary yet either)

* Some of the MERGED_COMPARTMENT code looks a little like premature optimization to me, but it's hard to know how things will pan out at this early stage, so I'm not actually complaining about that yet ;)

::: services/fxaccounts/storage.jsm
@@ +46,5 @@
> +   * @param that
> +   *        Object to use for logging and "this" for callback.
> +   * @return Promise
> +   */
> +  load: function jsonLoad(filePath, that) {

I don't see a good reason for |that| - I think module level logging should be fine and the one use of |that| in a callback should just be the responsibility of whoever supplies the callback to ensure it is bound as expected (it seems unlikely someone supplying a callback here would *expect* the Storage object to be |this|)

Also, OS.File should probably be used - it's already promise based, does off-main-thread IO and would make it alot smaller (it's a minor concern for me that a module that only needs to load and save arbitrary JSON objects to disk would be 133 lines.)

@@ +54,5 @@
> +    if (that._log) {
> +      that._log.trace("Loading json from disk: " + filePath);
> +    }
> +
> +    let file = FileUtils.getFile("ProfD", path.split("/"), true);

path is declared above as a string which is then split - wouldn't just defining the array be clearer and better (ie, "let path = ["fxaccounts", filePath + ".json"];) - and ditto in save()

@@ +75,5 @@
> +        json = JSON.parse(string);
> +      } catch (ex) {
> +        if (that._log) {
> +          that._log.debug("Failed to load json: " +
> +                          CommonUtils.exceptionStr(ex));

wouldn't it be better to reject() in this case too (eg, you already do in file-not-found)

::: services/fxaccounts/tests/xpcshell/test_load_modules.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

I don't think this test adds any value - anything that would cause this test to fail would cause all other tests here to also fail IIUC, and they would fail in reasonably obvious ways.

Comment 10

[Zachary Carter [:zaach]]

Attachment: 909967_fxa_module.patch

Thanks for all of the feedback. I've cut a lot of the fat, made it into a service, and made storage plug-able and (coarsely) versioned.

The API is high level enough that we could implicitly create new accounts as needed – I'd say let's defer explicit API-level management to a later Milestone where account/device management is more in focus.

Comment 11

[Chris Karlof [:ckarlof]]

Looking good Zach. A few comments:

1) Magic strings like "signedInUser.json" should not be repeated in the code and should ideally be module level constants.

2) 

+function FileStorage(version="v1") {
+  this.version = version;
+}

I would change this to:

function JSONFileStorage(options) {
   // these should be required, so we need some error checking here
   this.baseDir = options.baseDir;
   this.filename = options.filename;
}

and change the signatures on get and set to

function get()
function set(contents)

This is a cleaner abstraction, IMO. It is something that could be used outside the context of FxAccounts.

3) Given the above change, I would move the versioning into the JSON payload and have the FxAccounts object handle it (not the FileStorage object). Maybe something like:

{
  version: 1,
  accountData: { ... }
}

Comment 12

[Zachary Carter [:zaach]]

Attachment: 909967_fxa_module-2.patch

Incorporated some of Chris' feedback.

Comment 13

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

Comment on attachment 803316 [details] [diff] [review]
909967_fxa_module-2.patch

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

This iteration is much better, thanks.  I'm not comfortable offering r+ as I don't feel I'm on-top of the project enough (eg, comments about versioning reflect my lack of knowledge more than the code), but IMO this (with comments addressed) is good enough to get things rolling.

::: services/fxaccounts/FxAccounts.jsm
@@ +52,5 @@
> +   *
> +   * @return Promise
> +   *         The promise resolves to null on success or is rejected on error
> +   */
> +  setSignedInUser: function setSignedInUser(credentials) {

I'm not clear on the strategy re versioning.  How do you know the caller of this function will be passing a version 1 object?  If the version is incremented (which presumably means slightly different data in |credentials|, how is that handled?  If you are sure every caller of this function will be all updated at the same time to use the new version, why is it necessary to store the version?  Conversely, if some consumers might be passing an "old" version, shouldn't the version number be passed with the data (and presumably they will also want data in the old version to be returned via getSignedInUser()?

@@ +90,5 @@
> +    }
> +
> +    return this._signedInUserStorage.get()
> +      .then((user) => user.version === this.version ? user.accountData : null,
> +            () => null);

should this store the user in the cache?  It looks like after startup but before setSignedInUser() is called, every getSignedInUser() will perform IO.

@@ +99,5 @@
> +   *
> +   * @return Promise
> +   *         The promise is rejected if a storage error occurs
> +   */
> +  signOut: function signOut() {

it looks like once signOut() is called, every future getSignedInUser will perform IO - maybe the _signedInUser cache needs to differentiate "no user logged in" from "don't yet know who the logged in user is".

::: services/fxaccounts/FxAccountsService.js
@@ +48,5 @@
> +    }
> +
> +    try {
> +      this._loadFxAccounts();
> +    } catch (ex) {

we need to report the error here - although I'm not sure it makes sense to handle it here - aren't callers going to get upset if "null" is returned (ie, what action would you expect callers to take in this case?).  I can't see what exceptions this is likely to catch - _loadFxAccouts doesn't actually load anything best I can tell.

@@ +57,5 @@
> +  },
> +
> +  _loadFxAccounts: function () {
> +    let ns = {};
> +    // Lazy import so application startup isn't adversely affected.

XPCOMUtils.defineLazyModuleGetter could help here.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +5,5 @@
> +
> +const {interfaces: Ci, results: Cr, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/services/fxaccounts/FxAccounts.jsm");
> +let accounts = new FxAccounts();

IIUC, this test doesn't actually cause anything to be loaded from disk as the getSignedInUser calls will be satisfied by the cache.  We should ensure disk loading works.

Also, should this (or another) test be using the service rather than importing FxAccounts.jsm directly?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 803316 [details] [diff] [review]
909967_fxa_module-2.patch

Looks fine, let's get this landed and iterate. Some nits to address before landing:

>diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm

>+this.EXPORTED_SYMBOLS = ["FxAccounts", "JSONStorage"];

Does JSONStorage really need to be exported? Don't see a use elsewhere at the moment.

>+FxAccounts.prototype = Object.freeze({

>+  setSignedInUser: function setSignedInUser(credentials) {

>+    return this._signedInUserStorage.set(record)
>+      .then(() => null);

Why the extra chained promise that returns null? Is it to avoid "leaking" the result of the _signedInUserStorage.set promise? Seems more common to have promises just return undefined for these cases, so you could just return |this._signedInUserStorage.set(record)| directly AFAICT. Same comment applies to getSignedInUser and signOut.

>+JSONStorage.prototype = Object.freeze({

>+  _prepareDir: function () {
>+    let dir = this.baseDir;
>+    return OS.File.exists(dir)
>+      .then((exists) => exists ? null : OS.File.makeDir(dir, {ignoreExisting: true}));

Why call exists() before makeDir()? Can't you just call makeDir? Presumably that's optimal, given ignoreExisting:true.

>diff --git a/services/fxaccounts/FxAccountsService.js b/services/fxaccounts/FxAccountsService.js

I really don't see any benefit to XPCOM here. Why not just use FxAccounts directly? You can enforce a singleton in a JSM by exporting a reference to a single instance (and renaming the constructor).

>diff --git a/services/fxaccounts/Makefile.in b/services/fxaccounts/Makefile.in

This should be simpler now, as of https://groups.google.com/forum/#!topic/mozilla.dev.platform/EirpsT8JeYE. New makefile/moz.build additions require build peer review (see https://wiki.mozilla.org/Build_config and https://lists.mozilla.org/pipermail/dev-platform/2013-July/000388.html)

>+MODULES_PATH = $(FINAL_TARGET)/modules/services/fxaccounts

I don't think you need a special path for this module.

>+PP_TARGETS += MODULES

Does the module need to be preprocessed? Doesn't look like it.

>+TESTING_JS_MODULE_DIR := services/fxaccounts

This shouldn't be added since you're not adding any TESTING_JS_MODULEs, AIUI.

>diff --git a/services/fxaccounts/tests/xpcshell/test_accounts.js b/services/fxaccounts/tests/xpcshell/test_accounts.js

>+add_task(function test_set_signed_in_user() {
>+  let result = yield accounts.setSignedInUser(credentials);
>+  do_check_null(result);
>+
>+  run_next_test();
>+});

>+add_task(function test_signout() {
>+  let result = yield accounts.signOut();
>+  do_check_null(result);
>+
>+  run_next_test();
>+});

Doesn't seem useful to test the return value of these (you just care that they are resolved at all), so AIUI you could re-write as:

>+add_task(function test_set_signed_in_user() {
>+  accounts.setSignedInUser(credentials).then(run_next_test);
>+});

(could also add an error handler to catch failures, I guess)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Richard Newman [:rnewman] from bug 910479 comment #3)
> * Flat files. Need to consider if these are encrypted, and whether they live
> inside a profile directory or outside it (again, for SITB).

Encryption/obfuscation is worth considering. Local attackers have other alternatives, but we should avoid making it "easy" to steal sync keys.

Comment 16

[Chris Karlof [:ckarlof]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> >diff --git a/services/fxaccounts/FxAccountsService.js b/services/fxaccounts/FxAccountsService.js
> 
> I really don't see any benefit to XPCOM here. Why not just use FxAccounts
> directly? You can enforce a singleton in a JSM by exporting a reference to a
> single instance (and renaming the constructor).
> 

For better or worse, we created a service. I don't know all the pros and cons of doing a service vs not, but if there aren't significant drawbacks, I propose we leave it the way it is.

Comment 17

[Zachary Carter [:zaach]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> Comment on attachment 803316 [details] [diff] [review]
> 909967_fxa_module-2.patch
> 
> Looks fine, let's get this landed and iterate. Some nits to address before
> landing:
> 
> >diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm
> 
> >+this.EXPORTED_SYMBOLS = ["FxAccounts", "JSONStorage"];
> 
> Does JSONStorage really need to be exported? Don't see a use elsewhere at
> the moment.
Yeah, we can remove that now.

> 
> >+FxAccounts.prototype = Object.freeze({
> 
> >+  setSignedInUser: function setSignedInUser(credentials) {
> 
> >+    return this._signedInUserStorage.set(record)
> >+      .then(() => null);
> 
> Why the extra chained promise that returns null? Is it to avoid "leaking"
> the result of the _signedInUserStorage.set promise? Seems more common to
> have promises just return undefined for these cases, so you could just
> return |this._signedInUserStorage.set(record)| directly AFAICT. Same comment
> applies to getSignedInUser and signOut.

True, though getSignedInUser is different – we need to return undefined when there is an error getting the current user from storage (which, for JSONStorage, indicates the storage object for the user hasn't been created yet). Perhaps there's a better way to handle that.

> 
> >+MODULES_PATH = $(FINAL_TARGET)/modules/services/fxaccounts
> 
> I don't think you need a special path for this module.
I'm not sure what the resultant import path would be when this is omitted (right now, it is resource://gre/modules/services/fxaccounts/FxAccounts.jsm). I borrowed this format from the other modules in the service directory. 

> 
> >+PP_TARGETS += MODULES
> 
> Does the module need to be preprocessed? Doesn't look like it.

Removing this line breaks things for me (module not found), but it shouldn't need preprocessing.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Chris Karlof [:ckarlof] from comment #16)
> For better or worse, we created a service. I don't know all the pros and
> cons of doing a service vs not, but if there aren't significant drawbacks, I
> propose we leave it the way it is.

If there's no reason to do it, we shouldn't complicate things unnecessarily. Switching it back is easy; happy to help.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to Zachary Carter [:zaach] from comment #17)
> I'm not sure what the resultant import path would be when this is omitted
> (right now, it is
> resource://gre/modules/services/fxaccounts/FxAccounts.jsm). I borrowed this
> format from the other modules in the service directory. 

> Removing this line breaks things for me (module not found), but it shouldn't
> need preprocessing.

Just add FxAccounts.jsm to EXTRA_JS_MODULES in your moz.build, and then import it using "resource://gre/modules/FxAccounts.jsm". You can remove the rest of the Makefile gunk - gps or another build peer can help advise on how it should work (and should review any Makefile additions anyhow).

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 803316 [details] [diff] [review]
909967_fxa_module-2.patch

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

Why is this code in /services versus say /toolkit?

::: services/fxaccounts/Makefile.in
@@ +6,5 @@
> +topsrcdir = @top_srcdir@
> +srcdir    = @srcdir@
> +VPATH     = @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk

Everything up to this point except for the license header can be removed.

@@ +16,5 @@
> +TESTING_JS_MODULE_DIR := services/fxaccounts
> +
> +include $(topsrcdir)/config/rules.mk
> +
> +$(FINAL_TARGET)/components/FxAccountsService.js: FxAccounts.jsm

Why?

::: services/moz.build
@@ +21,5 @@
>  if CONFIG['MOZ_SERVICES_METRICS']:
>      PARALLEL_DIRS += ['metrics']
>  
>  if CONFIG['MOZ_SERVICES_SYNC']:
> +    PARALLEL_DIRS += ['sync', 'fxaccounts']

Why are you tying Firefox Accounts to Sync? If it's a standalone component, we should put it behind a new flag.

Comment 21

[Zachary Carter [:zaach]]

Attachment: 909967_fxa_module-3.patch

Comment 22

[Zachary Carter [:zaach]]

Thanks Mark, I've incorporated your feedback into the latest patch.

(In reply to Mark Hammond (:markh) from comment #13)
> Comment on attachment 803316 [details] [diff] [review]
> 909967_fxa_module-2.patch
> 
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +52,5 @@
> > +   *
> > +   * @return Promise
> > +   *         The promise resolves to null on success or is rejected on error
> > +   */
> > +  setSignedInUser: function setSignedInUser(credentials) {
> 
> I'm not clear on the strategy re versioning.  How do you know the caller of
> this function will be passing a version 1 object?  If the version is
> incremented (which presumably means slightly different data in
> |credentials|, how is that handled?  If you are sure every caller of this
> function will be all updated at the same time to use the new version, why is
> it necessary to store the version?  Conversely, if some consumers might be
> passing an "old" version, shouldn't the version number be passed with the
> data (and presumably they will also want data in the old version to be
> returned via getSignedInUser()?
> 
All consumers should be updated at the same time. However, the version currently found in storage could have come from a previous/future iteration (e.g., if an older/newer profile directory is copied over), so we store the version number to detect this.

For future versions, we can add a method to transition between two versions, or clear the session if transition is not possible.

Comment 23

[Zachary Carter [:zaach]]

Attachment: 909967_fxa_module-4.patch

Tweak of the last patch. The constructor of the exported instance seems to get set to Object rather than FxAccounts, so let's just export a reference to the constructor.

Comment 24

[Zachary Carter [:zaach]]

Attachment: 909967_fxa_module-4.patch update

I should qrefresh first...

Comment 25

[Sam Penrose]

A fairly basic point WRT division of responsibilities between FxAccounts and FxAccountManager from 911378 which has somehow remained unclear to me: if the signedInUser blob stored by FxAccounts holds the token, shouldn't it provide a setter for FxAccountManager? Right now it looks like the manager has to do this (details elided):

   userBlob = getUserFromFxAccounts(); // sessionToken field is null, because only manager can set it;
   token = getTokenUsingAssertion(userBlob.assertion);
   userBlob.sessionToken = token;
   fxAccountsInstance.setSignedInUser(userBlob);

Note that ownership of the FxA.email field / FxAM.account field belongs to FxA, not FxAM.

Comment 26

[Chris Karlof [:ckarlof]]

(In reply to Sam Penrose from comment #25)
> A fairly basic point WRT division of responsibilities between FxAccounts and
> FxAccountManager from 911378 which has somehow remained unclear to me: if
> the signedInUser blob stored by FxAccounts holds the token, shouldn't it
> provide a setter for FxAccountManager? Right now it looks like the manager
> has to do this (details elided):
> 
>    userBlob = getUserFromFxAccounts(); // sessionToken field is null,
> because only manager can set it;
>    token = getTokenUsingAssertion(userBlob.assertion);
>    userBlob.sessionToken = token;
>    fxAccountsInstance.setSignedInUser(userBlob);
> 
> Note that ownership of the FxA.email field / FxAM.account field belongs to
> FxA, not FxAM.

Yeah, confusing. There's two tokens, one is a sessionToken with the FxA server (managed by the FxAccounts module), and one is the Sync 2.0 Hawk signing token (managed by the Sync identity manager replacement). We should be more clear about the naming of these tokens in the code. Currently, the FxA session token leaks outside of the module, particularly in getSignedInUser. Another problem is that the FxAccount module only exports assertions for a fixed audience (the sync token server). This is not what we want long term. We should probably have a method on the FxAccount module called getAssertionForAudience, but that would require jwcrypto in native code, which I didn't want to block this sprint. 

Good job making me reveal my dirty little secret, Sam.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 807534 [details] [diff] [review]
909967_fxa_module-4.patch update

>diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm

>+const defaultBaseDir = OS.Path.join(OS.Constants.Path.profileDir, "fxaccounts");
>+const defaultStorageOptions = {
>+  filename: 'signedInUser.json',
>+  baseDir: defaultBaseDir,
>+};

Out of curiosity, why have the fxaccounts subfolder? Do we expect other related files to be stored?

>+  getSignedInUser: function getSignedInUser() {

>+    if (typeof this._signedInUser !== 'undefined') {

nit: using !== undefined rather than a simple null check sort of implies that you care to distinguish this case from it being null, but that can't be the case because:

>+      deferred.resolve(this._signedInUser.accountData);

would throw in that case. So can this just be if (this._signedInUser)?

>+    return this._signedInUserStorage.get()
>+      .then((user) => {
>+          if (user.version === this.version) {
>+            this._signedInUser = user;
>+            return user.accountData;
>+          }
>+        },

This should have an explicit return undefined; fallback, I suppose.

>+        (err) => void 0);

undefined?

>diff --git a/services/fxaccounts/tests/xpcshell/test_accounts.js b/services/fxaccounts/tests/xpcshell/test_accounts.js

>+Cu.import("resource://gre/modules/FxAccounts.jsm");
>+
>+let accounts = new FxAccounts();

Why not just test the fxAccounts instance?

A test that the file actually does get written out to disk correctly would also be valuable (using something like http://hg.mozilla.org/mozilla-central/annotate/d50f590056fd/toolkit/components/search/tests/xpcshell/head_search.js#l109).

r=m with those addressed.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 807534 [details] [diff] [review]
909967_fxa_module-4.patch update

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

r+ conditional on comment being addressed. Review covered just the build bits.

::: services/fxaccounts/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXTRA_JS_MODULES := FxAccounts.jsm

This should be in moz.build and this Makefile.in should be deleted.

Comment 29

[Zachary Carter [:zaach]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #27)
> Comment on attachment 807534 [details] [diff] [review]
> 909967_fxa_module-4.patch update
> 
> A test that the file actually does get written out to disk correctly would
> also be valuable (using something like
> http://hg.mozilla.org/mozilla-central/annotate/d50f590056fd/toolkit/
> components/search/tests/xpcshell/head_search.js#l109).

There is a test that deletes the cache before calling getSignedInUser, which forces it to be read and parsed from disk. I can make the comment more explicit.

Comment 30

[Zachary Carter [:zaach]]

https://hg.mozilla.org/projects/elm/rev/cdfd1aa2a0e8

Comment 31

[James Bonacci [:jbonacci]]

OK, well this is exciting! Another fix into ELM...
Somebody please ping me when this nightly build is ready to install and test...

Comment 32

[James Bonacci [:jbonacci]]

I will be looking at this with an ELM build.
I assume this is desktop only...

Comment 33

[Chris Karlof [:ckarlof]]

Reopening this to land a refactored version in m-c. This version supports more of the background operations (cert signing, key fetch), and should support both FxA Desktop and B2G efforts.

Comment 34

[Zachary Carter [:zaach]]

Attachment: Refactor of FxAccounts.jsm that relies on FxAccountsClient.jsm

Comment 36

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

Comment on attachment 830543 [details] [diff] [review]
Refactor of FxAccounts.jsm that relies on FxAccountsClient.jsm

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

LGTM

::: services/fxaccounts/FxAccounts.jsm
@@ +68,5 @@
> +  // set() makes sure that polling is happening, if necessary
> +  // get() does not wait for verification, and returns an object even if
> +  // unverified. The caller of get() must check .isVerified .
> +  // The "fxaccounts:onlogin" event will fire only when the verified state
> +  // goes from false to true, so callers must register their event handler

s/event handler/observer/

@@ +72,5 @@
> +  // goes from false to true, so callers must register their event handler
> +  // and then call get(). In particular, it will not fire when the account
> +  // was found to be verified in a previous boot: if our stored state says
> +  // the account is verified, the event will never fire. So callers must do:
> +  //  register event handler (go)

ditto - "register notification observer"

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +116,5 @@
> +  _now: function() {
> +    return this._now_is;
> +  },
> +  _fetchKeys: function(keyFetchToken) {
> +    dump("== _fetchKeys\n");

a bit of a nit, but most |dump| calls here should probably use the "_" function, as defined and used in head_helpers.js - the sync tests use this extensively - eg: _("== _fetchKeys");

Comment 37

[Zachary Carter [:zaach]]

Attachment: fix nits, update to use FxAccountsClient component

Comment 38

[Zachary Carter [:zaach]]

Attachment: small refactor; creates instance of fxaclient component when needed

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 831972 [details] [diff] [review]
small refactor; creates instance of fxaclient component when needed

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

::: services/fxaccounts/FxAccounts.jsm
@@ +19,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "jwcrypto",
> +                                  "resource://gre/modules/identity/jwcrypto.jsm");
> +
> +
> +const defaultStorageFilename = "signedInUser.json";

Please write constants in all caps.

@@ +32,5 @@
> + * @param signedInUserStorage is a storage instance for getting/setting
> + *                            the signedInUser. Uses JSONStorage by default.
> + * @return instance
> + */
> +this.FxAccounts = function(signedInUserStorage) {

Please split this object into two, the one exposing the public API and delegating all calls to an internal module that then has a few "private" helpers. Otherwise all those _* methods are exported. Here's an example of what I mean:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/Messenger.jsm

@@ +54,5 @@
> +
> +  getReady: function() {
> +    // kick things off. This must be called after construction. I return a
> +    // promise that fires when everything is ready to go, but the caller is
> +    // free to ignore it.

Having to explicitly call |getReady| after creating a new instance seems like a bad API. I think this should all happen behind the scenes lazily whenever someone accesses one of the public methods. AFAIU polling should only be started by |getSignedInUser| and |setSignedInUser| right? |getSignedInUser| would start polling when called by the sync service at startup, |setSignedInUser| would start polling after account creation in about:accounts. All other methods should probably just throw when the user isn't verified yet.

@@ +116,5 @@
> +        }
> +      });
> +  },
> +
> +  _isReady: function _isReady(data) {

|isReady| is a bad function name, imo. How about |isUserVerified|, |isUserEmailVerified| or something?.

@@ +125,5 @@
> +    // get us to the verified state, then get the keys. This returns a
> +    // promise that will fire when we are completely ready.
> +    return this._whenVerified(data)
> +      .then(this._notifyLoginObservers)
> +      .then(() => data);

Looks like this could be:

return this._whenVerified(data)
         .then(() => {
           this._notifyLoginObservers();
           return data;
         });

That's a little easier to read I think.

@@ +134,5 @@
> +      return Promise.resolve(data);
> +    }
> +    if (!this._whenVerifiedPromise) {
> +      // poll for 5 minutes
> +      this._pollTimeRemaining = 5 * 60 * 1000;

Please no magic constants.

@@ +145,5 @@
> +  _pollEmailStatus: function _pollEmailStatus(sessionToken, why) {
> +    log(" entering _pollEmailStatus ("+(why||"")+")\n");
> +    this._checkEmailStatus(sessionToken)
> +      .then(response => {
> +        log(" - response: "+JSON.stringify(response)+"\n");

Even if |log| is a no-op we're still paying the price of JSON.stringify() every time. Maybe that's negligible but I'd rather avoid that.

@@ +157,5 @@
> +              this._whenVerifiedPromise.resolve(data);
> +              delete this._whenVerifiedPromise;
> +            });
> +        } else {
> +          this._pollTimeRemaining -= 3000;

Another magic constant.

@@ +159,5 @@
> +            });
> +        } else {
> +          this._pollTimeRemaining -= 3000;
> +          if (this._pollTimeRemaining > 0) {
> +            log("-=*=- starting setTimeout()\n");

Looks like |log| should maybe automatically add "\n"?

@@ +160,5 @@
> +        } else {
> +          this._pollTimeRemaining -= 3000;
> +          if (this._pollTimeRemaining > 0) {
> +            log("-=*=- starting setTimeout()\n");
> +            setTimeout(() => this._pollEmailStatus(sessionToken, "timer"), 3000);

Another magic constant.

@@ +168,5 @@
> +    },
> +
> +  _checkEmailStatus: function _checkEmailStatus(sessionToken) {
> +    let fxAccountsClient = Cc['@mozilla.org/fxaccounts/fxaccounts-client;1']
> +                           .createInstance(Ci.nsIFxAccountsClient);

This should be defined at the top and called gFxAccountsClient.

@@ +233,5 @@
> +  },
> +
> +  _fetchKeys: function _fetchKeys(keyFetchToken) {
> +    let fxAccountsClient = Cc['@mozilla.org/fxaccounts/fxaccounts-client;1']
> +                           .createInstance(Ci.nsIFxAccountsClient);

Same.

@@ +267,5 @@
> +  },
> +
> +  keyLifetime: 12*3600*1000, // 12 hours
> +  certLifetime: 6*3600*1000, // 6 hours
> +  assertionLifetime: 5*1000, // 5 minutes

That would be great to have declared as consts at the top of the file, accompanied by a short comment.

@@ +296,5 @@
> +  },
> +
> +  _now: function() {
> +    return Date.now();
> +  },

This seems weird, just use Date.now() :)

@@ +408,5 @@
> +   */
> +  signOut: function signOut() {
> +    this._signedInUser = null;
> +    return this._signedInUserStorage.set(null).then(() => {
> +      Services.obs.notifyObservers(null, "fxaccounts:onlogout", null);

Should we maybe have a |_notifyObservers| method that takes a |topic| argument?

@@ +440,5 @@
> +  this.baseDir = options.baseDir;
> +  this.path = OS.Path.join(options.baseDir, options.filename);
> +}
> +
> +JSONStorage.prototype = Object.freeze({

Using Object.freeze() is unnecessary, we're not exporting JSONStorage.

@@ +452,5 @@
> +  },
> +});
> +
> +// A getter for the instance to export
> +XPCOMUtils.defineLazyGetter(this, "fxAccounts", function() {

I don't understand why we export this as a separate symbol. Why can't we just let the client instantiate? Do we really need multiple instances? It looks like this should always have the same storage and maybe just be a normal JSM?

Comment 40

[Zachary Carter [:zaach]]

Thanks for the review Tim! Just a couple of notes:

(In reply to Tim Taubert [:ttaubert] from comment #39)
> Comment on attachment 831972 [details] [diff] [review]
> small refactor; creates instance of fxaclient component when needed
> 
> Review of attachment 831972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> 
> @@ +168,5 @@
> > +    },
> > +
> > +  _checkEmailStatus: function _checkEmailStatus(sessionToken) {
> > +    let fxAccountsClient = Cc['@mozilla.org/fxaccounts/fxaccounts-client;1']
> > +                           .createInstance(Ci.nsIFxAccountsClient);
> 
> This should be defined at the top and called gFxAccountsClient.

I believe the reason for creating local instances rather than keeping a module-wide reference around is so that memory can be released on b2g. I'd have to defer to fermj again on this.


> @@ +452,5 @@
> > +  },
> > +});
> > +
> > +// A getter for the instance to export
> > +XPCOMUtils.defineLazyGetter(this, "fxAccounts", function() {
> 
> I don't understand why we export this as a separate symbol. Why can't we
> just let the client instantiate? Do we really need multiple instances? It
> looks like this should always have the same storage and maybe just be a
> normal JSM?

In practice, all consumers would use the shared instance. The constructor is for added flexibility during testing. Is a normal JSM one that only exports a singleton?

Comment 41

[Sam Penrose]

Thanks for the review! In addition to :zaach's comments, a couple questions.

(In reply to Tim Taubert [:ttaubert] from comment #39)
> Comment on attachment 831972 [details] [diff] [review]
 
> @@ +32,5 @@
> > + * @param signedInUserStorage is a storage instance for getting/setting
> > + *                            the signedInUser. Uses JSONStorage by default.
> > + * @return instance
> > + */
> > +this.FxAccounts = function(signedInUserStorage) {
> 
> Please split this object into two, the one exposing the public API and
> delegating all calls to an internal module that then has a few "private"
> helpers. Otherwise all those _* methods are exported. Here's an example of
> what I mean:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/src/Messenger.jsm

I am in the process of doing this, and it works, but there appears to be a fundamental conflict between keeping the exported_symbols few/terse and unit testing. If you look at the current test_accounts.js, you'll see the FxAccounts object wrapped in a Mock that overrides, e.g., _fetchKeys. Once I follow your instruction to move _fetchKeys to an inaccessible object, the only way to override it is to open an injection point in the FxAccounts instance. Worse, if the tests need to change private state (as they do to test timing, for example), then the instance has to de-privatize it somehow. Here's my current approach:

InternalMethods = function(mock) {
  // all "_" methods moved here as in Messenger.jsm
  if (mock) {
    Object.keys(mock).forEach((prop) => {
     log('-- ** --> mocking: ', prop);
      this[prop] = mock[prop];
    });
  } else {
    // normal construction
  }
}
let internal = null;
this.FxAccounts = function(mockInternal) {
  internal = new InternalMethods(mockInternal);
  if (mockInternal) {
    return internal; // Holy oxymorons, Batman!
  }
}

This is necessary because:
> 
> @@ +296,5 @@
> > +  },
> > +
> > +  _now: function() {
> > +    return Date.now();
> > +  },
> 
> This seems weird, just use Date.now() :)

gets mocked to "return this._now_is", which is set on internal. Richard Newman suggested this pattern for testing expiration here:
  https://bugzilla.mozilla.org/show_bug.cgi?id=911378#c35
Since it's landed once, can we land it again :-)?

Comment 42

[Sam Penrose]

The waitForReady parameter to setSignedInUser() 1) is unused 2) has extensive docs. Can I delete the parameter and the docs?

Comment 43

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxAccountsClient is a jsm again

Didn't want to clobber the review process by modifying the main patch

Comment 44

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 831972 [details] [diff] [review]
small refactor; creates instance of fxaclient component when needed

We need to update the moz.build bit in this patch to include the FxAccountsClient.jsm:

EXTRA_JS_MODULE = [
  'FxAccounts.jsm',
  'FxAccountsClient.jsm'
]

Comment 45

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: small refactor; creates instance of fxaclient component when needed

Added small moz.build fix so this patch can be applied after bug 935232

Comment 46

[Zachary Carter [:zaach]]

I talked with Sam.

Comment 47

[Sam Penrose]

Attachment: 909967_25_Nov_edited.patch

This fulfills all of :taubert's feedback (thanks Tim!) modulo the specific points Zack and I raised. I had to edit the specific diff commands by hand; Zack and/or Jed can you tell me if they look right? If so, we can move on to r?

Comment 48

[Zachary Carter [:zaach]]

Comment on attachment 8338672 [details] [diff] [review]
909967_25_Nov_edited.patch

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

I get an error trying to apply the patch: "fatal: corrupt patch at line 762". It appears the offsets may be wrong.

The patch should also add FxAccounts.jsm to services/fxaccounts/moz.build; something like:


> EXTRA_JS_MODULES += ['FxAccounts.jsm',
>                      'FxAccountsClient.jsm']

::: services/fxaccounts/FxAccounts.jsm
@@ +24,5 @@
> +const ASSERTION_LIFETIME = 5*60*1000; // 5 minutes
> +const KEY_LIFETIME = 12*3600*1000;    // 12 hours
> +const CERT_LIFETIME = 6*3600*1000;    // 6 hours
> +const POLL_SESSION = 5*60*1000;       // 5 minutes
> +const POLL_STEP = 3 * 1000;           // 3 seconds

Nit: consistent spacing around operator.

@@ +364,5 @@
> +   *          isVerified: email verification status
> +   *        }
> +   *        or null if no user is signed in
> +   */
> +  getKeys: function(data) {

This method should use internal.getAccountData() rather than accepting data as an argument. We forgot to change this during a previous refactor, but it's needed for Sync.

Comment 49

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxAccounts.jsm

I also had trouble applying the previous patch.  I've prepared a new patch that contains the previous patch's material and also addresses most of of the issues in Comment 48.  (In addition, I've made it import the FxAccountsClient.jsm - it was still written to use a component.)

The getKeys() function still needs to be revised.  Zach, does whenVerified() need to be revised in the same way?

Comment 50

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

Comment on attachment 8339937 [details] [diff] [review]
FxAccounts.jsm

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

This is looking pretty good to me.

::: services/fxaccounts/FxAccounts.jsm
@@ +26,5 @@
> +const CERT_LIFETIME = 1000 * 3600 * 6;    // 6 hours
> +const POLL_SESSION = 1000 * 60 * 5;       // 5 minutes
> +const POLL_STEP = 1000 * 3;               // 3 seconds
> +
> +function log(...args) {

maybe import Console.jsm and use console.log?  TBH though, I'm not sure if exists in b2g, but might be worth a look.

@@ +284,5 @@
> +  }
> +  internal = new InternalMethods(mocks);
> +  if (mockInternal) { // not mocks
> +    this.internal = internal;
> +  }

I assume this is for testing, but it's not very clear.  Some comments might be useful, or if the test suite can just monkey-patch the object things might be clearer.

@@ +292,5 @@
> +
> +  getReady: function() {
> +    // kick things off. This must be called after construction. I return a
> +    // promise that fires when everything is ready to go, but the caller is
> +    // free to ignore it.

it doesn't look like a promise is returned to me.

@@ +364,5 @@
> +  getKeys: function(data) {
> +    if (data.kA && data.kB) {
> +      return Promise.resolve(data);
> +    }
> +    if (!internal.whenKeysReadyPromise) {

I don't really understand whenKeysReadyPromise and specifically how things work correctly if getKeys() is called twice with data for different users.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +170,5 @@
> +  do_check_eq(!!data.isVerified, true);
> +  do_check_eq(data.kA, expandHex("11"));
> +  do_check_eq(data.kB, expandHex("66"));
> +  do_check_eq(data.keyFetchToken, undefined);
> +});

eg, if we called a.getKeys() again after a setSignedInUser with different creds, will it work as expected?

Comment 51

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxAccounts.jsm

Comment 52

[Sam Penrose]

(In reply to Mark Hammond [:markh] from comment #50)
> Comment on attachment 8339937 [details] [diff] [review]
> FxAccounts.jsm
> 
> Review of attachment 8339937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking pretty good to me.
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +26,5 @@
> > +const CERT_LIFETIME = 1000 * 3600 * 6;    // 6 hours
> > +const POLL_SESSION = 1000 * 60 * 5;       // 5 minutes
> > +const POLL_STEP = 1000 * 3;               // 3 seconds
> > +
> > +function log(...args) {
> 
> maybe import Console.jsm and use console.log?  TBH though, I'm not sure if
> exists in b2g, but might be worth a look.

Done (toolkit/modules/Log.jsm).

> 
> @@ +284,5 @@
> > +  }
> > +  internal = new InternalMethods(mocks);
> > +  if (mockInternal) { // not mocks
> > +    this.internal = internal;
> > +  }
> 
> I assume this is for testing, but it's not very clear.  Some comments might
> be useful, or if the test suite can just monkey-patch the object things
> might be clearer.

Commented. It's wacky due to Taubert's request for a different approach to information hiding after we'd chosen a mocking approach.

> 
> @@ +292,5 @@
> > +
> > +  getReady: function() {
> > +    // kick things off. This must be called after construction. I return a
> > +    // promise that fires when everything is ready to go, but the caller is
> > +    // free to ignore it.
> 
> it doesn't look like a promise is returned to me.

This routine has been replaced by loadAndPoll(), to address a concern of Taubert's.

> 
> @@ +364,5 @@
> > +  getKeys: function(data) {
> > +    if (data.kA && data.kB) {
> > +      return Promise.resolve(data);
> > +    }
> > +    if (!internal.whenKeysReadyPromise) {
> 
> I don't really understand whenKeysReadyPromise and specifically how things
> work correctly if getKeys() is called twice with data for different users.

This routine has been refactored to no longer take a data argument per Zack's feedback.

> 
> ::: services/fxaccounts/tests/xpcshell/test_accounts.js
> @@ +170,5 @@
> > +  do_check_eq(!!data.isVerified, true);
> > +  do_check_eq(data.kA, expandHex("11"));
> > +  do_check_eq(data.kB, expandHex("66"));
> > +  do_check_eq(data.keyFetchToken, undefined);
> > +});
> 
> eg, if we called a.getKeys() again after a setSignedInUser with different
> creds, will it work as expected?

r? on its way.

Comment 53

[Sam Penrose]

Comment on attachment 8341215 [details] [diff] [review]
FxAccounts.jsm

Per last response to your feedback. Thanks so much Mark!

Comment 54

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

Comment on attachment 8341215 [details] [diff] [review]
FxAccounts.jsm

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

I think this still needs some work (or more clarification if I'm wrong about some of these points) - in particular, fetchAndUnwrapKeys doesn't resolve with the expected values, I think the user of the somewhat-static promises will have problems in various use-cases and (somewhat related) I think the tests need to cover some multiple-user cases (ie, that a second user logging in works as expected and doesn't end up with promises being resolved due to a previous user's actions)

::: services/fxaccounts/FxAccounts.jsm
@@ +29,5 @@
> +const POLL_SESSION = 1000 * 60 * 5;       // 5 minutes
> +const POLL_STEP = 1000 * 3;               // 3 seconds
> +
> +let log = Log.repository.getLogger("Services.FxAccounts");
> +log.level = Log.Level.Debug;

hard-coding Debug here is a bit of a shame, but I can live with that if we can agree bug 943998 can also "fix" this and provide a consistent mechanism for adjusting log levels in these modules?

@@ +50,5 @@
> +      this[prop] = mock[prop];
> +    });
> +  }
> +  if (!this.signedInUserStorage) {
> +    // Normal initialization.

I assume this.signedInUserStorage will only be non-null during tests?  If so, maybe change this comment to read: // Normal (ie, non-test) initialization

@@ +74,5 @@
> +    log.debug("== fetchAndUnwrapKeys");
> +    return Task.spawn(function task() {
> +      // Sign out if we don't have a key fetch token.
> +      if (!keyFetchToken) {
> +        yield internal.signOut();

please use |this| instead of |internal| everywhere inside this object.

@@ +75,5 @@
> +    return Task.spawn(function task() {
> +      // Sign out if we don't have a key fetch token.
> +      if (!keyFetchToken) {
> +        yield internal.signOut();
> +        return;

it seems undefined will *always* be the value of the resolved promise.  In this case it should be null, and at the end of the function you should 'return data'.  To do this the function will need to be declared with "function* task()" - note the '*'.

@@ +103,5 @@
> +    // "audience" should look like "http://123done.org".
> +    // The generated assertion will expire in two minutes.
> +    jwcrypto.generateAssertion(cert, keyPair, audience, function(err, signed) {
> +      if (err) {
> +        d.reject(err);

log.error(...) probably makes sense here?  (or maybe just log.debug if this is going to be hit regularly - eg, after something expires normally)

@@ +124,5 @@
> +    let willBeValidUntil = internal.now() + CERT_LIFETIME;
> +    return internal.getCertificateSigned(data.sessionToken,
> +                                         keyPair.serializedPublicKey,
> +                                         CERT_LIFETIME)
> +      .then((cert) => {internal.cert = {cert: cert,

this indentation is weird, especially with the object creation on the first line - just start the function on its own line.

@@ +155,5 @@
> +        d.reject(err);
> +      } else {
> +        log.debug(" getKeyPair got keypair");
> +        internal.keyPair = { keyPair: kp,
> +                          validUntil: willBeValidUntil };

strange indentation

@@ +177,5 @@
> +          internal.signedInUser = user;
> +        }
> +        deferred.resolve(user ? user.accountData : undefined);
> +      },
> +      (err) => {deferred.resolve(undefined)}

is null a better choice?

@@ +212,5 @@
> +    return Date.now();
> +  },
> +
> +  pollEmailStatus: function pollEmailStatus(sessionToken, why) {
> +    log.debug(" entering pollEmailStatus (" + (why||"") + ")");

please add spaces around operators

@@ +228,5 @@
> +              delete internal.whenVerifiedPromise;
> +            });
> +        } else {
> +          internal.pollTimeRemaining -= POLL_STEP;
> +          if (internal.pollTimeRemaining > 0) {

if we stop polling, it looks like the promise will never be resolved.  It seems like it should maybe be rejected (although see my other concerns about the use of "static" promises).  Also, isn't this timer going to continue if the user changes?

@@ +260,5 @@
> +  whenVerified: function(data) {
> +    if (data.isVerified) {
> +      return Promise.resolve(data);
> +    }
> +    if (!internal.whenVerifiedPromise) {

I don't understand the use of a "static" promise here.  Let's say userA logs in but isn't verified (and userA never actually verifies).  Then userB logs in and verifies - isn't userA going to have the promise resolved even though they haven't verified?  Maybe I'm missing something, but the tests don't seem to handle this (or any "multiple users") case.

A solution to this might be to reject and set to null all such promises on login/logout?

@@ +287,5 @@
> +    mocks = null;
> +  }
> +  internal = new InternalMethods(mocks);
> +  if (mockInternal) { // not mocks
> +    this.internal = internal;

this.internal doesn't seem used - why does it need to be set?

@@ +303,5 @@
> +  // was found to be verified in a previous boot: if our stored state says
> +  // the account is verified, the event will never fire. So callers must do:
> +  //   register notification observer (go)
> +  //   userdata = get()
> +  //   if (userdata.isVerified()) {go()}

If this pattern is common, I wonder if it would be useful to provide a helper function that returns a promise which is only resolved when verified?

@@ +357,5 @@
> +    return internal.getUserAccountData().then((data) => {
> +      if (data.kA && data.kB) {
> +        return Promise.resolve(data);
> +      }
> +      if (!internal.whenKeysReadyPromise) {

I don't think there is test coverage of this line and below - specifically, I think that fetchAndUnwrapKeys is always going to resolve with undefined - see above.

@@ +361,5 @@
> +      if (!internal.whenKeysReadyPromise) {
> +        internal.whenKeysReadyPromise = Promise.defer();
> +        internal.fetchAndUnwrapKeys(data.keyFetchToken)
> +          .then((data) => {
> +            internal.whenKeysReadyPromise.resolve(data);

same problem here re static promises.  It looks like this is never reset, so getKeys() for a different user looks like it will be resolved immediately.  Also, should the promise be rejected if fetchAndUnwrapKeys fails?

@@ +398,5 @@
> +      });
> +  },
> +
> +  /**
> +   * Return a BrowserID-compatible (?) assertion for use by RPs.

A comment should probably be added to say that if the user isn't verified this will return null, so the caller must do the "isVerified/addObserver" dance mentioned in the comments for this class.  (OTOH though, I don't really see why it should work like this, as it just adds pain to the consumer of the class - why not have the promise wait for verification?)

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +40,5 @@
> +    assertion: "foobar",
> +    sessionToken: "dead",
> +    kA: "beef",
> +    kB: "cafe",
> +    isVerified: true

can we have some tests where verification comes in later?  This will also help test the use of promises and multiple users (eg, that after a user is set that doesn't ever verify, it isn't treated as being verified when a second user is, and similarly, if the first user does verify after a second user is logged in, we don't erroneously consider the second user as verified.)

@@ +160,5 @@
> +    wrapKB: expandBytes("22"),
> +  });
> +
> +  yield a.setSignedInUser(creds);
> +  yield a.getKeys();

The test should check it got what it expected - currently the value is undefined, which isn't what I'd expect.
There should also be a test for the signed-out case, where (IIUC) null should be returned.

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8341215 [details] [diff] [review]
FxAccounts.jsm

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

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +9,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +
> +// BackstagePass that gives us atob when building on b2g.
> +// (This is not necessary when building browser; I know not why.)
> +let atob = Cu.import("resource://gre/modules/Log.jsm").atob;

Change this line to:

Cu.importGlobalProperties(['atob']);

until bug 937114 is fixed.

Comment 56

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxAccounts.jsm.wip.patch

Work in progress

- Mocks fxa client
- Tries to solve concurrent sign-in issue (the "static promises" problem)
- Test polling for verification
- Test for observer notification on verification + keyfetch
- Test for userA and userB overlapping sign-in

Not ready for review yet - still addressing issues in comments above

Comment 57

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxAccounts.jsm

Addressing questions in Comment 54.

Comment 58

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

Comment on attachment 8343397 [details] [diff] [review]
FxAccounts.jsm

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

A quick drive-by, but this is looking awesome, thanks!

::: services/fxaccounts/FxAccounts.jsm
@@ +110,5 @@
> +   * Reset state such that any previous flow is canceled.
> +   */
> +  abortExistingFlow: function abortExistingFlow() {
> +    if (this.currentTimer) {
> +      log.warn("Surprised to find a pending setTimeout; Clearing it.");

is this really surprising?  Also, the fact you don't reset this.currentTimer in this block will not cause problems, but might cause this message to be printed when it's not actually pending.

@@ +160,5 @@
> +      if (!this.whenKeysReadyPromise) {
> +        this.whenKeysReadyPromise = Promise.defer();
> +        this.fetchAndUnwrapKeys(data.keyFetchToken)
> +          .then((data) => {
> +            this.whenKeysReadyPromise.resolve(data);

I wonder if you need to check if this.whenKeysReadyPromise is non null here (ie, ensure that abortExistingFlow wasn't called between creating the promise and fetchAndUnwrapKeys completing)?

@@ +265,5 @@
> +
> +  getKeyPair: function(mustBeValidUntil) {
> +    log.debug("getKeyPair");
> +    if (this.keyPair) {
> +      log.debug("keyPair valid until: " + this.keyPair.validUntil + " " + mustBeValidUntil);

not important, but note the log module allows an object as a second param that will be jsonified - so something like:
  log.debug("getKeyPair:", {mustBeValidUntil: mustBeValidUntil, keyPair: keyPair});
should generate useful output in all cases.

@@ +279,5 @@
> +      if (err) {
> +        d.reject(err);
> +      } else {
> +        log.debug("getKeyPair got keypair");
> +        this.keyPair = { 

trailing space here

@@ +306,5 @@
> +          this.signedInUser = user;
> +        }
> +        deferred.resolve(user ? user.accountData : null);
> +      },
> +      (err) => {deferred.resolve(null)}

a little surprised the promise isn't rejected here, but there may be a good reason (ie, I'm fine with this if you are confident this is the most reasonable thing to do)

@@ +335,5 @@
> +    // Get us to the verified state, then get the keys. This returns a
> +    // promise that will fire when we are completely ready.
> +    //
> +    // Login is truly complete once keys have been fetched, so once
> +    // getKeys() obtains and stores kA and kB, it will fire the 

trailing space here

@@ +356,5 @@
> +  },
> +
> +
> +  notifyObservers: function(topic, data = null) {
> +    log.debug("Notifying observers of user login: " + data); 

trailing space

@@ +381,5 @@
> +      }
> +      this.pollTimeRemaining = this.POLL_SESSION;
> +    }
> +
> +    log.debug("entering pollEmailStatus (" + (why||"") + ") " + myGenerationCount);

ubernit, but still need spaces around '||'

@@ +407,5 @@
> +              // XXX when we fire onlogin, we're saying that the
> +              // user is verified AND that we have all the keys.
> +              // sync can listen for this and know when to start up
> +              // without having to inquire and poke around.
> +              this.whenVerifiedPromise.resolve(data);

isn't there still an (unlikely) chance this.whenVerifiedPromise will have gone null while getUserAccountData completes?  It almost looks as though the generationCount check above isn't that useful - you could just charge ahead and abort if you find whenVerifiedPromise is null at the end (hrmph - maybe it's not that simple - you wouldn't want to call setUserAccountData in that case).

Anyway, just food-for-thought for now.

@@ +419,5 @@
> +            this.currentTimer = setTimeout(() => {
> +              this.pollEmailStatus(sessionToken, "timer")}, this.POLL_STEP);
> +            log.debug("started timer " + this.currentTimer);
> +          } else {
> +            this.whenVerifiedPromise.reject(

ditto here - isn't there an unlikely chance whenVerifiedPromise might be null here?

@@ +433,5 @@
> +    return this.signedInUserStorage.get().then((record) => {
> +      record.accountData = accountData;
> +      this.signedInUser = record;
> +      return this.signedInUserStorage.set(record)
> +        .then(() => {return accountData});

up above in line 342 you have:

   .then((data) => this.getKeys(data));

If you were to follow the same style here, it looks like this line could read:

  .then(() => accountData);

(I'm not that bothered by this though)

@@ +537,5 @@
> +          // If the email is not verified, start polling for verification,
> +          // but return null right away.  We don't want to return a promise
> +          // that might not be fulfilled for a long time.
> +          internal.startVerifiedCheck(credentials);
> +          return data;

maybe delete this line and fall through?

Comment 59

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

As I was submitting this comment, I collided mid-air with :markh's latest feedback!

Mark, thank you for the incredible immediacy of your reviews.

I will post my comments here anyway to give some record of how we tried to adapt the patch in response to the critique.

(In reply to Mark Hammond [:markh] from comment #54)
> Comment on attachment 8341215 [details] [diff] [review]
> FxAccounts.jsm

Thank you for all the time and thought you've put into this review, Mark.

> Review of attachment 8341215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this still needs some work (or more clarification if I'm wrong about
> some of these points) - in particular, fetchAndUnwrapKeys doesn't resolve
> with the expected values, I think the user of the somewhat-static promises
> will have problems in various use-cases and (somewhat related) I think the
> tests need to cover some multiple-user cases (ie, that a second user logging
> in works as expected and doesn't end up with promises being resolved due to
> a previous user's actions)

Your comments were extremely helpful.

I've added tests to assure that fetchAndUnwrapKeys resolves with the expected values.

Static promises are only used internally, never passed to callers, and I have added checks to abort an in-progress "userA" promise if a "userB" flow begins.

There are now tests covering the multiple-user case.

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +29,5 @@
> > +const POLL_SESSION = 1000 * 60 * 5;       // 5 minutes
> > +const POLL_STEP = 1000 * 3;               // 3 seconds
> > +
> > +let log = Log.repository.getLogger("Services.FxAccounts");
> > +log.level = Log.Level.Debug;
> 
> hard-coding Debug here is a bit of a shame, but I can live with that if we
> can agree bug 943998 can also "fix" this and provide a consistent mechanism
> for adjusting log levels in these modules?

I've updated the code to respond to the pref specified in bug 943998.

> @@ +50,5 @@
> > +      this[prop] = mock[prop];
> > +    });
> > +  }
> > +  if (!this.signedInUserStorage) {
> > +    // Normal initialization.
> 
> I assume this.signedInUserStorage will only be non-null during tests?  If
> so, maybe change this comment to read: // Normal (ie, non-test)
> initialization

Ok

> @@ +74,5 @@
> > +    log.debug("== fetchAndUnwrapKeys");
> > +    return Task.spawn(function task() {
> > +      // Sign out if we don't have a key fetch token.
> > +      if (!keyFetchToken) {
> > +        yield internal.signOut();
> 
> please use |this| instead of |internal| everywhere inside this object.

Done

> @@ +75,5 @@
> > +    return Task.spawn(function task() {
> > +      // Sign out if we don't have a key fetch token.
> > +      if (!keyFetchToken) {
> > +        yield internal.signOut();
> > +        return;
> 
> it seems undefined will *always* be the value of the resolved promise.  In
> this case it should be null, and at the end of the function you should
> 'return data'.  To do this the function will need to be declared with
> "function* task()" - note the '*'.

It was very exciting to learn what 'function*' actually does.  Being able to yield *and* return from the same function is pretty nifty.  I think this is now implemented correctly.

> @@ +103,5 @@
> > +    // "audience" should look like "http://123done.org".
> > +    // The generated assertion will expire in two minutes.
> > +    jwcrypto.generateAssertion(cert, keyPair, audience, function(err, signed) {
> > +      if (err) {
> > +        d.reject(err);
> 
> log.error(...) probably makes sense here?  (or maybe just log.debug if this
> is going to be hit regularly - eg, after something expires normally)

Yes, this is uncommon enough that it should be deemed an error.  Updated.

> @@ +124,5 @@
> > +    let willBeValidUntil = internal.now() + CERT_LIFETIME;
> > +    return internal.getCertificateSigned(data.sessionToken,
> > +                                         keyPair.serializedPublicKey,
> > +                                         CERT_LIFETIME)
> > +      .then((cert) => {internal.cert = {cert: cert,
> 
> this indentation is weird, especially with the object creation on the first
> line - just start the function on its own line.

Better now?

> @@ +155,5 @@
> > +        d.reject(err);
> > +      } else {
> > +        log.debug(" getKeyPair got keypair");
> > +        internal.keyPair = { keyPair: kp,
> > +                          validUntil: willBeValidUntil };
> 
> strange indentation

Fixed

> @@ +177,5 @@
> > +          internal.signedInUser = user;
> > +        }
> > +        deferred.resolve(user ? user.accountData : undefined);
> > +      },
> > +      (err) => {deferred.resolve(undefined)}
> 
> is null a better choice?

I think so.  Updated.

> @@ +212,5 @@
> > +    return Date.now();
> > +  },
> > +
> > +  pollEmailStatus: function pollEmailStatus(sessionToken, why) {
> > +    log.debug(" entering pollEmailStatus (" + (why||"") + ")");
> 
> please add spaces around operators

Yes

> @@ +228,5 @@
> > +              delete internal.whenVerifiedPromise;
> > +            });
> > +        } else {
> > +          internal.pollTimeRemaining -= POLL_STEP;
> > +          if (internal.pollTimeRemaining > 0) {
> 
> if we stop polling, it looks like the promise will never be resolved.  It
> seems like it should maybe be rejected (although see my other concerns about
> the use of "static" promises).  Also, isn't this timer going to continue if
> the user changes?

Fixed to reject the promise if polling times out.  There is a test for this, too.

If the user changes, a new function |abortExistingFlow()| is called.  It cancels an existing timer, if there is one, and rejects the static promises with an error.

Additionally - and this is tricky, so I hope I've got it right - functions that want to touch storage check to see if if a new user hasn't signed in since they began running.  The |generationCount| variable is used for functions to see if they should still be running mid-execution.  If the count has changed during the function's execution, it will abort.

Something like this will need to be done for the FxAccountsClient.jsm as well, so we can cancel xhr requests mid-flight.  I'd propose pursuing that in a follow-up bug.

> @@ +260,5 @@
> > +  whenVerified: function(data) {
> > +    if (data.isVerified) {
> > +      return Promise.resolve(data);
> > +    }
> > +    if (!internal.whenVerifiedPromise) {
> 
> I don't understand the use of a "static" promise here.  Let's say userA logs
> in but isn't verified (and userA never actually verifies).  Then userB logs
> in and verifies - isn't userA going to have the promise resolved even though
> they haven't verified?  Maybe I'm missing something, but the tests don't
> seem to handle this (or any "multiple users") case.

This was a fantastic point and I am so glad you raised it.

We've decided to keep the static promises, but with the improvements noted above.  Also, note that the promises are never returned to outside callers - they are only for internal processes.

There is a test to ensure that the overlapping userA/userB case is handled correctly.

> A solution to this might be to reject and set to null all such promises on
> login/logout?

Ach! I forgot logout.  Updating the patch now ...

> @@ +287,5 @@
> > +    mocks = null;
> > +  }
> > +  internal = new InternalMethods(mocks);
> > +  if (mockInternal) { // not mocks
> > +    this.internal = internal;
> 
> this.internal doesn't seem used - why does it need to be set?

It is necessary for the tests, so that they can access the internal methods.

> @@ +303,5 @@
> > +  // was found to be verified in a previous boot: if our stored state says
> > +  // the account is verified, the event will never fire. So callers must do:
> > +  //   register notification observer (go)
> > +  //   userdata = get()
> > +  //   if (userdata.isVerified()) {go()}
> 
> If this pattern is common, I wonder if it would be useful to provide a
> helper function that returns a promise which is only resolved when verified?

It's not entirely clear to me what the best thing to do here is.

The main question is, if the user turned off the browser while we were polling for verification, what should we do when they turn it back on?

We could simply put the burden on the first caller of |getSignedInUser()|, though that would add needless delay for that app's sign-in.

It would be nice to push the polling task on a "nice" queue on browser start-up, so that it could be initiated when the dust had settled from browser start-up.  But I don't know if such a thing exists.  (Do you know?)

If it's acceptable to you, I'd like to solve this problem in a follow-up bug, engaging in particular with the sync engineers (:nalexander et al.).

> @@ +357,5 @@
> > +    return internal.getUserAccountData().then((data) => {
> > +      if (data.kA && data.kB) {
> > +        return Promise.resolve(data);
> > +      }
> > +      if (!internal.whenKeysReadyPromise) {
> 
> I don't think there is test coverage of this line and below - specifically,
> I think that fetchAndUnwrapKeys is always going to resolve with undefined -
> see above.

Quite right, and it was broken.  Fixed, and a test added.

> @@ +361,5 @@
> > +      if (!internal.whenKeysReadyPromise) {
> > +        internal.whenKeysReadyPromise = Promise.defer();
> > +        internal.fetchAndUnwrapKeys(data.keyFetchToken)
> > +          .then((data) => {
> > +            internal.whenKeysReadyPromise.resolve(data);
> 
> same problem here re static promises.  It looks like this is never reset, so
> getKeys() for a different user looks like it will be resolved immediately. 
> Also, should the promise be rejected if fetchAndUnwrapKeys fails?

I think this is fixed, too.  See comments above.

> @@ +398,5 @@
> > +      });
> > +  },
> > +
> > +  /**
> > +   * Return a BrowserID-compatible (?) assertion for use by RPs.
> 
> A comment should probably be added to say that if the user isn't verified
> this will return null, so the caller must do the "isVerified/addObserver"
> dance mentioned in the comments for this class.  (OTOH though, I don't
> really see why it should work like this, as it just adds pain to the
> consumer of the class - why not have the promise wait for verification?)

I'm torn here.  On the one hand, I agree, it would be nice for callers just to say |getAssertion()| and be done with it.  On the other hand, I'm not sure callers would always want UI to be triggered.  I'm also wary to return such a long-lived promise.

I've taken the easy way out and am returning null when there isn't a verified and signed-in user.

I believe the way sync will work (and sync will be the first user of this api) will be to |getSignedInUser()| before trying |getAssertion()|.  In that case, the present (wimpy) version of the api should be ok at least as a start.

Should we file a follow-up bug for the improved version?

> ::: services/fxaccounts/tests/xpcshell/test_accounts.js
> @@ +40,5 @@
> > +    assertion: "foobar",
> > +    sessionToken: "dead",
> > +    kA: "beef",
> > +    kB: "cafe",
> > +    isVerified: true
> 
> can we have some tests where verification comes in later?  This will also
> help test the use of promises and multiple users (eg, that after a user is
> set that doesn't ever verify, it isn't treated as being verified when a
> second user is, and similarly, if the first user does verify after a second
> user is logged in, we don't erroneously consider the second user as
> verified.)

I've added a test for verification polling.  I'm mocking the |FxAccountsClient| and at some point setting a flag on the mocked client to simulate the user's verification of his or her email.

We can't actually have the situation you describe where a first user begins to sign in, then a second user begins to sign in, then the first user verifies.  There can be only one user, and every |setSignedInUser()| call sets this user.

As you remarked earlier, it is conceivable that user A signs in, then user B, and then user B verifies.  In such a case, no promises or events should be fired for user A.  The test_overlapping_flows test shows that this works correctly.

> @@ +160,5 @@
> > +    wrapKB: expandBytes("22"),
> > +  });
> > +
> > +  yield a.setSignedInUser(creds);
> > +  yield a.getKeys();
> 
> The test should check it got what it expected - currently the value is
> undefined, which isn't what I'd expect.
> There should also be a test for the signed-out case, where (IIUC) null
> should be returned.

I believe this is now fixed and addressed in the tests.

Thank you again for all your comments and help!
Cheers,
Jed

Comment 60

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Mark Hammond [:markh] from comment #58)
> Comment on attachment 8343397 [details] [diff] [review]
> FxAccounts.jsm
> 
> Review of attachment 8343397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A quick drive-by, but this is looking awesome, thanks!

Thank you for your review!

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +110,5 @@
> > +   * Reset state such that any previous flow is canceled.
> > +   */
> > +  abortExistingFlow: function abortExistingFlow() {
> > +    if (this.currentTimer) {
> > +      log.warn("Surprised to find a pending setTimeout; Clearing it.");
> 
> is this really surprising?  Also, the fact you don't reset this.currentTimer
> in this block will not cause problems, but might cause this message to be
> printed when it's not actually pending.

Right on both counts.  Thanks for catching the failure to zero-out currentTimer.  Fixed.

> @@ +160,5 @@
> > +      if (!this.whenKeysReadyPromise) {
> > +        this.whenKeysReadyPromise = Promise.defer();
> > +        this.fetchAndUnwrapKeys(data.keyFetchToken)
> > +          .then((data) => {
> > +            this.whenKeysReadyPromise.resolve(data);
> 
> I wonder if you need to check if this.whenKeysReadyPromise is non null here
> (ie, ensure that abortExistingFlow wasn't called between creating the
> promise and fetchAndUnwrapKeys completing)?

Yes, I think we'd better.  Fixed.

> @@ +265,5 @@
> > +
> > +  getKeyPair: function(mustBeValidUntil) {
> > +    log.debug("getKeyPair");
> > +    if (this.keyPair) {
> > +      log.debug("keyPair valid until: " + this.keyPair.validUntil + " " + mustBeValidUntil);
> 
> not important, but note the log module allows an object as a second param
> that will be jsonified - so something like:
>   log.debug("getKeyPair:", {mustBeValidUntil: mustBeValidUntil, keyPair:
> keyPair});
> should generate useful output in all cases.

Oh neat.  I didn't know.  Thanks!

> @@ +279,5 @@
> > +      if (err) {
> > +        d.reject(err);
> > +      } else {
> > +        log.debug("getKeyPair got keypair");
> > +        this.keyPair = { 
> 
> trailing space here

Fixed

> @@ +306,5 @@
> > +          this.signedInUser = user;
> > +        }
> > +        deferred.resolve(user ? user.accountData : null);
> > +      },
> > +      (err) => {deferred.resolve(null)}
> 
> a little surprised the promise isn't rejected here, but there may be a good
> reason (ie, I'm fine with this if you are confident this is the most
> reasonable thing to do)

If the file does not exist (because setSignedInUser, which creates it, has not yet been called), we'll get an ENOENT error.  I've updated the code to resolve(null) if the file does not exist, and to reject(err) in all other cases.

> @@ +335,5 @@
> > +    // Get us to the verified state, then get the keys. This returns a
> > +    // promise that will fire when we are completely ready.
> > +    //
> > +    // Login is truly complete once keys have been fetched, so once
> > +    // getKeys() obtains and stores kA and kB, it will fire the 
> 
> trailing space here

Fixed.

> @@ +356,5 @@
> > +  },
> > +
> > +
> > +  notifyObservers: function(topic, data = null) {
> > +    log.debug("Notifying observers of user login: " + data); 
> 
> trailing space

Ach!  Sorry.

> @@ +381,5 @@
> > +      }
> > +      this.pollTimeRemaining = this.POLL_SESSION;
> > +    }
> > +
> > +    log.debug("entering pollEmailStatus (" + (why||"") + ") " + myGenerationCount);
> 
> ubernit, but still need spaces around '||'

Even better, because we depend on they 'why' argument, I've removed the '||' operator entirely :)

> @@ +407,5 @@
> > +              // XXX when we fire onlogin, we're saying that the
> > +              // user is verified AND that we have all the keys.
> > +              // sync can listen for this and know when to start up
> > +              // without having to inquire and poke around.
> > +              this.whenVerifiedPromise.resolve(data);
> 
> isn't there still an (unlikely) chance this.whenVerifiedPromise will have
> gone null while getUserAccountData completes?  It almost looks as though the
> generationCount check above isn't that useful - you could just charge ahead
> and abort if you find whenVerifiedPromise is null at the end (hrmph - maybe
> it's not that simple - you wouldn't want to call setUserAccountData in that
> case).
> 
> Anyway, just food-for-thought for now.

I think perhaps we need them both.  First, yes, we need to check that the promise is not null - thank you.  And second, is it not possible that the promise could have been nulled and recreated in the midst of the polling loop, escaping the notice of this check?  In which case, we benefit from having the generation counter.

> @@ +419,5 @@
> > +            this.currentTimer = setTimeout(() => {
> > +              this.pollEmailStatus(sessionToken, "timer")}, this.POLL_STEP);
> > +            log.debug("started timer " + this.currentTimer);
> > +          } else {
> > +            this.whenVerifiedPromise.reject(
> 
> ditto here - isn't there an unlikely chance whenVerifiedPromise might be
> null here?

Yes indeed, thank you.

> @@ +433,5 @@
> > +    return this.signedInUserStorage.get().then((record) => {
> > +      record.accountData = accountData;
> > +      this.signedInUser = record;
> > +      return this.signedInUserStorage.set(record)
> > +        .then(() => {return accountData});
> 
> up above in line 342 you have:
> 
>    .then((data) => this.getKeys(data));
> 
> If you were to follow the same style here, it looks like this line could
> read:
> 
>   .then(() => accountData);
> 
> (I'm not that bothered by this though)

Ah, but that's a nice catch.  Let's keep the style consistent.  Fixed.

> @@ +537,5 @@
> > +          // If the email is not verified, start polling for verification,
> > +          // but return null right away.  We don't want to return a promise
> > +          // that might not be fulfilled for a long time.
> > +          internal.startVerifiedCheck(credentials);
> > +          return data;
> 
> maybe delete this line and fall through?

Yes

Comment 61

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Filed follow-up bugs:

Bug 947056 - Server should be able to tell FxAccounts.jsm to back off or stop polling

Bug 947061 - Strategy for resuming email verification after browser restart

Comment 62

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxAccounts.jsm

Comment 63

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

Comment on attachment 8343495 [details] [diff] [review]
FxAccounts.jsm

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

LGTM, thanks!

::: services/fxaccounts/FxAccounts.jsm
@@ +266,5 @@
> +  },
> +
> +  getKeyPair: function(mustBeValidUntil) {
> +    log.debug("getKeyPair");
> +    if (this.keyPair) {

Seeing the logger will jsonify the params, I think the first log.debug can be dropped, and just uncondtionally log:

log.debug("keyPair", {
  validUntil: this.keyPair,
  mustBeValidUntil: mustBeValidUntil
});

unless there is something else in keyPair that is too sensitive to appear in logs.

@@ +395,5 @@
> +    }
> +
> +    this.checkEmailStatus(sessionToken)
> +      .then((response) => {
> +        log.debug("checkEmailStatus -> " + JSON.stringify(response));

just pass |response| as the second param

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +41,5 @@
> +    // simulate a call to /recovery_email/status
> +    let deferred = Promise.defer();
> +
> +    // it takes a little time for a network call to complete
> +    do_timeout(50, () => {

these look like a recipe for oranges on TBPL, but we can deal with that if it happens.

Comment 64

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Mark Hammond [:markh] from comment #63)
> Comment on attachment 8343495 [details] [diff] [review]
> FxAccounts.jsm

Mark, thanks again for such a speedy and helpful review.

> Review of attachment 8343495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, thanks!
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +266,5 @@
> > +  },
> > +
> > +  getKeyPair: function(mustBeValidUntil) {
> > +    log.debug("getKeyPair");
> > +    if (this.keyPair) {
> 
> Seeing the logger will jsonify the params, I think the first log.debug can
> be dropped, and just uncondtionally log:
> 
> log.debug("keyPair", {
>   validUntil: this.keyPair,
>   mustBeValidUntil: mustBeValidUntil
> });
> 
> unless there is something else in keyPair that is too sensitive to appear in
> logs.

I agree, we're making more noise than we need to here.  And actually, I don't think we even need to log any details of the keypair.  So I will simplify this.

> @@ +395,5 @@
> > +    }
> > +
> > +    this.checkEmailStatus(sessionToken)
> > +      .then((response) => {
> > +        log.debug("checkEmailStatus -> " + JSON.stringify(response));
> 
> just pass |response| as the second param

Ah yes.  Thanks.

> ::: services/fxaccounts/tests/xpcshell/test_accounts.js
> @@ +41,5 @@
> > +    // simulate a call to /recovery_email/status
> > +    let deferred = Promise.defer();
> > +
> > +    // it takes a little time for a network call to complete
> > +    do_timeout(50, () => {
> 
> these look like a recipe for oranges on TBPL, but we can deal with that if
> it happens.

You're right.  We don't need to pretend that we are experiencing real-world latency here.  We can resolve immediately.  Thanks for catching that.

Comment 65

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

When trying to |log.debug("string", object)|, I find that |object| never gets JSON.stringified - it just gets dropped.  Looking at the Log.jsm code, I don't immediately see what's going wrong.  It looks like what we're doing should work.  Only it doesn't work.

Spoke with :markh on IRC about this, and he has said we can go back to concatenating strings in the |log.debug()| calls and proceed with r+.

I will do that, since strings are still strings, and we are only trying to log some debug messages here.

Comment 66

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxAccounts.jsm.wip.patch

r=markh

Comment 67

[Ryan VanderMeulen [:RyanVM]]

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

Comment 68

[Ryan VanderMeulen [:RyanVM]]

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

Comment 69

[James Bonacci [:jbonacci]]

Nice. Another qa+ to add to our "must verify" pile...

Comment 71

[Tim Taubert [:ttaubert] (inactive)]

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