Closed Bug 1047133 Opened 10 years ago Closed 10 years ago

Persist FxA OAuth token between browser sessions

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
5

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Iteration:
35.3
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: MattN, Assigned: jaws)

References

()

Details

(Whiteboard: [loop-uplift])

User Story

As a user, I shouldn't have to log in to Loop again just because I restarted the browser.

Attachments

(2 files, 3 obsolete files)

It should be persisted somewhere on disk (e.g. password manager)
Flags: firefox-backlog+
Since existing Loop tokens are in preferences I guess we can do the same here too.

This bug should probably wait for bug 1047146 so that it can also update the UI at startup to reflect the logged-in state. (e.g. the username).
Points: --- → 5
Depends on: 1047146
Whiteboard: [qa+] → [qa+][not required for Fx34]
Flags: qe-verify+
Whiteboard: [qa+][not required for Fx34] → [not required for Fx34]
(In reply to Matthew N. [:MattN] from comment #1)
> Since existing Loop tokens are in preferences I guess we can do the same
> here too.

Preferences, or some form of storage, are probably better than password manager, as the password manager might sync the value to other clients, which might not be desirable/intended.
The profile information from bug 1047146 should also be persisted I think.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 35.2
QA Contact: anthony.s.hughes
Assignee: MattN+bmo → jaws
The main pieces of work I see are:
* persisting the OAuth tokens and maybe profile data to disk
* Verifying at startup that the OAuth token is still valid (by re-fetching the profile data from the server). If not, run the logout code.
Whiteboard: [not required for Fx34] → [we really want this for Fx34]
Whiteboard: [we really want this for Fx34] → [we really want this for Fx34][loop-uplift]
Depends on: 1071247
You may want/need to remove the hack from bug 1071259 in this patch or for testing but that hack may need to stay in the tree until after bug 1071247 is deployed to production.
I forgot that we need to re-register using the new push URL with the server so we would need something like:
1306       return gRegisteredDeferred.promise.then(Task.async(function*() {
1307         if (gPushHandler.pushUrl) {
1308           yield MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, gPushHandler.pushUrl);
1309         } else {
1310           throw new Error("No pushUrl for FxA registration");
1311         }
           }

This also achieves the following:
(In reply to Matthew N. [:MattN] from comment #4)
> * Verifying at startup that the OAuth token is still valid…
Priority: -- → P2
Attached patch WIP Patch (obsolete) — Splinter Review
I'm still getting 401-unauthorized when the call URL is requested, putting this up here to checkpoint.
Attachment #8495476 - Flags: feedback?(MattN+bmo)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Created attachment 8495476 [details] [diff] [review]
> WIP Patch
> 
> I'm still getting 401-unauthorized when the call URL is requested, putting
> this up here to checkpoint.

I don't know if the fix is on prod but you shouldn't see that on the dev server.
Comment on attachment 8495476 [details] [diff] [review]
WIP Patch

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

I'll try repro the 401 now.

::: browser/app/profile/firefox.js
@@ +1615,5 @@
>  pref("loop.feedback.product", "Loop");
>  pref("loop.debug.websocket", false);
>  pref("loop.debug.sdk", false);
> +pref("loop.fxa_oauth_tokendata", "");
> +pref("loop.fxa_oauth_profile", "");

Nit: Periods are a more common delimeter for namespacing.

::: browser/components/loop/MozLoopService.jsm
@@ +66,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "gDNSService",
>                                     "@mozilla.org/network/dns-service;1",
>                                     "nsIDNSService");
>  
> +function setJsonPref(aName, aValue) {

Nit: It seems more common to uppercase "JSON" like how gDNSService has upper case for the acronym and how OAuth has an uppercase "A".

@@ +307,5 @@
> +   *
> +   * @return {Object} OAuth token
> +   */
> +  get fxAOAuthTokenData() {
> +    return getJsonPref("loop.fxa_oauth_tokendata");

This is now causing a pref lookup every time it's accessed which isn't ideal but I heard people arguing that the pref service hash table is efficient enough so perhaps this is acceptable nowadays.

@@ +312,5 @@
> +  },
> +
> +  /** Sets MozLoopService Firefox Accounts OAuth token.
> +   *
> +   * @param {Object} OAuth token

You're missing the parameter name here

@@ +323,5 @@
> +   *
> +   * @return {Object} Profile data
> +   */
> +  get fxAOAuthProfile() {
> +    return getJsonPref("loop.fxa_oauth_profile");

We already have a MozLoopService.userProfile so I think this plus the setter should be folder into that.

@@ +328,5 @@
> +  },
> +
> +  /** Sets MozLoopService Firefox Accounts Profile data.
> +   *
> +   * @param {Object} Profile data

You're missing the parameter name here

@@ +993,5 @@
>    // Kick off the push notification service into registering after a timeout
>    // this ensures we're not doing too much straight after the browser's finished
>    // starting up.
>    gInitializeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  gInitializeTimer.initWithCallback(function* initializationCallback() {

Are you sure nsITimer supports a generator function as a callback? I think you may need Task.async(…) here.

@@ +998,5 @@
>      gInitializeTimer = null;
> +
> +    yield MozLoopService.register().then(Task.async(function*() {
> +      if (gPushHandler.pushUrl) {
> +        yield MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, gPushHandler.pushUrl);

I think this approach makes sense

@@ +1014,5 @@
> +      token: MozLoopServiceInternal.fxAOAuthTokenData.access_token,
> +    });
> +    client.fetchProfile().then(result => {
> +      MozLoopServiceInternal.fxAOAuthProfile = result;
> +      MozLoopServiceInternal.notifyStatusChanged();

It probably makes sense to refactor some of this this out of logInToFxA into a new function since logInToFxA is kinda large anyways. You can probably have an argument to control the arg that gets used with notifyStatusChanged. That also makes it easier to unit test the profile stuff with the new created helper function.

@@ +1358,5 @@
>     * @return {Promise} that resolves when the FxA login flow is complete.
>     */
>    logInToFxA: function() {
> +    if (MozLoopServiceInternal.fxAOAuthTokenData &&
> +        !!Services.prefs.getCharPref(this.getSessionTokenPrefName(LOOP_SESSION_TYPE.FXA))) {

What's a case where you can have one without the other after your patch? We need to make sure we don't accidentally mix and match hawk tokens with a different OAuth token. If we know of the specific cases, I would prefer we fix it so we null out the other when we get into that state e.g. after an error.
Attachment #8495476 - Flags: feedback?(MattN+bmo) → feedback+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Created attachment 8495476 [details] [diff] [review]
> WIP Patch
> 
> I'm still getting 401-unauthorized when the call URL is requested, putting
> this up here to checkpoint.

FTR: the issue was the workaround from bug 1071259.
Blocks: 1073141
Attached patch Patch (obsolete) — Splinter Review
As discussed over IRC, I didn't fold the setter in to the .userProfile property because I wanted to keep the setter internal.
Attachment #8495476 - Attachment is obsolete: true
Attachment #8496204 - Flags: review?(MattN+bmo)
Comment on attachment 8496204 [details] [diff] [review]
Patch

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

I think we need some tests for this change since it's important code.  I think we can do all the testing we want without Marionette since all of the state we need to modify is accessible to the test via JS and there's not so much of it to reset.

I think you should add (xpcshell [preferred] or bc) tests that cover the case of having an existing FxA session upon restart (e.g. with no push or loop registration: just clear those variables).

Since you're not doing in this bug (which is fine), can you file a follow-up that refreshes the profile information after some cache period?

::: browser/components/loop/MozLoopService.jsm
@@ +312,5 @@
>    urlExpiryTimeIsInFuture: function() {
>      return this.expiryTimeSeconds * 1000 > Date.now();
>    },
>  
> +  /** Retrieves MozLoopService Firefox Accounts OAuth token.

I've never seen multiline JSDoc comments starting on the same line as the opening tag before: "/**" and I prefer consistency.

@@ +1002,5 @@
>    // starting up.
>    gInitializeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  gInitializeTimer.initWithCallback(Task.async(function* initializationCallback() {
> +    yield MozLoopService.register().then(Task.async(function*() {
> +      if (!MozLoopService.fxAOAuthTokenData) {

After looking at this more, I found out that this function might not run at startup if the user never copied a call URL before (which then sets the expiry. See MozLoopService.initialize). If you login, clear the loop.urlsExpiryTimeSeconds pref and restart, you should see the problem. 

For scalability reasons we don't want to change this to register every user who doesn't open the panel or isn't logged in but we should do either of the following if the user is logged in via FxA on startup:
A) initialize the push handler  (preferred due to less server load)
or
B) do the guest registration first

@@ +1008,5 @@
> +        return;
> +      }
> +
> +      if (gPushHandler.pushUrl) {
> +        yield MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, gPushHandler.pushUrl);

Could you handle the |error.code === 401 && error.errno === INVALID_AUTH_TOKEN| case and call logout here otherwise you're going to get in the bad state where registerWithLoopServer cleared the session token pref but not the userProfile one (like we saw with the hack in nsBrowserGlue).

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +2,4 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  /**
>   * Test FxA logins with Loop.

Can you add to the tests so they sanity check the values of the two prefs you added? I already do this for loop.hawk-session-token.fxa at least once.

::: browser/components/nsBrowserGlue.js
@@ -743,5 @@
>      FormValidationHandler.uninit();
> -
> -    // XXX: Temporary hack to allow Loop FxA login after a restart to work.
> -    // Remove this once bug 1071247 is deployed.
> -    Services.prefs.clearUserPref("loop.hawk-session-token.fxa");

I'm fine with removing this in the same patch and duping the bug for removing this here but please test that your patch works fine for users who are using the production Loop server.
Attachment #8496204 - Flags: review?(MattN+bmo) → review-
Iteration: 35.2 → 35.3
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to Matthew N. [:MattN] from comment #12)
> I think we need some tests for this change since it's important code.  I
> think we can do all the testing we want without Marionette since all of the
> state we need to modify is accessible to the test via JS and there's not so
> much of it to reset.
> 
> I think you should add (xpcshell [preferred] or bc) tests that cover the
> case of having an existing FxA session upon restart (e.g. with no push or
> loop registration: just clear those variables).

Added tests.

> Since you're not doing in this bug (which is fine), can you file a follow-up
> that refreshes the profile information after some cache period?

Filed bug 1076426.
 
> @@ +1002,5 @@
> >    // starting up.
> >    gInitializeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +  gInitializeTimer.initWithCallback(Task.async(function* initializationCallback() {
> > +    yield MozLoopService.register().then(Task.async(function*() {
> > +      if (!MozLoopService.fxAOAuthTokenData) {
> 
> After looking at this more, I found out that this function might not run at
> startup if the user never copied a call URL before (which then sets the
> expiry. See MozLoopService.initialize). If you login, clear the
> loop.urlsExpiryTimeSeconds pref and restart, you should see the problem. 
> 
> For scalability reasons we don't want to change this to register every user
> who doesn't open the panel or isn't logged in but we should do either of the
> following if the user is logged in via FxA on startup:
> A) initialize the push handler  (preferred due to less server load)
> or
> B) do the guest registration first

After talking with you over video chat, we decided that option B should be the simplest route to go for the time being, and that we could file a follow-up bug to work on reducing the number of registrations for logged-in users.

Filed bug 1076428.
 
> ::: browser/components/nsBrowserGlue.js
> @@ -743,5 @@
> >      FormValidationHandler.uninit();
> > -
> > -    // XXX: Temporary hack to allow Loop FxA login after a restart to work.
> > -    // Remove this once bug 1071247 is deployed.
> > -    Services.prefs.clearUserPref("loop.hawk-session-token.fxa");
> 
> I'm fine with removing this in the same patch and duping the bug for
> removing this here but please test that your patch works fine for users who
> are using the production Loop server.

I worked around this by adding a new hidden pref, loop.autologin-after-restart. If `loop.autologin-after-restart` is created as a Boolean pref and set to True, then the hawk-session-token will not be cleared on shutdown. This way QA and developers can use and test this patch, but people using the production server will be unaffected.
Attachment #8496204 - Attachment is obsolete: true
Attachment #8498568 - Flags: review?(MattN+bmo)
Depends on: 1076428
Comment on attachment 8498568 [details] [diff] [review]
Patch v2

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

Really close but I'd like to take another look.

::: browser/components/loop/MozLoopService.jsm
@@ +327,5 @@
> +   *
> +   * @param {Object} aTokenData OAuth token
> +   */
> +  set fxAOAuthTokenData(aTokenData) {
> +    setJSONPref("loop.fxa_oauth.tokendata", aTokenData);

It may be a good idea to have this setter also clear fxAOAuthProfile if this gets cleared since in the initialize timer callback you only clear MozLoopServiceInternal.fxAOAuthProfile in one case when I think you normally should clear both if you clear the token data. Do you think that's too easy that it becomes a footgun? We'd have to make sure it doesn't break tests that intentionally try to set these prefs in a mismatched state.

@@ +584,5 @@
>  
>          // XXX Bubble the precise details up to the UI somehow (bug 1013248).
>          log.error("Failed to register with the loop server. Error: ", error);
>          this.setError("registration", error);
> +        gRegisteredDeferred.reject(error.errno);

I think this should just be |error| to pass along all the info an in case it ends up getting passed to setError again later.

@@ +1013,5 @@
> +      }
> +
> +      if (gPushHandler.pushUrl) {
> +        log.debug("MozLoopService: Initializing with already logged-in account");
> +        let registeredPromise = MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, gPushHandler.pushUrl);

Wrap this line to 100

@@ +1014,5 @@
> +
> +      if (gPushHandler.pushUrl) {
> +        log.debug("MozLoopService: Initializing with already logged-in account");
> +        let registeredPromise = MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, gPushHandler.pushUrl);
> +        yield registeredPromise.then(() => {

I don't think this yield is necessary and therefore the function* on line 1008 can be a normal function without Task.async.

I think this whole gInitializeTimer callback could be cleaner since it's currently mixing generator patterns with Promise.then(…) code flow. I think you may want to use try…catch around |var foo = somePromise;| instead of |.then| to help.

@@ +1018,5 @@
> +        yield registeredPromise.then(() => {
> +          initializedPromise.resolve();
> +        }, Task.async(function*(error) {
> +          if (error.code === 401 && error.errno === INVALID_AUTH_TOKEN) {
> +            yield MozLoopService.logOutFromFxA();

This is going to be handled by bug 1047164 but you should remove the errno check per bug 1047164. Whoever lands last should remove this.

@@ +1023,5 @@
> +          }
> +          initializedPromise.reject(error);
> +        }));
> +      } else {
> +        MozLoopServiceInternal.fxAOAuthProfile = null;

Why only clear this and not a full logout? Should we clear this at all? I think we could be fine not clearing this as long as we have an error in gErrors that makes sense.

@@ +1027,5 @@
> +        MozLoopServiceInternal.fxAOAuthProfile = null;
> +        initializedPromise.reject("No pushUrl for FxA registration");
> +      }
> +    }), (error) => {
> +      initializedPromise.reject(error);

I think this rejection and the one two above should end up calling setError somewhere to the error is visible to the user.

@@ +1056,5 @@
>  
>      // Don't do anything if loop is not enabled.
>      if (!Services.prefs.getBoolPref("loop.enabled") ||
>          Services.prefs.getBoolPref("loop.throttled")) {
>        return;

This should return a promise to match the new return value.

@@ +1062,5 @@
>  
> +    let initializedPromise = Promise.defer();
> +
> +    if (!MozLoopServiceInternal.fxAOAuthTokenData) {
> +      MozLoopServiceInternal.fxAOAuthProfile = null;

Perhaps you did this in an earlier version and I'm changing my mind but it seems like we could have the fxAOAuthProfile getter return null if !MozLoopServiceInternal.fxAOAuthTokenData instead of doing this.

@@ +1071,5 @@
> +    if (MozLoopServiceInternal.urlExpiryTimeIsInFuture() ||
> +        MozLoopServiceInternal.fxAOAuthTokenData) {
> +      gInitializeTimerFunc(initializedPromise, mockPushHandler, mockWebSocket);
> +    } else {
> +      initializedPromise.resolve();

I wonder if the resolution value should be something like "registration not needed" to make it easier for debugging why a registration didn't happen.

::: browser/components/loop/test/xpcshell/test_loopservice_restart.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

Can you double-check that we already have a test that makes sure we don't do a push registration if all call URLs expired and there is no FxA session? You may want to update it to test that the promise still resolves in that case.

@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Cu.import("resource://services-common/utils.js");

I think this is unused since I don't see references to CommonUtils but correct me if I'm wrong.

@@ +3,5 @@
> +
> +Cu.import("resource://services-common/utils.js");
> +
> +const fakeFxaTokenData = '{"token_type":"bearer","access_token":"1bad3e44b12f77a88fe09f016f6a37c42e40f974bc7a8b432bb0d2f0e37e1752","scope":"profile"}';
> +const fakeFxaProfile = '{"email":"test@example.com","uid":"999999994d9f4b08a2cbfc0999999999","avatar":null}';

Nit: Uppercase const names

@@ +17,5 @@
> +  Services.prefs.setCharPref(LOOP_FXA_PROFILE_PREF, fakeFxaProfile);
> +
> +  MozLoopService.initialize(mockPushHandler).then(() => {
> +    Assert.equal(Services.prefs.getCharPref(LOOP_FXA_PROFILE_PREF), "",
> +                 "FXA profile pref should be cleared if there is no token");

Also check gPushHandler has a URL

@@ +21,5 @@
> +                 "FXA profile pref should be cleared if there is no token");
> +  }, (error) => {
> +    do_throw("Initializing without a token should fulfill the promise");
> +  }).then(() => {
> +    run_next_test();

add_task and using yield would make this easier to read

@@ +32,5 @@
> +
> +  let guestRegistrationComplete = false;
> +  loopServer.registerPathHandler("/registration", (request, response) => {
> +    if (!guestRegistrationComplete) {
> +      guestRegistrationComplete = true;

Can you swap the order of the two bodies to avoid the negation here?

@@ +41,5 @@
> +        code: 401,
> +        errno: 110,
> +        error: {
> +          error: "Unauthorized",
> +          message: "Unknown credentials",

Have you actually seen this format returned from the server? My understanding is that the server returns a JSON object with 4 properties and the hawk code changes the format a bit. See this example from the OAuth server (which may differ from the Loop server for some reason): https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md#errors

@@ +46,5 @@
> +          statusCode: 401
> +        }
> +      }));
> +    }
> +    response.processAsync();

I don't think the processAsync is needed since there isn't anything async about this. I noticed some other file had the same thing so maybe this was copied.

@@ +51,5 @@
> +    response.finish();
> +  });
> +
> +  MozLoopService.initialize(mockPushHandler).then(() => {
> +    do_throw("Initializing with an invalid token should fulfill the promise");

I think this text is incorrect

@@ +68,5 @@
> +  Services.prefs.setCharPref(LOOP_FXA_PROFILE_PREF, fakeFxaProfile);
> +  Services.prefs.setCharPref(LOOP_FXA_TOKEN_PREF, fakeFxaTokenData);
> +  loopServer.registerPathHandler("/registration", (request, response) => {
> +    response.setStatusLine(null, 200, "OK");
> +    response.processAsync();

Same here about processAsync

@@ +76,5 @@
> +  MozLoopService.initialize(mockPushHandler).then(() => {
> +    Assert.equal(Services.prefs.getCharPref(LOOP_FXA_TOKEN_PREF), fakeFxaTokenData,
> +                 "FXA pref should still be set after initialization");
> +    Assert.equal(Services.prefs.getCharPref(LOOP_FXA_PROFILE_PREF), fakeFxaProfile,
> +                 "FXA profile should still be set after initialization");

Also check gPushHandler has a URL

@@ +82,5 @@
> +  });
> +});
> +
> +function run_test()
> +{

Nit: brace on the same line to match usual JS style and the rest of the file.

@@ +84,5 @@
> +
> +function run_test()
> +{
> +  setupFakeLoopServer();
> +  Services.prefs.setIntPref("loop.initialDelay", 0);

Nit: Maybe make a note that this is just to speed up the test.
Attachment #8498568 - Flags: review?(MattN+bmo) → feedback+
We need this.  We'll likely build bug 1077996 on top of this.
Priority: P2 → P1
Attached patch Patch v3Splinter Review
(In reply to Matthew N. [:MattN] from comment #14)
> ::: browser/components/loop/MozLoopService.jsm
> @@ +327,5 @@
> > +   *
> > +   * @param {Object} aTokenData OAuth token
> > +   */
> > +  set fxAOAuthTokenData(aTokenData) {
> > +    setJSONPref("loop.fxa_oauth.tokendata", aTokenData);
> 
> It may be a good idea to have this setter also clear fxAOAuthProfile if this
> gets cleared since in the initialize timer callback you only clear
> MozLoopServiceInternal.fxAOAuthProfile in one case when I think you normally
> should clear both if you clear the token data. Do you think that's too easy
> that it becomes a footgun? We'd have to make sure it doesn't break tests
> that intentionally try to set these prefs in a mismatched state.

I dilly-dallied with this for a while, but the profile data is entirely
dependent on the token data, so I went ahead and made the setter for the
token data clear the profile data if the token data was being cleared. I
also made the profile data getter return null if the token data was null.
The latter case shouldn't happen in practice (profile data present without
token data) but some Nightly profiles may already be in this state and
it feels good to have this symmetrical.

> @@ +1014,5 @@
> > +
> > +      if (gPushHandler.pushUrl) {
> > +        log.debug("MozLoopService: Initializing with already logged-in account");
> > +        let registeredPromise = MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, gPushHandler.pushUrl);
> > +        yield registeredPromise.then(() => {
> 
> I don't think this yield is necessary and therefore the function* on line
> 1008 can be a normal function without Task.async.
> 
> I think this whole gInitializeTimer callback could be cleaner since it's
> currently mixing generator patterns with Promise.then(…) code flow. I think
> you may want to use try…catch around |var foo = somePromise;| instead of
> |.then| to help.

I tried to make this cleaner in this new patch, but I'm not sure what
more could be done here. Callback-hell has been replaced with promise-hell.

> @@ +1023,5 @@
> > +          }
> > +          initializedPromise.reject(error);
> > +        }));
> > +      } else {
> > +        MozLoopServiceInternal.fxAOAuthProfile = null;
> 
> Why only clear this and not a full logout? Should we clear this at all? I
> think we could be fine not clearing this as long as we have an error in
> gErrors that makes sense.

This now only sets an error and relies on hawkRequest to call logOutFromFxA.

> @@ +41,5 @@
> > +        code: 401,
> > +        errno: 110,
> > +        error: {
> > +          error: "Unauthorized",
> > +          message: "Unknown credentials",
> 
> Have you actually seen this format returned from the server? My
> understanding is that the server returns a JSON object with 4 properties and
> the hawk code changes the format a bit. See this example from the OAuth
> server (which may differ from the Loop server for some reason):
> https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md#errors

This was copied from http://hg.mozilla.org/mozilla-central/diff/a79533c6430c/browser/components/loop/test/xpcshell/test_loopservice_token_invalid.js
which I assumed to be correct. I've fixed this in the test_loopservice_token_invalid.js file now.

> @@ +46,5 @@
> > +          statusCode: 401
> > +        }
> > +      }));
> > +    }
> > +    response.processAsync();
> 
> I don't think the processAsync is needed since there isn't anything async
> about this. I noticed some other file had the same thing so maybe this was
> copied.

processAsync is needed, and it is used by all of the other Loop xpcshell
tests that use registerPathHandler. Without processAsync, the server encounters
an Internal Server Error. Switching to synchronous responses requires using
seizePower, not a big change or anything, but I'd rather that we maintain consistency with the tests.

Pushed to tryserver,
https://tbpl.mozilla.org/?tree=Try&rev=e02c141734c0
Attachment #8498568 - Attachment is obsolete: true
Attachment #8500885 - Flags: review?(MattN+bmo)
Depends on: 1079656
Comment on attachment 8500885 [details] [diff] [review]
Patch v3

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

I filed bug 1079656 for the Account option not working after a restart. I have a patch with these changes that I will attach for your review and then you can fold them in with my r+.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> > @@ +1014,5 @@
> > > +
> > > +      if (gPushHandler.pushUrl) {
> > > +        log.debug("MozLoopService: Initializing with already logged-in account");
> > > +        let registeredPromise = MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, gPushHandler.pushUrl);
> > > +        yield registeredPromise.then(() => {
> > 
> > I don't think this yield is necessary and therefore the function* on line
> > 1008 can be a normal function without Task.async.
> > 
> > I think this whole gInitializeTimer callback could be cleaner since it's
> > currently mixing generator patterns with Promise.then(…) code flow. I think
> > you may want to use try…catch around |var foo = somePromise;| instead of
> > |.then| to help.
> 
> I tried to make this cleaner in this new patch, but I'm not sure what
> more could be done here. Callback-hell has been replaced with promise-hell.

Sorry, I meant to say |var foo = yield somePromise;| but that doesn't matter anymore.

> > @@ +46,5 @@
> > > +          statusCode: 401
> > > +        }
> > > +      }));
> > > +    }
> > > +    response.processAsync();
> > 
> > I don't think the processAsync is needed since there isn't anything async
> > about this. I noticed some other file had the same thing so maybe this was
> > copied.
> 
> processAsync is needed, and it is used by all of the other Loop xpcshell
> tests that use registerPathHandler. Without processAsync, the server
> encounters
> an Internal Server Error. Switching to synchronous responses requires using
> seizePower, not a big change or anything, but I'd rather that we maintain
> consistency with the tests.

I already didn't use processAsync in a test that I wrote since I knew that it wasn't needed. I'm not sure what you're seeing but it works fine for me without processAsync and finish.

::: browser/components/loop/MozLoopService.jsm
@@ +653,5 @@
>          }
>  
>          log.error("Failed to register with the loop server. Error: ", error);
>          this.setError("registration", error);
> +        gRegisteredDeferred.reject(error);

I think you should also do gRegisteredDeferred = null; to match other places unless there is a good reason not to. I see that onPushRegistered then would try to reject the nulled gRegisteredDeferred so I wrapped it in an if.

@@ +1087,5 @@
> +        initializedPromise.resolve("initialized to guest status");
> +        return;
> +      }
> +
> +      if (gPushHandler.pushUrl) {

Inverting this an using an early return reduces nesting. I don't even think this is possible here so it should be removed. See https://mxr.mozilla.org/mozilla-central/source/browser/components/loop/MozLoopPushHandler.jsm?rev=8b4d28f1b874#173 The one existing line in this file which checks .pushUrl isn't done after a registration so it does need the check.

@@ +1096,5 @@
> +        registeredPromise.then(() => {
> +          initializedPromise.resolve("initialized to logged-in status");
> +        }, error => {
> +          log.debug("MozLoopService: error logging in using cached auth token");
> +          MozLoopServiceInternal.setError("invalidtoken", {});

Where is this cleared? I think you should be using "login" here as that's what's used for registering with LOOP_SESSION_TYPE.FXA and then it will get cleared upon manual re-login.

Also, setError should be getting called with hawk errors, not empty objects. Why aren't you passing |error|?

I'm also confused because initializedPromise rejection handler also calls setError. Perhaps that should be removed so the individual rejection sites can use a more specific errorType.

@@ +1100,5 @@
> +          MozLoopServiceInternal.setError("invalidtoken", {});
> +          initializedPromise.reject(error);
> +        });
> +      } else {
> +        MozLoopServiceInternal.setError("initialization", {});

Wouldn't "registration" be better since this means the push registration didn't work? You never clear this error either which may be fine for this one since we don't have a way for the user to recover.

@@ +1139,3 @@
>      }
>  
> +    let initializedPromise = Promise.defer();

