Bug 1436637 - Revert back to a sync observer in Service. r?markh draft
authorEdouard Oger <eoger@fastmail.com>
Thu, 08 Feb 2018 12:14:46 +0800
changeset 752440 6e2231306f93ee643989acd2c22a76382654f6dc
parent 752350 8cc2427a322caa1e2c09ca3957335f88e573dc7a
push id98264
push userbmo:eoger@fastmail.com
push dateThu, 08 Feb 2018 06:03:00 +0000
reviewersmarkh
bugs1436637
milestone60.0a1
Bug 1436637 - Revert back to a sync observer in Service. r?markh MozReview-Commit-ID: 2X7mo3stuy7
services/sync/modules/service.js
services/sync/tests/unit/test_clients_engine.js
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -292,17 +292,16 @@ Sync11Service.prototype = {
     this.scheduler = new SyncScheduler(this);
     this.errorHandler = new ErrorHandler(this);
 
     this._log = Log.repository.getLogger("Sync.Service");
     this._log.manageLevelFromPref("services.sync.log.logger.service.main");
 
     this._log.info("Loading Weave " + WEAVE_VERSION);
 
-    this.asyncObserver = Async.asyncObserver(this, this._log);
     this._clusterManager = this.identity.createClusterManager(this);
     this.recordManager = new RecordManager(this);
 
     this.enabled = true;
 
     await this._registerEngines();
 
     let ua = Cc["@mozilla.org/network/protocol;1?name=http"].
@@ -310,21 +309,20 @@ Sync11Service.prototype = {
     this._log.info(ua);
 
     if (!this._checkCrypto()) {
       this.enabled = false;
       this._log.info("Could not load the Weave crypto component. Disabling " +
                       "Weave, since it will not work correctly.");
     }
 
-    Svc.Obs.add("weave:service:setup-complete", this.asyncObserver);
-    Svc.Obs.add("sync:collection_changed", this.asyncObserver); // Pulled from FxAccountsCommon
-    Svc.Obs.add("fxaccounts:device_disconnected", this.asyncObserver);
-    // We use a different synchronous observer to make testing easier.
-    Services.prefs.addObserver(PREFS_BRANCH + "engine.", this.prefObserver.bind(this));
+    Svc.Obs.add("weave:service:setup-complete", this);
+    Svc.Obs.add("sync:collection_changed", this); // Pulled from FxAccountsCommon
+    Svc.Obs.add("fxaccounts:device_disconnected", this);
+    Services.prefs.addObserver(PREFS_BRANCH + "engine.", this);
 
     if (!this.enabled) {
       this._log.info("Firefox Sync disabled.");
     }
 
     this._updateCachedURLs();
 
     let status = this._checkSetup();
@@ -408,53 +406,62 @@ Sync11Service.prototype = {
       } catch (ex) {
         this._log.warn("Could not register engine " + name, ex);
       }
     }
 
     this.engineManager.setDeclined(declined);
   },
 
