967313 - FxA-enabled sync doesn't think I'm signed in until I restart

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ryan Kelly [:rfkelly]]

STR:

* Start a clean profile
* Tools=>Setup Sync and log in with FxA
* Go to "Tools" menu again, expecting there to be a "Sync Now" option
* It still says "Setup Sync"
* Clicking through "Setup Sync" again tells me "please sign in to reconnect [email]"
* Clicking that link tells me I'm already signed in

Restarting Nightly seems to fix the problem, so there's probably some cached state that's not getting updated properly.  Waiting 10s as suggested by Chris doesn't seem to have any effect.

Comment 1

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

I believe the problem here is that in this flow, browserid_identity hasn't yet been initialized and thus doesn't see the observer notifications.  A light-weight fix isn't that trivial - we can't really initialize it early, else sync will create a new instance and initialize that.  I think we simply need to force sync to fully init during this flow.  I'll look into this.

Comment 2

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

Attachment: 0001-Bug-967313-ensure-Sync-is-initialized-before-we-set-.patch

This patch ensures sync is fully initialized before setting the fxAccounts user.  It also adds a number of additional logging calls to browserid_identity which is useful to diagnose when things go wrong.

It's a bit of a shame that we need to wait for it to fully initialize, but not doing this means the user would not see the doorhanger if they are already verified (as sync might not have initialized the identity module by the time the observer fires).

I think it's worth getting feedback from both Chris and Tim, so requesting review from both.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8369873 [details] [diff] [review]
0001-Bug-967313-ensure-Sync-is-initialized-before-we-set-.patch

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

We could also fix all of my comments below by making the Weave service provide a whenReady() promise that calls ensureLoaded()? I fear that we might need that again somewhere else.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +168,5 @@
> +      return;
> +    }
> +    // Not initialized - we need to do the ensureLoaded/observer dance.
> +    Services.obs.addObserver(function onReady() {
> +      Services.obs.removeObserver(onReady, "weave:service:ready");

If the user closes the page before we received the notification we will probably leak the document.

@@ +171,5 @@
> +    Services.obs.addObserver(function onReady() {
> +      Services.obs.removeObserver(onReady, "weave:service:ready");
> +      doSignin();
> +    }, "weave:service:ready", false);
> +    xps.ensureLoaded();

I think we should move all of the above to a function returning a promise that resolves when sync is ready.

Comment 4

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

Attachment: 0001-Bug-967313-ensure-Sync-is-initialized-before-we-set-.patch

Yeah, using a promise is a big improvement.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8370369 [details] [diff] [review]
0001-Bug-967313-ensure-Sync-is-initialized-before-we-set-.patch

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

FTR, there is code in browser-syncui.js that does the same. We should fix that too sometimes.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +147,5 @@
> +    let xps = Cc["@mozilla.org/weave/service;1"]
> +              .getService(Ci.nsISupports)
> +              .wrappedJSObject;
> +    xps.whenLoaded().then(() => {
> +      return fxAccounts.setSignedInUser(accountData)

Nit: missing semicolon

::: services/sync/Weave.js
@@ +87,5 @@
> +    Services.obs.addObserver(function onReady() {
> +      Services.obs.removeObserver(onReady, "weave:service:ready");
> +      deferred.resolve();
> +    }, "weave:service:ready", false);
> +    this.ensureLoaded();

We could in theory end up with multiple observers but caching the promise would just complicate the code. I don't think it really hurts having it like this.

Comment 6

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

(In reply to Tim Taubert [:ttaubert] from comment #5)
> FTR, there is code in browser-syncui.js that does the same. We should fix
> that too sometimes.

Yeah - I opened bug 968035.
 
> Nit: missing semicolon

Oops - fixed.

> We could in theory end up with multiple observers but caching the promise
> would just complicate the code. I don't think it really hurts having it like
> this.

Agreed, given that the more users of this there are, the more of them that are likely to hit the "early resolve" case.

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/94cbbe810eb8

Comment 9

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

Comment on attachment 8370369 [details] [diff] [review]
0001-Bug-967313-ensure-Sync-is-initialized-before-we-set-.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: Poor first experience using Firefox Accounts based sync
Testing completed (on m-c, etc.): Landed on central
Risk to taking this patch (and alternatives if risky): Low risk, limited to new FxA work.
String or IDL/UUID changes made by this patch:None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2286ce77e07

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

I ran into this again when my night updated to a build from https://hg.mozilla.org/mozilla-central/rev/1e9f169c9715 which I believe includes this patch.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

The update I just got built from https://hg.mozilla.org/mozilla-central/rev/dedf12c4e805 didn't require re-signing in, so maybe I was wrong about the previous update including this patch.

Comment 13

[Tracy Walker [:tracy]]

Verified with latest builds on Nightly and Aurora