1139657 - Expose a method on FxAccounts for retrieving profile information

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Zachary Carter [:zaach]]

The FxAccounts module should expose a way for consumers to retrieve profile information for the currently signed in account. Currently the only useful data is the profile image but this should expand to include a display name soon.

Comment 1

[Zachary Carter [:zaach]]

Attachment: Expose a method on FxAccounts for retrieving profile information

Comment 2

[Zachary Carter [:zaach]]

Attachment: Expose a method on FxAccounts for retrieving profile information

Comment 3

[Zachary Carter [:zaach]]

This should use the OAuth token caching (currently being discussed on IRC) once that's ready. Right now it  requests a new one per-browser session, per-signed-in account.

Comment 4

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

Comment on attachment 8572907 [details] [diff] [review]
Expose a method on FxAccounts for retrieving profile information

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

We discussed this a little on IRC, but I think some of this is too focused on the profileImage rather than the entire profile.  Eg, we will eventually want to get stuff like "display name" from the profile, and we'll probably want a cache-and-validate strategy for the profile in it's entirety - and the profileImage URL should fall naturally from that.  Something like:

* cache the entire profile.
* as soon as the profile is requested use the cached data, but also...
* ... unconditionally re-fetch the profile and unconditionally issue a profile-changed notification. Later, we'd change this to "issue a conditional request for new profile info using an etag and only issue the profile-changed notification if it actually is modified.
* expand the profile change notification to simply be "something in the profile changed" and not that just the image changed (ie, if the user changed their display name and avatar, both should obviously update)

This means at startup there is a risk of stale info being used for a few seconds, but this is also going to happen when Fx is already running when the profile changes - there'll be some period when the stale data is in the UI, but eventually the profile-change notification will correct it.

The end result is that the patch should probably just treat the entire profile object as a JSON blob with no special casing of the image URL.

Zaach, does that sound like what we discussed and sound OK to you?