This is a Deferred, not a Promise so the variable name here and in gInitializeTimerFunc should change to deferredInitialization or something like that.

@@ +1145,5 @@
> +    if (MozLoopServiceInternal.urlExpiryTimeIsInFuture() ||
> +        MozLoopServiceInternal.fxAOAuthTokenData) {
> +      gInitializeTimerFunc(initializedPromise, mockPushHandler, mockWebSocket);
> +    } else {
> +      initializedPromise.resolve("registration not needed");

This could be done with an early return instead:

    // If expiresTime is not in the future and the user hasn't
    // previously authenticated then skip registration.
    if (!MozLoopServiceInternal.urlExpiryTimeIsInFuture() &&
        !MozLoopServiceInternal.fxAOAuthTokenData) {
      return Promise.resolve("registration not needed");
    }

@@ +1150,3 @@
>      }
> +
> +    initializedPromise.promise.then(() => {}, error => {

This is what |.catch| is for.

@@ +1152,5 @@
> +    initializedPromise.promise.then(() => {}, error => {
> +      MozLoopServiceInternal.setError("initialization", error);
> +    });
> +
> +    return initializedPromise.promise;

I think this should return the promise returned by .catch instead so the setError happens before other code using this return value.

::: browser/components/loop/test/xpcshell/test_loopservice_busy.js
@@ +102,5 @@
>      // Revert original Chat.open implementation
>      Chat.open = openChatOrig;
>  
>      // Revert fake login state
> +    MozLoopServiceInternal.fxAOAuthTokenData = null;

This is less confusing if it's fxAOAuthProfile since people may not realize fxAOAuthTokenData also clears fxAOAuthProfile.

::: browser/components/loop/test/xpcshell/test_loopservice_restart.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const FAKE_FXA_TOKEN_DATA = '{"token_type":"bearer","access_token":"1bad3e44b12f77a88fe09f016f6a37c42e40f974bc7a8b432bb0d2f0e37e1752","scope":"profile"}';
> +const FAKE_FXA_PROFILE = '{"email":"test@example.com","uid":"999999994d9f4b08a2cbfc0999999999","avatar":null}';

Nit: line more than 100 characters.

@@ +106,5 @@
> +  });
> +
> +  run_next_test();
> +};
> + 
\ No newline at end of file