-  async observe(subject, topic, data) {
-    try {
-      switch (topic) {
-        // Ideally this observer should be in the SyncScheduler, but it would require
-        // some work to know about the sync specific engines. We should move this there once it does.
-        case "sync:collection_changed":
-          // We check if we're running TPS here to avoid TPS failing because it
-          // couldn't get to get the sync lock, due to us currently syncing the
-          // clients engine.
-          if (data.includes("clients") && !Svc.Prefs.get("testing.tps", false)) {
-            // [] = clients collection only
-            await this.sync({why: "collection_changed", engines: []});
-          }
-          break;
-        case "fxaccounts:device_disconnected":
-          data = JSON.parse(data);
-          if (!data.isLocalDevice) {
-            await this.clientsEngine.updateKnownStaleClients();
-          }
-          break;
-        case "weave:service:setup-complete":
-          let status = this._checkSetup();
-          if (status != STATUS_DISABLED && status != CLIENT_NOT_CONFIGURED) {
-            this._startTracking();
-          }
-          break;
-      }
-    } catch (e) {
-      this._log.error(e);
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
+                                         Ci.nsISupportsWeakReference]),
+
+  observe(subject, topic, data) {
+    switch (topic) {
+      // Ideally this observer should be in the SyncScheduler, but it would require
+      // some work to know about the sync specific engines. We should move this there once it does.
+      case "sync:collection_changed":
+        // We check if we're running TPS here to avoid TPS failing because it
+        // couldn't get to get the sync lock, due to us currently syncing the
+        // clients engine.
+        if (data.includes("clients") && !Svc.Prefs.get("testing.tps", false)) {
+          // Sync in the background (it's fine not to wait on the returned promise
+          // because sync() has a lock).
+          // [] = clients collection only
+          this.sync({why: "collection_changed", engines: []}).catch(e => {
+            this._log.error(e);
+          });
+        }
+        break;
+      case "fxaccounts:device_disconnected":
+        data = JSON.parse(data);
+        if (!data.isLocalDevice) {
+          // Refresh the known stale clients list in the background.
+          this.clientsEngine.updateKnownStaleClients().catch(e => {
+            this._log.error(e);
+          });
+        }
+        break;
+      case "weave:service:setup-complete":
+        let status = this._checkSetup();
+        if (status != STATUS_DISABLED && status != CLIENT_NOT_CONFIGURED) {
+          this._startTracking();
+        }
+        break;
+      case "nsPref:changed":
+        if (this._ignorePrefObserver) {
+          return;
+        }
+        const engine = data.slice((PREFS_BRANCH + "engine.").length);
+        this._handleEngineStatusChanged(engine);
+        break;
     }
   },
 
-  prefObserver(subject, topic, data) {
-    if (this._ignorePrefObserver) {
-      return;
-    }
-    const engine = data.slice((PREFS_BRANCH + "engine.").length);
+  _handleEngineStatusChanged(engine) {
     this._log.trace("Status for " + engine + " engine changed.");
     if (Svc.Prefs.get("engineStatusChanged." + engine, false)) {
       // The enabled status being changed back to what it was before.
       Svc.Prefs.reset("engineStatusChanged." + engine);
     } else {
       // Remember that the engine status changed locally until the next sync.
       Svc.Prefs.set("engineStatusChanged." + engine, true);
     }
@@ -1089,17 +1096,16 @@ Sync11Service.prototype = {
 
     return reason;
   },
 
   async sync({engines, why} = {}) {
     let dateStr = Utils.formatTimestamp(new Date());
     this._log.debug("User-Agent: " + Utils.userAgent);
     await this.promiseInitialized;
-    await this.asyncObserver.promiseObserversComplete();
     this._log.info(`Starting sync at ${dateStr} in browser session ${browserSessionID}`);
     return this._catch(async function() {
       // Make sure we're logged in.
       if (this._shouldLogin()) {
         this._log.debug("In sync: should login.");
         if (!(await this.login())) {
           this._log.debug("Not syncing: login returned false.");
           return;
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -1749,23 +1749,21 @@ add_task(async function test_other_clien
   }
 });
 
 add_task(async function device_disconnected_notification_updates_known_stale_clients() {
   const spyUpdate = sinon.spy(engine, "updateKnownStaleClients");
 
   Services.obs.notifyObservers(null, "fxaccounts:device_disconnected",
                                JSON.stringify({ isLocalDevice: false }));
-  await Service.asyncObserver.promiseObserversComplete();
   ok(spyUpdate.calledOnce, "updateKnownStaleClients should be called");
   spyUpdate.reset();
 
   Services.obs.notifyObservers(null, "fxaccounts:device_disconnected",
                                JSON.stringify({ isLocalDevice: true }));
-  await Service.asyncObserver.promiseObserversComplete();
   ok(spyUpdate.notCalled, "updateKnownStaleClients should not be called");
 
   spyUpdate.restore();
 });
 
 add_task(async function update_known_stale_clients() {
   const makeFakeClient = (id) => ({ id, fxaDeviceId: `fxa-${id}` });
   const clients = [makeFakeClient("one"), makeFakeClient("two"), makeFakeClient("three")];