Bug 1664949 - don't clear the active theme pref on startup so it gets synced wrong, r=zombie,mixedpuppy

Before bug 1660557, this was actually clearing an old migration pref, but now
it's clearing the actual theme pref used by Sync.

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

toolkit/mozapps/extensions/internal/AddonTestUtils.jsm

@@ -1492,14 +1492,22 @@ var AddonTestUtils = {
    * @param {string} event
    *        The name of the AddonListener event handler method for which
    *        an event is expected.
+   * @param {function} checkFn [optional]
+   *        A function to check if this is the right event. Should return true
+   *        for the event that it wants, false otherwise. Will be passed
+   *        all the relevant arguments.
+   *        If not passed, any event will do to resolve the promise.
    * @returns {Promise<Array>}
    *        Resolves to an array containing the event handler's
    *        arguments the first time it is called.
    */
-  promiseAddonEvent(event) {
+  promiseAddonEvent(event, checkFn) {
     return new Promise(resolve => {
       let listener = {
         [event](...args) {
+          if (typeof checkFn == "function" && !checkFn(...args)) {
+            return;
+          }
           AddonManager.removeAddonListener(listener);
           resolve(args);
         },

toolkit/mozapps/extensions/internal/XPIInstall.jsm

@@ -113,7 +113,7 @@ const PREF_XPI_DIRECT_WHITELISTED = "xpinstall.whitelist.directRequest";
 const PREF_XPI_FILE_WHITELISTED = "xpinstall.whitelist.fileRequest";
 const PREF_XPI_WHITELIST_REQUIRED = "xpinstall.whitelist.required";
 
-const PREF_SELECTED_LWT = "extensions.activeThemeID";
+const PREF_SELECTED_THEME = "extensions.activeThemeID";
 
 const TOOLKIT_ID = "toolkit@mozilla.org";
 
@@ -210,10 +210,10 @@ const LOGGER_ID = "addons.xpi";
 // (Requires AddonManager.jsm)
 var logger = Log.repository.getLogger(LOGGER_ID);
 
-// Stores the ID of the lightweight theme which was selected during the
-// last session, if any. When installing a new built-in theme with this
-// ID, it will be automatically enabled.
-let lastLightweightTheme = null;
+// Stores the ID of the theme which was selected during the last session,
+// if any. When installing a new built-in theme with this ID, it will be
+// automatically enabled.
+let lastSelectedTheme = null;
 
 function getJarURI(file, path = "") {
   if (file instanceof Ci.nsIFile) {
@@ -4378,9 +4378,11 @@ var XPIInstall = {
    *          installed.
    */
   async installBuiltinAddon(base) {
-    if (lastLightweightTheme === null) {
-      lastLightweightTheme = Services.prefs.getCharPref(PREF_SELECTED_LWT, "");
-      Services.prefs.clearUserPref(PREF_SELECTED_LWT);
+    // We have to get this before the install, as the install will overwrite
+    // the pref. We then keep the value for this run, so we can restore
+    // the selected theme once it becomes available.
+    if (lastSelectedTheme === null) {
+      lastSelectedTheme = Services.prefs.getCharPref(PREF_SELECTED_THEME, "");
     }
 
     let baseURL = Services.io.newURI(base);
@@ -4400,20 +4402,14 @@ var XPIInstall = {
     // If this is a theme, decide whether to enable it. Themes are
     // disabled by default. However:
     //
-    // If a lightweight theme was selected in the last session, and this
-    // theme has the same ID, then we clearly want to enable it.
-    //
-    // If it is the default theme, more specialized behavior applies:
-    //
     // We always want one theme to be active, falling back to the
-    // default theme when the active theme is disabled. The first time
-    // we install the default theme, though, there likely aren't any
-    // other theme add-ons installed yet, in which case we want to
-    // enable it immediately.
+    // default theme when the active theme is disabled.
+    // During a theme migration, such as a change in the path to the addon, we
+    // will need to ensure a correct theme is enabled.
     if (addon.type === "theme") {
       if (
-        addon.id === lastLightweightTheme ||
-        (!lastLightweightTheme.endsWith("@mozilla.org") &&
+        addon.id === lastSelectedTheme ||
+        (!lastSelectedTheme.endsWith("@mozilla.org") &&
           addon.id === AddonSettings.DEFAULT_THEME_ID &&
           !XPIDatabase.getAddonsByType("theme").some(theme => !theme.disabled))
       ) {

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

@@ -1324,7 +1324,7 @@ async function setInitialState(addon, initialState) {
   }
 }
 
-async function setupBuiltinExtension(extensionData) {
+async function setupBuiltinExtension(extensionData, location = "ext-test") {
   let xpi = await AddonTestUtils.createTempWebExtensionFile(extensionData);
 
   // The built-in location requires a resource: URL that maps to a
@@ -1334,7 +1334,7 @@ async function setupBuiltinExtension(extensionData) {
   let resProto = Services.io
     .getProtocolHandler("resource")
     .QueryInterface(Ci.nsIResProtocolHandler);
-  resProto.setSubstitution("ext-test", base);
+  resProto.setSubstitution(location, base);
 }
 
 async function installBuiltinExtension(extensionData, waitForStartup = true) {

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

@@ -8,6 +8,8 @@ XPCOMUtils.defineLazyGetter(this, "Management", () => {
 });
 
 add_task(async function setup() {
+  let scopes = AddonManager.SCOPE_PROFILE | AddonManager.SCOPE_APPLICATION;
+  Services.prefs.setIntPref("extensions.enabledScopes", scopes);
   createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "42.0", "42.0");
 
   await promiseStartupManager();
@@ -49,3 +51,82 @@ add_task(async function test_update_of_disabled_theme() {
   ok(addon.userDisabled, "Theme is still disabled after an update");
   await addon.uninstall();
 });
+
+add_task(async function test_builtin_location_migration() {
+  const ADDON_ID = "mytheme@mozilla.org";
+
+  let themeDef = {
+    manifest: {
+      applications: { gecko: { id: ADDON_ID } },
+      version: "1.0",
+      theme: {},
+    },
+  };
+
+  await setupBuiltinExtension(themeDef, "first-loc", false);
+  let themeInstalled = AddonTestUtils.promiseAddonEvent(
+    "onInstalled",
+    aAddon => aAddon.id == ADDON_ID
+  );
+  await AddonManager.maybeInstallBuiltinAddon(
+    ADDON_ID,
+    "1.0",
+    "resource://first-loc/"
+  );
+  await themeInstalled;
+
+  let addon = await AddonManager.getAddonByID(ADDON_ID);
+  await addon.enable();
+  Assert.ok(!addon.userDisabled, "Add-on should be enabled.");
+
+  Assert.equal(
+    Services.prefs.getCharPref("extensions.activeThemeID", ""),
+    ADDON_ID,
+    "Pref should be set."
+  );
+
+  let { addons: activeThemes } = await AddonManager.getActiveAddons(["theme"]);
+  Assert.equal(activeThemes.length, 1, "Should have 1 theme.");
+  Assert.equal(activeThemes[0].id, ADDON_ID, "Should have enabled the theme.");
+
+  // If we restart and update, and install a newer version of the theme,
+  // it should be activated.
+  await promiseShutdownManager();
+
+  // Force schema change and restart
+  Services.prefs.setIntPref("extensions.databaseSchema", 0);
+  await promiseStartupManager();
+
+  // Set up a new version of the builtin add-on.
+  let newDef = { manifest: Object.assign({}, themeDef.manifest) };
+  newDef.manifest.version = "1.1";
+  await setupBuiltinExtension(newDef, "second-loc");
+  themeInstalled = AddonTestUtils.promiseAddonEvent(
+    "onInstalled",
+    aAddon => aAddon.id == ADDON_ID
+  );
+
+  await AddonManager.maybeInstallBuiltinAddon(
+    ADDON_ID,
+    "1.1",
+    "resource://second-loc/"
+  );
+  await themeInstalled;
+
+  let newAddon = await AddonManager.getAddonByID(ADDON_ID);
+  Assert.ok(!newAddon.userDisabled, "Add-on should be enabled.");
+
+  ({ addons: activeThemes } = await AddonManager.getActiveAddons(["theme"]));
+  Assert.equal(activeThemes.length, 1, "Should still have 1 theme.");
+  Assert.equal(
+    activeThemes[0].id,
+    ADDON_ID,
+    "Should still have the theme enabled."
+  );
+  Assert.equal(
+    Services.prefs.getCharPref("extensions.activeThemeID", ""),
+    ADDON_ID,
+    "Pref should still be set."
+  );
+  await promiseShutdownManager();
+});