Nit: whitespace
Attachment #8500885 - Flags: review?(MattN+bmo) → review+
Attached patch v.3 Review fixesSplinter Review
r=me on v.3 with these folded in.
Attachment #8501557 - Flags: feedback?(jaws)
Comment on attachment 8501557 [details] [diff] [review]
v.3 Review fixes

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

::: browser/components/loop/MozLoopService.jsm
@@ +1173,5 @@
>  
>      if (Services.prefs.getPrefType("loop.fxa.enabled") == Services.prefs.PREF_BOOL) {
>        gFxAEnabled = Services.prefs.getBoolPref("loop.fxa.enabled");
>        if (!gFxAEnabled) {
>          this.logOutFromFxA();

This should probably be `return this.logOutFromFxA();` now that this function returns a promise.
Attachment #8501557 - Flags: feedback?(jaws) → feedback+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Comment on attachment 8501557 [details] [diff] [review]
> v.3 Review fixes
> 
> Review of attachment 8501557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +1173,5 @@
> >  
> >      if (Services.prefs.getPrefType("loop.fxa.enabled") == Services.prefs.PREF_BOOL) {
> >        gFxAEnabled = Services.prefs.getBoolPref("loop.fxa.enabled");
> >        if (!gFxAEnabled) {
> >          this.logOutFromFxA();
> 
> This should probably be `return this.logOutFromFxA();` now that this
> function returns a promise.

We don't want to return in that case but we should be yielding on the logout otherwise the fxAOAuthTokenData check below may get the old login data that logOutFromFxA will delete.
https://hg.mozilla.org/integration/fx-team/rev/b2afd3b9c1b4
Flags: in-testsuite+
Whiteboard: [we really want this for Fx34][loop-uplift] → [we really want this for Fx34][loop-uplift][fixed in fx-team]
Whiteboard: [we really want this for Fx34][loop-uplift][fixed in fx-team] → [behind pref loop.autologin-after-restart until bug 1071272][loop-uplift][fixed in fx-team]
https://hg.mozilla.org/mozilla-central/rev/b2afd3b9c1b4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [behind pref loop.autologin-after-restart until bug 1071272][loop-uplift][fixed in fx-team] → [behind pref loop.autologin-after-restart until bug 1071272][loop-uplift]
Target Milestone: --- → mozilla35
Paul, assuming you get a new Nightly build in time, can you please verify this is fixed?
Flags: needinfo?(paul.silaghi)
Comment on attachment 8500885 [details] [diff] [review]
Patch v3

Approval Request Comment
Part of the staged Loop aurora second uplift set
Attachment #8500885 - Flags: approval-mozilla-aurora?
Verified fixed 35.0a1 (2014-10-10) Win 7, Ubuntu 13.04, OS X 10.9.5
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
How about Private Browsing? Do we really want to keep signed-in a user who signs-in in Private mode ?
(In reply to Paul Silaghi, QA [:pauly] from comment #27)
> How about Private Browsing? Do we really want to keep signed-in a user who
> signs-in in Private mode ?

Yes, this came up during privacy review and we were told to not worry about it for now and that we will work on a better solution in the future.
Flags: needinfo?(paul.silaghi)
(In reply to Paul Silaghi, QA [:pauly] from comment #27)
> How about Private Browsing? Do we really want to keep signed-in a user who
> signs-in in Private mode ?

See bug 1005019.
Whiteboard: [behind pref loop.autologin-after-restart until bug 1071272][loop-uplift] → [loop-uplift]
Verified fixed FF 34b1 Win 7
Flags: needinfo?(paul.silaghi)
Comment on attachment 8500885 [details] [diff] [review]
Patch v3

Already landed in aurora, approving for posterity
Attachment #8500885 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.