Bug 1549192 Fix extension apis that handle addon disabling r=kmag
authorAndrew Swan <aswan@mozilla.com>
Thu, 09 May 2019 19:59:21 -0700
changeset 532531 46b6ab028e3cb6259bae478a38c4b590384e7d4c
parent 532530 90b6ea6c5c2ac2736134dbbc6d21c6515b13e8a3
child 532532 998d83fa2ae988509435ba88de2c640f4607f2fa
push id11268
push usercsabou@mozilla.com
push dateTue, 14 May 2019 15:24:22 +0000
treeherdermozilla-beta@5fb7fcd568d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1549192
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1549192 Fix extension apis that handle addon disabling r=kmag API implementations that check shutdownReason for values other than APP_SHUTDOWN during extension shutdown are inherently broken since an addon may be disabled or uninstalled between browser sessions, in which case code that is meant to run in these cases will not get executed. This patch fixes existing api implementations that are broken in this way. Also ensure that APIs' onDisabled methods get called properly when an extension is disabled between browser sessions. Differential Revision: https://phabricator.services.mozilla.com/D30604
browser/components/extensions/ext-browser.json
browser/components/extensions/parent/ext-chrome-settings-overrides.js
browser/components/extensions/parent/ext-sidebarAction.js
browser/components/extensions/parent/ext-url-overrides.js
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionParent.jsm
toolkit/components/extensions/ExtensionPreferencesManager.jsm
--- a/browser/components/extensions/ext-browser.json
+++ b/browser/components/extensions/ext-browser.json
@@ -30,17 +30,17 @@
     "scopes": ["addon_parent"],
     "paths": [
       ["captivePortal"]
     ]
   },
   "chrome_settings_overrides": {
     "url": "chrome://browser/content/parent/ext-chrome-settings-overrides.js",
     "scopes": [],
-    "events": ["update", "uninstall"],
+    "events": ["update", "uninstall", "disable"],
     "schema": "chrome://browser/content/schemas/chrome_settings_overrides.json",
     "manifest": ["chrome_settings_overrides"]
   },
   "commands": {
     "url": "chrome://browser/content/parent/ext-commands.js",
     "schema": "chrome://browser/content/schemas/commands.json",
     "scopes": ["addon_parent"],
     "events": ["uninstall"],
@@ -170,16 +170,17 @@
     "paths": [
       ["sessions"]
     ]
   },
   "sidebarAction": {
     "url": "chrome://browser/content/parent/ext-sidebarAction.js",
     "schema": "chrome://browser/content/schemas/sidebar_action.json",
     "scopes": ["addon_parent"],
+    "events": ["uninstall"],
     "manifest": ["sidebar_action"],
     "paths": [
       ["sidebarAction"]
     ]
   },
   "tabs": {
     "url": "chrome://browser/content/parent/ext-tabs.js",
     "schema": "chrome://browser/content/schemas/tabs.json",
@@ -188,17 +189,17 @@
     "paths": [
       ["tabs"]
     ]
   },
   "urlOverrides": {
     "url": "chrome://browser/content/parent/ext-url-overrides.js",
     "schema": "chrome://browser/content/schemas/url_overrides.json",
     "scopes": ["addon_parent"],
-    "events": ["uninstall"],
+    "events": ["update", "uninstall", "disable"],
     "manifest": ["chrome_url_overrides"],
     "paths": [
       ["urlOverrides"]
     ]
   },
   "windows": {
     "url": "chrome://browser/content/parent/ext-windows.js",
     "schema": "chrome://browser/content/schemas/windows.json",
--- a/browser/components/extensions/parent/ext-chrome-settings-overrides.js
+++ b/browser/components/extensions/parent/ext-chrome-settings-overrides.js
@@ -170,16 +170,23 @@ this.chrome_settings_overrides = class e
 
     let haveSearchProvider = manifest && manifest.chrome_settings_overrides &&
                              manifest.chrome_settings_overrides.search_provider;
     if (!haveSearchProvider) {
       this.removeSearchSettings(id);
     }
   }
 
