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

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ryan Feeley [:rfeeley]]

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.

Comment 1

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

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.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Marco Bonardo [:mak]]

Ryan, any idea on how you reproduced this bug? May my guess in comment 2 be correct?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Mark and John couldn't reproduce either - going to close this. Ryan (or anyone else), if you still see this, please re-open.

Comment 5

[Tracy Walker [:tracy]]

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?

Comment 6

[Marco Bonardo [:mak]]

(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.

Comment 7

[Ryan Feeley [:rfeeley]]

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.

Comment 8

[Marco Mucci [:MarcoM]]

Hi Tracy, can you verify?

Comment 9

[Tracy Walker [:tracy]]

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.

Comment 10

[Marco Mucci [:MarcoM]]

Removing from iteration IT-30C-29A-28B.2 as bug has been reopened.  Will be prioritized for an upcoming iteration.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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"

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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.

Comment 13

[Mark Hammond [:markh] [: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)

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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!

Comment 15

[Ryan Feeley [:rfeeley]]

Thanks guys! Definitely would prefer it said "Set Up Sync…" while unverified, but even more important is that it links to the preferences panel.

Comment 16

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (email [e-mail]) 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.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/662beba60653

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8389526 [details] [diff] [review]
patch

I need a different Aurora patch because bug 976683 didn't land there.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: aurora patch

This includes the fixes in bug 982893.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Attachment: aurora patch

Comment 23

[Tracy Walker [:tracy]]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b8b3d35de14