Bug 1549192 Remove extension shutdownReason footgun r=kmag
authorAndrew Swan <aswan@mozilla.com>
Thu, 09 May 2019 19:46:38 -0700
changeset 473708 998d83fa2ae988509435ba88de2c640f4607f2fa
parent 473707 46b6ab028e3cb6259bae478a38c4b590384e7d4c
child 473709 e0a622476b7756daebac7ccb7a9bbeb5dfe3cac0
child 473775 271512a8bcc3196dfb14e603d39bc57d9c169454
push id36010
push userapavel@mozilla.com
push dateTue, 14 May 2019 04:11:16 +0000
treeherdermozilla-central@e0a622476b77 [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 Remove extension shutdownReason footgun r=kmag Checking extension.shutdownReason for any purpose other than detecting app shutdown is unreliable, since actions such as disabing, uninstalling, etc. may happen ito an extension after it has shut down. Remove the temptation for api authors to write incorrect code by removing extension.shutdownReason and replacing it with an isAppShutdown boolean passed to shutdown handlers. Differential Revision: https://phabricator.services.mozilla.com/D30605
browser/components/extensions/parent/ext-browserAction.js
browser/components/extensions/parent/ext-commands.js
browser/components/extensions/parent/ext-devtools.js
browser/components/extensions/parent/ext-menus.js
browser/components/extensions/parent/ext-omnibox.js
browser/components/extensions/parent/ext-pageAction.js
browser/components/extensions/parent/ext-sidebarAction.js
browser/extensions/formautofill/api.js
browser/extensions/report-site-issue/experimentalAPIs/l10n.js
mobile/android/components/extensions/ext-browserAction.js
mobile/android/components/extensions/ext-pageAction.js
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionCommon.jsm
toolkit/components/extensions/parent/ext-protocolHandlers.js
toolkit/components/extensions/parent/ext-theme.js
--- a/browser/components/extensions/parent/ext-browserAction.js
+++ b/browser/components/extensions/parent/ext-browserAction.js
@@ -110,17 +110,17 @@ this.browserAction = class extends Exten
 
   handleLocationChange(eventType, tab, fromBrowse) {
     if (fromBrowse) {
       this.tabContext.clear(tab);
       this.updateOnChange(tab);
     }
   }
 
-  onShutdown(reason) {
+  onShutdown() {
     browserActionMap.delete(this.extension);
 
     this.tabContext.shutdown();
     CustomizableUI.destroyWidget(this.id);
 
     this.clearPopup();
   }
 
--- a/browser/components/extensions/parent/ext-commands.js
+++ b/browser/components/extensions/parent/ext-commands.js
@@ -15,17 +15,17 @@ this.commands = class extends ExtensionA
       extension: this.extension,
       onCommand: (name) => this.emit("command", name),
     });
     this.extension.shortcuts = shortcuts;
     await shortcuts.loadCommands();
     await shortcuts.register();
   }
 
