The Sync item in the Tools menu is not responding and misnamed in the unverified state

VERIFIED FIXED in Firefox 29

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: rfeeley, Assigned: Gavin)

Tracking

unspecified
Firefox 30
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox29+ verified, firefox30 verified)

Details

(Whiteboard: p=2 s=it-30c-29a-28b.3 [qa!])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Before the user verifies their account, the Sync item in the Tools menu should (still) say “Set up Sync…” and when clicked it should bring up the Sync Preferences panel. Currently it says "Sync Now" and when clicked, nothing is launched.
Blocks: 905997
Whiteboard: [qa+]
While I'm confident I have seen this before, I can't reproduce it - the tools menu does indeed show "setup sync" while in this state.

It would be great if QA could help here with a reliable STR.
Whiteboard: [qa+] → p=0 [qa+]
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: p=0 [qa+] → p=2 s=it-30c-29a-28b.2 [qa+]
I couldn't reproduce comment 0 as well, the only case where I see a not working Sync Now entry is if I sign out from about:account after I signed in. Then Sync Now is shown in the menu, but it doesn't do anything. The menu panel still shows my account like if I'm connected:

1. create an account
2. verify the mail
3. open Sync prefs and choose Manage
4. click Sign out
5. -- Here you see Tools / Sync now and it does nothing
6. open Sync prefs, choose Disconnect and close preferences dialog
7. here you see Tools / Set up sync, as expected

So I wonder if comment 0 was seen during extensive testing, an account had been just signed off and he was trying to create a new one.
Though, the bug I reproduced would be solved by bug 968353.

The only other possibility is that there is a race condition that causes gSyncUI.updateUI to be invoked at the wrong time, or not be invoked when needed, though this requires STR since it's hard to ensure the calls sequence between sync and fxa just by looking at the code.
Ryan, any idea on how you reproduced this bug? May my guess in comment 2 be correct?
Flags: needinfo?(rfeeley)
Keywords: steps-wanted
QA Contact: twalker
Mark and John couldn't reproduce either - going to close this. Ryan (or anyone else), if you still see this, please re-open.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
Keywords: steps-wanted
WFM, as well.

Also, from comment #2,; 5. -- Here you see Tools / Sync now and it does nothing

Sync Now, does trigger a sync for me.  As I believe it should.  Signing out of FxA (steps 3 and 4) isn't the same as disconnecting sync, right?
Status: RESOLVED → VERIFIED
Whiteboard: p=2 s=it-30c-29a-28b.2 [qa+] → p=2 s=it-30c-29a-28b.2 [qa!]
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #5)
> Sync Now, does trigger a sync for me.  As I believe it should.  Signing out
> of FxA (steps 3 and 4) isn't the same as disconnecting sync, right?

Currently it isn't, but probably it should be, see bug 968353.
(Reporter)

Updated

5 years ago
Flags: needinfo?(rfeeley)
(Reporter)

Comment 7

5 years ago
Still an issue for me. I created a new account, and before I clicked the verification link in the email, I am in this state:

https://www.dropbox.com/s/xcsm0v1we4js7al/unverified-set-up-now.png

(You may need to copy/paste this URL into a new tab, as BugzillaJS messes with it)

The desired behaviour is that in this state is saying Set Up Sync… and launches the Sync Preferences panel.
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
Hi Tracy, can you verify?
Flags: needinfo?(twalker)
retried this

1) start new account process
2) do not respond to the verification email
3) check Tools menu. As expected it still says Set Up Sync
4) check Prefs Sync panel, As expected it says the account is not verified.

This is what I expect, thus WFM'd the bug.

However, I continued with the following steps:

5) Still have not responded to the account verification email.
6) Restart the browser (in this case it was for an update)
7) check Tools menu. It now says Sync Now
8) check Prefs Sync panel, as expected it says the account is not verified.

The states are mismatched, Tools Menu should not be in Sync Now state.
Flags: needinfo?(twalker)
Whiteboard: p=2 s=it-30c-29a-28b.2 [qa!] → p=2 s=it-30c-29a-28b.2 [qa+]
Removing from iteration IT-30C-29A-28B.2 as bug has been reopened.  Will be prioritized for an upcoming iteration.
Whiteboard: p=2 s=it-30c-29a-28b.2 [qa+] → p=2 [qa+]
The core issue here seems to be that _needSetup in browser-syncUI returns "false" when in an unverified state after signing in, but eventually returns to returning "true" when the email verification polling times out.

