666043 - Service._skipScheduledRetry is wrong

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

In bug 623689 comment 19, I said:

> As discussed on IRC, I think we only want to call _scheduleAtInterval() if the
> login failure wasn't due to LOGIN_FAILED_INVALID_PASSPHRASE or
> LOGIN_FAILED_LOGIN_REJECTED.

upon which rnewman landed:

> +  _skipScheduledRetry: function _skipScheduledRetry() {
> +    return [LOGIN_FAILED_INVALID_PASSPHRASE,
> +            LOGIN_FAILED_LOGIN_REJECTED].indexOf(Status.login) == -1;
> +  },
> +  
>    sync: function sync() {
...
> +          // Try again later, just as if we threw an error... only without the
> +          // error count.
> +          if (!this._skipScheduledRetry())
> +            this._scheduleAtInterval(MASTER_PASSWORD_LOCKED_RETRY_INTERVAL);
> +    
> +          return;
> +        }
>        }

Unless I'm horribly mistaken, this implements exactly the opposite logic: _skipScheduledRetry() returns false if Status.login is either LOGIN_FAILED_INVALID_PASSPHRASE and LOGIN_FAILED_LOGIN_REJECTED, but it should return true, so that we *don't* call _scheduleAtInterval().

Comment 1

[Richard Newman [:rnewman]]

Indeed! Good catch.

Comment 2

[Philipp von Weitershausen [:philikon]]

Assigning this to Marina as this code gets touched in bug 664792, and Instant Sync will likely exercise this code path much more than ever.

Comment 3

[Marina Samuel [:emtwo]]

Attachment: patch for 666043

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 541819 [details] [diff] [review]
patch for 666043

>-add_test(function test_login_rejected_masterpassword_interval() {
>-  _("Test LOGIN_FAILED_LOGIN_REJECTED status results in reschedule at MASTER_PASSWORD interval");
>+add_test(function test_no_username_masterpassword_interval() {
>+  _("Test LOGIN_FAILED_NO_USERNAME status results in reschedule at MASTER_PASSWORD interval");

That's an odd condition to test. Not having a username shouldn't really have us sync at all (SyncScheduler.checkSyncStatus should take care of that), and if our observer ever encounters this particular condition, we should really not reschedule another sync.

So, in fact it's not just skipScheduledRetry that's wrong. I think the root of the problem is the "weave:service:login:error" observer in SyncScheduler. (Not your fault, you just copied it over from Service, of course.) We should really only invoke SyncScheduler.scheduleAtInterval(MASTER_PASSWORD_LOCKED_RETRY_INTERVAL) if Status.login == MASTER_PASSWORD_LOCKED. I don't understand why Richard and I made the condition test what Status.login is *not* rather than checking what it *is*.

To summarize, I think we can just get rid of skipScheduledRetry() altogether and just check for Status.login == MASTER_PASSWORD_LOCKED explicitly. For the test, you'll want to fake that obviously. Best would probably be if you monkey-patched Service.verifyLogin() to set Status.login and return false. For good measure, you might also want to override Utils.mpLocked(), though it might not even be necessary.

Comment 5

[Marina Samuel [:emtwo]]

Attachment: V2: patch for 666043

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 542076 [details] [diff] [review]
V2: patch for 666043

>+add_test(function test_masterpassword_locked_retry_interval() {
>+  _("Test Status.login = MASTER_PASSWORD_LOCKED results in reschedule at MASTER_PASSWORD interval");
...
>-  Service.sync();
>+  Service.login();

We should continue to call Service.sync() since Service.login() is becoming just an implementation detail of syncing.

r=me with that.

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 542076 [details] [diff] [review]
V2: patch for 666043

Found some more niggles:

>+add_test(function test_masterpassword_locked_retry_interval() {
>+  _("Test Status.login = MASTER_PASSWORD_LOCKED results in reschedule at MASTER_PASSWORD interval");
...
>+  let rescheduleInteval = false;

Typo: rescheduleInte*r*val.

>+  Service.verifyLogin = function () {
>+    Status.login = MASTER_PASSWORD_LOCKED;
>+    return false;
>+  }

Nit: missing semicolon.

>   run_next_test();

Need to kill the server:

  server.stop(run_next_test);

Comment 8

[Philipp von Weitershausen [:philikon]]

I took the liberty of addressing comment 6 and comment 7 myself so it could be pushed to s-c:

http://hg.mozilla.org/services/services-central/rev/4efc7f9b8413

Comment 9

[Philipp von Weitershausen [:philikon]]

STRs for QA:

* Enable a master password on a sync-enabled profile
* Don't unlock the master password right after starting the browser, but wait a minute or so (so that the autoconnect will have fired)
* Unlock the master password. Sync should trigger a sync within the next 15 minutes.

Comment 10

[Tony Chung [:tchung]]

(In reply to comment #9)
> STRs for QA:
> 
> * Enable a master password on a sync-enabled profile
> * Don't unlock the master password right after starting the browser, but
> wait a minute or so (so that the autoconnect will have fired)
> * Unlock the master password. Sync should trigger a sync within the next 15
> minutes.

Verified on s-c branch given STRs

Comment 11

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/4efc7f9b8413