Bug 671066 - Unknown error on conclusion of easy setup of second client. r=rnewman
authorPhilipp von Weitershausen <philipp@weitershausen.de>
Wed, 13 Jul 2011 15:20:07 -0700
changeset 72776 96db7dbde55e5674ccd5421b6ceffadb170c09e0
parent 72775 8f022250cf9268e5aa2960c2462179030096d9b7
child 72777 98ffad2ab85ec67685d8408fd7c0b9bc559b48e3
push id20771
push userpweitershausen@mozilla.com
push dateThu, 14 Jul 2011 15:47:26 +0000
treeherdermozilla-central@af2c10b34059 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs671066
milestone8.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 671066 - Unknown error on conclusion of easy setup of second client. r=rnewman Part 2: Make sure we don't run into the kNotLoggedIn reason first and then ignore it. It's a useless sentinel anyway, so remove it altogether.
services/sync/modules/constants.js
services/sync/modules/policies.js
services/sync/modules/service.js
services/sync/tests/unit/test_interval_triggers.js
services/sync/tests/unit/test_service_login.js
services/sync/tests/unit/test_syncscheduler.js
--- a/services/sync/modules/constants.js
+++ b/services/sync/modules/constants.js
@@ -194,17 +194,16 @@ JPAKE_ERROR_INVALID:                   "
 JPAKE_ERROR_NODATA:                    "jpake.error.nodata",
 JPAKE_ERROR_KEYMISMATCH:               "jpake.error.keymismatch",
 JPAKE_ERROR_WRONGMESSAGE:              "jpake.error.wrongmessage",
 JPAKE_ERROR_USERABORT:                 "jpake.error.userabort",
 
 // Ways that a sync can be disabled (messages only to be printed in debug log)
 kSyncMasterPasswordLocked:             "User elected to leave Master Password locked",
 kSyncWeaveDisabled:                    "Weave is disabled",
