Closed Bug 622387 Opened 10 years ago Closed 9 years ago

Wrong Sync Key error should only throw an error once


(Firefox :: Sync, defect)

Not set





(Reporter: rnewman, Assigned: emtwo)



(Whiteboard: [verified in services])


(1 file, 2 obsolete files)

... and only changing the sync key should re-enable timed syncs, or unset a flag, or whatever approach we take to resolving this.

As it currently stands, a machine with an incorrect sync key will keep throwing a yellow bar every few minutes until you enter the correct key or disable Sync.
Over to you, Marina. Error policy, ho!
Assignee: nobody → msamuel
The errors were coming up if the globalScore > syncThreshold for every score increment since the globalScore is only reset to 0 on sync:start but this error results in a sync:error.

A possible solution done here is to ignore score-triggered syncs if the login was not successful.

Still needs unit tests but I quickly manually tested this and it seems to work.
Attachment #556194 - Flags: feedback?(philipp)
Comment on attachment 556194 [details] [diff] [review]
WIP: Show recovery key error only once & resume scheduled syncs on recovery key update

Good sleuthing!

>diff --git a/browser/base/content/syncGenericChange.js b/browser/base/content/syncGenericChange.js
>--- a/browser/base/content/syncGenericChange.js
>+++ b/browser/base/content/syncGenericChange.js
>@@ -192,16 +192,17 @@ let Change = {
>   doChangePassphrase: function Change_doChangePassphrase() {
>     let pp = Weave.Utils.normalizePassphrase(this._passphraseBox.value);
>     if (this._updatingPassphrase) {
>       Weave.Service.passphrase = pp;
>       if (Weave.Service.login()) {
>         this._updateStatus("change.recoverykey.success", "success");
>         Weave.Service.persistLogin();
>+        Weave.SyncScheduler.delayedAutoConnect(0);

Hmm, yeaaaah. That makes sense.

There are a lot of implementation details exposed to the UI code here: verifying the credentials, persisting them, resuming scheduled syncs. Ideally, we'd be abstracting all that away. Ideally.

The rest looks good. Needs tests, though.
Attachment #556194 - Flags: feedback?(philipp)
* Added some tests
* removed some whitespace
Attachment #556194 - Attachment is obsolete: true
Attachment #556690 - Flags: review?(philipp)
Comment on attachment 556690 [details] [diff] [review]
V1: Show recovery key error only once & resume scheduled syncs on recovery key update

Just nits regarding comments for clarity:

>+add_test(function test_sync_not_triggered() {
>+  let server = sync_httpd_setup();
>+  setUp();
>+  // Ensure we don't actually try to sync.
>+  function onSyncStart() {
>+    do_throw("Should not get here!");
>+  }
>+  Svc.Obs.add("weave:service:sync:start", onSyncStart);
>+  // First wait >100ms (nsITimers can take up to that much time to fire, so
>+  // we can account for the timer in delayedAutoconnect) and then one event
>+  // loop tick (to account for a possible call to weave:service:sync:start).
>+  Utils.namedTimer(function() {
>+    Utils.nextTick(function() {
>+      Svc.Obs.remove("weave:service:sync:start", onSyncStart);
>+      do_check_eq(Status.login, LOGIN_FAILED_LOGIN_REJECTED);
>+      Service.startOver();
>+      server.stop(run_next_test);
>+    });
>+  }, 150, {}, "timer");
>+  tracker.score += SCORE_INCREMENT_XLARGE;

It took me a while to figure out that by simply setting Status.login = LOGIN_FAILED_LOGIN_REJECTED, you're preventing the score update from working. Can you add a comment above that line to explain that, e.g.

  // Faking incorrect credentials to prevent score update.

or so. Also, naming the test function something like test_incorrect_credentials_sync_not_triggered() would help, too.

>+add_test(function test_clients_engine_sync_not_triggered() {
>+  _("Ensure that client engine score changes don't trigger a sync if Status.login != LOGIN_SUCCEEDED.");

I don't see how this tests a code path that wasn't tested before, so it just seems redundant to me. I like the explanatory text, though. Can you add that for the above test? :)

r=me with those addressed
Attachment #556690 - Flags: review?(philipp) → review+
Attachment #556690 - Attachment is obsolete: true
1) Set up 2 linked profiles
2) Sync Preferences -> My Sync Key -> Generate a New Key -> Change Recovery Key on device A
3) Verify:
* an error is produced on the first scheduled sync on device B and not on following syncs
* an error is produced on every "Sync Now"
* scheduled syncs resume as expected after Sync Key is updated on device B
Also, triggering an instant sync should not trigger an error
verified with s-c builds from 20110907
Whiteboard: [fixed in services] → [verified in services]
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.