Closed Bug 1082970 Opened 10 years ago Closed 10 years ago

Sync no longer shows login failed notification bar on user-initiated sync

Categories

(Firefox :: Sync, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.1
Tracking Status
firefox35 --- verified
firefox36 --- verified

People

(Reporter: markh, Assigned: akligman)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 1045046 has the following change:

   doSync: function SUI_doSync() {
-    setTimeout(function() Weave.Service.errorHandler.syncAndReportErrors(), 0);
+    let needsSetup = this._needsSetup();
+    let loginFailed = this._loginFailed();
+
+    if (!(loginFailed || needsSetup)) {
+      setTimeout(function () Weave.Service.errorHandler.syncAndReportErrors(), 0);
+    }

This means that in a login-failed state, syncAndReportErrors() isn't called - so errors aren't reported.

The change to updateUI also looks suspect to me - it looks like that too will hide sync error states - but only when cloudSync is set up, so that might be more subtle.

I can't see in that bug why these changes were made.  Alan, can you offer any insights?
Flags: needinfo?(akligman)
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
I think this addresses your concerns. I split the branch in updateUI to handle cloudsync and weave separately.
Attachment #8505191 - Flags: feedback?(mhammond)
Flags: needinfo?(akligman)
Comment on attachment 8505191 [details] [diff] [review]
0001-Bug-1082970-Sync-no-longer-shows-login-failed-notifi.patch

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

In the first change it looks like this will allow multiple sync menu items to appear - eg, both "sync now" and "please reauth".  In the second change we are still doing a needsSetup() check which wasn't necessary before, and I'd need to spend more time to verify it's going to cause similar issues.

What I still don't understand is why the cloud stuff needs to take precedence or be checked here at all.  Cloud sync uses sync, right?  So I don't understand why sync's existing error states aren't the only things that matter here?  ie, why would we ever want to enable "sync now" if sync is in an error state?

If couldsync actually uses the sync engines etc, I think we should arrange for sync to reflect the correct state, then it would seem to make all these checks unnecessary.
Attachment #8505191 - Flags: feedback?(mhammond) → feedback-
(In reply to Mark Hammond [:markh] from comment #2)
> Cloud sync uses sync, right?

Not at all. They're totally distinct.
"Sync Now" is being used here to cause weave to sync (if it's setup) and emit an event to cloudsync adapters.

Based on feedback from UX (https://bugzilla.mozilla.org/show_bug.cgi?id=1045046#c3) it's OK to show "Sync Now" if there are cloudsync adapters available. In that case, the cloudsync checks should take precedence over Weave.

Cloudsync doesn't use sync, it's separate.

syncAndReportErrors should only be called on manual sync, which isn't possible if needsSetup is true. Normally, if loginFailed was true then "Sync Now" would not be visible. So in either case, if no cloudsync adapters are installed the behavior should be unchanged.

Is there an easy way to reproduce this? It would be helpful if I could try this locally.
(In reply to Richard Newman [:rnewman] from comment #3)
> (In reply to Mark Hammond [:markh] from comment #2)
> > Cloud sync uses sync, right?
> 
> Not at all. They're totally distinct.

Oops - I'm not sure what I was looking at, but thought I saw some sync code touched when landing it.  I can't find what I was looking at now, so apologies for the confusion.

(In reply to Alan K [:ack] from comment #4)
> "Sync Now" is being used here to cause weave to sync (if it's setup) and
> emit an event to cloudsync adapters.
> 
> Based on feedback from UX
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1045046#c3) it's OK to show
> "Sync Now" if there are cloudsync adapters available. In that case, the
> cloudsync checks should take precedence over Weave.

OK - in that case I guess that first block is fine (ie, it seems by design that those items only apply to cloudsync) - but I doubt it's OK to have duplicates, which IIUC your recent patch would have caused.

> syncAndReportErrors should only be called on manual sync, which isn't
> possible if needsSetup is true. Normally, if loginFailed was true then "Sync
> Now" would not be visible. So in either case, if no cloudsync adapters are
> installed the behavior should be unchanged.

But I still don't understand the motivation behind that change.  If cloudsync is distinct from sync, then why was it necessary for doSync to change the semantics for sync itself?  IOW, why wasn't it enough to just notify your observers without changing how it works for sync?

> Is there an easy way to reproduce this? It would be helpful if I could try
> this locally.

* Setup sync on 2 profiles.
* On one of the profiles change the password.
* Start the other profile and wait for the automatic error notification to appear (it may take some time for the tokens etc to go stale before this happens).
* Close the error notification bar.
* Hit the sync button - the notification bar should come back.
(In reply to Mark Hammond [:markh] from comment #5)
> But I still don't understand the motivation behind that change.

Oh - yes I do :)  The issue is that now the sync button is enabled when it wouldn't have been!  So yeah, you are probably correct that the second part of that patch will do what we need - just check needsSetup but not the login state.
The code in updateUI is needed to ensure that only one of the menu options is visible at a time, so no change there.

Updated the branch in doSync to reflect discussion above.
Assignee: nobody → akligman
Attachment #8505191 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8505879 - Flags: review?(mhammond)
Comment on attachment 8505879 [details] [diff] [review]
0001-Bug-1082970-Sync-no-longer-shows-login-failed-notifi.patch

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

Thanks for nursing me through this :)
Attachment #8505879 - Flags: review?(mhammond) → review+
No problem, thanks for being vigilant!
Updated patch to reflect r+.
Attachment #8505879 - Attachment is obsolete: true
Attachment #8505900 - Flags: review+
Bad commit message, fixed.
Attachment #8505900 - Attachment is obsolete: true
Attachment #8505902 - Flags: review+
Keywords: checkin-needed
Hi,

do we need a try here ?
Keywords: checkin-needed
Probably not necessary, but let's be safe: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43eb47a6d914
Keywords: checkin-needed
Comment on attachment 8505902 [details] [diff] [review]
0001-Bug-1082970-Sync-no-longer-shows-login-failed-notifi.patch

Approval Request Comment
[Feature/regressing bug #]: 1082970
[User impact if declined]: login failure notifications not displayed on user-initiated sync
[Describe test coverage new/current, TBPL]:
[Risks and why]: not risky
[String/UUID change made/needed]: no strings
Attachment #8505902 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f0e0f05aa2d3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Iteration: --- → 36.1
Attachment #8505902 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: twalker
I was able to reproduce this issue on Firefox 36.0a1 (2014-11-14) using Windows 7 x64. After the password was changed in the first profile, the notification bar appeared only a single time and clicking on Sync button nothing happened.

Verified fixed on Latest Firefox 37.0a1 (2015-01-06) using Windows 7 x64, Ubuntu 14.04 x86 and Mac OSX 10.9.5. In Latest Nightly, the notification bar appears every time you click on Sync button.

Mark, please confirm that this is the expected result!
Flags: needinfo?(mhammond)
SGTM - thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(mhammond)
Marking tracking flags as verified fixed according with my testing on Latest Firefox 36.0a2 (2015-01-07) and on Firefox 35 RC build 2 (20150106233618) using Windows 7 x64, Ubuntu 14.04 x86 and Mac OSX 10.9.5.
You need to log in before you can comment on or make changes to this bug.