Bug 1571453 - Handle exceptions in bootstrapped extension methods better. r=mkmelin a=jorgk
authorGeoff Lankow <geoff@darktrojan.net>
Tue, 06 Aug 2019 19:58:36 +1200
changeset 35289 4d87c1eeaf459c63c013e206c36252dc75a7bac0
parent 35288 064d9eae4999cb48ca01a7b15ece76780592dc6e
child 35290 a221fedbe06298daff382f00ab1e5890c3f6b1b3
push id2477
push usermozilla@jorgk.com
push dateTue, 13 Aug 2019 21:14:23 +0000
treeherdercomm-beta@436d8b620b6a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmkmelin, jorgk
bugs1571453
Bug 1571453 - Handle exceptions in bootstrapped extension methods better. r=mkmelin a=jorgk
common/test/xpcshell/test_bootstrap.js
mail/components/extensions/parent/ext-legacy.js
--- a/common/test/xpcshell/test_bootstrap.js
+++ b/common/test/xpcshell/test_bootstrap.js
@@ -108,16 +108,29 @@ const ADDONS = {
         type: "bootstrap",
       },
       manifest_version: 2,
     }),
     "bootstrap.js": BOOTSTRAP_MONITOR_BOOTSTRAP_JS,
   },
 };
 
+var startupCacheMonitor = {
+  notificationFired: false,
+  observe() {
+    this.notificationFired = true;
+  },
+  // Checks that the notification has been fired since the last time this was called.
+  check(expected) {
+    equal(this.notificationFired, expected);
+    this.notificationFired = false;
+  },
+};
+Services.obs.addObserver(startupCacheMonitor, "startupcache-invalidate");
+
 var testserver = AddonTestUtils.createHttpServer({hosts: ["example.com"]});
 
 const XPIS = {};
 for (let [name, addon] of Object.entries(ADDONS)) {
   XPIS[name] = AddonTestUtils.createTempXPIFile(addon);
   testserver.registerFile(`/addons/${name}.xpi`, XPIS[name]);
 }
 
