1186348 - Don't have the FxA identity manager initialize as it is created.

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

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

Attachment: 0001-first-wip.patch

Back when we rushed the FxA identity in, we did lots of initialization as soon as the identity was created. This causes work to be done as soon as Sync initializes rather than as a Sync is happening, and even spins a nested event-loop while doing so.

Since then we've tweaked the Sync login stuff to suite us (eg, added ensureLoggedIn and unlockAndVerifyAuthState) so everything works delaying the login until it's actually needed. Doing so means we don't need to send our own "login failed" notification and generally makes things a little nicer. I'm currently motivated to do this by changes we are making to "weave:ui:login:error", which we currently send at a sub-optimal time.

Comment 1

[Chris Karlof [:ckarlof]]

Comment on attachment 8637077 [details] [diff] [review]
0001-first-wip.patch

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

Nice direction, but I'd like to understand the consequences of delaying identity initialization a little more.

::: browser/base/content/browser-syncui.js
@@ -248,5 @@
>      // a network/server error, don't show errors.  If it weren't for the legacy
>      // provider we could just check LOGIN_FAILED_LOGIN_REJECTED, but the legacy
>      // provider has states like LOGIN_FAILED_INVALID_PASSPHRASE which we
>      // probably do want to surface.
> -    if (Weave.Status.login == Weave.LOGIN_FAILED_NOT_READY ||

Is the case that we don't have the opportunity to have a login error until we're initialized, which ensureLoggedIn guarantees?

::: services/sync/modules/browserid_identity.js
@@ -121,5 @@
>    initialize: function() {
>      for (let topic of OBSERVER_TOPICS) {
>        Services.obs.addObserver(this, topic, false);
>      }
> -    return this.initializeWithCurrentIdentity();

Nice idea, but I'm wonder if the logged in state will surface properly in the UI. E.g., if the user opens the hamburger menu, then this might trigger initializeWithCurrentIdentity if it hasn't been initialized yet, correct?

@@ -624,5 @@
>          // this._shouldHaveSyncKeyBundle being true ensures everything that cares knows
>          // that there is no authentication dance still under way.
>          this._shouldHaveSyncKeyBundle = true;
>          Weave.Status.login = this._authFailureReason;
> -        Services.obs.notifyObservers(null, "weave:ui:login:error", null);

I thought this was needed because if we get an authentication error due to an FxA auth error, then nothing else will surface the user-visible error. Has something changed?

It's also curious we fire this in the case of network errors, which may explain why some people see the bar when resuming from sleep.

::: services/sync/modules/service.js
@@ -693,5 @@
> -    // If the identity isn't ready it  might not know the username...
> -    if (!this.identity.readyToAuthenticate) {
> -      this._log.info("Not ready to authenticate in verifyLogin.");
> -      this.status.login = LOGIN_FAILED_NOT_READY;
> -      return false;

Is it your assertion that this code is unreachable now that we call ensureLoggedIn before we call verifyLogin?

Comment 2

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

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

> ::: browser/base/content/browser-syncui.js
> @@ -248,5 @@
> >      // a network/server error, don't show errors.  If it weren't for the legacy
> >      // provider we could just check LOGIN_FAILED_LOGIN_REJECTED, but the legacy
> >      // provider has states like LOGIN_FAILED_INVALID_PASSPHRASE which we
> >      // probably do want to surface.
> > -    if (Weave.Status.login == Weave.LOGIN_FAILED_NOT_READY ||
> 
> Is the case that we don't have the opportunity to have a login error until
> we're initialized, which ensureLoggedIn guarantees?

Yep - LOGIN_FAILED_NOT_READY was a work-around before we had ensureLoggedIn() - that now waits until the login process is completed.

> ::: services/sync/modules/browserid_identity.js
> @@ -121,5 @@
> >    initialize: function() {
> >      for (let topic of OBSERVER_TOPICS) {
> >        Services.obs.addObserver(this, topic, false);
> >      }
> > -    return this.initializeWithCurrentIdentity();
> 
> Nice idea, but I'm wonder if the logged in state will surface properly in
> the UI. E.g., if the user opens the hamburger menu, then this might trigger
> initializeWithCurrentIdentity if it hasn't been initialized yet, correct?

I don't think so - the UI tends to use the FxA state rather than the Sync state. An observer notification from FxA (eg, a login) will trigger that initialize, but that's OK too - we've always supported the fact that an initialize may already have been done and this doesn't change that - it just makes it less likely that it has.

> @@ -624,5 @@
> >          // this._shouldHaveSyncKeyBundle being true ensures everything that cares knows
> >          // that there is no authentication dance still under way.
> >          this._shouldHaveSyncKeyBundle = true;
> >          Weave.Status.login = this._authFailureReason;
> > -        Services.obs.notifyObservers(null, "weave:ui:login:error", null);
> 
> I thought this was needed because if we get an authentication error due to
> an FxA auth error, then nothing else will surface the user-visible error.
> Has something changed?

Yep. This notification was necessary because Sync wasn't driving the login process. Now that Sync is, Sync itself will notice the failure during login and send that same notification itself. It is true that this patch will cause initialization to happen a little later, so the "needs reauth" UI may not show up until a little later as Sync initializes - but given that's only 10 seconds, that seems OK to me.

> It's also curious we fire this in the case of network errors, which may
> explain why some people see the bar when resuming from sleep.

The UI code that handles this notification ignores network errors. I'm fairly confident most of the error bars we saw before were not during login but an error during the Sync itself (eg, after waking from sleep we'd start Syncing with an old stale token and Sync itself would fire a "sync failed" notification rather than a "login failed" notification.)
> 
> ::: services/sync/modules/service.js
> @@ -693,5 @@
> > -    // If the identity isn't ready it  might not know the username...
> > -    if (!this.identity.readyToAuthenticate) {
> > -      this._log.info("Not ready to authenticate in verifyLogin.");
> > -      this.status.login = LOGIN_FAILED_NOT_READY;
> > -      return false;
> 
> Is it your assertion that this code is unreachable now that we call
> ensureLoggedIn before we call verifyLogin?

This code is still reached, but we don't need to check this state as ensureLoggedIn will simply wait until it *is* ready. Before we had ensureLoggedIn, the login process was purely synchronous and had no way of waiting, so it returned a state saying "not ready yet". Now it can wait for it to become ready making that state unnecessary.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8637077 [details] [diff] [review]
0001-first-wip.patch

I don't think I have enough brain this week to add anything to what Chris wrote :)

Comment 4

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

Attachment: 0001-Bug-1186348-don-t-initialize-the-sync-identity-and-t.patch

I dropped the ball on this, but I hope I previously answered most of your concerns. I think this patch is good to go.

Comment 5

[Chris Karlof [:ckarlof]]

Comment on attachment 8652704 [details] [diff] [review]
0001-Bug-1186348-don-t-initialize-the-sync-identity-and-t.patch

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

::: services/sync/modules/browserid_identity.js
@@ +119,5 @@
> +    // and a background fetch of account data just so we can set this.account,
> +    // so we have a username available before we've actually done a login.
> +    this._fxaService.getSignedInUser().then(accountData => {
> +      this.account = accountData ? accountData.email : null;
> +    });

I don't believe this addition was in your previous patch. What does this address? Is this so we can initialize the UI to look correct (from your previous comments, I'd say no, since you say it that fetches the state directly from the FxAccounts)?

Comment 6

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

(In reply to Chris Karlof [:ckarlof] from comment #5)
> I don't believe this addition was in your previous patch. What does this
> address? Is this so we can initialize the UI to look correct (from your
> previous comments, I'd say no, since you say it that fetches the state
> directly from the FxAccounts)?

Yes, nice spot! It's really just so code reading the preference services.sync.username (eg, https://dxr.mozilla.org/mozilla-central/search?q=services.sync.username&redirect=false&case=true&limit=74&offset=0) will see a value before Sync has started. Other code uses this as a "flag" to indicate whether Sync is configured (rather than initialized).

In practice, it is really just for tests - in the "real world", as soon as we get an FxA login notification we start syncing, which also causes this pref to be written. But some tests (notably the fxa-migration test) assume that as soon as the account is signed in that preference exists.

I'm not too bothered by it, but we could try and avoid that if necessary.

Comment 7

[Chris Karlof [:ckarlof]]

> I'm not too bothered by it, but we could try and avoid that if necessary.

I'm ok with it.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efcd5fd0dba5

Comment 9

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

*sigh* - that try failed due to even greater test suckage than I thought. https://treeherder.mozilla.org/#/jobs?repo=try&revision=de434ba3a395 has a minor tweak (we only set the username when we have an account and don't set it to null when there's no account)

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/822627cafb0b