Bug 781952 - Part 1: Refactor engine syncing logic out of service.js; r=rnewman
authorGregory Szorc <gps@mozilla.com>
Tue, 14 Aug 2012 11:34:14 -0700
changeset 111034 0dfcb4b9a89b6e7ae60744a70d62fb0ebe91bbf5
parent 111033 7a7aff2f35d33cdb857902753bdee8c45b5c1c67
child 111035 172d339df58589cb94c456586de9ff5ae72a8c4f
push id93
push usernmatsakis@mozilla.com
push dateWed, 31 Oct 2012 21:26:57 +0000
reviewersrnewman
bugs781952
milestone17.0a1
Bug 781952 - Part 1: Refactor engine syncing logic out of service.js; r=rnewman
services/sync/Makefile.in
services/sync/modules/constants.js
services/sync/modules/service.js
services/sync/modules/stages/enginesync.js
services/sync/tests/unit/test_load_modules.js
--- a/services/sync/Makefile.in
+++ b/services/sync/Makefile.in
@@ -51,16 +51,20 @@ sync_engine_modules := \
   clients.js \
   forms.js \
   history.js \
   passwords.js \
   prefs.js \
   tabs.js \
   $(NULL)
 
+sync_stage_modules := \
+  enginesync.js \
+  $(NULL)
+
 DIRS += locales
 TEST_DIRS += tests
 
 EXTRA_COMPONENTS := \
   SyncComponents.manifest \
   Weave.js \
   $(NULL)
 
@@ -70,9 +74,13 @@ PREF_JS_EXPORTS := $(srcdir)/services-sy
 SYNC_MAIN_FILES := $(addprefix modules/,$(sync_modules))
 SYNC_MAIN_DEST = $(FINAL_TARGET)/modules/services-sync
 INSTALL_TARGETS += SYNC_MAIN
 
 SYNC_ENGINES_FILES := $(addprefix modules/engines/,$(sync_engine_modules))
 SYNC_ENGINES_DEST = $(FINAL_TARGET)/modules/services-sync/engines
 INSTALL_TARGETS += SYNC_ENGINES
 
+SYNC_STAGES_FILES := $(addprefix modules/stages/,$(sync_stage_modules))
+SYNC_STAGES_DEST = $(FINAL_TARGET)/modules/services-sync/stages
+INSTALL_TARGETS += SYNC_STAGES
+
 include $(topsrcdir)/config/rules.mk
--- a/services/sync/modules/constants.js
+++ b/services/sync/modules/constants.js
@@ -174,11 +174,13 @@ kFirstSyncChoiceNotMade:               "
 
 // Application IDs
 FIREFOX_ID:                            "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}",
 FENNEC_ID:                             "{a23983c0-fd0e-11dc-95ff-0800200c9a66}",
 SEAMONKEY_ID:                          "{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}",
 TEST_HARNESS_ID:                       "xuth@mozilla.org",
 
 MIN_PP_LENGTH:                         12,
-MIN_PASS_LENGTH:                       8
+MIN_PASS_LENGTH:                       8,
+
+LOG_DATE_FORMAT:                       "%Y-%m-%d %H:%M:%S",
 
 }))];
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -14,33 +14,32 @@ const Cu = Components.utils;
 const CLUSTER_BACKOFF = 5 * 60 * 1000; // 5 minutes
 
 // How long a key to generate from an old passphrase.
 const PBKDF2_KEY_BYTES = 16;
 
 const CRYPTO_COLLECTION = "crypto";
 const KEYS_WBO = "keys";
 
-const LOG_DATE_FORMAT = "%Y-%m-%d %H:%M:%S";
-
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://services-common/utils.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/engines/clients.js");
 Cu.import("resource://services-common/preferences.js");
 Cu.import("resource://services-sync/identity.js");
 Cu.import("resource://services-common/log4moz.js");
 Cu.import("resource://services-sync/resource.js");
 Cu.import("resource://services-sync/rest.js");
 Cu.import("resource://services-sync/status.js");
 Cu.import("resource://services-sync/policies.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/main.js");
