Bug 1286915 - Implement a validator for the sync addons engine r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 22 Sep 2016 17:29:01 -0400
changeset 417702 fb4af9d5b395e9a6104dcb563a0879a1df7343dd
parent 417692 14b0ea0c7b1cecaf522b813e47e9ffdee4c1eec3
child 532142 957f3793ab29e6cae53ecf2afb0674da9c81d0f3
push id30459
push userbmo:tchiovoloni@mozilla.com
push dateMon, 26 Sep 2016 16:35:09 +0000
reviewersmarkh
bugs1286915
milestone52.0a1
Bug 1286915 - Implement a validator for the sync addons engine r?markh MozReview-Commit-ID: 2sGNs9BUoXt
services/sync/modules/engines/addons.js
services/sync/tests/tps/test_addon_nonrestartless_xpi.js
services/sync/tests/tps/test_addon_reconciling.js
services/sync/tests/tps/test_addon_sanity.js
services/sync/tests/tps/test_addon_wipe.js
services/sync/tps/extensions/tps/resource/tps.jsm
--- a/services/sync/modules/engines/addons.js
+++ b/services/sync/modules/engines/addons.js
@@ -39,26 +39,27 @@
 var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://services-sync/addonutils.js");
 Cu.import("resource://services-sync/addonsreconciler.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/constants.js");
+Cu.import("resource://services-sync/collection_validator.js");
 Cu.import("resource://services-common/async.js");
 
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
                                   "resource://gre/modules/AddonManager.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "AddonRepository",
                                   "resource://gre/modules/addons/AddonRepository.jsm");
 
-this.EXPORTED_SYMBOLS = ["AddonsEngine"];
+this.EXPORTED_SYMBOLS = ["AddonsEngine", "AddonValidator"];
 
 // 7 days in milliseconds.
 const PRUNE_ADDON_CHANGES_THRESHOLD = 60 * 60 * 24 * 7 * 1000;
 
 /**
  * AddonRecord represents the state of an add-on in an application.
  *
  * Each add-on has its own record for each application ID it is installed
@@ -171,17 +172,17 @@ AddonsEngine.prototype = {
         continue;
       }
 
       // Keep newest modified time.
       if (id in changes && changeTime < changes[id]) {
           continue;
       }
 
-      if (!this._store.isAddonSyncable(addons[id])) {
+      if (!this.isAddonSyncable(addons[id])) {
         continue;
       }
 
       this._log.debug("Adding changed add-on from changes log: " + id);
       let addon = addons[id];
       changes[addon.guid] = changeTime.getTime() / 1000;
     }
 
@@ -229,16 +230,20 @@ AddonsEngine.prototype = {
    * This will synchronously load the reconciler's state from the file
    * system (if needed) and refresh the state of the reconciler.
    */
   _refreshReconcilerState: function _refreshReconcilerState() {
     this._log.debug("Refreshing reconciler state");
     let cb = Async.makeSpinningCallback();
     this._reconciler.refreshGlobalState(cb);
     cb.wait();
+  },
+
+  isAddonSyncable(addon, ignoreRepoCheck) {
+    return this._store.isAddonSyncable(addon, ignoreRepoCheck);
   }
 };
 
 /**
  * This is the primary interface between Sync and the Addons Manager.
  *
  * In addition to the core store APIs, we provide convenience functions to wrap
  * Add-on Manager APIs with Sync-specific semantics.
@@ -528,19 +533,22 @@ AddonsStore.prototype = {
     return Async.waitForSyncCallback(cb);
   },
 
   /**
    * Determines whether an add-on is suitable for Sync.
    *
    * @param  addon
    *         Addon instance
+   * @param ignoreRepoCheck
+   *         Should we skip checking the Addons repository (primarially useful
+   *         for testing and validation).
    * @return Boolean indicating whether it is appropriate for Sync
    */
