Bug 633062 p7 - Make most of the addon reconciler async. r?markh draft
authorEdouard Oger <eoger@fastmail.com>
Thu, 07 Dec 2017 17:56:50 -0500
changeset 749592 50c4253bac2d31455407d66ca9da65d05b0b3296
parent 749591 0ea264c5e47ff45a1e2c1ca30a3122e7c9116011
child 749593 e1655fd29cfc446184c7ce71a192ad61aba1b9be
child 749611 efb4ea0b7e86e96bebdfb1a183ca2fb0be225fc2
push id97449
push userbmo:eoger@fastmail.com
push dateWed, 31 Jan 2018 19:28:47 +0000
reviewersmarkh
bugs633062
milestone60.0a1
Bug 633062 p7 - Make most of the addon reconciler async. r?markh Except observers, which will be handled in the last commit of this series. MozReview-Commit-ID: IvwyleXBcNH
services/sync/modules/addonsreconciler.js
services/sync/modules/engines/addons.js
services/sync/tests/unit/test_addons_engine.js
services/sync/tests/unit/test_addons_store.js
--- a/services/sync/modules/addonsreconciler.js
+++ b/services/sync/modules/addonsreconciler.js
@@ -324,17 +324,17 @@ AddonsReconciler.prototype = {
 
     let installs;
     let addons = await AddonManager.getAllAddons();
 
     let ids = {};
 
     for (let addon of addons) {
       ids[addon.id] = true;
-      this.rectifyStateFromAddon(addon);
+      await this.rectifyStateFromAddon(addon);
     }
 
     // Look for locally-defined add-ons that no longer exist and update their
     // record.
     for (let [id, addon] of Object.entries(this._addons)) {
       if (id in ids) {
         continue;
       }
@@ -360,17 +360,17 @@ AddonsReconciler.prototype = {
       if (installFound) {
         continue;
       }
 
       if (addon.installed) {
         addon.installed = false;
         this._log.debug("Adding change because add-on not present in " +
                         "Add-on Manager: " + id);
-        this._addChange(new Date(), CHANGE_UNINSTALLED, addon);
+        await this._addChange(new Date(), CHANGE_UNINSTALLED, addon);
       }
     }
 
     // See note for _shouldPersist.
     if (this._shouldPersist) {
       await this.saveState();
     }
   },
@@ -382,17 +382,17 @@ AddonsReconciler.prototype = {
    * apply changes to the local state to reflect it."
    *
    * This function could result in change listeners being called if the local
    * state differs from the passed add-on's state.
    *
    * @param addon
    *        Addon instance being updated.
    */
-  rectifyStateFromAddon(addon) {
+  async rectifyStateFromAddon(addon) {
     this._log.debug(`Rectifying state for addon ${addon.name} (version=${addon.version}, id=${addon.id})`);
 
     let id = addon.id;
     let enabled = !addon.userDisabled;
     let guid = addon.syncGUID;
     let now = new Date();
 
     if (!(id in this._addons)) {
@@ -405,17 +405,17 @@ AddonsReconciler.prototype = {
         type: addon.type,
         scope: addon.scope,
         foreignInstall: addon.foreignInstall,
         isSyncable: addon.isSyncable,
       };
       this._addons[id] = record;
       this._log.debug("Adding change because add-on not present locally: " +
                       id);
-      this._addChange(now, CHANGE_INSTALLED, record);
+      await this._addChange(now, CHANGE_INSTALLED, record);
       return;
     }
 
     let record = this._addons[id];
     record.isSyncable = addon.isSyncable;
 
     if (!record.installed) {
       // It is possible the record is marked as uninstalled because an
@@ -426,17 +426,17 @@ AddonsReconciler.prototype = {
       }
     }
 
     if (record.enabled != enabled) {
       record.enabled = enabled;
       record.modified = now;
       let change = enabled ? CHANGE_ENABLED : CHANGE_DISABLED;
       this._log.debug("Adding change because enabled state changed: " + id);
-      this._addChange(new Date(), change, record);
+      await this._addChange(new Date(), change, record);
     }
 
     if (record.guid != guid) {
       record.guid = guid;
       // We don't record a change because the Sync engine rectifies this on its
       // own. This is tightly coupled with Sync. If this code is ever lifted
       // outside of Sync, this exception should likely be removed.
     }
@@ -447,25 +447,25 @@ AddonsReconciler.prototype = {
    *
    * @param date
    *        Date at which the change occurred.
    * @param change
    *        The type of the change. A CHANGE_* constant.
    * @param state
    *        The new state of the add-on. From this.addons.
    */
-  _addChange: function _addChange(date, change, state) {
+  async _addChange(date, change, state) {
     this._log.info("Change recorded for " + state.id);
     this._changes.push([date, change, state.id]);
 
     for (let listener of this._listeners) {
       try {
-        listener.changeListener(date, change, state);
+        await listener.changeListener(date, change, state);
       } catch (ex) {
-        this._log.warn("Exception calling change listener", ex);
+        this._log.error("Exception calling change listener", ex);
       }
     }
   },
 
   /**
    * Obtain the set of changes to add-ons since the date passed.
    *
    * This will return an array of arrays. Each entry in the array has the
@@ -527,17 +527,17 @@ AddonsReconciler.prototype = {
     }
 
     return null;
   },
 
   /**
    * Handler that is invoked as part of the AddonManager listeners.
    */
-  _handleListener(action, addon, requiresRestart) {
+  async _handleListener(action, addon, requiresRestart) {
     // Since this is called as an observer, we explicitly trap errors and
     // log them to ourselves so we don't see errors reported elsewhere.
     try {
       let id = addon.id;
       this._log.debug("Add-on change: " + action + " to " + id);
 
       // We assume that every event for non-restartless add-ons is
       // followed by another event and that this follow-up event is the most
@@ -551,69 +551,69 @@ AddonsReconciler.prototype = {
       switch (action) {
         case "onEnabling":
         case "onEnabled":
         case "onDisabling":
         case "onDisabled":
         case "onInstalled":
         case "onInstallEnded":
         case "onOperationCancelled":
-          this.rectifyStateFromAddon(addon);
+          await this.rectifyStateFromAddon(addon);
           break;
 
         case "onUninstalling":
         case "onUninstalled":
           let id = addon.id;
           let addons = this.addons;
           if (id in addons) {
             let now = new Date();
             let record = addons[id];
             record.installed = false;
             record.modified = now;
             this._log.debug("Adding change because of uninstall listener: " +
                             id);
-            this._addChange(now, CHANGE_UNINSTALLED, record);
+            await this._addChange(now, CHANGE_UNINSTALLED, record);
           }
       }
 
       // See note for _shouldPersist.
       if (this._shouldPersist) {
-        Async.promiseSpinningly(this.saveState());
+        await this.saveState();
       }
     } catch (ex) {
       this._log.warn("Exception", ex);
     }
   },
 
   // AddonListeners
   onEnabling: function onEnabling(addon, requiresRestart) {
-    this._handleListener("onEnabling", addon, requiresRestart);
+    Async.promiseSpinningly(this._handleListener("onEnabling", addon, requiresRestart));
   },
   onEnabled: function onEnabled(addon) {
-    this._handleListener("onEnabled", addon);
+    Async.promiseSpinningly(this._handleListener("onEnabled", addon));
   },
   onDisabling: function onDisabling(addon, requiresRestart) {
-    this._handleListener("onDisabling", addon, requiresRestart);
+    Async.promiseSpinningly(this._handleListener("onDisabling", addon, requiresRestart));
   },
   onDisabled: function onDisabled(addon) {
-    this._handleListener("onDisabled", addon);
+    Async.promiseSpinningly(this._handleListener("onDisabled", addon));
   },
   onInstalling: function onInstalling(addon, requiresRestart) {
-    this._handleListener("onInstalling", addon, requiresRestart);
+    Async.promiseSpinningly(this._handleListener("onInstalling", addon, requiresRestart));
   },
   onInstalled: function onInstalled(addon) {
-    this._handleListener("onInstalled", addon);
+    Async.promiseSpinningly(this._handleListener("onInstalled", addon));
   },
   onUninstalling: function onUninstalling(addon, requiresRestart) {
-    this._handleListener("onUninstalling", addon, requiresRestart);
+    Async.promiseSpinningly(this._handleListener("onUninstalling", addon, requiresRestart));
   },
   onUninstalled: function onUninstalled(addon) {
-    this._handleListener("onUninstalled", addon);
+    Async.promiseSpinningly(this._handleListener("onUninstalled", addon));
   },
   onOperationCancelled: function onOperationCancelled(addon) {
-    this._handleListener("onOperationCancelled", addon);
+    Async.promiseSpinningly(this._handleListener("onOperationCancelled", addon));
   },
 
   // InstallListeners
   onInstallEnded: function onInstallEnded(install, addon) {
-    this._handleListener("onInstallEnded", addon);
+    Async.promiseSpinningly(this._handleListener("onInstallEnded", addon));
   }
 };
--- a/services/sync/modules/engines/addons.js
+++ b/services/sync/modules/engines/addons.js
@@ -43,17 +43,16 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource://gre/modules/Preferences.jsm");
 ChromeUtils.import("resource://services-sync/addonutils.js");
 ChromeUtils.import("resource://services-sync/addonsreconciler.js");
 ChromeUtils.import("resource://services-sync/engines.js");
 ChromeUtils.import("resource://services-sync/record.js");
 ChromeUtils.import("resource://services-sync/util.js");
 ChromeUtils.import("resource://services-sync/constants.js");
 ChromeUtils.import("resource://services-sync/collection_validator.js");
-ChromeUtils.import("resource://services-common/async.js");
 
 ChromeUtils.defineModuleGetter(this, "AddonManager",
                                "resource://gre/modules/AddonManager.jsm");
 ChromeUtils.defineModuleGetter(this, "AddonRepository",
                                "resource://gre/modules/addons/AddonRepository.jsm");
 
 this.EXPORTED_SYMBOLS = ["AddonsEngine", "AddonValidator"];
 
@@ -377,17 +376,17 @@ AddonsStore.prototype = {
     // We wouldn't get here if the incoming record was for a deletion. So,
     // check for pending uninstall and cancel if necessary.
     if (addon.pendingOperations & AddonManager.PENDING_UNINSTALL) {
       addon.cancelUninstall();
 
       // We continue with processing because there could be state or ID change.
     }
 
-    this.updateUserDisabled(addon, !record.enabled);
+    await this.updateUserDisabled(addon, !record.enabled);
   },
 
   /**
    * Provide core Store API to determine if a record exists.
    */
   async itemExists(guid) {
     let addon = this.reconciler.getAddonStateFromSyncGUID(guid);
 
@@ -646,34 +645,34 @@ AddonsStore.prototype = {
    * not catch or handle exceptions thrown by the addon manager. If no action
    * is needed it will return immediately.
    *
    * @param addon
    *        Addon instance to manipulate.
    * @param value
    *        Boolean to which to set userDisabled on the passed Addon.
    */
-  updateUserDisabled(addon, value) {
+  async updateUserDisabled(addon, value) {
     if (addon.userDisabled == value) {
       return;
     }
 
     // A pref allows changes to the enabled flag to be ignored.
     if (Svc.Prefs.get("addons.ignoreUserEnabledChanges", false)) {
       this._log.info("Ignoring enabled state change due to preference: " +
                      addon.id);
       return;
     }
 
     AddonUtils.updateUserDisabled(addon, value);
     // updating this flag doesn't send a notification for appDisabled addons,
     // meaning the reconciler will not update its state and may resync the
     // addon - so explicitly rectify the state (bug 1366994)
     if (addon.appDisabled) {
-      this.reconciler.rectifyStateFromAddon(addon);
+      await this.reconciler.rectifyStateFromAddon(addon);
     }
   },
 };
 
 /**
  * The add-ons tracker keeps track of real-time changes to add-ons.
  *
  * It hooks up to the reconciler and receives notifications directly from it.
@@ -691,24 +690,24 @@ AddonsTracker.prototype = {
   get store() {
     return this.engine._store;
   },
 
   /**
    * This callback is executed whenever the AddonsReconciler sends out a change
    * notification. See AddonsReconciler.addChangeListener().
    */
-  changeListener: function changeHandler(date, change, addon) {
+  async changeListener(date, change, addon) {
     this._log.debug("changeListener invoked: " + change + " " + addon.id);
     // Ignore changes that occur during sync.
     if (this.ignoreAll) {
       return;
     }
 
-    if (!Async.promiseSpinningly(this.store.isAddonSyncable(addon))) {
+    if (!(await this.store.isAddonSyncable(addon))) {
       this._log.debug("Ignoring change because add-on isn't syncable: " +
                       addon.id);
       return;
     }
 
     if (this.addChangedID(addon.guid, date.getTime() / 1000)) {
       this.score += SCORE_INCREMENT_XLARGE;
     }
--- a/services/sync/tests/unit/test_addons_engine.js
+++ b/services/sync/tests/unit/test_addons_engine.js
@@ -138,17 +138,17 @@ add_task(async function test_get_changed
     enabled:        true,
     installed:      true,
     modified:       new Date(),
     type:           "UNSUPPORTED",
     scope:          0,
     foreignInstall: false
   };
   reconciler.addons.DUMMY = record;
-  reconciler._addChange(record.modified, CHANGE_INSTALLED, record);
+  await reconciler._addChange(record.modified, CHANGE_INSTALLED, record);
 
   changes = await engine.getChangedIDs();
   _(JSON.stringify(changes));
   Assert.equal(0, Object.keys(changes).length);
 
   await resetReconciler();
 });
 
--- a/services/sync/tests/unit/test_addons_store.js
+++ b/services/sync/tests/unit/test_addons_store.js
@@ -90,19 +90,19 @@ function createAndStartHTTPServer(port) 
   }
   return null; /* not hit, but keeps eslint happy! */
 }
 
 // A helper function to ensure that the reconciler's current view of the addon
 // is the same as the addon itself. If it's not, then the reconciler missed a
 // change, and is likely to re-upload the addon next sync because of the change
 // it missed.
-function checkReconcilerUpToDate(addon) {
+async function checkReconcilerUpToDate(addon) {
   let stateBefore = Object.assign({}, store.reconciler.addons[addon.id]);
-  store.reconciler.rectifyStateFromAddon(addon);
+  await store.reconciler.rectifyStateFromAddon(addon);
   let stateAfter = store.reconciler.addons[addon.id];
   deepEqual(stateBefore, stateAfter);
 }
 
 add_task(async function setup() {
   await Service.engineManager.register(AddonsEngine);
   engine     = Service.engineManager.get("addons");
   tracker    = engine._tracker;
@@ -138,26 +138,26 @@ add_task(async function test_apply_enabl
 
   _("Ensure application of a disable record works as expected.");
   let records = [];
   records.push(createRecordForThisApp(addon.syncGUID, addon.id, false, false));
   let failed = await store.applyIncomingBatch(records);
   Assert.equal(0, failed.length);
   addon = await AddonManager.getAddonByID(addon.id);
   Assert.ok(addon.userDisabled);
-  checkReconcilerUpToDate(addon);
+  await checkReconcilerUpToDate(addon);
   records = [];
 
   _("Ensure enable record works as expected.");
   records.push(createRecordForThisApp(addon.syncGUID, addon.id, true, false));
   failed = await store.applyIncomingBatch(records);
   Assert.equal(0, failed.length);
   addon = await AddonManager.getAddonByID(addon.id);
   Assert.ok(!addon.userDisabled);
-  checkReconcilerUpToDate(addon);
+  await checkReconcilerUpToDate(addon);
   records = [];
 
   _("Ensure enabled state updates don't apply if the ignore pref is set.");
   records.push(createRecordForThisApp(addon.syncGUID, addon.id, false, false));
   Svc.Prefs.set("addons.ignoreUserEnabledChanges", true);
   failed = await store.applyIncomingBatch(records);
   Assert.equal(0, failed.length);
   addon = await AddonManager.getAddonByID(addon.id);
@@ -180,26 +180,26 @@ add_task(async function test_apply_enabl
   store.reconciler.pruneChangesBeforeDate(Date.now() + 10);
   store.reconciler._changes = [];
   let records = [];
   records.push(createRecordForThisApp(addon.syncGUID, addon.id, false, false));
   let failed = await store.applyIncomingBatch(records);
   Assert.equal(0, failed.length);
   addon = await AddonManager.getAddonByID(addon.id);
   Assert.ok(addon.userDisabled);
-  checkReconcilerUpToDate(addon);
+  await checkReconcilerUpToDate(addon);
   records = [];
 
   _("Ensure enable record works as expected.");
   records.push(createRecordForThisApp(addon.syncGUID, addon.id, true, false));
   failed = await store.applyIncomingBatch(records);
   Assert.equal(0, failed.length);
   addon = await AddonManager.getAddonByID(addon.id);
   Assert.ok(!addon.userDisabled);
-  checkReconcilerUpToDate(addon);
+  await checkReconcilerUpToDate(addon);
   records = [];
 
   await uninstallAddon(addon);
 });
 
 add_task(async function test_ignore_different_appid() {
   _("Ensure that incoming records with a different application ID are ignored.");