Closed Bug 967313 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: rfkelly, Assigned: markh)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → mhammond
Blocks: 905997
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [qa+]
Component: Firefox Sync: UI → FxA
Product: Mozilla Services → Firefox
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.
Attachment #8369873 - Flags: review?(ttaubert)
Attachment #8369873 - Flags: review?(ckarlof)
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.
Attachment #8369873 - Flags: review?(ttaubert) → feedback+
Yeah, using a promise is a big improvement.
Attachment #8369873 - Attachment is obsolete: true
Attachment #8369873 - Flags: review?(ckarlof)
Attachment #8370369 - Flags: review?(ttaubert)
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.
Attachment #8370369 - Flags: review?(ttaubert) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/94cbbe810eb8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Duplicate of this bug: 968043
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
Attachment #8370369 - Flags: approval-mozilla-aurora?
Attachment #8370369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
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.
Verified with latest builds on Nightly and Aurora
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.