-  isAddonSyncable: function isAddonSyncable(addon) {
+  isAddonSyncable: function isAddonSyncable(addon, ignoreRepoCheck = false) {
     // Currently, we limit syncable add-ons to those that are:
     //   1) In a well-defined set of types
     //   2) Installed in the current profile
     //   3) Not installed by a foreign entity (i.e. installed by the app)
     //      since they act like global extensions.
     //   4) Is not a hotfix.
     //   5) The addons XPIProvider doesn't veto it (i.e not being installed in
     //      the profile directory, or any other reasons it says the addon can't
@@ -587,18 +595,19 @@ AddonsStore.prototype = {
     // tests are fixed (but keeping it doesn't hurt either)
     if (this._extensionsPrefs.get("hotfix.id", null) == addon.id) {
       this._log.debug(addon.id + " not syncable: is a hotfix.");
       return false;
     }
 
     // If the AddonRepository's cache isn't enabled (which it typically isn't
     // in tests), getCachedAddonByID always returns null - so skip the check
-    // in that case.
-    if (!AddonRepository.cacheEnabled) {
+    // in that case. We also provide a way to specifically opt-out of the check
+    // even if the cache is enabled, which is used by the validators.
+    if (ignoreRepoCheck || !AddonRepository.cacheEnabled) {
       return true;
     }
 
     let cb = Async.makeSyncCallback();
     AddonRepository.getCachedAddonByID(addon.id, cb);
     let result = Async.waitForSyncCallback(cb);
 
     if (!result) {
@@ -731,8 +740,74 @@ AddonsTracker.prototype = {
     this.reconciler.addChangeListener(this);
   },
 
   stopTracking: function() {
     this.reconciler.removeChangeListener(this);
     this.reconciler.stopListening();
   },
 };
+
+class AddonValidator extends CollectionValidator {
+  constructor(engine = null) {
+    super("addons", "id", [
+      "addonID",
+      "enabled",
+      "applicationID",
+      "source"
+    ]);
+    this.engine = engine;
+  }
+
+  getClientItems() {
+    return Promise.all([
+      new Promise(resolve =>
+        AddonManager.getAllAddons(resolve)),
+      new Promise(resolve =>
+        AddonManager.getAddonsWithOperationsByTypes(["extension", "theme"], resolve)),
+    ]).then(([installed, addonsWithPendingOperation]) => {
+      // Addons pending install won't be in the first list, but addons pending
+      // uninstall/enable/disable will be in both lists.
+      let all = new Map(installed.map(addon => [addon.id, addon]));
+      for (let addon of addonsWithPendingOperation) {
+        all.set(addon.id, addon);
+      }
+      // Convert to an array since Map.prototype.values returns an iterable
+      return [...all.values()];
+    });
+  }
+
+  normalizeClientItem(item) {
+    let enabled = !item.userDisabled;
+    if (item.pendingOperations & AddonManager.PENDING_ENABLE) {
+      enabled = true;
+    } else if (item.pendingOperations & AddonManager.PENDING_DISABLE) {
+      enabled = false;
+    }
+    return {
+      enabled,
+      id: item.syncGUID,
+      addonID: item.id,
+      applicationID: Services.appinfo.ID,
+      source: "amo", // check item.foreignInstall?
+      original: item
+    };
+  }
+
+  normalizeServerItem(item) {
+    let guid = this.engine._findDupe(item);
+    if (guid) {
+      item.id = guid;
+    }
+    return item;
+  }
+
+  clientUnderstands(item) {
+    return item.applicationID === Services.appinfo.ID;
+  }
+
+  syncedByClient(item) {
+    return !item.original.hidden &&
+           !item.original.isSystem &&
+           !(item.original.pendingOperations & AddonManager.PENDING_UNINSTALL) &&
+           this.engine.isAddonSyncable(item.original, true);
+  }
+}
--- a/services/sync/tests/tps/test_addon_nonrestartless_xpi.js
+++ b/services/sync/tests/tps/test_addon_nonrestartless_xpi.js
@@ -28,70 +28,78 @@ var phases = {
 const id = "unsigned-xpi@tests.mozilla.org";
 
 Phase("phase01", [
   [Addons.verifyNot, [id]],
   [Addons.install, [id]],
   [Sync]
 ]);
 Phase("phase02", [
-  [Addons.verify, [id], STATE_ENABLED]
+  [Addons.verify, [id], STATE_ENABLED],
+  [Sync]
 ]);
 Phase("phase03", [
   [Addons.verifyNot, [id]],
   [Sync]
 ]);
 Phase("phase04", [
   [Addons.verify, [id], STATE_ENABLED],
+  [Sync]
 ]);
 
 // Now we disable the add-on
 Phase("phase05", [
   [EnsureTracking],
   [Addons.setEnabled, [id], STATE_DISABLED],
   [Sync]
 ]);
 Phase("phase06", [
   [Addons.verify, [id], STATE_DISABLED],
+  [Sync]
 ]);
 Phase("phase07", [
   [Addons.verify, [id], STATE_ENABLED],
   [Sync]
 ]);
 Phase("phase08", [
-  [Addons.verify, [id], STATE_DISABLED]
+  [Addons.verify, [id], STATE_DISABLED],
+  [Sync]
 ]);
 
 // Now we re-enable it again.
 Phase("phase09", [
   [EnsureTracking],
   [Addons.setEnabled, [id], STATE_ENABLED],
   [Sync]
 ]);
 Phase("phase10", [
   [Addons.verify, [id], STATE_ENABLED],
+  [Sync]
 ]);
 Phase("phase11", [
   [Addons.verify, [id], STATE_DISABLED],
   [Sync]
 ]);
 Phase("phase12", [
-  [Addons.verify, [id], STATE_ENABLED]
+  [Addons.verify, [id], STATE_ENABLED],
+  [Sync]
 ]);
 
 // And we uninstall it
 
 Phase("phase13", [
   [EnsureTracking],
   [Addons.verify, [id], STATE_ENABLED],
   [Addons.uninstall, [id]],
   [Sync]
 ]);
 Phase("phase14", [
-  [Addons.verifyNot, [id]]
+  [Addons.verifyNot, [id]],
+  [Sync]
 ]);
 Phase("phase15", [
   [Addons.verify, [id], STATE_ENABLED],
   [Sync]
 ]);
 Phase("phase16", [
-  [Addons.verifyNot, [id]]
+  [Addons.verifyNot, [id]],
+  [Sync]
 ]);
--- a/services/sync/tests/tps/test_addon_reconciling.js
+++ b/services/sync/tests/tps/test_addon_reconciling.js
@@ -29,16 +29,19 @@ Phase("phase02", [
   [Sync],
   [Addons.verify, [id], STATE_ENABLED]
 ]);
 
 // Now we disable in one and uninstall in the other.
 Phase("phase03", [
   [Sync], // Get GUID updates, potentially.
   [Addons.setEnabled, [id], STATE_DISABLED],
+  // We've changed the state, but don't want this profile to sync until phase5,
+  // so if we ran a validation now we'd be expecting to find errors.
+  [Addons.skipValidation]
 ]);
 Phase("phase04", [
   [EnsureTracking],
   [Addons.uninstall, [id]],
   [Sync]
 ]);
 
 // When we sync, the uninstall should take precedence because it was newer.
--- a/services/sync/tests/tps/test_addon_sanity.js
+++ b/services/sync/tests/tps/test_addon_sanity.js
@@ -20,10 +20,11 @@ Phase("phase1", [
   [Addons.verifyNot, [id]],
 
   // But it should be marked for Sync.
   [Sync]
 ]);
 
 Phase("phase2", [
   // Add-on should be present after restart
-  [Addons.verify, [id], STATE_ENABLED]
+  [Addons.verify, [id], STATE_ENABLED],
+  [Sync] // Sync to ensure everything is initialized enough for the addon validator to run
 ]);
--- a/services/sync/tests/tps/test_addon_wipe.js
+++ b/services/sync/tests/tps/test_addon_wipe.js
@@ -25,10 +25,11 @@ Phase("phase01", [
 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]
+  [Addons.verify, [id2], STATE_ENABLED],
+  [Sync] // Sync to ensure that the addon validator can run without error
 ]);
--- a/services/sync/tps/extensions/tps/resource/tps.jsm
+++ b/services/sync/tps/extensions/tps/resource/tps.jsm
@@ -21,16 +21,17 @@ Cu.import("resource://gre/modules/AppCon
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://services-common/async.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/main.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/bookmark_validator.js");
 Cu.import("resource://services-sync/engines/passwords.js");
 Cu.import("resource://services-sync/engines/forms.js");
+Cu.import("resource://services-sync/engines/addons.js");
 // TPS modules
 Cu.import("resource://tps/logger.jsm");
 
 // Module wrappers for tests
 Cu.import("resource://tps/modules/addons.jsm");
 Cu.import("resource://tps/modules/bookmarks.jsm");
 Cu.import("resource://tps/modules/forms.jsm");
 Cu.import("resource://tps/modules/history.jsm");
@@ -108,16 +109,17 @@ var TPS = {
   _syncErrors: 0,
   _syncWipeAction: null,
   _tabsAdded: 0,
   _tabsFinished: 0,
   _test: null,
   _triggeredSync: false,
   _usSinceEpoch: 0,
   _requestedQuit: false,
+  shouldValidateAddons: false,
   shouldValidateBookmarks: false,
   shouldValidatePasswords: false,
   shouldValidateForms: false,
 
   _init: function TPS__init() {
     // Check if Firefox Accounts is enabled
     let service = Cc["@mozilla.org/weave/service;1"]
                   .getService(Components.interfaces.nsISupports)
@@ -459,16 +461,17 @@ var TPS = {
     }
     catch(e) {
       DumpPasswords();
       throw(e);
     }
   },
 
   HandleAddons: function (addons, action, state) {
+    this.shouldValidateAddons = true;
     for (let entry of addons) {
       Logger.logInfo("executing action " + action.toUpperCase() +
                      " on addon " + JSON.stringify(entry));
       let addon = new Addon(this, entry);
       switch(action) {
         case ACTION_ADD:
           addon.install();
           break;
@@ -656,92 +659,93 @@ var TPS = {
       if (serverRecordDumpStr) {
         Logger.logInfo("Server bookmark records:\n" + serverRecordDumpStr + "\n");
       }
       this.DumpError("Bookmark validation failed", e);
     }
     Logger.logInfo("Bookmark validation finished");
   },
 
-  ValidatePasswords() {
-    let serverRecordDumpStr;
-    try {
-      Logger.logInfo("About to perform password validation");
-      let pwEngine = Weave.Service.engineManager.get("passwords");
-      let validator = new PasswordValidator();
-      let serverRecords = validator.getServerItems(pwEngine);
-      let clientRecords = Async.promiseSpinningly(validator.getClientItems());
-      serverRecordDumpStr = JSON.stringify(serverRecords);
-
-      let { problemData } = validator.compareClientWithServer(clientRecords, serverRecords);
-
-      for (let { name, count } of problemData.getSummary()) {
-        if (count) {
-          Logger.logInfo(`Validation problem: "${name}": ${JSON.stringify(problemData[name])}`);
-        }
-        Logger.AssertEqual(count, 0, `Password validation error of type ${name}`);
-      }
-    } catch (e) {
-      // Dump the client records (should always be doable)
-      DumpPasswords();
-      // Dump the server records if gotten them already.
-      if (serverRecordDumpStr) {
-        Logger.logInfo("Server password records:\n" + serverRecordDumpStr + "\n");
-      }
-      this.DumpError("Password validation failed", e);
-    }
-    Logger.logInfo("Password validation finished");
-  },
-
-  ValidateForms() {
+  ValidateCollection(engineName, ValidatorType) {
     let serverRecordDumpStr;
     let clientRecordDumpStr;
     try {
-      Logger.logInfo("About to perform form validation");
-      let engine = Weave.Service.engineManager.get("forms");
-      let validator = new FormValidator();
+      Logger.logInfo(`About to perform validation for "${engineName}"`);
+      let engine = Weave.Service.engineManager.get(engineName);
+      let validator = new ValidatorType(engine);
       let serverRecords = validator.getServerItems(engine);
       let clientRecords = Async.promiseSpinningly(validator.getClientItems());
-      clientRecordDumpStr = JSON.stringify(clientRecords, undefined, 2);
-      serverRecordDumpStr = JSON.stringify(serverRecords, undefined, 2);
+      try {
+        // This substantially improves the logs for addons while not making a
+        // substantial difference for the other two
+        clientRecordDumpStr = JSON.stringify(clientRecords.map(r => {
+          let res = validator.normalizeClientItem(r);
+          delete res.original; // Try and prevent cyclic references
+          return res;
+        }));
+      } catch (e) {
+        // ignore the error, the dump string is just here to make debugging easier.
+        clientRecordDumpStr = "<Cyclic value>";
+      }
+      try {
+        serverRecordDumpStr = JSON.stringify(serverRecords);
+      } catch (e) {
+        // as above
+        serverRecordDumpStr = "<Cyclic value>";
+      }
       let { problemData } = validator.compareClientWithServer(clientRecords, serverRecords);
       for (let { name, count } of problemData.getSummary()) {
         if (count) {
           Logger.logInfo(`Validation problem: "${name}": ${JSON.stringify(problemData[name])}`);
         }
-        Logger.AssertEqual(count, 0, `Form validation error of type ${name}`);
+        Logger.AssertEqual(count, 0, `Validation error for "${engineName}" of type "${name}"`);
       }
     } catch (e) {
       // Dump the client records if possible
       if (clientRecordDumpStr) {
-        Logger.logInfo("Client forms records:\n" + clientRecordDumpStr + "\n");
+        Logger.logInfo(`Client state for ${engineName}:\n${clientRecordDumpStr}\n`);
       }
       // Dump the server records if gotten them already.
       if (serverRecordDumpStr) {
-        Logger.logInfo("Server forms records:\n" + serverRecordDumpStr + "\n");
+        Logger.logInfo(`Server state for ${engineName}:\n${serverRecordDumpStr}\n`);
       }
-      this.DumpError("Form validation failed", e);
+      this.DumpError(`Validation failed for ${engineName}`, e);
     }
-    Logger.logInfo("Form validation finished");
+    Logger.logInfo(`Validation finished for ${engineName}`);
+  },
+
+  ValidatePasswords() {
+    return this.ValidateCollection("passwords", PasswordValidator);
+  },
+
+  ValidateForms() {
+    return this.ValidateCollection("forms", FormValidator);
+  },
+
+  ValidateAddons() {
+    return this.ValidateCollection("addons", AddonValidator);
   },
 
   RunNextTestAction: function() {
     try {
       if (this._currentAction >=
           this._phaselist[this._currentPhase].length) {
         // Run necessary validations and then finish up
         if (this.shouldValidateBookmarks) {
           this.ValidateBookmarks();
         }
         if (this.shouldValidatePasswords) {
           this.ValidatePasswords();
         }
         if (this.shouldValidateForms) {
           this.ValidateForms();
         }
+        if (this.shouldValidateAddons) {
+          this.ValidateAddons();
+        }
         // we're all done
         Logger.logInfo("test phase " + this._currentPhase + ": " +
                        (this._errors ? "FAIL" : "PASS"));
         this._phaseFinished = true;
         this.quit();
         return;
       }
 
@@ -1132,16 +1136,19 @@ var Addons = {
     TPS.HandleAddons(addons, ACTION_DELETE);
   },
   verify: function Addons__verify(addons, state) {
     TPS.HandleAddons(addons, ACTION_VERIFY, state);
   },
   verifyNot: function Addons__verifyNot(addons) {
     TPS.HandleAddons(addons, ACTION_VERIFY_NOT);
   },
+  skipValidation() {
+    TPS.shouldValidateAddons = false;
+  }
 };
 
 var Bookmarks = {
   add: function Bookmarks__add(bookmarks) {
     TPS.HandleBookmarks(bookmarks, ACTION_ADD);
   },
   modify: function Bookmarks__modify(bookmarks) {
     TPS.HandleBookmarks(bookmarks, ACTION_MODIFY);