Bug 1366994 - prevent appDisabled addons from hanging sync. r=tcsc
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 26 May 2017 12:42:10 +1000
changeset 409541 c25cbb6d95e8e31afe7427de7ac580a7b1776cc6
parent 409540 043aebcd2dbbcdee90253617c809afd4ada3d86e
child 409542 501a1089d16723c1a9945e17a3d786b41de1415f
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1366994
milestone55.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 1366994 - prevent appDisabled addons from hanging sync. r=tcsc MozReview-Commit-ID: 4RRCPlwUZrG
services/sync/modules/addonutils.js
services/sync/modules/engines/addons.js
services/sync/modules/policies.js
services/sync/tests/unit/test_addons_store.js
--- a/services/sync/modules/addonutils.js
+++ b/services/sync/modules/addonutils.js
@@ -390,114 +390,30 @@ AddonUtilsInternal.prototype = {
     this._log.info(`Add-on "${addon.id}" is able to be installed`);
     return true;
   },
 
 
   /**
    * Update the user disabled flag for an add-on.
    *
-   * The supplied callback will be called when the operation is
-   * complete. If the new flag matches the existing or if the add-on
-   * isn't currently active, the function will fire the callback
-   * immediately. Else, the callback is invoked when the AddonManager
-   * reports the change has taken effect or has been registered.
-   *
-   * The callback receives as arguments:
-   *
-   *   (Error) Encountered error during operation or null on success.
-   *   (Addon) The add-on instance being operated on.
+   * If the new flag matches the existing or if the add-on
+   * isn't currently active, the function will return immediately.
    *
    * @param addon
    *        (Addon) Add-on instance to operate on.
    * @param value
    *        (bool) New value for add-on's userDisabled property.
-   * @param cb
-   *        (function) Callback to be invoked on completion.
    */
-  updateUserDisabled: function updateUserDisabled(addon, value, cb) {
+  updateUserDisabled(addon, value) {
     if (addon.userDisabled == value) {
-      cb(null, addon);
       return;
     }
 
-    let listener = {
-      onEnabling: (wrapper, needsRestart) => {
-        this._log.debug("onEnabling: " + wrapper.id);
-        if (wrapper.id != addon.id) {
-          return;
-        }
-
-        // We ignore the restartless case because we'll get onEnabled shortly.
-        if (!needsRestart) {
-          return;
-        }
-
-        AddonManager.removeAddonListener(listener);
-        cb(null, wrapper);
-      },
-
-      onEnabled: wrapper => {
-        this._log.debug("onEnabled: " + wrapper.id);
-        if (wrapper.id != addon.id) {
-          return;
-        }
-
-        AddonManager.removeAddonListener(listener);
-        cb(null, wrapper);
-      },
-
-      onDisabling: (wrapper, needsRestart) => {
-        this._log.debug("onDisabling: " + wrapper.id);
-        if (wrapper.id != addon.id) {
-          return;
-        }
-
-        if (!needsRestart) {
-          return;
-        }
-
-        AddonManager.removeAddonListener(listener);
-        cb(null, wrapper);
-      },
-
-      onDisabled: wrapper => {
-        this._log.debug("onDisabled: " + wrapper.id);
-        if (wrapper.id != addon.id) {
-          return;
-        }
-
-        AddonManager.removeAddonListener(listener);
-        cb(null, wrapper);
-      },
-
-      onOperationCancelled: wrapper => {
-        this._log.debug("onOperationCancelled: " + wrapper.id);
-        if (wrapper.id != addon.id) {
-          return;
-        }
-
-        AddonManager.removeAddonListener(listener);
-        cb(new Error("Operation cancelled"), wrapper);
-      }
-    };
-
-    // The add-on listeners are only fired if the add-on is active. If not, the
-    // change is silently updated and made active when/if the add-on is active.
-
-    if (!addon.appDisabled) {
-      AddonManager.addAddonListener(listener);
-    }
-
     this._log.info("Updating userDisabled flag: " + addon.id + " -> " + value);
     addon.userDisabled = !!value;
-
-    if (!addon.appDisabled) {
-      cb(null, addon);
-    }
-    // Else the listener will handle invoking the callback.
   },
 
 };
 
 XPCOMUtils.defineLazyGetter(this, "AddonUtils", function() {
   return new AddonUtilsInternal();
 });
