Bug 712542 - Apply add-on state before install when installing through Sync; r=Unfocused, r=rnewman
authorGregory Szorc <gps@mozilla.com>
Mon, 20 Feb 2012 14:53:03 -0800
changeset 87256 dec32ab636c6bf0d9afaaa89ee3581d8b7044181
parent 87255 1e6b5a9bbdccddd000666c7f5724d1aacd74455f
child 87257 a22548d04fc3cd10c339a39243a66060c554b08b
push id22103
push userbmo@edmorley.co.uk
push dateTue, 21 Feb 2012 12:01:45 +0000
treeherdermozilla-central@4038ffaa5d82 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersUnfocused, rnewman
bugs712542
milestone13.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 712542 - Apply add-on state before install when installing through Sync; r=Unfocused, r=rnewman
services/sync/modules/engines/addons.js
services/sync/tests/unit/addon1-search.xml
services/sync/tests/unit/test_addons_engine.js
services/sync/tests/unit/test_addons_store.js
services/sync/tps/extensions/tps/modules/addons.jsm
--- 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) {