Bug 1651844 - Ensure that cleanup is run when startup() exits early r=rpl

In most cases, the presence of any errors causes an error to be thrown
at startup. Sometimes, that is not the case, and as a result the
extension is not properly unregistered.

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

toolkit/components/extensions/Extension.jsm

@@ -503,6 +503,14 @@ class ExtensionData {
     this.logger[severity](`Loading extension '${this.id}': ${message}`);
   }
 
+  ensureNoErrors() {
+    if (this.errors.length) {
+      // startup() repeatedly checks whether there are errors after parsing the
+      // extension/manifest before proceeding with starting up.
+      throw new Error(this.errors.join("\n"));
+    }
+  }
+
   /**
    * Returns the moz-extension: URL for the given path within this
    * extension.
@@ -2079,9 +2087,7 @@ class Extension extends ExtensionData {
   async loadManifest() {
     let manifest = await super.loadManifest();
 
-    if (this.errors.length) {
-      return Promise.reject({ errors: this.errors });
-    }
+    this.ensureNoErrors();
 
     return manifest;
   }
@@ -2431,11 +2437,11 @@ class Extension extends ExtensionData {
         this.state = "Startup: Initted locale";
       }
 
-      if (this.errors.length) {
-        return Promise.reject({ errors: this.errors });
-      }
+      this.ensureNoErrors();
 
       if (this.hasShutdown) {
+        // Startup was interrupted and shutdown() has taken care of unloading
+        // the extension and running cleanup logic.
         return;
       }
 
@@ -2540,17 +2546,10 @@ class Extension extends ExtensionData {
       this.emit("ready");
 
       this.state = "Startup: Complete";
-    } catch (errors) {
-      this.state = `Startup: Error: ${errors}`;
+    } catch (e) {
+      this.state = `Startup: Error: ${e}`;
 
-      for (let e of [].concat(errors)) {
-        dump(
-          `Extension error: ${e.message || e} ${e.filename || e.fileName}:${
-            e.lineNumber
-          } :: ${e.stack || new Error().stack}\n`
-        );
-        Cu.reportError(e);
-      }
+      Cu.reportError(e);
 
       if (this.policy) {
         this.policy.active = false;
@@ -2558,7 +2557,7 @@ class Extension extends ExtensionData {
 
       this.cleanupGeneratedFile();
 
-      throw errors;
+      throw e;
     } finally {
       ExtensionTelemetry.extensionStartup.stopwatchFinish(this);
     }

toolkit/components/extensions/test/xpcshell/test_ext_extension_startup_failure.js

@@ -0,0 +1,35 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+const { ExtensionTestCommon } = ChromeUtils.import(
+  "resource://testing-common/ExtensionTestCommon.jsm"
+);
+
+add_task(async function extension_startup_early_error() {
+  const EXTENSION_ID = "@extension-with-package-error";
+  let extension = ExtensionTestCommon.generate({
+    manifest: {
+      applications: { gecko: { id: EXTENSION_ID } },
+    },
+  });
+
+  extension.initLocale = async function() {
+    // Simulate error that happens during startup.
+    extension.packagingError("dummy error");
+  };
+
+  let startupPromise = extension.startup();
+
+  await Assert.rejects(
+    startupPromise,
+    /dummy error/,
+    "Extension with packaging error should fail to load"
+  );
+
+  Assert.equal(
+    WebExtensionPolicy.getByID(EXTENSION_ID),
+    null,
+    "WebExtensionPolicy should be unregistered"
+  );
+});

toolkit/components/extensions/test/xpcshell/xpcshell-common.ini

@@ -82,6 +82,7 @@ skip-if = os == "android"
 [test_ext_extensionSettingsStore.js]
 [test_ext_extension_content_telemetry.js]
 skip-if = os == "android" # checking for telemetry needs to be updated: 1384923
+[test_ext_extension_startup_failure.js]
 [test_ext_extension_startup_telemetry.js]
 [test_ext_file_access.js]
 [test_ext_geckoProfiler_control.js]