-  onShutdown(reason) {
+  onShutdown() {
     this.extension.shortcuts.unregister();
   }
 
   getAPI(context) {
     return {
       commands: {
         getAll: () => this.extension.shortcuts.allCommands(),
         update: (args) => this.extension.shortcuts.updateCommand(args),
--- a/browser/components/extensions/parent/ext-devtools.js
+++ b/browser/components/extensions/parent/ext-devtools.js
@@ -351,17 +351,17 @@ this.devtools = class extends ExtensionA
     if (!this.isDevToolsPageDisabled()) {
       this.pageDefinition.build();
     }
 
     DevToolsShim.on("toolbox-created", this.onToolboxCreated);
     DevToolsShim.on("toolbox-destroy", this.onToolboxDestroy);
   }
 
-  onShutdown(reason) {
+  onShutdown() {
     DevToolsShim.off("toolbox-created", this.onToolboxCreated);
     DevToolsShim.off("toolbox-destroy", this.onToolboxDestroy);
 
     // Shutdown the extension devtools_page from all existing toolboxes.
     this.pageDefinition.shutdown();
     this.pageDefinition = null;
 
     // Iterate over the existing toolboxes and unlist the devtools webextension from them.
--- a/browser/components/extensions/parent/ext-menus.js
+++ b/browser/components/extensions/parent/ext-menus.js
@@ -1095,17 +1095,17 @@ this.menusInternal = class extends Exten
     super(extension);
 
     if (!gMenuMap.size) {
       menuTracker.register();
     }
     gMenuMap.set(extension, new Map());
   }
 
-  onShutdown(reason) {
+  onShutdown() {
     let {extension} = this;
 
     if (gMenuMap.has(extension)) {
       gMenuMap.delete(extension);
       gRootItems.delete(extension);
       gShownMenuItems.delete(extension);
       gOnShownSubscribers.delete(extension);
       if (!gMenuMap.size) {
--- a/browser/components/extensions/parent/ext-omnibox.js
+++ b/browser/components/extensions/parent/ext-omnibox.js
@@ -15,17 +15,17 @@ this.omnibox = class extends ExtensionAP
       // This will throw if the keyword is already registered.
       ExtensionSearchHandler.registerKeyword(keyword, extension);
       this.keyword = keyword;
     } catch (e) {
       extension.manifestError(e.message);
     }
   }
 
-  onShutdown(reason) {
+  onShutdown() {
     ExtensionSearchHandler.unregisterKeyword(this.keyword);
   }
 
   getAPI(context) {
     let {extension} = context;
     return {
       omnibox: {
         setDefaultSuggestion: (suggestion) => {
--- a/browser/components/extensions/parent/ext-pageAction.js
+++ b/browser/components/extensions/parent/ext-pageAction.js
@@ -113,26 +113,26 @@ this.pageAction = class extends Extensio
           if (this.isShown(tab)) {
             this.updateButton(window);
           }
         }
       }
     }
   }
 
-  onShutdown(reason) {
+  onShutdown(isAppShutdown) {
     pageActionMap.delete(this.extension);
 
     this.tabContext.shutdown();
 
     // Removing the browser page action causes PageActions to forget about it
     // across app restarts, so don't remove it on app shutdown, but do remove
     // it on all other shutdowns since there's no guarantee the action will be
     // coming back.
-    if (reason != "APP_SHUTDOWN" && this.browserPageAction) {
+    if (!isAppShutdown && this.browserPageAction) {
       this.browserPageAction.remove();
       this.browserPageAction = null;
     }
   }
 
   // Returns the value of the property |prop| for the given tab, where
   // |prop| is one of "show", "title", "icon", "popup".
   getProperty(tab, prop) {
--- a/browser/components/extensions/parent/ext-sidebarAction.js
+++ b/browser/components/extensions/parent/ext-sidebarAction.js
@@ -83,24 +83,24 @@ this.sidebarAction = class extends Exten
 
     sidebarActionMap.set(extension, this);
   }
 
   onReady() {
     this.build();
   }
 
-  onShutdown(reason) {
+  onShutdown(isAppShutdown) {
     sidebarActionMap.delete(this.this);
 
     this.tabContext.shutdown();
 
     // Don't remove everything on app shutdown so session restore can handle
     // restoring open sidebars.
-    if (reason === "APP_SHUTDOWN") {
+    if (isAppShutdown) {
       return;
     }
 
     for (let window of windowTracker.browserWindows()) {
       let {document, SidebarUI} = window;
       if (SidebarUI.currentID === this.id) {
         SidebarUI.hide();
       }
--- a/browser/extensions/formautofill/api.js
+++ b/browser/extensions/formautofill/api.js
@@ -125,18 +125,18 @@ this.formautofill = class extends Extens
 
     // Listen for the autocomplete popup message to lazily append our stylesheet related to the popup.
     Services.mm.addMessageListener("FormAutoComplete:MaybeOpenPopup", onMaybeOpenPopup);
 
     formAutofillParent.init().catch(Cu.reportError);
     Services.mm.loadFrameScript("chrome://formautofill/content/FormAutofillFrameScript.js", true, true);
   }
 
-  onShutdown(reason) {
-    if (reason == "APP_SHUTDOWN") {
+  onShutdown(isAppShutdown) {
+    if (isAppShutdown) {
       return;
     }
 
     resProto.setSubstitution(RESOURCE_HOST, null);
 
     this.chromeHandle.destruct();
     this.chromeHandle = null;
 
--- a/browser/extensions/report-site-issue/experimentalAPIs/l10n.js
+++ b/browser/extensions/report-site-issue/experimentalAPIs/l10n.js
@@ -11,18 +11,18 @@ var {Services} = ChromeUtils.import("res
 XPCOMUtils.defineLazyGetter(this, "l10nStrings", function() {
   return Services.strings.createBundle(
     "chrome://webcompat-reporter/locale/webcompat.properties");
 });
 
 let l10nManifest;
 
 this.l10n = class extends ExtensionAPI {
-  onShutdown(reason) {
-    if (reason !== "APP_SHUTDOWN" && l10nManifest) {
+  onShutdown(isAppShutdown) {
+    if (!isAppShutdown && l10nManifest) {
       Components.manager.removeBootstrappedManifestLocation(l10nManifest);
     }
   }
   getAPI(context) {
     // Until we move to Fluent (bug 1446164), we're stuck with
     // chrome.manifest for handling localization since its what the
     // build system can handle for localized repacks.
     if (context.extension.rootURI instanceof Ci.nsIJARURI) {
--- a/mobile/android/components/extensions/ext-browserAction.js
+++ b/mobile/android/components/extensions/ext-browserAction.js
@@ -125,17 +125,17 @@ this.browserAction = class extends Exten
   onManifestEntry(entryName) {
     let {extension} = this;
     let {manifest} = extension;
 
     let browserAction = new BrowserAction(manifest.browser_action, extension);
     browserActionMap.set(extension, browserAction);
   }
 
-  onShutdown(reason) {
+  onShutdown() {
     let {extension} = this;
 
     if (browserActionMap.has(extension)) {
       browserActionMap.get(extension).shutdown();
       browserActionMap.delete(extension);
     }
   }
 
--- a/mobile/android/components/extensions/ext-pageAction.js
+++ b/mobile/android/components/extensions/ext-pageAction.js
@@ -210,17 +210,17 @@ this.pageAction = class extends Extensio
   onManifestEntry(entryName) {
     let {extension} = this;
     let {manifest} = extension;
 
     let pageAction = new PageAction(manifest.page_action, extension);
     pageActionMap.set(extension, pageAction);
   }
 
-  onShutdown(reason) {
+  onShutdown() {
     let {extension} = this;
 
     if (pageActionMap.has(extension)) {
       pageActionMap.get(extension).shutdown();
       pageActionMap.delete(extension);
     }
   }
 
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -2028,17 +2028,16 @@ class Extension extends ExtensionData {
       // caches. These caches may keep the file open.
       file.remove(false);
     }).catch(Cu.reportError);
   }
 
   async shutdown(reason) {
     this.state = "Shutdown";
 
-    this.shutdownReason = reason;
     this.hasShutdown = true;
 
     if (!this.policy) {
       return;
     }
 
     if (this.hasPermission("storage") && ExtensionStorageIDB.selectedBackendPromises.has(this)) {
       this.state = "Shutdown: Storage";
@@ -2056,30 +2055,31 @@ class Extension extends ExtensionData {
 
     if (this.rootURI instanceof Ci.nsIJARURI) {
       this.state = "Shutdown: Flush jar cache";
       let file = this.rootURI.JARFile.QueryInterface(Ci.nsIFileURL).file;
       Services.ppmm.broadcastAsyncMessage("Extension:FlushJarCache", {path: file.path});
       this.state = "Shutdown: Flushed jar cache";
     }
 
-    if (this.cleanupFile || reason !== "APP_SHUTDOWN") {
+    const isAppShutdown = reason === "APP_SHUTDOWN";
+    if (this.cleanupFile || !isAppShutdown) {
       StartupCache.clearAddonData(this.id);
     }
 
     activeExtensionIDs.delete(this.id);
     sharedData.set("extensions/activeIDs", activeExtensionIDs);
 
     for (let key of this.sharedDataKeys) {
       sharedData.delete(key);
     }
 
     Services.ppmm.removeMessageListener(this.MESSAGE_EMIT_EVENT, this);
 
-    this.updatePermissions(this.shutdownReason);
+    this.updatePermissions(reason);
 
     if (!this.manifest) {
       this.state = "Shutdown: Complete: No manifest";
       this.policy.active = false;
 
       return this.cleanupGeneratedFile();
     }
 
@@ -2087,17 +2087,17 @@ class Extension extends ExtensionData {
 
     for (let obj of this.onShutdown) {
       obj.close();
     }
 
     ParentAPIManager.shutdownExtension(this.id, reason);
 
     Management.emit("shutdown", this);
-    this.emit("shutdown");
+    this.emit("shutdown", isAppShutdown);
 
     const TIMED_OUT = Symbol();
 
     this.state = "Shutdown: Emit shutdown";
     let result = await Promise.race([
       this.broadcast("Extension:Shutdown", {id: this.id}),
       promiseTimeout(CHILD_SHUTDOWN_TIMEOUT_MS).then(() => TIMED_OUT),
     ]);
--- a/toolkit/components/extensions/ExtensionCommon.jsm
+++ b/toolkit/components/extensions/ExtensionCommon.jsm
@@ -329,19 +329,19 @@ class EventEmitter {
  * once for each extension that uses the API.
  */
 class ExtensionAPI extends EventEmitter {
   constructor(extension) {
     super();
 
     this.extension = extension;
 
-    extension.once("shutdown", () => {
+    extension.once("shutdown", (what, isAppShutdown) => {
       if (this.onShutdown) {
-        this.onShutdown(extension.shutdownReason);
+        this.onShutdown(isAppShutdown);
       }
       this.extension = null;
     });
   }
 
   destroy() {
   }
 
--- a/toolkit/components/extensions/parent/ext-protocolHandlers.js
+++ b/toolkit/components/extensions/parent/ext-protocolHandlers.js
@@ -45,21 +45,21 @@ this.protocolHandlers = class extends Ex
         protoInfo.preferredApplicationHandler = handler;
         protoInfo.alwaysAskBeforeHandling = false;
       }
       handlers.appendElement(handler);
       handlerService.store(protoInfo);
     }
   }
 
-  onShutdown(shutdownReason) {
+  onShutdown(isAppShutdown) {
     let {extension} = this;
     let {manifest} = extension;
 
-    if (shutdownReason === "APP_SHUTDOWN") {
+    if (isAppShutdown) {
       return;
     }
 
     for (let handlerConfig of manifest.protocol_handlers) {
       let protoInfo = protocolService.getProtocolHandlerInfo(handlerConfig.protocol);
       let appHandlers = protoInfo.possibleApplicationHandlers;
       for (let i = 0; i < appHandlers.length; i++) {
         let handler = appHandlers.queryElementAt(i, Ci.nsISupports);
--- a/toolkit/components/extensions/parent/ext-theme.js
+++ b/toolkit/components/extensions/parent/ext-theme.js
@@ -378,18 +378,18 @@ this.theme = class extends ExtensionAPI 
       extension,
       details: manifest.theme,
       darkDetails: manifest.dark_theme,
       experiment: manifest.theme_experiment,
       startupData: extension.startupData,
     });
   }
 
-  onShutdown(reason) {
-    if (reason === "APP_SHUTDOWN") {
+  onShutdown(isAppShutdown) {
+    if (isAppShutdown) {
       return;
     }
 
     let {extension} = this;
     for (let [windowId, theme] of windowOverrides) {
       if (theme.extension === extension) {
         Theme.unload(windowId);
       }