Bug 712542 - Apply add-on state before install when installing through Sync; r=Unfocused, r=rnewman
--- a/services/sync/modules/engines/addons.js
+++ b/services/sync/modules/engines/addons.js
@@ -312,22 +312,22 @@ AddonsStore.prototype = {
Store.prototype.applyIncoming.call(this, record);
},
/**
* Provides core Store API to create/install an add-on from a record.
*/
create: function create(record) {
- // Ideally, we'd set syncGUID and userDisabled on install. For now, we
- // make the changes post-installation.
- // TODO Set syncGUID and userDisabled in one step, during install.
-
let cb = Async.makeSpinningCallback();
- this.installAddonsFromIDs([record.addonID], cb);
+ this.installAddons([{
+ id: record.addonID,
+ syncGUID: record.id,
+ enabled: record.enabled
+ }], cb);
// This will throw if there was an error. This will get caught by the sync
// engine and the record will try to be applied later.
let results = cb.wait();
let addon;
for each (let a in results.addons) {
if (a.id == record.addonID) {
@@ -337,22 +337,16 @@ AddonsStore.prototype = {
}
// This should never happen, but is present as a fail-safe.
if (!addon) {
throw new Error("Add-on not found after install: " + record.addonID);
}
this._log.info("Add-on installed: " + record.addonID);
- this._log.info("Setting add-on Sync GUID to remote: " + record.id);
- addon.syncGUID = record.id;
-
- cb = Async.makeSpinningCallback();
- this.updateUserDisabled(addon, !record.enabled, cb);
- cb.wait();
},
/**
* Provides core Store API to remove/uninstall an add-on from a record.
*/
remove: function remove(record) {
// If this is called, the payload is empty, so we have to find by GUID.
let addon = this.getAddonByGUID(record.id);
@@ -677,51 +671,80 @@ AddonsStore.prototype = {
addon.iconURL,
addon.version
);
},
/**
* Installs an add-on from an AddonSearchResult instance.
*
+ * The options argument defines extra options to control the install.
+ * Recognized keys in this map are:
+ *
+ * syncGUID - Sync GUID to use for the new add-on.
+ * enabled - Boolean indicating whether the add-on should be enabled upon
+ * install.
+ *
* When complete it calls a callback with 2 arguments, error and result.
*
* If error is falsy, result is an object. If error is truthy, result is
* null.
*
* The result object has the following keys:
*
* id ID of add-on that was installed.
* install AddonInstall that was installed.
* addon Addon that was installed.
*
* @param addon
* AddonSearchResult to install add-on from.
+ * @param options
+ * Object with additional metadata describing how to install add-on.
* @param cb
* Function to be invoked with result of operation.
*/
installAddonFromSearchResult:
- function installAddonFromSearchResult(addon, cb) {
+ function installAddonFromSearchResult(addon, options, cb) {
this._log.info("Trying to install add-on from search result: " + addon.id);
this.getInstallFromSearchResult(addon, function(error, install) {
if (error) {
cb(error, null);
return;
}
if (!install) {
cb(new Error("AddonInstall not available: " + addon.id), null);
return;
}
try {
this._log.info("Installing " + addon.id);
+ let log = this._log;
let listener = {
+ onInstallStarted: function(install) {
+ if (!options) {
+ return;
+ }
+
+ if (options.syncGUID) {
+ log.info("Setting syncGUID of " + install.name +": " +
+ options.syncGUID);
+ install.addon.syncGUID = options.syncGUID;
+ }
+
+ // We only need to change userDisabled if it is disabled because
+ // enabled is the default.
+ if ("enabled" in options && !options.enabled) {
+ log.info("Marking add-on as disabled for install: " +
+ install.name);
+ install.addon.userDisabled = true;
+ }
+ },
onInstallEnded: function(install, addon) {
install.removeListener(listener);
cb(null, {id: addon.id, install: install, addon: addon});
},
onInstallFailed: function(install) {
install.removeListener(listener);
@@ -887,42 +910,54 @@ AddonsStore.prototype = {
if (!addon.appDisabled) {
callback(null, addon);
return;
}
// Else the listener will handle invoking the callback.
},
/**
- * Installs multiple add-ons specified by their IDs.
+ * Installs multiple add-ons specified by metadata.
+ *
+ * The first argument is an array of objects. Each object must have the
+ * following keys:
+ *
+ * id - public ID of the add-on to install.
+ * syncGUID - syncGUID for new add-on.
+ * enabled - boolean indicating whether the add-on should be enabled.
*
* The callback will be called when activity on all add-ons is complete. The
* callback receives 2 arguments, error and result.
*
* If error is truthy, it contains a string describing the overall error.
*
* The 2nd argument to the callback is always an object with details on the
* overall execution state. It contains the following keys:
*
* installedIDs Array of add-on IDs that were installed.
* installs Array of AddonInstall instances that were installed.
* addons Array of Addon instances that were installed.
* errors Array of errors encountered. Only has elements if error is
* truthy.
*
- * @param ids
- * Array of add-on string IDs to install.
+ * @param installs
+ * Array of objects describing add-ons to install.
* @param cb
* Function to be called when all actions are complete.
*/
- installAddonsFromIDs: function installAddonsFromIDs(ids, cb) {
+ installAddons: function installAddons(installs, cb) {
if (!cb) {
throw new Error("Invalid argument: cb is not defined.");
}
+ let ids = [];
+ for each (let addon in installs) {
+ ids.push(addon.id);
+ }
+
AddonRepository.getAddonsByIDs(ids, {
searchSucceeded: function searchSucceeded(addons, addonsLength, total) {
this._log.info("Found " + addonsLength + "/" + ids.length +
" add-ons during repository search.");
let ourResult = {
installedIDs: [],
installs: [],
@@ -1004,17 +1039,25 @@ AddonsStore.prototype = {
if (!expectedInstallCount) {
cb(null, ourResult);
return;
}
// Start all the installs asynchronously. They will report back to us
// as they finish, eventually triggering the global callback.
for each (let addon in toInstall) {
- this.installAddonFromSearchResult(addon, installCallback);
+ let options = {};
+ for each (let install in installs) {
+ if (install.id == addon.id) {
+ options = install;
+ break;
+ }
+ }
+
+ this.installAddonFromSearchResult(addon, options, installCallback);
}
}.bind(this),
searchFailed: function searchFailed() {
cb(new Error("AddonRepository search failed"), null);
}.bind(this)
});
new file mode 100644
--- /dev/null
+++ b/services/sync/tests/unit/addon1-search.xml
@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<searchresults total_results="1">
+ <addon id="5617">
+ <name>Non-Restartless Test Extension</name>
+ <type id="1">Extension</type>
+ <guid>addon1@tests.mozilla.org</guid>
+ <slug>addon11</slug>
+ <version>1.0</version>
+
+ <compatible_applications><application>
+ <name>Firefox</name>
+ <application_id>1</application_id>
+ <min_version>3.6</min_version>
+ <max_version>*</max_version>
+ <appID>xpcshell@tests.mozilla.org</appID>
+ </application></compatible_applications>
+ <all_compatible_os><os>ALL</os></all_compatible_os>
+
+ <install os="ALL" size="485">http://127.0.0.1:8888/addon1.xpi</install>
+ <created epoch="1252903662">
+ 2009-09-14T04:47:42Z
+ </created>
+ <last_updated epoch="1315255329">
+ 2011-09-05T20:42:09Z
+ </last_updated>
+ </addon>
+</searchresults>
--- a/services/sync/tests/unit/test_addons_engine.js
+++ b/services/sync/tests/unit/test_addons_engine.js
@@ -3,16 +3,22 @@
"use strict";
Cu.import("resource://gre/modules/AddonManager.jsm");
Cu.import("resource://gre/modules/Services.jsm");
Cu.import("resource://services-sync/addonsreconciler.js");
Cu.import("resource://services-sync/async.js");
Cu.import("resource://services-sync/engines/addons.js");
+Cu.import("resource://services-sync/ext/Preferences.js");
+Cu.import("resource://services-sync/service.js");
+
+let prefs = new Preferences();
+prefs.set("extensions.getAddons.get.url",
+ "http://localhost:8888/search/guid:%IDS%");
loadAddonTestFunctions();
startupManager();
Engines.register(AddonsEngine);
let engine = Engines.get("addons");
let reconciler = engine._reconciler;
let tracker = engine._tracker;
@@ -129,18 +135,115 @@ add_test(function test_get_changed_ids()
changes = engine.getChangedIDs();
_(JSON.stringify(changes));
do_check_eq(0, Object.keys(changes).length);
advance_test();
});
+add_test(function test_disabled_install_semantics() {
+ _("Ensure that syncing a disabled add-on preserves proper state.");
+
+ // This is essentially a test for bug 712542, which snuck into the original
+ // add-on sync drop. It ensures that when an add-on is installed that the
+ // disabled state and incoming syncGUID is preserved, even on the next sync.
+
+ Svc.Prefs.set("addons.ignoreRepositoryChecking", true);
+
+ const USER = "foo";
+ const PASSWORD = "password";
+ const PASSPHRASE = "abcdeabcdeabcdeabcdeabcdea";
+ const ADDON_ID = "addon1@tests.mozilla.org";
+
+ Service.username = USER;
+ Service.password = PASSWORD;
+ Service.passphrase = PASSPHRASE;
+ Service.serverURL = TEST_SERVER_URL;
+ Service.clusterURL = TEST_CLUSTER_URL;
+
+ generateNewKeys();
+
+ let contents = {
+ meta: {global: {engines: {addons: {version: engine.version,
+ syncID: engine.syncID}}}},
+ crypto: {},
+ addons: {}
+ };
+
+ let server = new SyncServer();
+ server.registerUser(USER, "password");
+ server.createContents(USER, contents);
+ server.start();
+
+ let amoServer = new nsHttpServer();
+ amoServer.registerFile("/search/guid:addon1%40tests.mozilla.org",
+ do_get_file("addon1-search.xml"));
+
+ let installXPI = ExtensionsTestPath("/addons/test_install1.xpi");
+ amoServer.registerFile("/addon1.xpi", do_get_file(installXPI));
+ amoServer.start(8888);
+
+ // Insert an existing record into the server.
+ let id = Utils.makeGUID();
+ let now = Date.now() / 1000;
+
+ let record = encryptPayload({
+ id: id,
+ applicationID: Services.appinfo.ID,
+ addonID: ADDON_ID,
+ enabled: false,
+ deleted: false,
+ source: "amo",
+ });
+ let wbo = new ServerWBO(id, record, now - 2);
+ server.insertWBO(USER, "addons", wbo);
+
+ _("Performing sync of add-ons engine.");
+ engine._sync();
+
+ // At this point the non-restartless extension should be staged for install.
+
+ // Don't need this server any more.
+ let cb = Async.makeSpinningCallback();
+ amoServer.stop(cb);
+ cb.wait();
+
+ // We ensure the reconciler has recorded the proper ID and enabled state.
+ let addon = reconciler.getAddonStateFromSyncGUID(id);
+ do_check_neq(null, addon);
+ do_check_eq(false, addon.enabled);
+
+ // We fake an app restart and perform another sync, just to make sure things
+ // are sane.
+ restartManager();
+
+ engine._sync();
+
+ // The client should not upload a new record. The old record should be
+ // retained and unmodified.
+ let collection = server.getCollection(USER, "addons");
+ do_check_eq(1, collection.count());
+
+ let payload = collection.payloads()[0];
+ do_check_neq(null, collection.wbo(id));
+ do_check_eq(ADDON_ID, payload.addonID);
+ do_check_false(payload.enabled);
+
+ server.stop(advance_test);
+});
+
function run_test() {
initTestLogging("Trace");
- Log4Moz.repository.getLogger("Sync.Engine.Addons").level = Log4Moz.Level.Trace;
+ Log4Moz.repository.getLogger("Sync.Engine.Addons").level =
+ Log4Moz.Level.Trace;
+ Log4Moz.repository.getLogger("Sync.Store.Addons").level = Log4Moz.Level.Trace;
+ Log4Moz.repository.getLogger("Sync.Tracker.Addons").level =
+ Log4Moz.Level.Trace;
Log4Moz.repository.getLogger("Sync.AddonsRepository").level =
Log4Moz.Level.Trace;
+ new SyncTestingInfrastructure();
+
reconciler.startListening();
advance_test();
}
--- a/services/sync/tests/unit/test_addons_store.js
+++ b/services/sync/tests/unit/test_addons_store.js
@@ -449,17 +449,17 @@ add_test(function test_source_uri_rewrit
Svc.Prefs.set("addons.ignoreRepositoryChecking", true);
// We resort to monkeypatching because of the API design.
let oldFunction = store.__proto__.installAddonFromSearchResult;
let installCalled = false;
store.__proto__.installAddonFromSearchResult =
- function testInstallAddon(addon, cb) {
+ function testInstallAddon(addon, metadata, cb) {
do_check_eq("http://127.0.0.1:8888/require.xpi?src=sync",
addon.sourceURI.spec);
installCalled = true;
store.getInstallFromSearchResult(addon, function (error, install) {
do_check_eq("http://127.0.0.1:8888/require.xpi?src=sync",
@@ -467,17 +467,17 @@ add_test(function test_source_uri_rewrit
cb(null, {id: addon.id, addon: addon, install: install});
});
};
let server = createAndStartHTTPServer(HTTP_PORT);
let installCallback = Async.makeSpinningCallback();
- store.installAddonsFromIDs(["rewrite@tests.mozilla.org"], installCallback);
+ store.installAddons([{id: "rewrite@tests.mozilla.org"}], installCallback);
installCallback.wait();
do_check_true(installCalled);
store.__proto__.installAddonFromSearchResult = oldFunction;
Svc.Prefs.reset("addons.ignoreRepositoryChecking");
server.stop(run_next_test);
});
@@ -487,17 +487,17 @@ add_test(function test_handle_empty_sour
Svc.Prefs.set("addons.ignoreRepositoryChecking", true);
let server = createAndStartHTTPServer(HTTP_PORT);
const ID = "missing-sourceuri@tests.mozilla.org";
let cb = Async.makeSpinningCallback();
- store.installAddonsFromIDs([ID], cb);
+ store.installAddons([{id: ID}], cb);
let result = cb.wait();
do_check_true("installedIDs" in result);
do_check_eq(0, result.installedIDs.length);
Svc.Prefs.reset("addons.ignoreRepositoryChecking");
server.stop(run_next_test);
});
--- a/services/sync/tps/extensions/tps/modules/addons.jsm
+++ b/services/sync/tps/extensions/tps/modules/addons.jsm
@@ -128,17 +128,17 @@ Addon.prototype = {
// For Install, the id parameter initially passed is really the filename
// for the addon's install .xml; we'll read the actual id from the .xml.
let cb = Async.makeSpinningCallback();
// We call the store's APIs for installation because it is simpler. If that
// API is broken, it should ideally be caught by an xpcshell test. But, if
// TPS tests fail, it's all the same: a genuite reported error.
let store = Engines.get("addons")._store;
- store.installAddonsFromIDs([this.id], cb);
+ store.installAddons([{id: this.id}], cb);
let result = cb.wait();
Logger.AssertEqual(1, result.installedIDs.length, "Exactly 1 add-on was installed.");
Logger.AssertEqual(this.id, result.installedIDs[0],
"Add-on was installed successfully: " + this.id);
},
setEnabled: function setEnabled(flag) {