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 88770 dec32ab636c6bf0d9afaaa89ee3581d8b7044181
parent 88769 1e6b5a9bbdccddd000666c7f5724d1aacd74455f
child 88771 a22548d04fc3cd10c339a39243a66060c554b08b
push id975
push userffxbld
push dateTue, 13 Mar 2012 21:39:16 +0000
treeherdermozilla-aurora@99faebf9dc36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersUnfocused, rnewman
bugs712542
milestone13.0a1
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) {