Bug 1543384 - Fix race in extension state setter r=kmag a=jcristau
authorRob Wu <rob@robwu.nl>
Thu, 23 May 2019 20:39:13 +0000
changeset 536465 fa195c94dc1082186356e97094feb62e4e1f5b01
parent 536464 5234a4220bae688d165edcd04567936f9d8ee823
child 536466 39c14a33468fde1a7316fc135243b647e750014f
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, jcristau
bugs1543384
milestone68.0
Bug 1543384 - Fix race in extension state setter r=kmag a=jcristau As a side effect of this patch, the format of the "state" value of "async shutdown timeout" crash reports will change, as follows: "Run manifest: " has been replaced with "Run manifest, ": ``` - Startup: Run manifest: asyncEmitManifestEntry("background") + Startup: Run manifest, asyncEmitManifestEntry("background") ``` Multiple states are now separated by ", " instead of ",": ``` - Startup: Run manifest: manifest_name,manifest_version + Startup: Run manifest, manifest_name, manifest_version ``` "Run manifest" will always have a "Startup: " in front of it: ``` - Startup: Emit Startup,Run manifest + Startup: Emit Startup, Startup: Run manifest ``` And removed the `manifest_*` event dispatch since it has no listeners. Differential Revision: https://phabricator.services.mozilla.com/D26986
toolkit/components/extensions/Extension.jsm
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -1381,16 +1381,17 @@ let pendingExtensions = new Map();
  * This class is the main representation of an active WebExtension
  * in the main process.
  * @extends ExtensionData
  */
 class Extension extends ExtensionData {
   constructor(addonData, startupReason) {
     super(addonData.resourceURI);
 
+    this.startupStates = new Set();
     this.state = "Not started";
 
     this.sharedDataKeys = new Set();
 
     this.uuid = UUIDMap.get(addonData.id);
     this.instanceId = getUniqueId();
 
     this.MESSAGE_EMIT_EVENT = `Extension:EmitEvent:${this.instanceId}`;
@@ -1487,16 +1488,34 @@ class Extension extends ExtensionData {
       this.policy.permissions = Array.from(this.permissions);
       this.policy.allowedOrigins = this.whiteListedHosts;
 
       this.cachePermissions();
     });
     /* eslint-enable mozilla/balanced-listeners */
   }
 
+  set state(startupState) {
+    this.startupStates.clear();
+    this.startupStates.add(startupState);
+  }
+
+  get state() {
+    return `${Array.from(this.startupStates).join(", ")}`;
+  }
+
+  async addStartupStatePromise(name, fn) {
+    this.startupStates.add(name);
+    try {
+      await fn();
+    } finally {
+      this.startupStates.delete(name);
+    }
+  }
+
   get restrictSchemes() {
     return !(this.isPrivileged && this.hasPermission("mozillaAddons"));
   }
 
   // Some helpful properties added elsewhere:
   /**
    * An object used to map between extension-visible tab ids and
    * native Tab object
@@ -1738,44 +1757,27 @@ class Extension extends ExtensionData {
     this.updateContentScripts();
   }
 
   updateContentScripts() {
     this.setSharedData("contentScripts", this.registeredContentScripts);
   }
 
   runManifest(manifest) {
-    let state = new Set();
-    let updateState = () => {
-      this.state = `Startup: Run manifest: ${Array.from(state)}`;
-    };
-
     let promises = [];
-    let addPromise = (name, promise) => {
-      if (promise) {
-        promises.push(promise);
-
-        state.add(name);
-        promise.finally(() => {
-          state.delete(name);
-          updateState();
-        });
-      }
+    let addPromise = (name, fn) => {
+      promises.push(this.addStartupStatePromise(name, fn));
     };
 
     for (let directive in manifest) {
       if (manifest[directive] !== null) {
-        addPromise(`manifest_${directive}`,
-                   Management.emit(`manifest_${directive}`, directive, this, manifest));
-
         addPromise(`asyncEmitManifestEntry("${directive}")`,
-                   Management.asyncEmitManifestEntry(this, directive));
+                   () => Management.asyncEmitManifestEntry(this, directive));
       }
     }
-    updateState();
 
     activeExtensionIDs.add(this.id);
     sharedData.set("extensions/activeIDs", activeExtensionIDs);
 
     pendingExtensions.delete(this.id);
     sharedData.set("extensions/pending", pendingExtensions);
 
     Services.ppmm.sharedData.flush();
@@ -1968,27 +1970,22 @@ class Extension extends ExtensionData {
       resolveReadyPromise(this.policy);
 
       // The "startup" Management event sent on the extension instance itself
       // is emitted just before the Management "startup" event,
       // and it is used to run code that needs to be executed before
       // any of the "startup" listeners.
       this.emit("startup", this);
 
-      let state = new Set(["Emit startup", "Run manifest"]);
-      this.state = `Startup: ${Array.from(state)}`;
+      this.startupStates.clear();
       await Promise.all([
-        Management.emit("startup", this).finally(() => {
-          state.delete("Emit startup");
-          this.state = `Startup: ${Array.from(state)}`;
-        }),
-        this.runManifest(this.manifest).finally(() => {
-          state.delete("Run manifest");
-          this.state = `Startup: ${Array.from(state)}`;
-        }),
+        this.addStartupStatePromise("Startup: Emit startup",
+                                    () => Management.emit("startup", this)),
+        this.addStartupStatePromise("Startup: Run manifest",
+                                    () => this.runManifest(this.manifest)),
       ]);
       this.state = "Startup: Ran manifest";
 
       Management.emit("ready", this);
       this.emit("ready");
 
       this.state = "Startup: Complete";
     } catch (errors) {