Bug 792990 - Properly handle add-ons when resetting Sync; r=rnewman
authorGregory Szorc <gps@mozilla.com>
Fri, 21 Sep 2012 11:22:59 -0700
changeset 111089 3e2c523a5c6fba13c2215e315c5498b71f2d103f
parent 111088 3c8a88b1c092c6fa38034b60a8adebebd1da187d
child 111090 9c064d735c2345e6473448f238485e928531288f
push id93
push usernmatsakis@mozilla.com
push dateWed, 31 Oct 2012 21:26:57 +0000
reviewersrnewman
bugs792990
milestone18.0a1
Bug 792990 - Properly handle add-ons when resetting Sync; r=rnewman Due to a bug in the add-on sync implementation, resetting Sync would cause all add-ons to be uninstalled and not replaced with the server data.
services/sync/modules/engines/addons.js
services/sync/tests/tps/all_tests.json
services/sync/tests/tps/test_addon_wipe.js
services/sync/tests/unit/test_addons_store.js
--- a/services/sync/modules/engines/addons.js
+++ b/services/sync/modules/engines/addons.js
@@ -332,25 +332,37 @@ AddonsStore.prototype = {
   },
 
   /**
    * Provides core Store API to update an add-on from a record.
    */
   update: function update(record) {
     let addon = this.getAddonByID(record.addonID);
 
-    // This should only happen if there is a race condition between an add-on
-    // being uninstalled locally and Sync calling this function after it
-    // determines an add-on exists.
+    // update() is called if !this.itemExists. And, since itemExists consults
+    // the reconciler only, we need to take care of some corner cases.
+    //
+    // First, the reconciler could know about an add-on that was uninstalled
+    // and no longer present in the add-ons manager.
     if (!addon) {
-      this._log.warn("Requested to update record but add-on not found: " +
-                     record.addonID);
+      this.create(record);
       return;
     }
 
+    // It's also possible that the add-on is non-restartless and has pending
+    // install/uninstall activity.
+    //
+    // 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();
   },
 
   /**
    * Provide core Store API to determine if a record exists.
    */
--- a/services/sync/tests/tps/all_tests.json
+++ b/services/sync/tests/tps/all_tests.json
@@ -20,13 +20,14 @@
     "test_privbrw_passwords.js",
     "test_privbrw_tabs.js",
     "test_bookmarks_in_same_named_folder.js",
     "test_client_wipe.js",
     "test_special_tabs.js",
     "test_addon_sanity.js",
     "test_addon_restartless_xpi.js",
     "test_addon_nonrestartless_xpi.js",
-    "test_addon_reconciling.js"
+    "test_addon_reconciling.js",
+    "test_addon_wipe.js"
   ]
 }
 
 
new file mode 100644
--- /dev/null
+++ b/services/sync/tests/tps/test_addon_wipe.js
@@ -0,0 +1,34 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// This test ensures that a client wipe followed by an "initial" sync will
+// restore add-ons. This test should expose flaws in the reconciling logic,
+// specifically around AddonsReconciler. This test is in response to bug
+// 792990.
+
+EnableEngines(["addons"]);
+
+let phases = {
+  "phase01": "profile1",
+  "phase02": "profile1",
+  "phase03": "profile1"
+};
+
+const id1 = "restartless-xpi@tests.mozilla.org";
+const id2 = "unsigned-xpi@tests.mozilla.org";
+
+Phase("phase01", [
+  [Addons.install, [id1]],
+  [Addons.install, [id2]],
+  [Sync]
+]);
+Phase("phase02", [
+  [Addons.verify, [id1], STATE_ENABLED],
+  [Addons.verify, [id2], STATE_ENABLED],
+  [Sync, SYNC_WIPE_CLIENT],
+  [Sync]
+]);
+Phase("phase03", [
+  [Addons.verify, [id1], STATE_ENABLED],
+  [Addons.verify, [id2], STATE_ENABLED]
+]);
--- a/services/sync/tests/unit/test_addons_store.js
+++ b/services/sync/tests/unit/test_addons_store.js
@@ -415,8 +415,34 @@ add_test(function test_wipe() {
 
   let addon = getAddonFromAddonManagerByID(addon1.id);
   do_check_eq(null, addon);
 
   Svc.Prefs.reset("addons.ignoreRepositoryChecking");
 
   run_next_test();
 });
+
+add_test(function test_wipe_and_install() {
+  _("Ensure wipe followed by install works.");
+
+  // This tests the reset sync flow where remote data is replaced by local. The
+  // receiving client will see a wipe followed by a record which should undo
+  // the wipe.
+  let installed = installAddon("test_bootstrap1_1");
+
+  let record = createRecordForThisApp(installed.syncGUID, installed.id, true,
+                                      false);
+
+  Svc.Prefs.set("addons.ignoreRepositoryChecking", true);
+  store.wipe();
+
+  let deleted = getAddonFromAddonManagerByID(installed.id);
+  do_check_null(deleted);
+
+  store.applyIncoming(record);
+
+  let fetched = getAddonFromAddonManagerByID(record.addonID);
+  do_check_true(!!fetched);
+
+  Svc.Prefs.reset("addons.ignoreRepositoryChecking");
+  run_next_test();
+});