Bug 1286915 - Implement a validator for the sync addons engine. r=markh

MozReview-Commit-ID: 2sGNs9BUoXt

--HG--
extra : transplant_source : %FBJ%F9%D5%B3%95%E9%A6%10M%CBV%3A%08y%A1%DFsC%DD

services/sync/modules/engines/addons.js

@@ -44,6 +44,7 @@ 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");
@@ -53,7 +54,7 @@ XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
 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;
@@ -176,7 +177,7 @@ AddonsEngine.prototype = {
           continue;
       }
 
-      if (!this._store.isAddonSyncable(addons[id])) {
+      if (!this.isAddonSyncable(addons[id])) {
         continue;
       }
 
@@ -234,6 +235,10 @@ AddonsEngine.prototype = {
     let cb = Async.makeSpinningCallback();
     this._reconciler.refreshGlobalState(cb);
     cb.wait();
+  },
+
+  isAddonSyncable(addon, ignoreRepoCheck) {
+    return this._store.isAddonSyncable(addon, ignoreRepoCheck);
   }
 };
 
@@ -533,9 +538,12 @@ AddonsStore.prototype = {
    *
    * @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
@@ -592,8 +600,9 @@ AddonsStore.prototype = {
 
     // 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.
+    // in that case. We also provide a way to specifically opt-out of the check
-    if (!AddonRepository.cacheEnabled) {
+    // even if the cache is enabled, which is used by the validators.
+    if (ignoreRepoCheck || !AddonRepository.cacheEnabled) {
       return true;
     }
 
@@ -736,3 +745,69 @@ AddonsTracker.prototype = {
     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);
+  }
+}

services/sync/tests/tps/test_addon_nonrestartless_xpi.js

@@ -33,7 +33,8 @@ Phase("phase01", [
   [Sync]
 ]);
 Phase("phase02", [
-  [Addons.verify, [id], STATE_ENABLED]
+  [Addons.verify, [id], STATE_ENABLED],
+  [Sync]
 ]);
 Phase("phase03", [
   [Addons.verifyNot, [id]],
@@ -41,6 +42,7 @@ Phase("phase03", [
 ]);
 Phase("phase04", [
   [Addons.verify, [id], STATE_ENABLED],
+  [Sync]
 ]);
 
 // Now we disable the add-on
@@ -51,13 +53,15 @@ Phase("phase05", [
 ]);
 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.
@@ -68,13 +72,15 @@ Phase("phase09", [
 ]);
 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
@@ -86,12 +92,14 @@ Phase("phase13", [
   [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]
 ]);

services/sync/tests/tps/test_addon_reconciling.js

@@ -34,6 +34,9 @@ Phase("phase02", [
 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],

services/sync/tests/tps/test_addon_sanity.js

@@ -25,5 +25,6 @@ Phase("phase1", [
 
 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
 ]);

services/sync/tests/tps/test_addon_wipe.js

@@ -30,5 +30,6 @@ Phase("phase02", [
 ]);
 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
 ]);

services/sync/tps/extensions/tps/resource/tps.jsm

@@ -26,6 +26,7 @@ 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");
 
@@ -113,6 +114,7 @@ var TPS = {
   _triggeredSync: false,
   _usSinceEpoch: 0,
   _requestedQuit: false,
+  shouldValidateAddons: false,
   shouldValidateBookmarks: false,
   shouldValidatePasswords: false,
   shouldValidateForms: false,
@@ -464,6 +466,7 @@ var TPS = {
   },
 
   HandleAddons: function (addons, action, state) {
+    this.shouldValidateAddons = true;
     for (let entry of addons) {
       Logger.logInfo("executing action " + action.toUpperCase() +
                      " on addon " + JSON.stringify(entry));
@@ -661,66 +664,64 @@ var TPS = {
     Logger.logInfo("Bookmark validation finished");
   },
 
-  ValidatePasswords() {
+  ValidateCollection(engineName, ValidatorType) {
-    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() {
     let serverRecordDumpStr;
     let clientRecordDumpStr;
     try {
-      Logger.logInfo("About to perform form validation");
+      Logger.logInfo(`About to perform validation for "${engineName}"`);
-      let engine = Weave.Service.engineManager.get("forms");
+      let engine = Weave.Service.engineManager.get(engineName);
-      let validator = new FormValidator();
+      let validator = new ValidatorType(engine);
       let serverRecords = validator.getServerItems(engine);
       let clientRecords = Async.promiseSpinningly(validator.getClientItems());
-      clientRecordDumpStr = JSON.stringify(clientRecords, undefined, 2);
+      try {
-      serverRecordDumpStr = JSON.stringify(serverRecords, undefined, 2);
+        // 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() {
@@ -737,6 +738,9 @@ var TPS = {
         if (this.shouldValidateForms) {
           this.ValidateForms();
         }
+        if (this.shouldValidateAddons) {
+          this.ValidateAddons();
+        }
         // we're all done
         Logger.logInfo("test phase " + this._currentPhase + ": " +
                        (this._errors ? "FAIL" : "PASS"));
@@ -1137,6 +1141,9 @@ var Addons = {
   verifyNot: function Addons__verifyNot(addons) {
     TPS.HandleAddons(addons, ACTION_VERIFY_NOT);
   },
+  skipValidation() {
+    TPS.shouldValidateAddons = false;
+  }
 };
 
 var Bookmarks = {