+  static onDisable(id) {
+    homepagePopup.clearConfirmation(id);
+
+    chrome_settings_overrides.processDefaultSearchSetting("disable", id);
+    chrome_settings_overrides.removeEngine(id);
+  }
+
   async onManifestEntry(entryName) {
     let {extension} = this;
     let {manifest} = extension;
 
     await ExtensionSettingsStore.initialize();
 
     let homepageUrl = manifest.chrome_settings_overrides.homepage;
 
@@ -188,18 +195,17 @@ this.chrome_settings_overrides = class e
       if (extension.startupReason == "ADDON_INSTALL" ||
           extension.startupReason == "ADDON_ENABLE") {
         inControl = await ExtensionPreferencesManager.setSetting(
           extension.id, "homepage_override", homepageUrl);
       } else {
         let item = await ExtensionPreferencesManager.getSetting("homepage_override");
         inControl = item && item.id == extension.id;
       }
-      // We need to add the listener here too since onPrefsChanged won't trigger on a
-      // restart (the prefs are already set).
+
       if (inControl) {
         Services.prefs.setBoolPref(HOMEPAGE_PRIVATE_ALLOWED, extension.privateBrowsingAllowed);
         // Also set this now as an upgraded browser will need this.
         Services.prefs.setBoolPref(HOMEPAGE_EXTENSION_CONTROLLED, true);
         if (extension.startupReason == "APP_STARTUP") {
           handleInitialHomepagePopup(extension.id, homepageUrl);
         } else {
           homepagePopup.addObserver(extension.id);
@@ -220,24 +226,16 @@ this.chrome_settings_overrides = class e
       extension.on("remove-permissions", async (ignoreEvent, permissions) => {
         if (permissions.permissions.includes("internal:privateBrowsingAllowed")) {
           let item = await ExtensionPreferencesManager.getSetting("homepage_override");
           if (item && item.id == extension.id) {
             Services.prefs.setBoolPref(HOMEPAGE_PRIVATE_ALLOWED, false);
           }
         }
       });
-
-      extension.callOnClose({
-        close: () => {
-          if (extension.shutdownReason == "ADDON_DISABLE") {
-            homepagePopup.clearConfirmation(extension.id);
-          }
-        },
-      });
     }
     if (manifest.chrome_settings_overrides.search_provider) {
       // Registering a search engine can potentially take a long while,
       // or not complete at all (when searchInitialized is never resolved),
       // so we are deliberately not awaiting the returned promise here.
       let searchStartupPromise =
         this.processSearchProviderManifestEntry().finally(() => {
           if (pendingSearchSetupTasks.get(extension.id) === searchStartupPromise) {
@@ -256,24 +254,16 @@ this.chrome_settings_overrides = class e
     let searchProvider = manifest.chrome_settings_overrides.search_provider;
     if (searchProvider.is_default) {
       await searchInitialized;
       if (!this.extension) {
         Cu.reportError(`Extension shut down before search provider was registered`);
         return;
       }
     }
-    extension.callOnClose({
-      close: () => {
-        if (extension.shutdownReason == "ADDON_DISABLE") {
-          chrome_settings_overrides.processDefaultSearchSetting("disable", extension.id);
-          chrome_settings_overrides.removeEngine(extension.id);
-        }
-      },
-    });
 
     let engineName = searchProvider.name.trim();
     if (searchProvider.is_default) {
       let engine = Services.search.getEngineByName(engineName);
       let defaultEngines = await Services.search.getDefaultEngines();
       if (engine && defaultEngines.some(defaultEngine => defaultEngine.name == engineName)) {
         // Needs to be called every time to handle reenabling, but
         // only sets default for install or enable.
--- a/browser/components/extensions/parent/ext-sidebarAction.js
+++ b/browser/components/extensions/parent/ext-sidebarAction.js
@@ -99,36 +99,42 @@ this.sidebarAction = class extends Exten
       return;
     }
 
     for (let window of windowTracker.browserWindows()) {
       let {document, SidebarUI} = window;
       if (SidebarUI.currentID === this.id) {
         SidebarUI.hide();
       }
-      if (SidebarUI.lastOpenedId === this.id &&
-          reason === "ADDON_UNINSTALL") {
-        SidebarUI.lastOpenedId = null;
-      }
       let menu = document.getElementById(this.menuId);
       if (menu) {
         menu.remove();
       }
       let button = document.getElementById(this.buttonId);
       if (button) {
         button.remove();
       }
       let header = document.getElementById("sidebar-switcher-target");
       header.removeEventListener("SidebarShown", this.updateHeader);
       SidebarUI.sidebars.delete(this.id);
     }
     windowTracker.removeOpenListener(this.windowOpenListener);
     windowTracker.removeCloseListener(this.windowCloseListener);
   }
 
+  static onUninstall(id) {
+    const sidebarId = `${makeWidgetId(id)}-sidebar-action`;
+    for (let window of windowTracker.browserWindows()) {
+      let {SidebarUI} = window;
+      if (SidebarUI.lastOpenedId === sidebarId) {
+        SidebarUI.lastOpenedId = null;
+      }
+    }
+  }
+
   build() {
     this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners
                        (evt, tab) => { this.updateWindow(tab.ownerGlobal); });
 
     let install = this.extension.startupReason === "ADDON_INSTALL";
     for (let window of windowTracker.browserWindows()) {
       this.updateWindow(window);
       let {SidebarUI} = window;
--- a/browser/components/extensions/parent/ext-url-overrides.js
+++ b/browser/components/extensions/parent/ext-url-overrides.js
@@ -88,67 +88,57 @@ ExtensionParent.apiManager.on("extension
       extensionId = (item && item.id) || setting.id;
       url = item && (item.value || item.initialValue);
     }
   }
   setNewTabURL(extensionId, url);
 });
 
 this.urlOverrides = class extends ExtensionAPI {
-  static onUninstall(id) {
+  static async onDisable(id) {
+    newTabPopup.clearConfirmation(id);
+    await ExtensionSettingsStore.initialize();
+    if (ExtensionSettingsStore.hasSetting(id, STORE_TYPE, NEW_TAB_SETTING_NAME)) {
+      ExtensionSettingsStore.disable(id, STORE_TYPE, NEW_TAB_SETTING_NAME);
+    }
+  }
+
+  static async onUninstall(id) {
     // TODO: This can be removed once bug 1438364 is fixed and all data is cleaned up.
     newTabPopup.clearConfirmation(id);
+
+    await ExtensionSettingsStore.initialize();
+    if (ExtensionSettingsStore.hasSetting(id, STORE_TYPE, NEW_TAB_SETTING_NAME)) {
+      ExtensionSettingsStore.removeSetting(id, STORE_TYPE, NEW_TAB_SETTING_NAME);
+    }
   }
 
-  processNewTabSetting(action) {
-    let {extension} = this;
-    ExtensionSettingsStore[action](extension.id, STORE_TYPE, NEW_TAB_SETTING_NAME);
+  static async onUpdate(id, manifest) {
+    if (!manifest.chrome_url_overrides ||
+        !manifest.chrome_url_overrides.newtab) {
+      await ExtensionSettingsStore.initialize();
+      if (ExtensionSettingsStore.hasSetting(id, STORE_TYPE, NEW_TAB_SETTING_NAME)) {
+        ExtensionSettingsStore.removeSetting(id, STORE_TYPE, NEW_TAB_SETTING_NAME);
+      }
+    }
   }
 
   async onManifestEntry(entryName) {
     let {extension} = this;
     let {manifest} = extension;
 
     await ExtensionSettingsStore.initialize();
 
     if (manifest.chrome_url_overrides.newtab) {
-      // Set up the shutdown code for the setting.
-      extension.callOnClose({
-        close: () => {
-          switch (extension.shutdownReason) {
-            case "ADDON_DISABLE":
-              this.processNewTabSetting("disable");
-              newTabPopup.clearConfirmation(extension.id);
-              break;
-
-            // We can remove the setting on upgrade or downgrade because it will be
-            // added back in when the manifest is re-read. This will cover the case
-            // where a new version of an add-on removes the manifest key.
-            case "ADDON_DOWNGRADE":
-            case "ADDON_UPGRADE":
-            case "ADDON_UNINSTALL":
-              this.processNewTabSetting("removeSetting");
-              break;
-          }
-        },
-      });
-
       let url = extension.baseURI.resolve(manifest.chrome_url_overrides.newtab);
 
       let item = await ExtensionSettingsStore.addSetting(
         extension.id, STORE_TYPE, NEW_TAB_SETTING_NAME, url,
         () => aboutNewTabService.newTabURL);
 
-      // If the extension was just re-enabled, change the setting to enabled.
-      // This is required because addSetting above is used for both add and update.
-      if (["ADDON_ENABLE", "ADDON_UPGRADE", "ADDON_DOWNGRADE"]
-          .includes(extension.startupReason)) {
-        item = ExtensionSettingsStore.enable(extension.id, STORE_TYPE, NEW_TAB_SETTING_NAME);
-      }
-
       // Set the newTabURL to the current value of the setting.
       if (item) {
         setNewTabURL(item.id, item.value || item.initialValue);
       }
 
       // We need to monitor permission change and update the preferences.
       // eslint-disable-next-line mozilla/balanced-listeners
       extension.on("add-permissions", async (ignoreEvent, permissions) => {
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -2084,17 +2084,17 @@ class Extension extends ExtensionData {
     }
 
     GlobalManager.uninit(this);
 
     for (let obj of this.onShutdown) {
       obj.close();
     }
 
-    ParentAPIManager.shutdownExtension(this.id);
+    ParentAPIManager.shutdownExtension(this.id, reason);
 
     Management.emit("shutdown", this);
     this.emit("shutdown");
 
     const TIMED_OUT = Symbol();
 
     this.state = "Shutdown: Emit shutdown";
     let result = await Promise.race([
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -14,16 +14,17 @@
 /* exported ExtensionParent */
 
 var EXPORTED_SYMBOLS = ["ExtensionParent"];
 
 const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetters(this, {
+  AddonManager: "resource://gre/modules/AddonManager.jsm",
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   DeferredTask: "resource://gre/modules/DeferredTask.jsm",
   E10SUtils: "resource://gre/modules/E10SUtils.jsm",
   ExtensionData: "resource://gre/modules/Extension.jsm",
   GeckoViewConnection: "resource://gre/modules/GeckoViewWebExtension.jsm",
   MessageChannel: "resource://gre/modules/MessageChannel.jsm",
   MessageManagerProxy: "resource://gre/modules/MessageManagerProxy.jsm",
@@ -107,16 +108,27 @@ let apiManager = new class extends Schem
     this.on("uninstall", (e, {id}) => {
       let modules = this.eventModules.get("uninstall");
       return Promise.all(Array.from(modules).map(async apiName => {
         let module = await this.asyncLoadModule(apiName);
         return module.onUninstall(id);
       }));
     });
     /* eslint-enable mozilla/balanced-listeners */
+
+    // Handle any changes that happened during startup
+    let disabledIds = AddonManager.getStartupChanges(AddonManager.STARTUP_CHANGE_DISABLED);
+    if (disabledIds.length > 0) {
+      this._callHandlers(disabledIds, "disable", "onDisable");
+    }
+
+    let uninstalledIds = AddonManager.getStartupChanges(AddonManager.STARTUP_CHANGE_UNINSTALLED);
+    if (uninstalledIds.length > 0) {
+      this._callHandlers(uninstalledIds, "uninstall", "onUninstall");
+    }
   }
 
   getModuleJSONURLs() {
     return Array.from(Services.catMan.enumerateCategory(CATEGORY_EXTENSION_MODULES),
                       ({value}) => value);
   }
 
   // Loads all the ext-*.js scripts currently registered.
@@ -178,16 +190,33 @@ let apiManager = new class extends Schem
       if (result.tabId) {
         if (sync) {
           return result;
         }
         target.messageManager.sendAsyncMessage("Extension:SetFrameData", result);
       }
     }
   }
+
+  // Call static handlers for the given event on the given extension ids,
+  // and set up a shutdown blocker to ensure they all complete.
+  _callHandlers(ids, event, method) {
+    let promises = Array.from(this.eventModules.get(event))
+                        .map(async modName => {
+                          let module = await this.asyncLoadModule(modName);
+                          return ids.map(id => module[method](id));
+                        }).flat();
+    if (event === "disable") {
+      promises.push(...ids.map(id => this.emit("disable", id)));
+    }
+
+    AsyncShutdown.profileBeforeChange.addBlocker(
+      `Extension API ${event} handlers for ${ids.join(",")}`,
+      Promise.all(promises));
+  }
 }();
 
 // A proxy for extension ports between two DISTINCT message managers.
 // This is used by ProxyMessenger, to ensure that a port always receives a
 // disconnection message when the other side closes, even if that other side
 // fails to send the message before the message manager disconnects.
 class ExtensionPortProxy {
   /**
@@ -820,27 +849,23 @@ ParentAPIManager = {
       for (let extension of GlobalManager.extensionMap.values()) {
         if (extension.parentMessageManager === mm) {
           extension.parentMessageManager = null;
         }
       }
     }
   },
 
-  shutdownExtension(extensionId) {
+  shutdownExtension(extensionId, reason) {
+    if (["ADDON_DISABLE", "ADDON_UNINSTALL"].includes(reason)) {
+      apiManager._callHandlers([extensionId], "disable", "onDisable");
+    }
+
     for (let [childId, context] of this.proxyContexts) {
       if (context.extension.id == extensionId) {
-        if (["ADDON_DISABLE", "ADDON_UNINSTALL"].includes(context.extension.shutdownReason)) {
-          let modules = apiManager.eventModules.get("disable");
-          Array.from(modules).map(async apiName => {
-            let module = await apiManager.asyncLoadModule(apiName);
-            module.onDisable(extensionId);
-          });
-        }
-
         context.shutdown();
         this.proxyContexts.delete(childId);
       }
     }
   },
 
   receiveMessage({name, data, target}) {
     try {
--- a/toolkit/components/extensions/ExtensionPreferencesManager.jsm
+++ b/toolkit/components/extensions/ExtensionPreferencesManager.jsm
@@ -35,20 +35,18 @@ XPCOMUtils.defineLazyGetter(this, "defau
   return new Preferences({defaultBranch: true});
 });
 
 /* eslint-disable mozilla/balanced-listeners */
 Management.on("uninstall", (type, {id}) => {
   ExtensionPreferencesManager.removeAll(id);
 });
 
-Management.on("shutdown", (type, extension) => {
-  if (extension.shutdownReason == "ADDON_DISABLE") {
-    this.ExtensionPreferencesManager.disableAll(extension.id);
-  }
+Management.on("disable", (type, id) => {
+  this.ExtensionPreferencesManager.disableAll(id);
 });
 
 Management.on("startup", async (type, extension) => {
   if (extension.startupReason == "ADDON_ENABLE") {
     this.ExtensionPreferencesManager.enableAll(extension.id);
   }
 });
 /* eslint-enable mozilla/balanced-listeners */