Bug 1366994 - prevent appDisabled addons from hanging sync. r?tcsc draft
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 26 May 2017 12:42:10 +1000
changeset 584844 f82e459688b3ea69233fb9d049173603b640a1aa
parent 584216 f81bcc23d37d7bec48f08b19a9327e93c54d37b5
child 630531 2a43a83c98d5a4fc154d84a9e3f95dd2744b4b3a
push id60899
push userbmo:markh@mozilla.com
push dateFri, 26 May 2017 02:42:51 +0000
reviewerstcsc
bugs1366994
milestone55.0a1
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
@@ -606,17 +606,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");