Bug 1672314 handle invalid addon startup data properly during startup r=rpl

We scan for addon changes twice, once early in startup (usually with no scanning) and once after ui startup.  We hold on to the startup data, and during both scans we restore that data into the addon location instances.  The problem here is if we install a builtin in-between these scans.  The new data from the install would get overwitten by the old data.  In some cases this caused addons to disappear (e.g. old data has incorrect path).  Other issues covered here is that we would never remove addon data for builtins removed from the system, and we would additionally mark builtins as sideloads, which caused other side effects (particularly with search addons) where we would not load the addon, but fortunately the search service later re-installes them.

Differential Revision: https://phabricator.services.mozilla.com/D95422

toolkit/mozapps/extensions/internal/XPIDatabase.jsm

@@ -712,6 +712,7 @@ class AddonInternal {
       if (this.userDisabled || this.softDisabled) {
         permissions |= AddonManager.PERM_CAN_ENABLE;
       } else if (this.type != "theme" || this.id != DEFAULT_THEME_ID) {
+        // We do not expose disabling the default theme.
         permissions |= AddonManager.PERM_CAN_DISABLE;
       }
     }
@@ -726,9 +727,10 @@ class AddonInternal {
     let changesAllowed = !this.location.locked && !this.pendingUninstall;
     if (changesAllowed) {
       // System add-on upgrades are triggered through a different mechanism (see updateSystemAddons())
-      let isSystem = this.location.isSystem;
+      // Builtin addons are only upgraded with Firefox (or app) updates.
+      let isSystem = this.location.isSystem || this.location.isBuiltin;
       // Add-ons that are installed by a file link cannot be upgraded.
-      if (!this.location.isLinkedAddon(this.id) && !isSystem) {
+      if (!isSystem && !this.location.isLinkedAddon(this.id)) {
         permissions |= AddonManager.PERM_CAN_UPGRADE;
       }
     }
@@ -2847,6 +2849,12 @@ this.XPIDatabaseReconcile = {
         );
       } else if (unsigned && !isNewInstall) {
         logger.warn("Not uninstalling existing unsigned add-on");
+      } else if (aLocation.name == KEY_APP_BUILTINS) {
+        // If a builtin has been removed from the build, we need to remove it from our
+        // data sets.  We cannot use location.isBuiltin since the system addon locations
+        // mix it up.
+        XPIDatabase.removeAddonMetadata(aAddonState);
+        aLocation.removeAddon(aId);
       } else {
         aLocation.installer.uninstallAddon(aId);
       }
@@ -2859,7 +2867,8 @@ this.XPIDatabaseReconcile = {
 
     // Assume that add-ons in the system add-ons install location aren't
     // foreign and should default to enabled.
-    aNewAddon.foreignInstall = isDetectedInstall && !aLocation.isSystem;
+    aNewAddon.foreignInstall =
+      isDetectedInstall && !aLocation.isSystem && !aLocation.isBuiltin;
 
     // appDisabled depends on whether the add-on is a foreignInstall so update
     aNewAddon.appDisabled = !XPIDatabase.isUsableAddon(aNewAddon);

toolkit/mozapps/extensions/internal/XPIProvider.jsm

@@ -967,6 +967,12 @@ var BuiltInLocation = new (class _BuiltInLocation extends XPIStateLocation {
   get enumerable() {
     return false;
   }
+
+  // Builtin addons are never linked, return false
+  // here for correct behavior elsewhere.
+  isLinkedAddon(/* aId */) {
+    return false;
+  }
 })();
 
 /**
@@ -1429,6 +1435,9 @@ var XPIStates = {
    */
   scanForChanges(ignoreSideloads = true) {
     let oldState = this.initialStateData || this.loadExtensionState();
+    // We're called twice, do not restore the second time as new data
+    // may have been inserted since the first call.
+    let shouldRestoreLocationData = !this.initialStateData;
     this.initialStateData = oldState;
 
     let changed = false;
@@ -1437,7 +1446,7 @@ var XPIStates = {
     for (let loc of XPIStates.locations()) {
       oldLocations.delete(loc.name);
 
-      if (oldState[loc.name]) {
+      if (shouldRestoreLocationData && oldState[loc.name]) {
         loc.restore(oldState[loc.name]);
       }
       changed = changed || loc.changed;

toolkit/mozapps/extensions/test/xpcshell/test_aom_startup.js

@@ -0,0 +1,187 @@
+"use strict";
+
+const { JSONFile } = ChromeUtils.import("resource://gre/modules/JSONFile.jsm");
+
+const aomStartup = Cc["@mozilla.org/addons/addon-manager-startup;1"].getService(
+  Ci.amIAddonManagerStartup
+);
+
+const gProfDir = do_get_profile();
+
+Services.prefs.setIntPref(
+  "extensions.enabledScopes",
+  AddonManager.SCOPE_PROFILE | AddonManager.SCOPE_APPLICATION
+);
+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "42.0", "42.0");
+
+const DUMMY_ID = "@dummy";
+const DUMMY_ADDONS = {
+  addons: {
+    "@dummy": {
+      lastModifiedTime: 1337,
+      rootURI: "resource:///modules/themes/dummy/",
+      version: "1",
+    },
+  },
+};
+
+const TEST_ADDON_ID = "@test-theme";
+const TEST_THEME = {
+  lastModifiedTime: 1337,
+  rootURI: "resource:///modules/themes/test/",
+  version: "1",
+};
+
+const TEST_ADDONS = {
+  addons: {
+    "@test-theme": TEST_THEME,
+  },
+};
+
+// Utility to write out various addonStartup.json files.
+async function writeAOMStartupData(data) {
+  let jsonFile = new JSONFile({
+    path: OS.Path.join(gProfDir.path, "addonStartup.json.lz4"),
+    compression: "lz4",
+  });
+  jsonFile.data = data;
+  await jsonFile._save();
+  return aomStartup.readStartupData();
+}
+
+// This tests that any buitin removed from the build will
+// get removed from the state data.
+add_task(async function test_startup_missing_builtin() {
+  let startupData = await writeAOMStartupData({
+    "app-builtin": DUMMY_ADDONS,
+  });
+  Assert.ok(
+    !!startupData["app-builtin"].addons[DUMMY_ID],
+    "non-existant addon is in startup data"
+  );
+
+  await AddonTestUtils.promiseStartupManager();
+  await AddonTestUtils.promiseShutdownManager();
+
+  // This data is flushed on shutdown, so we check it after shutdown.
+  startupData = aomStartup.readStartupData();
+  Assert.equal(
+    startupData["app-builtin"].addons[DUMMY_ID],
+    undefined,
+    "non-existant addon is removed from startup data"
+  );
+});
+
+// This test verifies that a builtin installed prior to the
+// second scan is not overwritten by old state data during
+// the scan.
+add_task(async function test_startup_default_theme_moved() {
+  let startupData = await writeAOMStartupData({
+    "app-profile": DUMMY_ADDONS,
+    "app-builtin": TEST_ADDONS,
+  });
+  Assert.ok(
+    !!startupData["app-profile"].addons[DUMMY_ID],
+    "non-existant addon is in startup data"
+  );
+  Assert.ok(
+    !!startupData["app-builtin"].addons[TEST_ADDON_ID],
+    "test addon is in startup data"
+  );
+
+  let themeDef = {
+    manifest: {
+      applications: { gecko: { id: TEST_ADDON_ID } },
+      version: "1.1",
+      theme: {},
+    },
+  };
+
+  await setupBuiltinExtension(themeDef, "second-loc");
+  await AddonTestUtils.promiseStartupManager("44");
+  await AddonManager.maybeInstallBuiltinAddon(
+    TEST_ADDON_ID,
+    "1.1",
+    "resource://second-loc/"
+  );
+  await AddonManagerPrivate.getNewSideloads();
+
+  let addon = await AddonManager.getAddonByID(TEST_ADDON_ID);
+  Assert.ok(!addon.foreignInstall, "addon was not marked as a foreign install");
+  Assert.equal(addon.version, "1.1", "addon version is correct");
+
+  await AddonTestUtils.promiseShutdownManager();
+
+  // This data is flushed on shutdown, so we check it after shutdown.
+  startupData = aomStartup.readStartupData();
+  Assert.equal(
+    startupData["app-builtin"].addons[TEST_ADDON_ID].version,
+    "1.1",
+    "startup data is correct in cache"
+  );
+  Assert.equal(
+    startupData["app-builtin"].addons[DUMMY_ID],
+    undefined,
+    "non-existant addon is removed from startup data"
+  );
+});
+
+// This test verifies that a builtin addon being updated
+// is not marked as a foreignInstall.
+add_task(async function test_startup_builtin_not_foreign() {
+  let startupData = await writeAOMStartupData({
+    "app-profile": DUMMY_ADDONS,
+    "app-builtin": {
+      addons: {
+        "@test-theme": {
+          ...TEST_THEME,
+          rootURI: "resource://second-loc/",
+        },
+      },
+    },
+  });
+  Assert.ok(
+    !!startupData["app-profile"].addons[DUMMY_ID],
+    "non-existant addon is in startup data"
+  );
+  Assert.ok(
+    !!startupData["app-builtin"].addons[TEST_ADDON_ID],
+    "test addon is in startup data"
+  );
+
+  let themeDef = {
+    manifest: {
+      applications: { gecko: { id: TEST_ADDON_ID } },
+      version: "1.1",
+      theme: {},
+    },
+  };
+
+  await setupBuiltinExtension(themeDef, "second-loc");
+  await AddonTestUtils.promiseStartupManager("43");
+  await AddonManager.maybeInstallBuiltinAddon(
+    TEST_ADDON_ID,
+    "1.1",
+    "resource://second-loc/"
+  );
+  await AddonManagerPrivate.getNewSideloads();
+
+  let addon = await AddonManager.getAddonByID(TEST_ADDON_ID);
+  Assert.ok(!addon.foreignInstall, "addon was not marked as a foreign install");
+  Assert.equal(addon.version, "1.1", "addon version is correct");
+
+  await AddonTestUtils.promiseShutdownManager();
+
+  // This data is flushed on shutdown, so we check it after shutdown.
+  startupData = aomStartup.readStartupData();
+  Assert.equal(
+    startupData["app-builtin"].addons[TEST_ADDON_ID].version,
+    "1.1",
+    "startup data is correct in cache"
+  );
+  Assert.equal(
+    startupData["app-builtin"].addons[DUMMY_ID],
+    undefined,
+    "non-existant addon is removed from startup data"
+  );
+});

toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js

@@ -299,3 +299,67 @@ add_task(async function test_theme_update() {
   addon = await AddonManager.getAddonByID(DEFAULT_THEME);
   ok(!addon.userDisabled, "default theme is enabled after upgrade");
 });
+
+add_task(async function test_builtin_theme_permissions() {
+  const ADDON_ID = "mytheme@mozilla.org";
+
+  let themeDef = {
+    manifest: {
+      applications: { gecko: { id: ADDON_ID } },
+      version: "1.0",
+      theme: {},
+    },
+  };
+
+  function checkPerms(addon) {
+    // builtin themes enable or disable based on disabled state
+    Assert.equal(
+      addon.userDisabled,
+      hasFlag(addon.permissions, AddonManager.PERM_CAN_ENABLE),
+      "enable permission is correct"
+    );
+    Assert.equal(
+      !addon.userDisabled,
+      hasFlag(addon.permissions, AddonManager.PERM_CAN_DISABLE),
+      "disable permission is correct"
+    );
+    // builtin themes do not get any other permission
+    Assert.ok(
+      !hasFlag(addon.permissions, AddonManager.PERM_CAN_INSTALL),
+      "cannot install by user"
+    );
+    Assert.ok(
+      !hasFlag(addon.permissions, AddonManager.PERM_CAN_UPGRADE),
+      "cannot upgrade"
+    );
+    Assert.ok(
+      !hasFlag(addon.permissions, AddonManager.PERM_CAN_UNINSTALL),
+      "cannot uninstall"
+    );
+    Assert.ok(
+      !hasFlag(
+        addon.permissions,
+        AddonManager.PERM_CAN_CHANGE_PRIVATEBROWSING_ACCESS
+      ),
+      "can change private browsing access"
+    );
+    Assert.ok(
+      hasFlag(addon.permissions, AddonManager.PERM_API_CAN_UNINSTALL),
+      "can uninstall via API"
+    );
+  }
+
+  await setupBuiltinExtension(themeDef, "first-loc", false);
+  await AddonManager.maybeInstallBuiltinAddon(
+    ADDON_ID,
+    "1.0",
+    "resource://first-loc/"
+  );
+
+  let addon = await AddonManager.getAddonByID(ADDON_ID);
+  checkPerms(addon);
+  await addon.enable();
+  checkPerms(addon);
+
+  await addon.uninstall();
+});

toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini

@@ -7,6 +7,7 @@ dupe-manifest =
 support-files =
   data/**
 
+[test_aom_startup.js]
 [test_AbuseReporter.js]
 [test_AddonRepository.js]
 [test_AddonRepository_cache.js]