622387 - Wrong Sync Key error should only throw an error once

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

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

Comment 1

[Richard Newman [:rnewman]]

Over to you, Marina. Error policy, ho!

Comment 2

[Marina Samuel [:emtwo]]

Attachment: WIP: Show recovery key error only once & resume scheduled syncs on recovery key update

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.

Comment 3

[Philipp von Weitershausen [:philikon]]

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.

Comment 4

[Marina Samuel [:emtwo]]

Attachment: V1: Show recovery key error only once & resume scheduled syncs on recovery key update

* Added some tests
* removed some whitespace

Comment 5

[Philipp von Weitershausen [:philikon]]

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");
>+
>+  Status.login = LOGIN_FAILED_LOGIN_REJECTED;
>+  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

Comment 6

[Marina Samuel [:emtwo]]

Attachment: V2: Show recovery key error only once & resume scheduled syncs on recovery key update

* addressing comment 5

Comment 7

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/services-central/rev/6c980882ea3a

Comment 8

[Marina Samuel [:emtwo]]

STR:
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

Comment 9

[Marina Samuel [:emtwo]]

Also, triggering an instant sync should not trigger an error

Comment 10

[Tracy Walker [:tracy]]

verified with s-c builds from 20110907

Comment 11

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/6c980882ea3a