@@ -264,16 +277,18 @@ add_task(async function test_1() {
   BootstrapMonitor.checkAddonInstalled(ID1, "1.0");
   BootstrapMonitor.checkAddonStarted(ID1, "1.0");
   equal(getStartupReason(), ADDON_INSTALL);
   equal(getStartupOldVersion(), undefined);
   do_check_in_crash_annotation(ID1, "1.0");
 
   let dir = do_get_addon_root_uri(profileDir, ID1);
   equal(b1.getResourceURI("bootstrap.js").spec, dir + "bootstrap.js");
+
+  startupCacheMonitor.check(false);
 });
 
 // Tests that disabling doesn't require a restart
 add_task(async function test_2() {
   let b1 = await AddonManager.getAddonByID(ID1);
   prepare_test({
     [ID1]: [
       ["onDisabling", false],
@@ -302,16 +317,17 @@ add_task(async function test_2() {
   let newb1 = await AddonManager.getAddonByID(ID1);
   notEqual(newb1, null);
   equal(newb1.version, "1.0");
   ok(!newb1.appDisabled);
   ok(newb1.userDisabled);
   ok(!newb1.isActive);
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Test that restarting doesn't accidentally re-enable
 add_task(async function test_3() {
   await promiseShutdownManager();
 
   BootstrapMonitor.checkAddonInstalled(ID1, "1.0");
   BootstrapMonitor.checkAddonNotStarted(ID1);
@@ -331,16 +347,17 @@ add_task(async function test_3() {
   let b1 = await AddonManager.getAddonByID(ID1);
   notEqual(b1, null);
   equal(b1.version, "1.0");
   ok(!b1.appDisabled);
   ok(b1.userDisabled);
   ok(!b1.isActive);
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Tests that enabling doesn't require a restart
 add_task(async function test_4() {
   let b1 = await AddonManager.getAddonByID(ID1);
   prepare_test({
     [ID1]: [
       ["onEnabling", false],
@@ -368,16 +385,17 @@ add_task(async function test_4() {
   let newb1 = await AddonManager.getAddonByID(ID1);
   notEqual(newb1, null);
   equal(newb1.version, "1.0");
   ok(!newb1.appDisabled);
   ok(!newb1.userDisabled);
   ok(newb1.isActive);
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Tests that a restart shuts down and restarts the add-on
 add_task(async function test_5() {
   await promiseShutdownManager();
   // By the time we've shut down, the database must have been written
   ok(gExtensionsJSON.exists());
 
@@ -397,16 +415,17 @@ add_task(async function test_5() {
   notEqual(b1, null);
   equal(b1.version, "1.0");
   ok(!b1.appDisabled);
   ok(!b1.userDisabled);
   ok(b1.isActive);
   ok(!b1.isSystem);
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Tests that installing an upgrade doesn't require a restart
 add_task(async function test_6() {
   prepare_test({}, [
     "onNewInstall",
   ]);
 
@@ -430,16 +449,18 @@ add_task(async function test_6() {
       }, [
         "onInstallStarted",
         "onInstallEnded",
       ], resolve);
       install.install();
     }),
   ]);
 
+  startupCacheMonitor.check(true);
+
   let b1 = await AddonManager.getAddonByID(ID1);
   notEqual(b1, null);
   equal(b1.version, "2.0");
   ok(!b1.appDisabled);
   ok(!b1.userDisabled);
   ok(b1.isActive);
   ok(!b1.isSystem);
   BootstrapMonitor.checkAddonInstalled(ID1, "2.0");
@@ -466,16 +487,17 @@ add_task(async function test_7() {
     ],
   });
 
   equal(b1.operationsRequiringRestart &
         AddonManager.OP_NEEDS_RESTART_UNINSTALL, 0);
   await b1.uninstall();
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(true);
 
   ensure_test_completed();
   BootstrapMonitor.checkAddonNotInstalled(ID1);
   BootstrapMonitor.checkAddonNotStarted(ID1);
   equal(getShutdownReason(), ADDON_DISABLE);
   equal(getShutdownNewVersion(), undefined);
   do_check_not_in_crash_annotation(ID1, "2.0");
 
@@ -483,25 +505,27 @@ add_task(async function test_7() {
   equal(b1, null);
 
   await promiseRestartManager();
 
   let newb1 = await AddonManager.getAddonByID(ID1);
   equal(newb1, null);
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Test that a bootstrapped extension dropped into the profile loads properly
 // on startup and doesn't cause an EM restart
 add_task(async function test_8() {
   await promiseShutdownManager();
 
   await manuallyInstall(XPIS.test_bootstrap1_1, profileDir, ID1);
 
+  startupCacheMonitor.check(false);
   await promiseStartupManager();
 
   let b1 = await AddonManager.getAddonByID(ID1);
   notEqual(b1, null);
   equal(b1.version, "1.0");
   ok(!b1.appDisabled);
   ok(!b1.userDisabled);
   ok(b1.isActive);
@@ -524,16 +548,17 @@ add_task(async function test_9() {
 
   await promiseStartupManager();
 
   let b1 = await AddonManager.getAddonByID(ID1);
   equal(b1, null);
   do_check_not_in_crash_annotation(ID1, "1.0");
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 
 // Tests that installing a downgrade sends the right reason
 add_task(async function test_10() {
   prepare_test({}, [
     "onNewInstall",
   ]);
@@ -559,16 +584,17 @@ add_task(async function test_10() {
       }, [
         "onInstallStarted",
         "onInstallEnded",
       ], resolve);
       install.install();
     }),
   ]);
 
+  startupCacheMonitor.check(false);
 
   let b1 = await AddonManager.getAddonByID(ID1);
   notEqual(b1, null);
   equal(b1.version, "2.0");
   ok(!b1.appDisabled);
   ok(!b1.userDisabled);
   ok(b1.isActive);
   ok(!b1.isSystem);
@@ -602,16 +628,18 @@ add_task(async function test_10() {
       }, [
         "onInstallStarted",
         "onInstallEnded",
       ], resolve);
       install.install();
     }),
   ]);
 
+  startupCacheMonitor.check(true);
+
   b1 = await AddonManager.getAddonByID(ID1);
   notEqual(b1, null);
   equal(b1.version, "1.0");
   ok(!b1.appDisabled);
   ok(!b1.userDisabled);
   ok(b1.isActive);
   ok(!b1.isSystem);
   BootstrapMonitor.checkAddonInstalled(ID1, "1.0");
@@ -621,16 +649,17 @@ add_task(async function test_10() {
   equal(getStartupOldVersion(), 2);
   equal(getShutdownReason(), ADDON_DOWNGRADE);
   equal(getShutdownNewVersion(), 1);
   equal(getUninstallNewVersion(), 1);
   do_check_in_crash_annotation(ID1, "1.0");
   do_check_not_in_crash_annotation(ID1, "2.0");
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Tests that uninstalling a disabled add-on still calls the uninstall method
 add_task(async function test_11() {
   let b1 = await AddonManager.getAddonByID(ID1);
   prepare_test({
     [ID1]: [
       ["onDisabling", false],
@@ -644,16 +673,17 @@ add_task(async function test_11() {
 
   BootstrapMonitor.checkAddonInstalled(ID1, "1.0");
   BootstrapMonitor.checkAddonNotStarted(ID1);
   equal(getShutdownReason(), ADDON_DISABLE);
   equal(getShutdownNewVersion(), undefined);
   do_check_not_in_crash_annotation(ID1, "1.0");
 
   await b1.uninstall();
+  startupCacheMonitor.check(true);
 
   ensure_test_completed();
   BootstrapMonitor.checkAddonNotInstalled(ID1);
   BootstrapMonitor.checkAddonNotStarted(ID1);
   do_check_not_in_crash_annotation(ID1, "1.0");
 
   await checkBootstrappedPref();
 });
@@ -676,16 +706,17 @@ add_task(async function test_12() {
   ok(!b1.isSystem);
   BootstrapMonitor.checkAddonInstalled(ID1, "1.0");
   BootstrapMonitor.checkAddonStarted(ID1, "1.0");
   equal(getStartupReason(), ADDON_INSTALL);
   equal(getStartupOldVersion(), undefined);
   do_check_in_crash_annotation(ID1, "1.0");
 
   await b1.uninstall();
+  startupCacheMonitor.check(true);
 
   await promiseRestartManager();
   await checkBootstrappedPref();
 });
 
 // Check that a bootstrapped extension in a non-profile location is loaded
 add_task(async function test_17() {
   await promiseShutdownManager();
@@ -699,25 +730,27 @@ add_task(async function test_17() {
   BootstrapMonitor.checkAddonInstalled(ID1, "1.0");
   BootstrapMonitor.checkAddonStarted(ID1, "1.0");
   notEqual(b1, null);
   equal(b1.version, "1.0");
   ok(b1.isActive);
   ok(!b1.isSystem);
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Check that installing a new bootstrapped extension in the profile replaces
 // the existing one
 add_task(async function test_18() {
   await Promise.all([
     BootstrapMonitor.promiseAddonStartup(ID1),
     promiseInstallFile(XPIS.test_bootstrap1_2),
   ]);
+  startupCacheMonitor.check(true);
 
   let b1 = await AddonManager.getAddonByID(ID1);
   // Should have installed and started
   BootstrapMonitor.checkAddonInstalled(ID1, "2.0");
   BootstrapMonitor.checkAddonStarted(ID1, "2.0");
   notEqual(b1, null);
   equal(b1.version, "2.0");
   ok(b1.isActive);
@@ -729,16 +762,17 @@ add_task(async function test_18() {
   equal(getStartupReason(), ADDON_UPGRADE);
 
   equal(getShutdownNewVersion(), 2);
   equal(getUninstallNewVersion(), 2);
   equal(getInstallOldVersion(), 1);
   equal(getStartupOldVersion(), 1);
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
 
 // Check that uninstalling the profile version reveals the non-profile one
 add_task(async function test_19() {
   let b1 = await AddonManager.getAddonByID(ID1);
   // The revealed add-on gets activated asynchronously
   await new Promise(resolve => {
     prepare_test({
@@ -748,16 +782,18 @@ add_task(async function test_19() {
         ["onInstalling", false],
         "onInstalled",
       ],
     }, [], resolve);
 
     b1.uninstall();
   });
 
+  startupCacheMonitor.check(true);
+
   b1 = await AddonManager.getAddonByID(ID1);
   // Should have reverted to the older version
   BootstrapMonitor.checkAddonInstalled(ID1, "1.0");
   BootstrapMonitor.checkAddonStarted(ID1, "1.0");
   notEqual(b1, null);
   equal(b1.version, "1.0");
   ok(b1.isActive);
   ok(!b1.isSystem);
@@ -768,9 +804,10 @@ add_task(async function test_19() {
   equal(getStartupReason(), ADDON_DOWNGRADE);
 
   equal(getShutdownNewVersion(), "1.0");
   equal(getUninstallNewVersion(), "1.0");
   equal(getInstallOldVersion(), "2.0");
   equal(getStartupOldVersion(), "2.0");
 
   await checkBootstrappedPref();
+  startupCacheMonitor.check(false);
 });
--- a/mail/components/extensions/parent/ext-legacy.js
+++ b/mail/components/extensions/parent/ext-legacy.js
@@ -301,35 +301,62 @@ this.legacy = class extends ExtensionAPI
     }
 
     let install = findMethod("install");
     let uninstall = findMethod("uninstall");
     let startup = findMethod("startup");
     let shutdown = findMethod("shutdown");
 
     return {
-      install: (...args) => install(...args),
+      install(...args) {
+        try {
+          install(...args);
+        } catch (ex) {
+          logger.warn(
+            `Exception running bootstrap method install on ${extension.id}`,
+            ex
+          );
+        }
+      },
 
       uninstall(...args) {
-        uninstall(...args);
-        // Forget any cached files we might've had from this extension.
-        Services.obs.notifyObservers(null, "startupcache-invalidate");
+        try {
+          uninstall(...args);
+        } catch (ex) {
+          logger.warn(
+            `Exception running bootstrap method uninstall on ${extension.id}`,
+            ex
+          );
+        } finally {
+          // Forget any cached files we might've had from this extension.
+          Services.obs.notifyObservers(null, "startupcache-invalidate");
+        }
       },
 
       startup(...args) {
         logger.debug(`Registering manifest for ${file.path}\n`);
         Components.manager.addBootstrappedManifestLocation(file);
-        return startup(...args);
+        try {
+          startup(...args);
+        } catch (ex) {
+          logger.warn(
+            `Exception running bootstrap method startup on ${extension.id}`,
+            ex
+          );
+        }
       },
 
       shutdown(data, reason) {
         try {
-          return shutdown(data, reason);
-        } catch (err) {
-          throw err;
+          shutdown(data, reason);
+        } catch (ex) {
+          logger.warn(
+            `Exception running bootstrap method shutdown on ${extension.id}`,
+            ex
+          );
         } finally {
           if (reason != BOOTSTRAP_REASONS.APP_SHUTDOWN) {
             logger.debug(`Removing manifest for ${file.path}\n`);
             Components.manager.removeBootstrappedManifestLocation(file);
           }
         }
       },
     };