Bug 1275139 (part 3) - don't Sync system addons, nor apply them when incoming. r?rhelmer draft
authorMark Hammond <mhammond@skippinet.com.au>
Tue, 31 May 2016 19:04:16 +1000
changeset 384372 96df77166a865344d75fdbb45781dc418ad3e222
parent 384371 9132865eadc423d44e1915711bb52d40b9468af6
child 524689 b322a7d7ad4c0ec4b8e86aae45816b4419a0e83a
push id22255
push userbmo:markh@mozilla.com
push dateWed, 06 Jul 2016 06:39:51 +0000
reviewersrhelmer
bugs1275139
milestone50.0a1
Bug 1275139 (part 3) - don't Sync system addons, nor apply them when incoming. r?rhelmer MozReview-Commit-ID: Jn0TNLy4cDJ
services/sync/modules/addonsreconciler.js
services/sync/modules/engines/addons.js
services/sync/tests/unit/test_addons_store.js
toolkit/mozapps/extensions/internal/XPIProvider.jsm
--- a/services/sync/modules/addonsreconciler.js
+++ b/services/sync/modules/addonsreconciler.js
@@ -429,17 +429,18 @@ AddonsReconciler.prototype = {
       let record = {
         id: id,
         guid: guid,
         enabled: enabled,
         installed: true,
         modified: now,
         type: addon.type,
         scope: addon.scope,
-        foreignInstall: addon.foreignInstall
+        foreignInstall: addon.foreignInstall,
+        isSyncable: addon.isSyncable,
       };
       this._addons[id] = record;
       this._log.debug("Adding change because add-on not present locally: " +
                       id);
       this._addChange(now, CHANGE_INSTALLED, record);
       return;
     }
 
--- a/services/sync/modules/engines/addons.js
+++ b/services/sync/modules/engines/addons.js
@@ -276,16 +276,24 @@ AddonsStore.prototype = {
       // is our current policy.
       if (record.source != "amo") {
         this._log.info("Ignoring unknown add-on source (" + record.source + ")" +
                        " for " + record.id);
         return;
       }
     }
 
+    // Ignore incoming records for which an existing non-syncable addon
+    // exists.
+    let existingMeta = this.reconciler.addons[record.addonID];
+    if (existingMeta && !this.isAddonSyncable(existingMeta)) {
+      this._log.info("Ignoring incoming record for an existing but non-syncable addon", record.addonID);
+      return;
+    }
+
     Store.prototype.applyIncoming.call(this, record);
   },
 
 
   /**
    * Provides core Store API to create/install an add-on from a record.
    */
   create: function create(record) {
@@ -529,17 +537,20 @@ AddonsStore.prototype = {
    */
   isAddonSyncable: function isAddonSyncable(addon) {
     // 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) Are installed from AMO
+    //   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
+    //      be synced)
+    //   6) Are installed from AMO
 
     // We could represent the test as a complex boolean expression. We go the
     // verbose route so the failure reason is logged.
     if (!addon) {
       this._log.debug("Null object passed to isAddonSyncable.");
       return false;
     }
 
@@ -549,26 +560,36 @@ AddonsStore.prototype = {
       return false;
     }
 
     if (!(addon.scope & AddonManager.SCOPE_PROFILE)) {
       this._log.debug(addon.id + " not syncable: not installed in profile.");
       return false;
     }
 
+    // If the addon manager says it's not syncable, we skip it.
+    if (!addon.isSyncable) {
+      this._log.debug(addon.id + " not syncable: vetoed by the addon manager.");
+      return false;
+    }
+
     // This may be too aggressive. If an add-on is downloaded from AMO and
     // manually placed in the profile directory, foreignInstall will be set.
     // Arguably, that add-on should be syncable.
     // TODO Address the edge case and come up with more robust heuristics.
     if (addon.foreignInstall) {
       this._log.debug(addon.id + " not syncable: is foreign install.");
       return false;
     }
 
     // Ignore hotfix extensions (bug 741670). The pref may not be defined.
+    // XXX - note that addon.isSyncable will be false for hotfix addons, so
+    // this check isn't strictly necessary - except for Sync tests which aren't
+    // setup to create a "real" hotfix addon. This can be removed once those
+    // 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.
--- a/services/sync/tests/unit/test_addons_store.js
+++ b/services/sync/tests/unit/test_addons_store.js
@@ -223,31 +223,35 @@ add_test(function test_addon_syncability
                 "addons.mozilla.org,other.example.com");
 
   do_check_false(store.isAddonSyncable(null));
 
   let addon = installAddon("test_bootstrap1_1");
   do_check_true(store.isAddonSyncable(addon));
 
   let dummy = {};
-  const KEYS = ["id", "syncGUID", "type", "scope", "foreignInstall"];
+  const KEYS = ["id", "syncGUID", "type", "scope", "foreignInstall", "isSyncable"];
   for (let k of KEYS) {
     dummy[k] = addon[k];
   }
 
   do_check_true(store.isAddonSyncable(dummy));
 
   dummy.type = "UNSUPPORTED";
   do_check_false(store.isAddonSyncable(dummy));
   dummy.type = addon.type;
 
   dummy.scope = 0;
   do_check_false(store.isAddonSyncable(dummy));
   dummy.scope = addon.scope;
 
+  dummy.isSyncable = false;
+  do_check_false(store.isAddonSyncable(dummy));
+  dummy.isSyncable = addon.isSyncable;
+
   dummy.foreignInstall = true;
   do_check_false(store.isAddonSyncable(dummy));
   dummy.foreignInstall = false;
 
   uninstallAddon(addon);
 
   do_check_false(store.isSourceURITrusted(null));
 
@@ -295,17 +299,17 @@ add_test(function test_ignore_hotfixes()
   // A hotfix extension is one that has the id the same as the
   // extensions.hotfix.id pref.
   let prefs = new Preferences("extensions.");
 
   let addon = installAddon("test_bootstrap1_1");
   do_check_true(store.isAddonSyncable(addon));
 
   let dummy = {};
-  const KEYS = ["id", "syncGUID", "type", "scope", "foreignInstall"];
+  const KEYS = ["id", "syncGUID", "type", "scope", "foreignInstall", "isSyncable"];
   for (let k of KEYS) {
     dummy[k] = addon[k];
   }
 
   // Basic sanity check.
   do_check_true(store.isAddonSyncable(dummy));
 
   prefs.set("hotfix.id", dummy.id);
--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -7403,16 +7403,27 @@ AddonWrapper.prototype = {
   },
 
   get isSystem() {
     let addon = addonFor(this);
     return (addon._installLocation.name == KEY_APP_SYSTEM_DEFAULTS ||
             addon._installLocation.name == KEY_APP_SYSTEM_ADDONS);
   },
 
+  // Returns true if Firefox Sync should sync this addon. Only non-hotfixes
+  // directly in the profile are considered syncable.
+  get isSyncable() {
+    let addon = addonFor(this);
+    let hotfixID = Preferences.get(PREF_EM_HOTFIX_ID, undefined);
+    if (hotfixID && hotfixID == addon.id) {
+      return false;
+    }
+    return (addon._installLocation.name == KEY_APP_PROFILE);
+  },
+
   isCompatibleWith: function(aAppVersion, aPlatformVersion) {
     return addonFor(this).isCompatibleWith(aAppVersion, aPlatformVersion);
   },
 
   uninstall: function(alwaysAllowUndo) {
     let addon = addonFor(this);
     XPIProvider.uninstallAddon(addon, alwaysAllowUndo);
   },