+Cu.import("resource://services-sync/stages/enginesync.js");
 
 const STORAGE_INFO_TYPES = [INFO_COLLECTIONS,
                             INFO_COLLECTION_USAGE,
                             INFO_COLLECTION_COUNTS,
                             INFO_QUOTA];
 
 /*
  * Service singleton
@@ -1183,238 +1182,29 @@ WeaveSvc.prototype = {
       }
       return this._lockedSync.apply(this, arguments);
     })();
   },
 
   /**
    * Sync up engines with the server.
    */
-  _lockedSync: function _lockedSync()
-    this._lock("service.js: sync",
-               this._notify("sync", "", function onNotify() {
-
-    this._log.info("In sync().");
-
-    let syncStartTime = Date.now();
-
-    Status.resetSync();
-
-    // Make sure we should sync or record why we shouldn't
-    let reason = this._checkSync();
-    if (reason) {
-      if (reason == kSyncNetworkOffline) {
-        Status.sync = LOGIN_FAILED_NETWORK_ERROR;
-      }
-      // this is a purposeful abort rather than a failure, so don't set
-      // any status bits
-      reason = "Can't sync: " + reason;
-      throw reason;
-    }
-
-    // if we don't have a node, get one.  if that fails, retry in 10 minutes
-    if (this.clusterURL == "" && !this._setCluster()) {
-      Status.sync = NO_SYNC_NODE_FOUND;
-      return;
-    }
-
-    // Ping the server with a special info request once a day.
-    let infoURL = this.infoURL;
-    let now = Math.floor(Date.now() / 1000);
-    let lastPing = Svc.Prefs.get("lastPing", 0);
-    if (now - lastPing > 86400) { // 60 * 60 * 24
-      infoURL += "?v=" + WEAVE_VERSION;
-      Svc.Prefs.set("lastPing", now);
-    }
-
-    // Figure out what the last modified time is for each collection
-    let info = this._fetchInfo(infoURL);
-
-    // Convert the response to an object and read out the modified times
-    for each (let engine in [Clients].concat(Engines.getAll()))
-      engine.lastModified = info.obj[engine.name] || 0;
-
-    if (!(this._remoteSetup(info)))
-      throw "aborting sync, remote setup failed";
-
-    // Make sure we have an up-to-date list of clients before sending commands
-    this._log.debug("Refreshing client list.");
-    if (!this._syncEngine(Clients)) {
-      // Clients is an engine like any other; it can fail with a 401,
-      // and we can elect to abort the sync.
-      this._log.warn("Client engine sync failed. Aborting.");
-      return;
-    }
-
-    // Wipe data in the desired direction if necessary
-    switch (Svc.Prefs.get("firstSync")) {
-      case "resetClient":
-        this.resetClient(Engines.getEnabled().map(function(e) e.name));
-        break;
-      case "wipeClient":
-        this.wipeClient(Engines.getEnabled().map(function(e) e.name));
-        break;
-      case "wipeRemote":
-        this.wipeRemote(Engines.getEnabled().map(function(e) e.name));
-        break;
-    }
-
-    if (Clients.localCommands) {
-      try {
-        if (!(Clients.processIncomingCommands())) {
-          Status.sync = ABORT_SYNC_COMMAND;
-          throw "aborting sync, process commands said so";
-        }
-
-        // Repeat remoteSetup in-case the commands forced us to reset
-        if (!(this._remoteSetup(info)))
-          throw "aborting sync, remote setup failed after processing commands";
-      }
-      finally {
-        // Always immediately attempt to push back the local client (now
-        // without commands).
-        // Note that we don't abort here; if there's a 401 because we've
-        // been reassigned, we'll handle it around another engine.
-        this._syncEngine(Clients);
-      }
-    }
-
-    // Update engines because it might change what we sync.
-    try {
-      this._updateEnabledEngines();
-    } catch (ex) {
-      this._log.debug("Updating enabled engines failed: " +
-                      Utils.exceptionStr(ex));
-      ErrorHandler.checkServerError(ex);
-      throw ex;
-    }
-
-    try {
-      for each (let engine in Engines.getEnabled()) {
-        // If there's any problems with syncing the engine, report the failure
-        if (!(this._syncEngine(engine)) || Status.enforceBackoff) {
-          this._log.info("Aborting sync");
-          break;
-        }
-      }
+  _lockedSync: function _lockedSync() {
+    return this._lock("service.js: sync",
+                      this._notify("sync", "", function onNotify() {
 
-      // If _syncEngine fails for a 401, we might not have a cluster URL here.
-      // If that's the case, break out of this immediately, rather than
-      // throwing an exception when trying to fetch metaURL.
-      if (!this.clusterURL) {
-        this._log.debug("Aborting sync, no cluster URL: " +
-                        "not uploading new meta/global.");
-        return;
-      }
-
-      // Upload meta/global if any engines changed anything
-      let meta = Records.get(this.metaURL);
-      if (meta.isNew || meta.changed) {
-        new Resource(this.metaURL).put(meta);
-        delete meta.isNew;
-        delete meta.changed;
-      }
-
-      // If there were no sync engine failures
-      if (Status.service != SYNC_FAILED_PARTIAL) {
-        Svc.Prefs.set("lastSync", new Date().toString());
-        Status.sync = SYNC_SUCCEEDED;
-      }
-    } finally {
-      Svc.Prefs.reset("firstSync");
-
-      let syncTime = ((Date.now() - syncStartTime) / 1000).toFixed(2);
-      let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);
-      this._log.info("Sync completed at " + dateStr
-                     + " after " + syncTime + " secs.");
-    }
-  }))(),
-
-
-  _updateEnabledEngines: function _updateEnabledEngines() {
-    this._log.info("Updating enabled engines: " + SyncScheduler.numClients + " clients.");
-    let meta = Records.get(this.metaURL);
-    if (meta.isNew || !meta.payload.engines)
-      return;
-
-    // If we're the only client, and no engines are marked as enabled,
-    // thumb our noses at the server data: it can't be right.
-    // Belt-and-suspenders approach to Bug 615926.
-    if ((SyncScheduler.numClients <= 1) &&
-        ([e for (e in meta.payload.engines) if (e != "clients")].length == 0)) {
-      this._log.info("One client and no enabled engines: not touching local engine status.");
-      return;
-    }
-
-    this._ignorePrefObserver = true;
+      let synchronizer = new EngineSynchronizer(this);
+      let cb = Async.makeSpinningCallback();
+      synchronizer.onComplete = cb;
 
-    let enabled = [eng.name for each (eng in Engines.getEnabled())];
-    for (let engineName in meta.payload.engines) {
-      if (engineName == "clients") {
-        // Clients is special.
-        continue;
-      }
-      let index = enabled.indexOf(engineName);
-      if (index != -1) {
-        // The engine is enabled locally. Nothing to do.
-        enabled.splice(index, 1);
-        continue;
-      }
-      let engine = Engines.get(engineName);
-      if (!engine) {
-        // The engine doesn't exist locally. Nothing to do.
-        continue;
-      }
-
-      if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
-        // The engine was disabled locally. Wipe server data and
-        // disable it everywhere.
-        this._log.trace("Wiping data for " + engineName + " engine.");
-        engine.wipeServer();
-        delete meta.payload.engines[engineName];
-        meta.changed = true;
-      } else {
-        // The engine was enabled remotely. Enable it locally.
-        this._log.trace(engineName + " engine was enabled remotely.");
-        engine.enabled = true;
-      }
-    }
-
-    // Any remaining engines were either enabled locally or disabled remotely.
-    for each (let engineName in enabled) {
-      let engine = Engines.get(engineName);
-      if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
-        this._log.trace("The " + engineName + " engine was enabled locally.");
-      } else {
-        this._log.trace("The " + engineName + " engine was disabled remotely.");
-        engine.enabled = false;
-      }
-    }
-
-    Svc.Prefs.resetBranch("engineStatusChanged.");
-    this._ignorePrefObserver = false;
-  },
-
-  // Returns true if sync should proceed.
-  // false / no return value means sync should be aborted.
-  _syncEngine: function _syncEngine(engine) {
-    try {
-      engine.sync();
-    }
-    catch(e) {
-      if (e.status == 401) {
-        // Maybe a 401, cluster update perhaps needed?
-        // We rely on ErrorHandler observing the sync failure notification to
-        // schedule another sync and clear node assignment values.
-        // Here we simply want to muffle the exception and return an
-        // appropriate value.
-        return false;
-      }
-    }
-    return true;
+      synchronizer.sync();
+      // wait() throws if the first argument is truthy, which is exactly what
+      // we want.
+      let result = cb.wait();
+    }))();
   },
 
   /**
    * If we have a passphrase, rather than a 25-alphadigit sync key,
    * use the provided sync ID to bootstrap it using PBKDF2.
    *
    * Store the new 'passphrase' back into the identity manager.
    *
new file mode 100644
--- /dev/null
+++ b/services/sync/modules/stages/enginesync.js
@@ -0,0 +1,277 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * This file contains code for synchronizing engines.
+ */
+
+const EXPORTED_SYMBOLS = ["EngineSynchronizer"];
+
+const {utils: Cu} = Components;
+
+Cu.import("resource://services-common/log4moz.js");
+Cu.import("resource://services-sync/constants.js");
+Cu.import("resource://services-sync/engines.js");
+Cu.import("resource://services-sync/engines/clients.js");
+Cu.import("resource://services-sync/policies.js");
+Cu.import("resource://services-sync/record.js");
+Cu.import("resource://services-sync/resource.js");
+Cu.import("resource://services-sync/status.js");
+Cu.import("resource://services-sync/util.js");
+
+/**
+ * Perform synchronization of engines.
+ *
+ * This was originally split out of service.js. The API needs lots of love.
+ */
+function EngineSynchronizer(service) {
+  this._log = Log4Moz.repository.getLogger("Sync.Synchronizer");
+  this._log.level = Log4Moz.Level[Svc.Prefs.get("log.logger.synchronizer")];
+
+  this.service = service;
+
+  this.onComplete = null;
+}
+
+EngineSynchronizer.prototype = {
+  sync: function sync() {
+    if (!this.onComplete) {
+      throw new Error("onComplete handler not installed.");
+    }
+
+    let startTime = Date.now();
+
+    Status.resetSync();
+
+    // Make sure we should sync or record why we shouldn't.
+    let reason = this.service._checkSync();
+    if (reason) {
+      if (reason == kSyncNetworkOffline) {
+        Status.sync = LOGIN_FAILED_NETWORK_ERROR;
+      }
+
+      // this is a purposeful abort rather than a failure, so don't set
+      // any status bits
+      reason = "Can't sync: " + reason;
+      this.onComplete(new Error("Can't sync: " + reason));
+      return;
+    }
+
+    // If we don't have a node, get one. If that fails, retry in 10 minutes.
+    if (this.service.clusterURL == "" && !this.service._setCluster()) {
+      Status.sync = NO_SYNC_NODE_FOUND;
+      this._log.info("No cluster URL found. Cannot sync.");
+      this.onComplete(null);
+      return;
+    }
+
+    // Ping the server with a special info request once a day.
+    let infoURL = this.service.infoURL;
+    let now = Math.floor(Date.now() / 1000);
+    let lastPing = Svc.Prefs.get("lastPing", 0);
+    if (now - lastPing > 86400) { // 60 * 60 * 24
+      infoURL += "?v=" + WEAVE_VERSION;
+      Svc.Prefs.set("lastPing", now);
+    }
+
+    // Figure out what the last modified time is for each collection
+    let info = this.service._fetchInfo(infoURL);
+
+    // Convert the response to an object and read out the modified times
+    for (let engine of [Clients].concat(Engines.getAll())) {
+      engine.lastModified = info.obj[engine.name] || 0;
+    }
+
+    if (!(this.service._remoteSetup(info))) {
+      this.onComplete(new Error("Aborting sync, remote setup failed"));
+      return;
+    }
+
+    // Make sure we have an up-to-date list of clients before sending commands
+    this._log.debug("Refreshing client list.");
+    if (!this._syncEngine(Clients)) {
+      // Clients is an engine like any other; it can fail with a 401,
+      // and we can elect to abort the sync.
+      this._log.warn("Client engine sync failed. Aborting.");
+      this.onComplete(null);
+      return;
+    }
+
+    // Wipe data in the desired direction if necessary
+    switch (Svc.Prefs.get("firstSync")) {
+      case "resetClient":
+        this.service.resetClient(Engines.getEnabled().map(function(e) e.name));
+        break;
+      case "wipeClient":
+        this.service.wipeClient(Engines.getEnabled().map(function(e) e.name));
+        break;
+      case "wipeRemote":
+        this.service.wipeRemote(Engines.getEnabled().map(function(e) e.name));
+        break;
+    }
+
+    if (Clients.localCommands) {
+      try {
+        if (!(Clients.processIncomingCommands())) {
+          Status.sync = ABORT_SYNC_COMMAND;
+          this.onComplete(new Error("Processed command aborted sync."));
+          return;
+        }
+
+        // Repeat remoteSetup in-case the commands forced us to reset
+        if (!(this.service._remoteSetup(info))) {
+          this.onComplete(new Error("Remote setup failed after processing commands."));
+          return;
+        }
+      }
+      finally {
+        // Always immediately attempt to push back the local client (now
+        // without commands).
+        // Note that we don't abort here; if there's a 401 because we've
+        // been reassigned, we'll handle it around another engine.
+        this._syncEngine(Clients);
+      }
+    }
+
+    // Update engines because it might change what we sync.
+    try {
+      this._updateEnabledEngines();
+    } catch (ex) {
+      this._log.debug("Updating enabled engines failed: " +
+                      Utils.exceptionStr(ex));
+      ErrorHandler.checkServerError(ex);
+      this.onComplete(ex);
+      return;
+    }
+
+    try {
+      for each (let engine in Engines.getEnabled()) {
+        // If there's any problems with syncing the engine, report the failure
+        if (!(this._syncEngine(engine)) || Status.enforceBackoff) {
+          this._log.info("Aborting sync for failure in " + engine.name);
+          break;
+        }
+      }
+
+      // If _syncEngine fails for a 401, we might not have a cluster URL here.
+      // If that's the case, break out of this immediately, rather than
+      // throwing an exception when trying to fetch metaURL.
+      if (!this.service.clusterURL) {
+        this._log.debug("Aborting sync, no cluster URL: " +
+                        "not uploading new meta/global.");
+        this.onComplete(null);
+        return;
+      }
+
+      // Upload meta/global if any engines changed anything
+      let meta = Records.get(this.service.metaURL);
+      if (meta.isNew || meta.changed) {
+        new Resource(this.service.metaURL).put(meta);
+        delete meta.isNew;
+        delete meta.changed;
+      }
+
+      // If there were no sync engine failures
+      if (Status.service != SYNC_FAILED_PARTIAL) {
+        Svc.Prefs.set("lastSync", new Date().toString());
+        Status.sync = SYNC_SUCCEEDED;
+      }
+    } finally {
+      Svc.Prefs.reset("firstSync");
+
+      let syncTime = ((Date.now() - startTime) / 1000).toFixed(2);
+      let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);
+      this._log.info("Sync completed at " + dateStr
+                     + " after " + syncTime + " secs.");
+    }
+
+    this.onComplete(null);
+  },
+
+  // Returns true if sync should proceed.
+  // false / no return value means sync should be aborted.
+  _syncEngine: function _syncEngine(engine) {
+    try {
+      engine.sync();
+    }
+    catch(e) {
+      if (e.status == 401) {
+        // Maybe a 401, cluster update perhaps needed?
+        // We rely on ErrorHandler observing the sync failure notification to
+        // schedule another sync and clear node assignment values.
+        // Here we simply want to muffle the exception and return an
+        // appropriate value.
+        return false;
+      }
+    }
+
+    return true;
+  },
+
+  _updateEnabledEngines: function _updateEnabledEngines() {
+    this._log.info("Updating enabled engines: " + SyncScheduler.numClients + " clients.");
+    let meta = Records.get(this.service.metaURL);
+    if (meta.isNew || !meta.payload.engines)
+      return;
+
+    // If we're the only client, and no engines are marked as enabled,
+    // thumb our noses at the server data: it can't be right.
+    // Belt-and-suspenders approach to Bug 615926.
+    if ((SyncScheduler.numClients <= 1) &&
+        ([e for (e in meta.payload.engines) if (e != "clients")].length == 0)) {
+      this._log.info("One client and no enabled engines: not touching local engine status.");
+      return;
+    }
+
+    this.service._ignorePrefObserver = true;
+
+    let enabled = [eng.name for each (eng in Engines.getEnabled())];
+    for (let engineName in meta.payload.engines) {
+      if (engineName == "clients") {
+        // Clients is special.
+        continue;
+      }
+      let index = enabled.indexOf(engineName);
+      if (index != -1) {
+        // The engine is enabled locally. Nothing to do.
+        enabled.splice(index, 1);
+        continue;
+      }
+      let engine = Engines.get(engineName);
+      if (!engine) {
+        // The engine doesn't exist locally. Nothing to do.
+        continue;
+      }
+
+      if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
+        // The engine was disabled locally. Wipe server data and
+        // disable it everywhere.
+        this._log.trace("Wiping data for " + engineName + " engine.");
+        engine.wipeServer();
+        delete meta.payload.engines[engineName];
+        meta.changed = true;
+      } else {
+        // The engine was enabled remotely. Enable it locally.
+        this._log.trace(engineName + " engine was enabled remotely.");
+        engine.enabled = true;
+      }
+    }
+
+    // Any remaining engines were either enabled locally or disabled remotely.
+    for each (let engineName in enabled) {
+      let engine = Engines.get(engineName);
+      if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
+        this._log.trace("The " + engineName + " engine was enabled locally.");
+      } else {
+        this._log.trace("The " + engineName + " engine was disabled remotely.");
+        engine.enabled = false;
+      }
+    }
+
+    Svc.Prefs.resetBranch("engineStatusChanged.");
+    this.service._ignorePrefObserver = false;
+  },
+
+};
+Object.freeze(EngineSynchronizer.prototype);
--- a/services/sync/tests/unit/test_load_modules.js
+++ b/services/sync/tests/unit/test_load_modules.js
@@ -1,8 +1,11 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
 const modules = [
   "addonutils.js",
   "addonsreconciler.js",
   "constants.js",
   "engines/addons.js",
   "engines/bookmarks.js",
   "engines/clients.js",
   "engines/forms.js",
@@ -16,16 +19,17 @@ const modules = [
   "keys.js",
   "main.js",
   "notifications.js",
   "policies.js",
   "record.js",
   "resource.js",
   "rest.js",
   "service.js",
+  "stages/enginesync.js",
   "status.js",
   "util.js",
 ];
 
 function run_test() {
   for each (let m in modules) {
     _("Attempting to load resource://services-sync/" + m);
     Cu.import("resource://services-sync/" + m, {});