-kSyncNotLoggedIn:                      "User is not logged in",
 kSyncNetworkOffline:                   "Network is offline",
 kSyncBackoffNotMet:                    "Trying to sync before the server said it's okay",
 kFirstSyncChoiceNotMade:               "User has not selected an action for first sync",
 
 // Application IDs
 FIREFOX_ID:                            "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}",
 FENNEC_ID:                             "{a23983c0-fd0e-11dc-95ff-0800200c9a66}",
 SEAMONKEY_ID:                          "{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}",
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -244,26 +244,18 @@ let SyncScheduler = {
     this.adjustSyncInterval();
   },
 
   /**
    * Check if we should be syncing and schedule the next sync, if it's not scheduled
    */
   checkSyncStatus: function checkSyncStatus() {
     // Should we be syncing now, if not, cancel any sync timers and return
-    // if we're in backoff, we'll schedule the next sync
-    let ignore = [kSyncBackoffNotMet];
-
-    // We're ready to sync even if we're not logged in... so long as the
-    // master password isn't locked.
-    if (Utils.mpLocked()) {
-      ignore.push(kSyncNotLoggedIn);
-      ignore.push(kSyncMasterPasswordLocked);
-    }
-
+    // if we're in backoff, we'll schedule the next sync.
+    let ignore = [kSyncBackoffNotMet, kSyncMasterPasswordLocked];
     let skip = Weave.Service._checkSync(ignore);
     this._log.trace("_checkSync returned \"" + skip + "\".");
     if (skip) {
       this.clearSyncTriggers();
       return;
     }
 
     // Only set the wait time to 0 if we need to sync right away
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -1061,17 +1061,17 @@ WeaveSvc.prototype = {
       if (this._autoTimer)
         this._autoTimer.clear();
 
       SyncScheduler.checkSyncStatus();
 
       return;
     }
 
-    let reason = this._checkSync([kSyncNotLoggedIn]);
+    let reason = this._checkSync();
 
     // Can't autoconnect if we're missing these values.
     if (!reason) {
       if (!this.username || !this.password || !this.passphrase)
         return;
 
       Utils.nextTick(this.sync, this);
     }
@@ -1387,18 +1387,16 @@ WeaveSvc.prototype = {
       reason = kSyncWeaveDisabled;
     else if (Services.io.offline)
       reason = kSyncNetworkOffline;
     else if (Status.minimumNextSync > Date.now())
       reason = kSyncBackoffNotMet;
     else if ((Status.login == MASTER_PASSWORD_LOCKED) &&
              Utils.mpLocked())
       reason = kSyncMasterPasswordLocked;
-    else if (!this._loggedIn)
-      reason = kSyncNotLoggedIn;
     else if (Svc.Prefs.get("firstSync") == "notReady")
       reason = kFirstSyncChoiceNotMade;
 
     if (ignore && ignore.indexOf(reason) != -1)
       return "";
 
     return reason;
   },
--- a/services/sync/tests/unit/test_interval_triggers.js
+++ b/services/sync/tests/unit/test_interval_triggers.js
@@ -50,20 +50,21 @@ function run_test() {
   Log4Moz.repository.getLogger("Sync.SyncScheduler").level = Log4Moz.Level.Trace;
 
   run_next_test();
 }
 
 add_test(function test_successful_sync_adjustSyncInterval() {
   _("Test successful sync calling adjustSyncInterval");
   let syncSuccesses = 0;
-  Svc.Obs.add("weave:service:sync:finish", function onSyncFinish() {
+  function onSyncFinish() {
     _("Sync success.");
     syncSuccesses++;
-  });
+  };
+  Svc.Obs.add("weave:service:sync:finish", onSyncFinish);
 
   let server = sync_httpd_setup();
   setUp();
 
   // Confirm defaults
   do_check_false(SyncScheduler.idle);
   do_check_false(SyncScheduler.numClients > 1);
   do_check_eq(SyncScheduler.syncInterval, SyncScheduler.singleDeviceInterval);
@@ -140,40 +141,34 @@ add_test(function test_successful_sync_a
   SyncScheduler.hasIncomingItems = true;
   Service.sync();
   do_check_eq(syncSuccesses, 8);
   do_check_false(SyncScheduler.idle);
   do_check_true(SyncScheduler.numClients > 1);
   do_check_false(SyncScheduler.hasIncomingItems); //gets reset to false
   do_check_eq(SyncScheduler.syncInterval, SyncScheduler.immediateInterval);
 
-  Records.clearCache();
-  Svc.Prefs.resetBranch("");
-  SyncScheduler.setDefaults();
-  Clients.resetClient();
-
+  Svc.Obs.remove("weave:service:sync:finish", onSyncFinish);
+  Service.startOver();
   server.stop(run_next_test);
 });
 
 add_test(function test_unsuccessful_sync_adjustSyncInterval() {
   _("Test unsuccessful sync calling adjustSyncInterval");
 
   let syncFailures = 0;
-  Svc.Obs.add("weave:service:sync:error", function onSyncError() {
+  function onSyncError() {
     _("Sync error.");
     syncFailures++;
-  });
+  }
+  Svc.Obs.add("weave:service:sync:error", onSyncError);
     
   _("Test unsuccessful sync calls adjustSyncInterval");
-  let origLockedSync = Service._lockedSync;
-  Service._lockedSync = function () {
-    // Force a sync fail.
-    Service._loggedIn = false;
-    origLockedSync.call(Service);
-  };
+  // Force sync to fail.
+  Svc.Prefs.set("firstSync", "notReady");
   
   let server = sync_httpd_setup();
   setUp();
 
   // Confirm defaults
   do_check_false(SyncScheduler.idle);
   do_check_false(SyncScheduler.numClients > 1);
   do_check_eq(SyncScheduler.syncInterval, SyncScheduler.singleDeviceInterval);
@@ -255,22 +250,18 @@ add_test(function test_unsuccessful_sync
   SyncScheduler.hasIncomingItems = true;
   Service.sync();
   do_check_eq(syncFailures, 8);
   do_check_false(SyncScheduler.idle);
   do_check_true(SyncScheduler.numClients > 1);
   do_check_false(SyncScheduler.hasIncomingItems); //gets reset to false
   do_check_eq(SyncScheduler.syncInterval, SyncScheduler.immediateInterval);
 
-  Records.clearCache();
-  Svc.Prefs.resetBranch("");
-  SyncScheduler.setDefaults();
-  Clients.resetClient();
-  Service._lockedSync = origLockedSync;
-
+  Service.startOver();
+  Svc.Obs.remove("weave:service:sync:error", onSyncError);
   server.stop(run_next_test);
 });
 
 add_test(function test_back_triggers_sync() {
   let server = sync_httpd_setup();
   setUp();
 
   // Single device: no sync triggered.
@@ -285,15 +276,16 @@ add_test(function test_back_triggers_syn
   Svc.Obs.add("weave:service:sync:finish", function onSyncFinish() {
     Svc.Obs.remove("weave:service:sync:finish", onSyncFinish);
 
     Records.clearCache();
     Svc.Prefs.resetBranch("");
     SyncScheduler.setDefaults();
     Clients.resetClient();
 
+    Service.startOver();
     server.stop(run_next_test);
   });
 
   SyncScheduler.idle = true;
   SyncScheduler.observe(null, "back", Svc.Prefs.get("scheduler.idleTime"));
   do_check_false(SyncScheduler.idle);
 });
--- a/services/sync/tests/unit/test_service_login.js
+++ b/services/sync/tests/unit/test_service_login.js
@@ -17,53 +17,63 @@ function login_handling(handler) {
     }
   };
 }
 
 function run_test() {
   let logger = Log4Moz.repository.rootLogger;
   Log4Moz.repository.rootLogger.addAppender(new Log4Moz.DumpAppender());
 
+  run_next_test();
+}
+
+add_test(function test_offline() {
   try {
     _("The right bits are set when we're offline.");
     Services.io.offline = true;
     do_check_eq(Service._ignorableErrorCount, 0);
     do_check_false(!!Service.login());
     do_check_eq(Status.login, LOGIN_FAILED_NETWORK_ERROR);
     do_check_eq(Service._ignorableErrorCount, 0);
     Services.io.offline = false;
   } finally {
     Svc.Prefs.resetBranch("");
+    run_next_test();
   }
- 
+});
+
+function setup() {
+  Service.serverURL = "http://localhost:8080/";
+  Service.clusterURL = "http://localhost:8080/";
+
   let janeHelper = track_collections_helper();
   let janeU      = janeHelper.with_updated_collection;
   let janeColls  = janeHelper.collections;
   let johnHelper = track_collections_helper();
   let johnU      = johnHelper.with_updated_collection;
   let johnColls  = johnHelper.collections;
 
-  do_test_pending();
-  let server = httpd_setup({
+  return httpd_setup({
     "/1.1/johndoe/info/collections": login_handling(johnHelper.handler),
     "/1.1/janedoe/info/collections": login_handling(janeHelper.handler),
-      
+
     // We need these handlers because we test login, and login
     // is where keys are generated or fetched.
     // TODO: have Jane fetch her keys, not generate them...
     "/1.1/johndoe/storage/crypto/keys": johnU("crypto", new ServerWBO("keys").handler()),
     "/1.1/johndoe/storage/meta/global": johnU("meta",   new ServerWBO("global").handler()),
     "/1.1/janedoe/storage/crypto/keys": janeU("crypto", new ServerWBO("keys").handler()),
     "/1.1/janedoe/storage/meta/global": janeU("meta",   new ServerWBO("global").handler())
   });
+}
+
+add_test(function test_login_logout() {
+  let server = setup();
 
   try {
-    Service.serverURL = "http://localhost:8080/";
-    Service.clusterURL = "http://localhost:8080/";
-
     _("Force the initial state.");
     Status.service = STATUS_OK;
     do_check_eq(Status.service, STATUS_OK);
 
     _("Try logging in. It won't work because we're not configured yet.");
     Service.login();
     do_check_eq(Status.service, CLIENT_NOT_CONFIGURED);
     do_check_eq(Status.login, LOGIN_FAILED_NO_USERNAME);
@@ -93,135 +103,142 @@ function run_test() {
     do_check_eq(Status.login, LOGIN_FAILED_LOGIN_REJECTED);
     do_check_false(Service.isLoggedIn);
 
     _("Try again with correct password.");
     Service.login("janedoe", "ilovejohn");
     do_check_eq(Status.service, STATUS_OK);
     do_check_eq(Status.login, LOGIN_SUCCEEDED);
     do_check_true(Service.isLoggedIn);
-    
+
     _("Calling login() with parameters when the client is unconfigured sends notification.");
     let notified = false;
     Svc.Obs.add("weave:service:setup-complete", function() {
       notified = true;
     });
     Service.username = "";
     Service.password = "";
-    Service.passphrase = "";    
+    Service.passphrase = "";
     Service.login("janedoe", "ilovejohn", "bar");
     do_check_true(notified);
     do_check_eq(Status.service, STATUS_OK);
     do_check_eq(Status.login, LOGIN_SUCCEEDED);
     do_check_true(Service.isLoggedIn);
 
     _("Logout.");
     Service.logout();
     do_check_false(Service.isLoggedIn);
 
     _("Logging out again won't do any harm.");
     Service.logout();
     do_check_false(Service.isLoggedIn);
 
-    /*
-     * Testing login-on-sync.
-     */
-    
+  } finally {
+    Svc.Prefs.resetBranch("");
+    server.stop(run_next_test);
+  }
+});
+
+add_test(function test_login_on_sync() {
+  let server = setup();
+  Service.username = "johndoe";
+  Service.password = "ilovejane";
+  Service.passphrase = "bar";
+
+  try {
     _("Sync calls login.");
     let oldLogin = Service.login;
     let loginCalled = false;
     Service.login = function() {
       loginCalled = true;
       Status.login = LOGIN_SUCCEEDED;
       this._loggedIn = false;           // So that sync aborts.
       return true;
-    }
-    try {
-      Service.sync();
-    } catch (ex) {}
-    
+    };
+
+    Service.sync();
+
     do_check_true(loginCalled);
     Service.login = oldLogin;
-    
+
     // Stub mpLocked.
     let mpLockedF = Utils.mpLocked;
     let mpLocked = true;
     Utils.mpLocked = function() mpLocked;
-    
+
     // Stub scheduleNextSync. This gets called within checkSyncStatus if we're
     // ready to sync, so use it as an indicator.
     let scheduleNextSyncF = SyncScheduler.scheduleNextSync;
     let scheduleCalled = false;
     SyncScheduler.scheduleNextSync = function(wait) {
       scheduleCalled = true;
       scheduleNextSyncF.call(this, wait);
-    }
-    
+    };
+
     // Autoconnect still tries to connect in the background (useful behavior:
     // for non-MP users and unlocked MPs, this will detect version expiry
     // earlier).
-    // 
+    //
     // Consequently, non-MP users will be logged in as in the pre-Bug 543784 world,
     // and checkSyncStatus reflects that by waiting for login.
-    // 
+    //
     // This process doesn't apply if your MP is still locked, so we make
     // checkSyncStatus accept a locked MP in place of being logged in.
-    // 
+    //
     // This test exercises these two branches.
-    
+
     _("We're ready to sync if locked.");
     Service.enabled = true;
     Services.io.offline = false;
     SyncScheduler.checkSyncStatus();
     do_check_true(scheduleCalled);
-    
+
+    _("... and also if we're not locked.");
     scheduleCalled = false;
     mpLocked = false;
-    
-    _("... and not if not.");
     SyncScheduler.checkSyncStatus();
-    do_check_false(scheduleCalled);
+    do_check_true(scheduleCalled);
     SyncScheduler.scheduleNextSync = scheduleNextSyncF;
-    
+
     // TODO: need better tests around master password prompting. See Bug 620583.
 
     mpLocked = true;
-    
+
     // Testing exception handling if master password dialog is canceled.
     // Do this by stubbing out Service.passphrase.
     let oldPP = Service.__lookupGetter__("passphrase");
     _("Old passphrase function is " + oldPP);
     Service.__defineGetter__("passphrase",
                            function() {
                              throw "User canceled Master Password entry";
                            });
-    
+
     let oldClearSyncTriggers = SyncScheduler.clearSyncTriggers;
     let oldLockedSync = Service._lockedSync;
-    
+
     let cSTCalled = false;
     let lockedSyncCalled = false;
-    
+
     SyncScheduler.clearSyncTriggers = function() { cSTCalled = true; };
     Service._lockedSync = function() { lockedSyncCalled = true; };
-    
+
     _("If master password is canceled, login fails and we report lockage.");
     do_check_false(!!Service.login());
     do_check_eq(Status.login, MASTER_PASSWORD_LOCKED);
     do_check_eq(Status.service, LOGIN_FAILED);
     _("Locked? " + Utils.mpLocked());
     _("checkSync reports the correct term.");
     do_check_eq(Service._checkSync(), kSyncMasterPasswordLocked);
-    
+
     _("Sync doesn't proceed and clears triggers if MP is still locked.");
     Service.sync();
-    
+
     do_check_true(cSTCalled);
     do_check_false(lockedSyncCalled);
 
     // N.B., a bunch of methods are stubbed at this point. Be careful putting
     // new tests after this point!
-    
+
   } finally {
     Svc.Prefs.resetBranch("");
-    server.stop(do_test_finished);
+    server.stop(run_next_test);
   }
-}
+});
--- a/services/sync/tests/unit/test_syncscheduler.js
+++ b/services/sync/tests/unit/test_syncscheduler.js
@@ -157,19 +157,17 @@ add_test(function test_masterpassword_lo
 
   do_check_true(loginFailed);
   do_check_eq(Status.login, MASTER_PASSWORD_LOCKED);
   do_check_true(rescheduleInterval);
 
   Service.verifyLogin = Service._verifyLogin;
   SyncScheduler.scheduleAtInterval = SyncScheduler._scheduleAtInterval;
 
-  Svc.Prefs.resetBranch("");
-  SyncScheduler.setDefaults();
-  Clients.resetClient();
+  Service.startOver();
   server.stop(run_next_test);
 });
 
 add_test(function test_calculateBackoff() {
   do_check_eq(Status.backoffInterval, 0);
 
   // Test no interval larger than the maximum backoff is used if
   // Status.backoffInterval is smaller.
@@ -199,16 +197,17 @@ add_test(function test_scheduleNextSync(
   Svc.Obs.add("weave:service:sync:finish", function onSyncFinish() {
     // Ensure this gets called after SyncScheduler's observer so we
     // can cancel the timer set by SyncScheduler.scheduleNextSync().
     Utils.nextTick(function () {
       SyncScheduler.setDefaults();
       Svc.Prefs.resetBranch("");
       SyncScheduler.syncTimer.clear();
       Svc.Obs.remove("weave:service:sync:finish", onSyncFinish);
+      Service.startOver();
       server.stop(run_next_test);
     }, this);
   });
   
   // Make sync happen faster
   SyncScheduler.singleDeviceInterval = 100;
   SyncScheduler.syncInterval = SyncScheduler.singleDeviceInterval;
 
@@ -238,22 +237,18 @@ add_test(function test_scheduleNextSync(
   do_check_true(SyncScheduler.nextSync - Date.now() <= expectedInterval);
   do_check_true(SyncScheduler.syncTimer.delay <= expectedInterval);
 });
 
 add_test(function test_handleSyncError() {
   let server = sync_httpd_setup();
   setUp();
  
-  let origLockedSync = Service._lockedSync;
-  Service._lockedSync = function () {
-    // Force a sync fail.
-    Service._loggedIn = false;
-    origLockedSync.call(Service);
-  };
+  // Force sync to fail.
+  Svc.Prefs.set("firstSync", "notReady");
 
   _("Ensure expected initial environment.");
   do_check_eq(SyncScheduler._syncErrors, 0);
   do_check_false(Status.enforceBackoff);
   do_check_eq(SyncScheduler.syncInterval, SyncScheduler.singleDeviceInterval);
   do_check_eq(Status.backoffInterval, 0);
 
   // Trigger sync with an error several times & observe
@@ -293,18 +288,17 @@ add_test(function test_handleSyncError()
   Service.sync();
   maxInterval = SyncScheduler._syncErrors * (2 * MINIMUM_BACKOFF_INTERVAL);
   do_check_true(SyncScheduler.nextSync <= Date.now() + maxInterval);
   do_check_true(SyncScheduler.syncTimer.delay <= maxInterval);
   do_check_eq(SyncScheduler._syncErrors, 4);
   do_check_true(Status.enforceBackoff);
   SyncScheduler.syncTimer.clear();
 
-  Service._lockedSync = origLockedSync;
-  SyncScheduler.setDefaults();
+  Service.startOver();
   server.stop(run_next_test);
 });
 
 add_test(function test_client_sync_finish_updateClientMode() {
   let server = sync_httpd_setup();
   setUp();
 
   // Confirm defaults.
@@ -329,30 +323,25 @@ add_test(function test_client_sync_finis
 
   // Goes back to single user if # clients is 1.
   do_check_eq(SyncScheduler.numClients, 1);
   do_check_eq(SyncScheduler.syncThreshold, SINGLE_USER_THRESHOLD);
   do_check_eq(SyncScheduler.syncInterval, SyncScheduler.singleDeviceInterval);
   do_check_false(SyncScheduler.numClients > 1);
   do_check_false(SyncScheduler.idle);
 
-  Svc.Prefs.resetBranch("");
-  SyncScheduler.setDefaults();
-  Clients.resetClient();
+  Service.startOver();
   server.stop(run_next_test);
 });
 
 add_test(function test_sync_at_startup() {
   Svc.Obs.add("weave:service:sync:finish", function onSyncFinish() {
     Svc.Obs.remove("weave:service:sync:finish", onSyncFinish);
 
-    Svc.Prefs.resetBranch("");
-    SyncScheduler.setDefaults();
-    Clients.resetClient();
-
+    Service.startOver();
     server.stop(run_next_test);
   });
 
   let server = sync_httpd_setup();
   setUp();
 
   Service.delayedAutoConnect(0);
 });
@@ -360,38 +349,35 @@ add_test(function test_sync_at_startup()
 let timer;
 add_test(function test_no_autoconnect_during_wizard() {
   let server = sync_httpd_setup();
   setUp();
 
   // Simulate the Sync setup wizard.
   Svc.Prefs.set("firstSync", "notReady");
 
-  // Ensure we don't actually try to sync.
-  function onSyncStart() {
+  // Ensure we don't actually try to sync (or log in for that matter).
+  function onLoginStart() {
     do_throw("Should not get here!");
   }
-  Svc.Obs.add("weave:service:sync:start", onSyncStart);
+  Svc.Obs.add("weave:service:login:start", onLoginStart);
 
   // First wait >100ms (nsITimers can take up to that much time to fire, so
   // we can account for the timer in delayedAutoconnect) and then two event
   // loop ticks (to account for the Utils.nextTick() in _autoConnect).
   let ticks = 2;
   function wait() {
     if (ticks) {
       ticks -= 1;
       Utils.nextTick(wait);
       return;
     }
-    Svc.Obs.remove("weave:service:sync:start", onSyncStart);
+    Svc.Obs.remove("weave:service:login:start", onLoginStart);
 
-    Svc.Prefs.resetBranch("");
-    SyncScheduler.setDefaults();
-    Clients.resetClient();
-
+    Service.startOver();
     server.stop(run_next_test);    
   }
   timer = Utils.namedTimer(wait, 150, {}, "timer");
 
   Service.delayedAutoConnect(0);
 });
 
 add_test(function test_idle_adjustSyncInterval() {