From a browser console log, after signing in with an unverified account:

> Services.wm.getMostRecentWindow("navigator:browser").gSyncUI._needsSetup()
false

> Weave.Status.checkSetup()
"success.status_ok"

1394478687123	Sync.BrowserIDManager	ERROR	Background fetch for key bundle failed: Error: User email verification timed out.

> Services.wm.getMostRecentWindow("navigator:browser").gSyncUI._needsSetup()
true

> Weave.Status.checkSetup()
"service.client_not_configured"
Created attachment 8388939 [details] [diff] [review]
patch

This fixes things, at least when Sync is initialized. "Sync now" is still shown for about 10 seconds on startup if you're not verified, but I don't think we can really fix that case unfortunately.
Assignee: mak77 → gavin.sharp
Status: REOPENED → ASSIGNED
Attachment #8388939 - Flags: feedback?(mhammond)
Comment on attachment 8388939 [details] [diff] [review]
patch

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

::: browser/base/content/browser-syncui.js
@@ -96,5 @@
>             firstSync == "notReady";
>    },
>  
>    _loginFailed: function () {
> -    // Referencing Weave.Service will implicitly initialize sync, and we don't

I think we should keep this block with an updated comment - something like:

Referencing Weave.Status will import many modules, and Weave.Status can only ever be LOGIN_FAILED_LOGIN_REJECTED if the service is ready - so like _needsSetup above, check service.ready first.

(Obviously another helper, eg, _isSyncReady() might make more sense)
Attachment #8388939 - Flags: feedback?(mhammond) → feedback+
Oops, sorry Marco - didn't realize I was stealing this. Since Mark and Tim are in town I decided to look into it, hope you don't mind!
(Reporter)

Comment 15

5 years ago
Thanks guys! Definitely would prefer it said "Set Up Sync…" while unverified, but even more important is that it links to the preferences panel.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #14)
> Oops, sorry Marco - didn't realize I was stealing this. Since Mark and Tim
> are in town I decided to look into it, hope you don't mind!

thanks for looking into it, it's nice we found the steps and a fix for it.
Created attachment 8389526 [details] [diff] [review]
patch
Attachment #8388939 - Attachment is obsolete: true
Attachment #8389526 - Flags: review?(mhammond)
Attachment #8389526 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/integration/fx-team/rev/662beba60653

Note that even with this patch, in the first ten seconds of a browser session (before sync initializes itself) we still show the incorrect menu item. Not really an easy way around that.
status-firefox29: --- → affected
tracking-firefox29: --- → +
Target Milestone: --- → Firefox 30
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/662beba60653
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: p=2 [qa+] → p=2 s=it-30c-29a-28b.3 [qa+]
Attachment #8389526 - Flags: approval-mozilla-aurora+
Comment on attachment 8389526 [details] [diff] [review]
patch

I need a different Aurora patch because bug 976683 didn't land there.
Attachment #8389526 - Flags: approval-mozilla-aurora+
Created attachment 8390119 [details] [diff] [review]
aurora patch

This includes the fixes in bug 982893.
Attachment #8390119 - Flags: review?(mhammond)
Depends on: 982893
Created attachment 8390159 [details] [diff] [review]
aurora patch
Attachment #8390119 - Attachment is obsolete: true
Attachment #8390119 - Flags: review?(mhammond)
Attachment #8390159 - Flags: review?(mhammond)
Attachment #8390159 - Flags: review?(mhammond) → review+
Following STR's in comment #9, this now looks good.  At step 7, after 10 seconds post restart have passed, the Tools Menu item says Set Up Sync.
Status: RESOLVED → VERIFIED
status-firefox30: --- → verified
Attachment #8390159 - Flags: approval-mozilla-aurora+
status-firefox29: fixed → verified
Whiteboard: p=2 s=it-30c-29a-28b.3 [qa+] → p=2 s=it-30c-29a-28b.3 [qa!]
Flags: firefox-backlog+
No longer blocks: 950073
You need to log in before you can comment on or make changes to this bug.