Bug 1145049 - Prevent caching tab actors in child processes. r=jryans

toolkit/devtools/server/actors/childtab.js

@@ -21,10 +21,14 @@ let { TabActor } = require("devtools/server/actors/webbrowser");
  *        The conection to the client.
  * @param chromeGlobal
  *        The content script global holding |content| and |docShell| properties for a tab.
+ * @param prefix
+ *        the prefix used in protocol to create IDs for each actor.
+ *        Used as ID identifying this particular TabActor from the parent process.
  */
-function ContentActor(connection, chromeGlobal)
+function ContentActor(connection, chromeGlobal, prefix)
 {
   this._chromeGlobal = chromeGlobal;
+  this._prefix = prefix;
   TabActor.call(this, connection, chromeGlobal);
   this.traits.reconfigure = false;
   this._sendForm = this._sendForm.bind(this);
@@ -49,32 +53,11 @@ Object.defineProperty(ContentActor.prototype, "title", {
 });
 
 ContentActor.prototype.exit = function() {
-  this._chromeGlobal.removeMessageListener("debug:form", this._sendForm);
-  this._sendForm = null;
-  TabActor.prototype.exit.call(this);
-};
-
-// Override form just to rename this._tabActorPool to this._tabActorPool2
-// in order to prevent it to be cleaned on detach.
-// We have to keep tab actors alive as we keep the ContentActor
-// alive after detach and reuse it for multiple debug sessions.
-ContentActor.prototype.form = function () {
-  let response = {
-    "actor": this.actorID,
-    "title": this.title,
-    "url": this.url
-  };
-
-  // Walk over tab actors added by extensions and add them to a new ActorPool.
-  let actorPool = new ActorPool(this.conn);
-  this._createExtraActors(DebuggerServer.tabActorFactories, actorPool);
-  if (!actorPool.isEmpty()) {
-    this._tabActorPool2 = actorPool;
-    this.conn.addActorPool(this._tabActorPool2);
+  if (this._sendForm) {
+    this._chromeGlobal.removeMessageListener("debug:form", this._sendForm);
+    this._sendForm = null;
   }
-
-  this._appendExtraActors(response);
-  return response;
+  return TabActor.prototype.exit.call(this);
 };
 
 /**

toolkit/devtools/server/actors/director-registry.js

@@ -111,7 +111,7 @@ const DirectorRegistry = exports.DirectorRegistry = {
 
 let gTrackedMessageManager = new Set();
 
-exports.setupParentProcess = function setupParentProcess({mm, childID}) {
+exports.setupParentProcess = function setupParentProcess({mm, prefix}) {
   // prevents multiple subscriptions on the same messagemanager
   if (gTrackedMessageManager.has(mm)) {
     return;
@@ -121,7 +121,7 @@ exports.setupParentProcess = function setupParentProcess({mm, childID}) {
   // listen for director-script requests from the child process
   mm.addMessageListener("debug:director-registry-request", handleChildRequest);
 
-  DebuggerServer.once("disconnected-from-child:" + childID, handleMessageManagerDisconnected);
+  DebuggerServer.once("disconnected-from-child:" + prefix, handleMessageManagerDisconnected);
 
   /* parent process helpers */
 

toolkit/devtools/server/actors/webapps.js

@@ -213,9 +213,9 @@ function WebappsActor(aConnection) {
   Cu.import("resource://gre/modules/AppsUtils.jsm");
   Cu.import("resource://gre/modules/FileUtils.jsm");
 
-  // Keep reference of already created app actors.
-  // key: app frame message manager, value: ContentActor's grip() value
-  this._appActorsMap = new Map();
+  // Keep reference of already connected app processes.
+  // values: app frame message manager
+  this._connectedApps = new Set();
 
   this.conn = aConnection;
   this._uploads = [];
@@ -960,24 +960,33 @@ WebappsActor.prototype = {
 
     // Only create a new actor, if we haven't already
     // instanciated one for this connection.
-    let map = this._appActorsMap;
+    let set = this._connectedApps;
     let mm = appFrame.QueryInterface(Ci.nsIFrameLoaderOwner)
                      .frameLoader
                      .messageManager;
-    let actor = map.get(mm);
-    if (!actor) {
+    if (!set.has(mm)) {
       let onConnect = actor => {
-        map.set(mm, actor);
+        set.add(mm);
         return { actor: actor };
       };
       let onDisconnect = mm => {
-        map.delete(mm);
+        set.delete(mm);
       };
       return DebuggerServer.connectToChild(this.conn, appFrame, onDisconnect)
                            .then(onConnect);
     }
 
-    return { actor: actor };
+    // We have to update the form as it may have changed
+    // if we detached the TabActor
+    let deferred = promise.defer();
+    let onFormUpdate = msg => {
+      mm.removeMessageListener("debug:form", onFormUpdate);
+      deferred.resolve({ actor: msg.json });
+    };
+    mm.addMessageListener("debug:form", onFormUpdate);
+    mm.sendAsyncMessage("debug:form");
+
+    return deferred.promise;
   },
 
   watchApps: function () {

toolkit/devtools/server/actors/webbrowser.js

@@ -876,10 +876,7 @@ TabActor.prototype = {
    * Called when the actor is removed from the connection.
    */
   disconnect: function BTA_disconnect() {
-    this._detach();
-    this._extraActors = null;
-    this._styleSheetActors.clear();
-    this._exited = true;
+    this.exit();
   },
 
   /**
@@ -901,6 +898,14 @@ TabActor.prototype = {
                        type: "tabDetached" });
     }
 
+    Object.defineProperty(this, "docShell", {
+      value: null,
+      configurable: true
+    });
+
+    this._extraActors = null;
+    this._styleSheetActors.clear();
+
     this._exited = true;
   },
 
@@ -1221,11 +1226,6 @@ TabActor.prototype = {
       this._tabActorPool = null;
     }
 
-    Object.defineProperty(this, "docShell", {
-      value: null,
-      configurable: true
-    });
-
     this._attached = false;
     return true;
   },
@@ -1822,7 +1822,10 @@ function RemoteBrowserTabActor(aConnection, aBrowser)
 
 RemoteBrowserTabActor.prototype = {
   connect: function() {
-    let connect = DebuggerServer.connectToChild(this._conn, this._browser);
+    let onDestroy = () => {
+      this._form = null;
+    };
+    let connect = DebuggerServer.connectToChild(this._conn, this._browser, onDestroy);
     return connect.then(form => {
       this._form = form;
       return this;
@@ -1835,15 +1838,21 @@ RemoteBrowserTabActor.prototype = {
   },
 
   update: function() {
-    let deferred = promise.defer();
-    let onFormUpdate = msg => {
-      this._mm.removeMessageListener("debug:form", onFormUpdate);
-      this._form = msg.json;
-      deferred.resolve(this);
-    };
-    this._mm.addMessageListener("debug:form", onFormUpdate);
-    this._mm.sendAsyncMessage("debug:form");
-    return deferred.promise;
+    // If the child happens to be crashed/close/detach, it won't have _form set,
+    // so only request form update if some code is still listening on the other side.
+    if (this._form) {
+      let deferred = promise.defer();
+      let onFormUpdate = msg => {
+        this._mm.removeMessageListener("debug:form", onFormUpdate);
+        this._form = msg.json;
+        deferred.resolve(this);
+      };
+      this._mm.addMessageListener("debug:form", onFormUpdate);
+      this._mm.sendAsyncMessage("debug:form");
+      return deferred.promise;
+    } else {
+      return this.connect();
+    }
   },
 
   form: function() {

toolkit/devtools/server/child.js

@@ -17,10 +17,6 @@ let chromeGlobal = this;
   const { dumpn } = DevToolsUtils;
   const { DebuggerServer, ActorPool } = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {});
 
-  if (!DebuggerServer.childID) {
-    DebuggerServer.childID = 1;
-  }
-
   if (!DebuggerServer.initialized) {
     DebuggerServer.init();
 
@@ -44,17 +40,16 @@ let chromeGlobal = this;
 
     let mm = msg.target;
     let prefix = msg.data.prefix;
-    let id = DebuggerServer.childID++;
 
     let conn = DebuggerServer.connectToParent(prefix, mm);
-    connections.set(id, conn);
+    connections.set(prefix, conn);
 
-    let actor = new DebuggerServer.ContentActor(conn, chromeGlobal);
+    let actor = new DebuggerServer.ContentActor(conn, chromeGlobal, prefix);
     let actorPool = new ActorPool(conn);
     actorPool.addActor(actor);
     conn.addActorPool(actorPool);
 
-    sendAsyncMessage("debug:actor", {actor: actor.form(), childID: id});
+    sendAsyncMessage("debug:actor", {actor: actor.form(), prefix: prefix});
   });
 
   addMessageListener("debug:connect", onConnect);
@@ -95,11 +90,11 @@ let chromeGlobal = this;
     // Call DebuggerServerConnection.close to destroy all child actors
     // (It should end up calling DebuggerServerConnection.onClosed
     // that would actually cleanup all actor pools)
-    let childID = msg.data.childID;
-    let conn = connections.get(childID);
+    let prefix = msg.data.prefix;
+    let conn = connections.get(prefix);
     if (conn) {
       conn.close();
-      connections.delete(childID);
+      connections.delete(prefix);
     }
   });
   addMessageListener("debug:disconnect", onDisconnect);

toolkit/devtools/server/docs/lazy-actor-modules.md

@@ -76,7 +76,7 @@ connected to the child process as parameter, e.g. in the **director-registry**:
 
 let gTrackedMessageManager = new Set();
 
-exports.setupParentProcess = function setupParentProcess({ mm, childID }) {
+exports.setupParentProcess = function setupParentProcess({ mm, prefix }) {
   if (gTrackedMessageManager.has(mm)) { return; }
   gTrackedMessageManager.add(mm);
 
@@ -84,7 +84,7 @@ exports.setupParentProcess = function setupParentProcess({ mm, childID }) {
   mm.addMessageListener("debug:director-registry-request", handleChildRequest);
 
   // time to unsubscribe from the disconnected message manager
-  DebuggerServer.once("disconnected-from-child:" + childID, handleMessageManagerDisconnected);
+  DebuggerServer.once("disconnected-from-child:" + prefix, handleMessageManagerDisconnected);
 
   function handleMessageManagerDisconnected(evt, { mm: disconnected_mm }) {
     ...
@@ -109,8 +109,8 @@ In the child process:
 In the parent process:
 - The DebuggerServer receives the DebuggerServer.setupInParent request
 - it tries to load the required module
-- it tries to call the **mod[setupParent]** method with the frame message manager and the childID
-  in the json parameter **{ mm, childID }**
+- it tries to call the **mod[setupParent]** method with the frame message manager and the prefix
+  in the json parameter **{ mm, prefix }**
   - the module setupParent helper use the mm to subscribe the messagemanager events
   - the module setupParent helper use the DebuggerServer object to subscribe *once* the
-    **"disconnected-from-child:CHILDID"** event (needed to unsubscribe the messagemanager events)
+    **"disconnected-from-child:PREFIX"** event (needed to unsubscribe the messagemanager events)

toolkit/devtools/server/main.js

@@ -809,13 +809,15 @@ var DebuggerServer = {
    *        The debugger server connection to use.
    * @param nsIDOMElement aFrame
    *        The browser element that holds the child process.
-   * @param function [aOnDisconnect]
-   *        Optional function to invoke when the child is disconnected.
+   * @param function [aOnDestroy]
+   *        Optional function to invoke when the child process closes
+   *        or the connection shuts down. (Need to forget about the
+   *        related TabActor)
    * @return object
    *         A promise object that is resolved once the connection is
    *         established.
    */
-  connectToChild: function(aConnection, aFrame, aOnDisconnect) {
+  connectToChild: function(aConnection, aFrame, aOnDestroy) {
     let deferred = defer();
 
     let mm = aFrame.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader
@@ -824,7 +826,6 @@ var DebuggerServer = {
 
     let actor, childTransport;
     let prefix = aConnection.allocID("child");
-    let childID = null;
     let netMonitor = null;
 
     // provides hook to actor modules that need to exchange messages
@@ -841,7 +842,7 @@ var DebuggerServer = {
           return false;
         }
 
-        m[setupParent]({ mm: mm, childID: childID });
+        m[setupParent]({ mm: mm, prefix: prefix });
 
         return true;
       } catch(e) {
@@ -855,10 +856,11 @@ var DebuggerServer = {
     mm.addMessageListener("debug:setup-in-parent", onSetupInParent);
 
     let onActorCreated = DevToolsUtils.makeInfallible(function (msg) {
+      if (msg.json.prefix != prefix) {
+        return;
+      }
       mm.removeMessageListener("debug:actor", onActorCreated);
 
-      childID = msg.json.childID;
-
       // Pipe Debugger message from/to parent/child via the message manager
       childTransport = new ChildDebuggerTransport(mm, prefix);
       childTransport.hooks = {
@@ -882,70 +884,65 @@ var DebuggerServer = {
     }).bind(this);
     mm.addMessageListener("debug:actor", onActorCreated);
 
-    let onMessageManagerClose = DevToolsUtils.makeInfallible(function (subject, topic, data) {
-      if (subject == mm) {
-        Services.obs.removeObserver(onMessageManagerClose, topic);
+    let destroy = DevToolsUtils.makeInfallible(function () {
+      // provides hook to actor modules that need to exchange messages
+      // between e10s parent and child processes
+      DebuggerServer.emit("disconnected-from-child:" + prefix, { mm: mm, prefix: prefix });
 
-        // provides hook to actor modules that need to exchange messages
-        // between e10s parent and child processes
-        this.emit("disconnected-from-child:" + childID, { mm: mm, childID: childID });
-
-        mm.removeMessageListener("debug:setup-in-parent", onSetupInParent);
-
-        if (childTransport) {
-          // If we have a child transport, the actor has already
-          // been created. We need to stop using this message manager.
-          childTransport.close();
-          childTransport = null;
-          aConnection.cancelForwarding(prefix);
-
-          // ... and notify the child process to clean the tab actors.
-          mm.sendAsyncMessage("debug:disconnect", { childID: childID });
-        } else {
-          // Otherwise, the app has been closed before the actor
-          // had a chance to be created, so we are not able to create
-          // the actor.
-          deferred.resolve(null);
-        }
-        if (actor) {
-          // The ContentActor within the child process doesn't necessary
-          // have to time to uninitialize itself when the app is closed/killed.
-          // So ensure telling the client that the related actor is detached.
-          aConnection.send({ from: actor.actor, type: "tabDetached" });
-          actor = null;
-        }
-
-        if (netMonitor) {
-          netMonitor.destroy();
-          netMonitor = null;
-        }
-
-        if (aOnDisconnect) {
-          aOnDisconnect(mm);
-        }
-      }
-    }).bind(this);
-    Services.obs.addObserver(onMessageManagerClose,
-                             "message-manager-close", false);
-
-    events.once(aConnection, "closed", () => {
       if (childTransport) {
-        // When the client disconnects, we have to unplug the dedicated
-        // ChildDebuggerTransport...
+        // If we have a child transport, the actor has already
+        // been created. We need to stop using this message manager.
         childTransport.close();
         childTransport = null;
         aConnection.cancelForwarding(prefix);
 
         // ... and notify the child process to clean the tab actors.
-        mm.sendAsyncMessage("debug:disconnect", { childID: childID });
-
-        if (netMonitor) {
-          netMonitor.destroy();
-          netMonitor = null;
-        }
+        mm.sendAsyncMessage("debug:disconnect", { prefix: prefix });
+      } else {
+        // Otherwise, the app has been closed before the actor
+        // had a chance to be created, so we are not able to create
+        // the actor.
+        deferred.resolve(null);
       }
+      if (actor) {
+        // The ContentActor within the child process doesn't necessary
+        // have time to uninitialize itself when the app is closed/killed.
+        // So ensure telling the client that the related actor is detached.
+        aConnection.send({ from: actor.actor, type: "tabDetached" });
+        actor = null;
+      }
+
+      if (netMonitor) {
+        netMonitor.destroy();
+        netMonitor = null;
+      }
+
+      if (aOnDestroy) {
+        aOnDestroy(mm);
+      }
+
+      // Cleanup all listeners
+      Services.obs.removeObserver(onMessageManagerClose, "message-manager-close");
+      mm.removeMessageListener("debug:setup-in-parent", onSetupInParent);
+      if (!actor) {
+        mm.removeMessageListener("debug:actor", onActorCreated);
+      }
+      events.off(aConnection, "closed", destroy);
     });
 
+    // Listen for app process exit
+    let onMessageManagerClose = function (subject, topic, data) {
+      if (subject == mm) {
+        destroy();
+      }
+    };
+    Services.obs.addObserver(onMessageManagerClose,
+                             "message-manager-close", false);
+
+    // Listen for connection close to cleanup things
+    // when user unplug the device or we lose the connection somehow.
+    events.on(aConnection, "closed", destroy);
+
     mm.sendAsyncMessage("debug:connect", { prefix: prefix });
 
     return deferred.promise;