::: services/fxaccounts/FxAccounts.jsm
@@ +221,5 @@
> +      .then(() => this.profile.getProfileImageUrl());
> +  },
> +
> +  // Instantiate a FxAccountsProfile with a fresh OAuth token if needed
> +  initProfile: function (options) {

it looks like this should be done like whenKeysReadyDeferred so that we don't end up with 2 in-flight promises when the profile is requested.

@@ +233,5 @@
> +      scope: "profile:avatar"
> +    };
> +
> +    if (this.profile) {
> +      return this.resolve(this.profile);

Similarly, I wonder if exposing |options| here makes sense - what happens if this is called twice with different options?  It looks like the second call would ignore the options and just reuse the profile fetched in the previous call with different options.  (Or if it just for testing, maybe add a comment saying that - I haven't got to the tests yet ;)

::: services/fxaccounts/FxAccountsProfile.jsm
@@ +1,4 @@
> +/* 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/. */
> +

I should have remembered to say this before (but I always forget for new modules I create too), but "use strict;" should be added.

@@ +51,5 @@
> +FxAccountsProfile.prototype = {
> +  _Request: RESTRequest,
> +
> +  tearDown: function () {
> +    this.stopListeningForProfileChanges();

I wonder if we should just make this listener a "singleton" to avoid the teardown complexity and the risk of multiple channels if a teardown is missed?  IIUC, this listener just sends an observer notification and the code observing will still need to do the "get new profile info" dance, so I don't think it's that big of a deal if the profile listener stays alive even after the user logs out - listeners for the observer will either have stopped listening, or abort early due to the lack of a signedInUser.

@@ +57,5 @@
> +    this.client = null;
> +  },
> +
> +  // Make a HEAD request to see if the image is available
> +  isImageAvailable: function (url) {

it would probably be helpful to use a leading underscore on "internal" methods, to help make it clearer what are implementation details and what is the actual interface.

@@ +102,5 @@
> +
> +  // Initialize a profile channel to listen for account changes.
> +  listenForProfileChanges: function () {
> +    let contentUri = this.contentUrl;
> +    debugger;

kill this

Comment 5

[Zachary Carter [:zaach]]

Attachment: Expose a method on FxAccounts for retrieving profile information

Comment 6

[Zachary Carter [:zaach]]

Attachment: Expose a method on FxAccounts for retrieving profile information

Comment 7

[Zachary Carter [:zaach]]

Attachment: Expose a method on FxAccounts for retrieving profile information

Fixed a failing test.

Comment 8

[Zachary Carter [:zaach]]

Attachment: Expose a method on FxAccounts for retrieving profile information

Comment 9

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

Comment on attachment 8574395 [details] [diff] [review]
Expose a method on FxAccounts for retrieving profile information

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

I think this looks good with the requested changes.

::: services/fxaccounts/FxAccounts.jsm
@@ +240,5 @@
> +      return this.resolve(this.profile);
> +    }
> +
> +    if (!this.initProfileDeferred) {
> +      this.initProfileDeferred = this.fxaInternal.getOAuthToken(oAuthOptions)

I think initProfileDeferred is a promise rather than a deferred, so this.initProfileDeferred.reject() above will fail.  It sucks this module is using deprecated "deferred" objects, but it should be possible to use PromiseUtils.jsm to create and use a deferred object here which chains to the real promise.

But thinking some more about this, I don't think we we need this.initProfileDeferred at all (and yeah, I know I asked for it :)  whenVerifiedDeferred etc are used outside the AccountState but this isn't - so I actually think we could just use a promise and the tweak below would make it safe.

(ie, just have this.initProfilePromise be a regular promise (which it already is in your patch) and skip the attempt to reject it in abort()), and use accountState.resolve below.)

@@ +1090,5 @@
> +          let profile = JSON.parse(JSON.stringify(profileData));
> +          // profileData doesn't include "verified", but it must be true
> +          // if we've gotten this far.
> +          profile.verified = true;
> +          return profile;

I think simply doing return accountState.resolve(profile) here will protect all we need (ie, cause rejection if the signed in user changed before this completed) and allow us to avoid needing a deferred object above.

@@ +1098,5 @@
> +
> +          return this.getSignedInUser().then(data => {
> +            // If we fail to fetch the profile and have no profile cached
> +            // we resort to using the account data for basic profile data.
> +            return {

and here too (although I wonder how useful this fallback is - the caller must already have access to the signed in user data, so I'd think it better they fallback if it makes sense in their context) - but I'm not too bothered if you think this is better.

::: services/fxaccounts/FxAccountsProfile.jsm
@@ +109,5 @@
> +  // Cache fetched data if it is different from what's in the cache.
> +  // Send out a notification if it has changed so that UI can update.
> +  _cacheProfile: function (profileData) {
> +    let currentState = this.currentAccountState;
> +    return currentState.getUserAccountData()

It seems there may be a race when the user has logged out while the profile data is being fetched - ie, that currentState will be null here.  Simply early-returning here should be fine, and the other requested change in FxAccounts.jsm (ie, to use accountState.resolve()) would mean this is safe.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +899,5 @@
> +    accountState.profile = mockProfile;
> +
> +    accountState.getProfile()
> +      .then(result => {
> +         do_check_true(!! result);

Please remove the space after the operators

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf60b84ee258
for xpcshell failures.  (See commit message of backout.)

Comment 11

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=8028276
repository: mozilla-inbound
start_time: 2015-03-24T20:17:43
who: dbaron[at]dbaron[dot]org
machine: tst-linux64-spot-1169
buildname: b2g_emulator_vm mozilla-inbound opt test xpcshell
revision: cd1ef82e74a4

TEST-UNEXPECTED-TIMEOUT | services/fxaccounts/tests/xpcshell/test_accounts.js | Test timed out
DMError: Timeout exceeded for shell call
TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
Automation Error: Exception caught while running tests
Return code: 1

Comment 12

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=8030329
repository: mozilla-inbound
start_time: 2015-03-24T20:19:37
who: dbaron[at]dbaron[dot]org
machine: tst-linux64-spot-323
buildname: b2g_emulator_vm mozilla-inbound opt test xpcshell
revision: 3779bb7888d3

TEST-UNEXPECTED-TIMEOUT | services/fxaccounts/tests/xpcshell/test_accounts.js | Test timed out
DMError: Timeout exceeded for shell call
TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
Automation Error: Exception caught while running tests
Return code: 1

Comment 13

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=8018983
repository: mozilla-inbound
start_time: 2015-03-24T17:47:40
who: dbaron[at]dbaron[dot]org
machine: tst-linux64-spot-182
buildname: b2g_emulator_vm mozilla-inbound opt test xpcshell
revision: ad9f2c49d807

TEST-UNEXPECTED-TIMEOUT | services/fxaccounts/tests/xpcshell/test_accounts.js | Test timed out
DMError: Timeout exceeded for shell call
TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
Automation Error: Exception caught while running tests
Return code: 1

Comment 14

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=8021292
repository: mozilla-inbound
start_time: 2015-03-24T17:25:01
who: dbaron[at]dbaron[dot]org
machine: tst-linux64-spot-020
buildname: b2g_emulator_vm mozilla-inbound opt test xpcshell
revision: 9dce1a90205e

TEST-UNEXPECTED-TIMEOUT | services/fxaccounts/tests/xpcshell/test_accounts.js | Test timed out
DMError: Timeout exceeded for shell call
TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
Automation Error: Exception caught while running tests
Return code: 1

Comment 15

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=8026813
repository: mozilla-inbound
start_time: 2015-03-24T19:46:49
who: dbaron[at]dbaron[dot]org
machine: tst-linux64-spot-1056
buildname: b2g_emulator_vm mozilla-inbound opt test xpcshell
revision: 43e18ef66807

TEST-UNEXPECTED-TIMEOUT | services/fxaccounts/tests/xpcshell/test_accounts.js | Test timed out
DMError: Timeout exceeded for shell call
TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
Automation Error: Exception caught while running tests
Return code: 1

Comment 16

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=8027721
repository: mozilla-inbound
start_time: 2015-03-24T20:08:05
who: dbaron[at]dbaron[dot]org
machine: tst-linux64-spot-108
buildname: b2g_emulator_vm mozilla-inbound opt test xpcshell
revision: 43ef69c1f6b5

TEST-UNEXPECTED-TIMEOUT | services/fxaccounts/tests/xpcshell/test_accounts.js | Test timed out
DMError: Timeout exceeded for shell call
TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
Automation Error: Exception caught while running tests
Return code: 1

Comment 17

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=8028615
repository: mozilla-inbound
start_time: 2015-03-24T20:22:56
who: dbaron[at]dbaron[dot]org
machine: tst-linux64-spot-1521
buildname: b2g_emulator_vm mozilla-inbound opt test xpcshell
revision: a6619aed2c9d

TEST-UNEXPECTED-TIMEOUT | services/fxaccounts/tests/xpcshell/test_accounts.js | Test timed out
DMError: Timeout exceeded for shell call
TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
Automation Error: Exception caught while running tests
Return code: 1

Comment 18

[Zachary Carter [:zaach]]

Hm. That error didn't show up in my try build from Monday: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78255b2ea4b2

I've started new one here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae148cd0901c

Comment 19

[Zachary Carter [:zaach]]

markh helped me spot the failure. Fixed in the latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aa76ca13b17

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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