Closed
Bug 666043
Opened 13 years ago
Closed 13 years ago
Service._skipScheduledRetry is wrong
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
VERIFIED
FIXED
mozilla7
People
(Reporter: philikon, Assigned: emtwo)
References
Details
(Whiteboard: [verified in services])
Attachments
(1 file, 1 obsolete file)
5.18 KB,
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Indeed! Good catch.
Reporter | ||
Comment 2•13 years ago
|
||
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.
Assignee: nobody → msamuel
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #541819 -
Flags: feedback?(philipp)
Reporter | ||
Comment 4•13 years ago
|
||
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.
Attachment #541819 -
Flags: feedback?(philipp)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #541819 -
Attachment is obsolete: true
Attachment #542076 -
Flags: review?(philipp)
Reporter | ||
Comment 6•13 years ago
|
||
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.
Attachment #542076 -
Flags: review?(philipp) → review+
Reporter | ||
Comment 7•13 years ago
|
||
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);
Reporter | ||
Comment 8•13 years ago
|
||
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
Status: NEW → ASSIGNED
Whiteboard: [fixed in services]
Reporter | ||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
(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
Whiteboard: [fixed in services] → [verified in services]
Reporter | ||
Comment 11•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
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.
Description
•