Bug 1366994 - prevent appDisabled addons from hanging sync. r?tcsc
MozReview-Commit-ID: 4RRCPlwUZrG
--- 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");