Bug 1577498 - Part 3: Ensure actor and conduit cleanup r=rpl

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

--HG--
extra : moz-landing-system : lando

toolkit/components/extensions/ConduitsChild.jsm

@@ -47,7 +47,7 @@ class BaseConduit {
    * @param {string} method
    * @param {boolean} query Flag indicating a response is expected.
    * @param {JSWindowActor} actor
-   * @param {{arg: any, sender: ConduitID}} data
+   * @param {{arg: object, sender: ConduitID}} data
    * @returns {Promise?}
    */
   _send(method, query, actor, data) {
@@ -94,10 +94,10 @@ class PointConduit extends BaseConduit {
    * Internal, sends messages via the actor, used by sendX stubs.
    * @param {string} method
    * @param {boolean} query
-   * @param {object} arg
+   * @param {object?} arg
    * @returns {Promise?}
    */
-  _send(method, query, arg) {
+  _send(method, query, arg = {}) {
     if (!this.actor) {
       throw new Error(`send${method} on closed conduit ${this.id}`);
     }
@@ -106,13 +106,17 @@ class PointConduit extends BaseConduit {
   }
 
   /**
-   * Closes the conduit from further IPC, notifies the parent side.
+   * Closes the conduit from further IPC, notifies the parent side by default.
+   * @param {boolean} silent
    */
-  close() {
-    if (this.actor) {
-      this.actor.conduits.delete(this.id);
-      this.actor.sendAsyncMessage("ConduitClosed", { arg: this.id });
+  close(silent = false) {
+    let { actor } = this;
+    if (actor) {
       this.actor = null;
+      actor.conduits.delete(this.id);
+      if (!silent) {
+        actor.sendAsyncMessage("ConduitClosed", { arg: this.id });
+      }
     }
   }
 }
@@ -139,25 +143,32 @@ class ConduitsChild extends JSWindowActorChild {
   }
 
   /**
-   * JSWindowActorChild method, routes the message to the target subject.
+   * JSWindowActor method, routes the message to the target subject.
    * @returns {Promise?}
    */
   receiveMessage({ name, data: { target, arg, query, sender } }) {
     let conduit = this.conduits.get(target);
     if (!conduit) {
-      throw new Error(`${name} ${arg.path} for closed conduit ${target}`);
+      throw new Error(`${name} for closed conduit ${target}: ${uneval(arg)}`);
     }
     return conduit._recv(name, arg, { sender, query, actor: this });
   }
 
   /**
-   * JSWindowActorChild method, called when the actor is getting destroyed.
-   * Can't IPC from this point, so silently closes all conduits.
+   * JSWindowActor method, called before actor is destroyed.
+   * Parent side will get the same call, so just silently close all conduits.
    */
   willDestroy() {
     for (let conduit of this.conduits.values()) {
-      conduit.actor = null;
+      conduit.close(true);
     }
     this.conduits.clear();
   }
+
+  /**
+   * JSWindowActor method, ensure cleanup (see bug 1596187).
+   */
+  didDestroy() {
+    this.willDestroy();
+  }
 }

toolkit/components/extensions/ConduitsParent.jsm

@@ -59,7 +59,7 @@ const Hub = {
   /** @type Map<ConduitID, ConduitAddress> Info about all child conduits. */
   remotes: new Map(),
 
-  /** @type Map<ConduitID, BroadcastConduit> All open prent conduits. */
+  /** @type Map<ConduitID, BroadcastConduit> All open parent conduits. */
   conduits: new Map(),
 
   /** @type Map<string, BroadcastConduit> Parent conduits by recvMethod. */
@@ -78,6 +78,10 @@ const Hub = {
   openConduit(conduit) {
     this.conduits.set(conduit.id, conduit);
     for (let name of conduit.address.recv || []) {
+      if (this.byMethod.get(name)) {
+        // For now, we only allow one parent conduit handling each recv method.
+        throw new Error(`Duplicate BroadcastConduit method name recv${name}`);
+      }
       this.byMethod.set(name, conduit);
     }
   },
@@ -111,6 +115,7 @@ const Hub = {
   recvConduitClosed(remote) {
     this.remotes.delete(remote.id);
     this.byActor.get(remote.actor).delete(remote);
+
     remote.actor = null;
     for (let conduit of this.onRemoteClosed.get(remote.id)) {
       conduit.subject.recvConduitClosed(remote);
@@ -125,6 +130,7 @@ const Hub = {
     for (let remote of this.byActor.get(actor)) {
       this.recvConduitClosed(remote);
     }
+    this.byActor.delete(actor);
   },
 };
 
@@ -149,10 +155,10 @@ class BroadcastConduit extends BaseConduit {
    * @param {string} method
    * @param {boolean} query
    * @param {ConduitID} target
-   * @param {any} arg
+   * @param {object?} arg
    * @returns {Promise<any>}
    */
-  _send(method, query, target, arg) {
+  _send(method, query, target, arg = {}) {
     if (!this.open) {
       throw new Error(`send${method} on closed conduit ${this.id}`);
     }
@@ -162,7 +168,7 @@ class BroadcastConduit extends BaseConduit {
   }
 
   /**
-   * Inditate the subject wants to listen for the specifric conduit closing.
+   * Indicate the subject wants to listen for the specific conduit closing.
    * The method `recvConduitClosed(address)` will be called.
    * @param {ConduitID} target
    */
@@ -185,7 +191,7 @@ class ConduitsParent extends JSWindowActorParent {
   }
 
   /**
-   * JSWindowActorParent method, routes the message to the target subject.
+   * JSWindowActor method, routes the message to the target subject.
    * @returns {Promise?}
    */
   receiveMessage({ name, data: { arg, query, sender } }) {
@@ -200,14 +206,22 @@ class ConduitsParent extends JSWindowActorParent {
     if (!conduit) {
       throw new Error(`Parent conduit for recv${name} not found`);
     }
+
     sender = Hub.remotes.get(sender);
     return conduit._recv(name, arg, { actor: this, query, sender });
   }
 
   /**
-   * JSWindowActorPaent method, called when the actor is getting destroyed.
+   * JSWindowActor method, called before actor is destroyed.
    */
   willDestroy() {
     Hub.actorClosed(this);
   }
+
+  /**
+   * JSWindowActor method, ensure cleanup (see bug 1596187).
+   */
+  didDestroy() {
+    this.willDestroy();
+  }
 }

toolkit/components/extensions/ExtensionChild.jsm

@@ -1173,13 +1173,19 @@ class ChildAPIManager {
 
     this.id = `${context.extension.id}.${context.contextId}`;
 
-    let global = context.contentWindow.getWindowGlobalChild();
-    this.conduit = global.getActor("Conduits").openConduit(this, {
+    this.conduit = context.openConduit(this, {
       id: this.id,
       send: ["CreateProxyContext", "APICall", "AddListener", "RemoveListener"],
       recv: ["CallResult", "RunListener"],
     });
 
+    this.conduit.sendCreateProxyContext({
+      childId: this.id,
+      extensionId: context.extension.id,
+      principal: context.principal,
+      ...contextData,
+    });
+
     this.listeners = new DefaultMap(() => ({
       ids: new Map(),
       listeners: new Map(),
@@ -1189,15 +1195,6 @@ class ChildAPIManager {
     // Map[callId -> Deferred]
     this.callPromises = new Map();
 
-    let params = {
-      childId: this.id,
-      extensionId: context.extension.id,
-      principal: context.principal,
-    };
-    Object.assign(params, contextData);
-
-    this.conduit.sendCreateProxyContext(params);
-
     this.permissionsChangedCallbacks = new Set();
     this.updatePermissions = null;
     if (this.context.extension.optionalPermissions.length) {

toolkit/components/extensions/ExtensionParent.jsm

@@ -902,7 +902,7 @@ ParentAPIManager = {
   proxyContexts: new Map(),
 
   init() {
-    // TODO: remove/replace everything that deals with message managers below.
+    // TODO: Bug 1595186 - remove/replace all usage of MessageManager below.
     Services.obs.addObserver(this, "message-manager-close");
 
     this.conduit = new BroadcastConduit(this, {
@@ -1064,11 +1064,8 @@ ParentAPIManager = {
   },
 
   async recvAPICall(data, { actor }) {
-    return this.call(data, actor.browsingContext.top.embedderElement);
-  },
-
-  async call(data, target) {
     let context = this.getContextById(data.childId);
+    let target = actor.browsingContext.top.embedderElement;
     if (context.parentMessageManager !== target.messageManager) {
       throw new Error("Got message on unexpected message manager");
     }
@@ -1127,12 +1124,9 @@ ParentAPIManager = {
     }
   },
 
-  recvAddListener(data, { actor }) {
-    this.addListener(data, actor.browsingContext.top.embedderElement);
-  },
-
-  async addListener(data, target) {
+  async recvAddListener(data, { actor }) {
     let context = this.getContextById(data.childId);
+    let target = actor.browsingContext.top.embedderElement;
     if (context.parentMessageManager !== target.messageManager) {
       throw new Error("Got message on unexpected message manager");
     }
@@ -1140,8 +1134,8 @@ ParentAPIManager = {
     let { childId } = data;
     let handlingUserInput = false;
 
-    // TODO: conduit.idleRunListener
-    let lowPriority = data.path.startsWith("webRequest."); // eslint-disable-line
+    // TODO: Bug 1587058 - Redesign webRequest event coelescing.
+    // let lowPriority = data.path.startsWith("webRequest.");
 
     let listener = async (...listenerArgs) => {
       let result = await this.conduit.queryRunListener(childId, {