--- a/services/sync/modules/engines/addons.js
+++ b/services/sync/modules/engines/addons.js
@@ -380,19 +380,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.
     }
 
-    let cb = Async.makeSpinningCallback();
-    this.updateUserDisabled(addon, !record.enabled, cb);
-    cb.wait();
+    this.updateUserDisabled(addon, !record.enabled);
   },
 
   /**
    * Provide core Store API to determine if a record exists.
    */
   itemExists: function itemExists(guid) {
     let addon = this.reconciler.getAddonStateFromSyncGUID(guid);
 
@@ -657,44 +655,44 @@ AddonsStore.prototype = {
     }
 
     return true;
   },
 
   /**
    * Update the userDisabled flag on an add-on.
    *
-   * This will enable or disable an add-on and call the supplied callback when
-   * the action is complete. If no action is needed, the callback gets called
-   * immediately.
+   * This will enable or disable an add-on. It has no return value and does
+   * 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.
-   * @param callback
-   *        Function to be called when action is complete. Will receive 2
-   *        arguments, a truthy value that signifies error, and the Addon
-   *        instance passed to this function.
    */
-  updateUserDisabled: function updateUserDisabled(addon, value, callback) {
+  updateUserDisabled(addon, value) {
     if (addon.userDisabled == value) {
-      callback(null, addon);
       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);
-      callback(null, addon);
       return;
     }
 
-    AddonUtils.updateUserDisabled(addon, value, callback);
+    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);
+    }
   },
 };
 
 /**
  * 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.
  */
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -608,17 +608,18 @@ ErrorHandler.prototype = {
     this._log.level = Log.Level[Svc.Prefs.get("log.logger.service.main")];
 
     let root = Log.repository.getLogger("Sync");
     root.level = Log.Level[Svc.Prefs.get("log.rootLogger")];
 
     let logs = ["Sync", "FirefoxAccounts", "Hawk", "Common.TokenServerClient",
                 "Sync.SyncMigration", "browserwindow.syncui",
                 "Services.Common.RESTRequest", "Services.Common.RESTRequest",
-                "BookmarkSyncUtils"
+                "BookmarkSyncUtils",
+                "addons.xpi",
                ];
 
     this._logManager = new LogManager(Svc.Prefs, logs, "sync");
   },
 
   observe: function observe(subject, topic, data) {
     this._log.trace("Handling " + topic);
     switch (topic) {
--- a/services/sync/tests/unit/test_addons_store.js
+++ b/services/sync/tests/unit/test_addons_store.js
@@ -87,16 +87,27 @@ function createAndStartHTTPServer(port) 
   } catch (ex) {
     _("Got exception starting HTTP server on port " + port);
     _("Error: " + Log.exceptionStr(ex));
     do_throw(ex);
   }
   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) {
+  let stateBefore = Object.assign({}, store.reconciler.addons[addon.id]);
+  store.reconciler.rectifyStateFromAddon(addon);
+  let stateAfter = store.reconciler.addons[addon.id];
+  deepEqual(stateBefore, stateAfter);
+}
+
 function run_test() {
   initTestLogging("Trace");
   Log.repository.getLogger("Sync.Engine.Addons").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.Tracker.Addons").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.AddonsRepository").level =
     Log.Level.Trace;
 
   reconciler.startListening();
@@ -132,40 +143,75 @@ add_test(function test_apply_enabled() {
 
   _("Ensure application of a disable record works as expected.");
   let records = [];
   records.push(createRecordForThisApp(addon.syncGUID, addon.id, false, false));
   let failed = store.applyIncomingBatch(records);
   do_check_eq(0, failed.length);
   addon = getAddonFromAddonManagerByID(addon.id);
   do_check_true(addon.userDisabled);
+  checkReconcilerUpToDate(addon);
   records = [];
 
   _("Ensure enable record works as expected.");
   records.push(createRecordForThisApp(addon.syncGUID, addon.id, true, false));
   failed = store.applyIncomingBatch(records);
   do_check_eq(0, failed.length);
   addon = getAddonFromAddonManagerByID(addon.id);
   do_check_false(addon.userDisabled);
+  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 = store.applyIncomingBatch(records);
   do_check_eq(0, failed.length);
   addon = getAddonFromAddonManagerByID(addon.id);
   do_check_false(addon.userDisabled);
   records = [];
 
   uninstallAddon(addon);
   Svc.Prefs.reset("addons.ignoreUserEnabledChanges");
   run_next_test();
 });
 
+add_test(function test_apply_enabled_appDisabled() {
+  _("Ensures that changes to the userEnabled flag apply when the addon is appDisabled.");
+
+  let addon = installAddon("test_install3"); // this addon is appDisabled by default.
+  do_check_true(addon.appDisabled);
+  do_check_false(addon.isActive);
+  do_check_false(addon.userDisabled);
+
+  _("Ensure application of a disable record works as expected.");
+  store.reconciler.pruneChangesBeforeDate(Date.now() + 10);
+  store.reconciler._changes = [];
+  let records = [];
+  records.push(createRecordForThisApp(addon.syncGUID, addon.id, false, false));
+  let failed = store.applyIncomingBatch(records);
+  do_check_eq(0, failed.length);
+  addon = getAddonFromAddonManagerByID(addon.id);
+  do_check_true(addon.userDisabled);
+  checkReconcilerUpToDate(addon);
+  records = [];
+
+  _("Ensure enable record works as expected.");
+  records.push(createRecordForThisApp(addon.syncGUID, addon.id, true, false));
+  failed = store.applyIncomingBatch(records);
+  do_check_eq(0, failed.length);
+  addon = getAddonFromAddonManagerByID(addon.id);
+  do_check_false(addon.userDisabled);
+  checkReconcilerUpToDate(addon);
+  records = [];
+
+  uninstallAddon(addon);
+  run_next_test();
+});
+
 add_test(function test_ignore_different_appid() {
   _("Ensure that incoming records with a different application ID are ignored.");
 
   // We test by creating a record that should result in an update.
   let addon = installAddon("test_bootstrap1_1");
   do_check_false(addon.userDisabled);
 
   let record = createRecordForThisApp(addon.syncGUID, addon.id, false, false);
@@ -333,32 +379,42 @@ add_test(function test_ignore_hotfixes()
   run_next_test();
 });
 
 
 add_test(function test_get_all_ids() {
   _("Ensures that getAllIDs() returns an appropriate set.");
 
   _("Installing two addons.");
+  // XXX - this test seems broken - at this point, before we've installed the
+  // addons below, store.getAllIDs() returns all addons installed by previous
+  // tests, even though those tests uninstalled the addon.
+  // So if any tests above ever add a new addon ID, they are going to need to
+  // be added here too.
+  // do_check_eq(0, Object.keys(store.getAllIDs()).length);
   let addon1 = installAddon("test_install1");
   let addon2 = installAddon("test_bootstrap1_1");
+  let addon3 = installAddon("test_install3");
 
   _("Ensure they're syncable.");
   do_check_true(store.isAddonSyncable(addon1));
   do_check_true(store.isAddonSyncable(addon2));
+  do_check_true(store.isAddonSyncable(addon3));
 
   let ids = store.getAllIDs();
 
   do_check_eq("object", typeof(ids));
-  do_check_eq(2, Object.keys(ids).length);
+  do_check_eq(3, Object.keys(ids).length);
   do_check_true(addon1.syncGUID in ids);
   do_check_true(addon2.syncGUID in ids);
+  do_check_true(addon3.syncGUID in ids);
 
   addon1.install.cancel();
   uninstallAddon(addon2);
+  uninstallAddon(addon3);
 
   run_next_test();
 });
 
 add_test(function test_change_item_id() {
   _("Ensures that changeItemID() works properly.");
 
   let addon = installAddon("test_bootstrap1_1");