Bug 1145049 - Prevent caching tab actors in child processes. r=jryans
☠☠ backed out by 698dff5de195 ☠ ☠
authorAlexandre Poirot <poirot.alex@gmail.com>
Wed, 15 Apr 2015 00:35:33 +0200
changeset 239317 69819d44541fce0bb91c6d20093e6670c771e59d
parent 239316 fb97412bef73d9e9e7a4b71ce510ae7b0f699f0a
child 239318 6c4285b179df8b999f4e41ab9c6274fbc3eb9ea0
push id28589
push userryanvm@gmail.com
push dateWed, 15 Apr 2015 19:13:10 +0000
treeherdermozilla-central@24ccca4707eb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjryans
bugs1145049
milestone40.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1145049 - Prevent caching tab actors in child processes. r=jryans
toolkit/devtools/server/actors/childtab.js
toolkit/devtools/server/actors/director-registry.js
toolkit/devtools/server/actors/webapps.js
toolkit/devtools/server/actors/webbrowser.js
toolkit/devtools/server/child.js
toolkit/devtools/server/docs/lazy-actor-modules.md
toolkit/devtools/server/main.js
--- a/toolkit/devtools/server/actors/childtab.js
+++ b/toolkit/devtools/server/actors/childtab.js
@@ -16,20 +16,24 @@ let { TabActor } = require("devtools/ser
  * Creates a tab actor for handling requests to the single tab, like
  * attaching and detaching. ContentActor respects the actor factories
  * registered with DebuggerServer.addTabActor.
  *
  * @param connection DebuggerServerConnection
  *        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);
   this._chromeGlobal.addMessageListener("debug:form", this._sendForm);
 
   Object.defineProperty(this, "docShell", {
     value: this._chromeGlobal.docShell,
     configurable: true
@@ -44,42 +48,21 @@ Object.defineProperty(ContentActor.proto
   get: function() {
     return this.window.document.title;
   },
   enumerable: true,
   configurable: true
 });
 
 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);
 };
 
 /**
  * On navigation events, our URL and/or title may change, so we update our
  * counterpart in the parent process that participates in the tab list.
  */
 ContentActor.prototype._sendForm = function() {
   this._chromeGlobal.sendAsyncMessage("debug:form", this.form());
--- a/toolkit/devtools/server/actors/director-registry.js
+++ b/toolkit/devtools/server/actors/director-registry.js
@@ -106,27 +106,27 @@ const DirectorRegistry = exports.Directo
 };
 
 /**
  * E10S parent/child setup helpers
  */
 
 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;
   }
   gTrackedMessageManager.add(mm);
 
   // 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 */
 
   function handleMessageManagerDisconnected(evt, { mm: disconnected_mm }) {
     // filter out not subscribed message managers
     if (disconnected_mm !== mm || !gTrackedMessageManager.has(mm)) {
       return;
     }
--- a/toolkit/devtools/server/actors/webapps.js
+++ b/toolkit/devtools/server/actors/webapps.js
@@ -208,19 +208,19 @@ function WebappsActor(aConnection) {
   debug("init");
   // Load actor dependencies lazily as this actor require extra environnement
   // preparation to work (like have a profile setup in xpcshell tests)
 
   Cu.import("resource://gre/modules/Webapps.jsm");
   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 = [];
   this._actorPool = new ActorPool(this.conn);
   this.conn.addActorPool(this._actorPool);
 }
 
 WebappsActor.prototype = {
@@ -955,34 +955,43 @@ WebappsActor.prototype = {
     }
 
     if (!this._isAppAllowedForURL(manifestURL)) {
       return notFoundError;
     }
 
     // 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 () {
     // For now, app open/close events are only implement on b2g
     if (Frames) {
       Frames.addObserver(this);
     }
     Services.obs.addObserver(this, "webapps-installed", false);
--- a/toolkit/devtools/server/actors/webbrowser.js
+++ b/toolkit/devtools/server/actors/webbrowser.js
@@ -871,20 +871,17 @@ TabActor.prototype = {
     this._appendExtraActors(response);
     return response;
   },
 
   /**
    * 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();
   },
 
   /**
    * Called by the root actor when the underlying tab is closed.
    */
   exit: function BTA_exit() {
     if (this.exited) {
       return;
@@ -896,16 +893,24 @@ TabActor.prototype = {
       this.threadActor._tabClosed = true;
     }
 
     if (this._detach()) {
       this.conn.send({ from: this.actorID,
                        type: "tabDetached" });
     }
 
+    Object.defineProperty(this, "docShell", {
+      value: null,
+      configurable: true
+    });
+
+    this._extraActors = null;
+    this._styleSheetActors.clear();
+
     this._exited = true;
   },
 
   /**
    * Return true if the given global is associated with this tab and should be
    * added as a debuggee, false otherwise.
    */
   _shouldAddNewGlobalAsDebuggee: function (wrappedGlobal) {
@@ -1216,21 +1221,16 @@ TabActor.prototype = {
     // Shut down actors that belong to this tab's pool.
     this.conn.removeActorPool(this._tabPool);
     this._tabPool = null;
     if (this._tabActorPool) {
       this.conn.removeActorPool(this._tabActorPool);
       this._tabActorPool = null;
     }
 
-    Object.defineProperty(this, "docShell", {
-      value: null,
-      configurable: true
-    });
-
     this._attached = false;
     return true;
   },
 
   // Protocol Request Handlers
 
   onAttach: function BTA_onAttach(aRequest) {
     if (this.exited) {
@@ -1817,38 +1817,47 @@ function RemoteBrowserTabActor(aConnecti
 {
   this._conn = aConnection;
   this._browser = aBrowser;
   this._form = null;
 }
 
 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;
     });
   },
 
   get _mm() {
     return this._browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader
            .messageManager;
   },
 
   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() {
     return this._form;
   },
 
   exit: function() {
     this._browser = null;
--- a/toolkit/devtools/server/child.js
+++ b/toolkit/devtools/server/child.js
@@ -12,20 +12,16 @@ let chromeGlobal = this;
 // more than once.
 (function () {
   let Cu = Components.utils;
   let { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
   const DevToolsUtils = devtools.require("devtools/toolkit/DevToolsUtils.js");
   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();
 
     // message manager helpers provided for actor module parent/child message exchange
     DebuggerServer.parentMessageManager = {
       sendSyncMessage: sendSyncMessage,
       addMessageListener: addMessageListener
     };
@@ -39,27 +35,26 @@ let chromeGlobal = this;
 
   let connections = new Map();
 
   let onConnect = DevToolsUtils.makeInfallible(function (msg) {
     removeMessageListener("debug:connect", onConnect);
 
     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);
 
   // Allows executing module setup helper from the parent process.
   // See also: DebuggerServer.setupInChild()
   let onSetupInChild = DevToolsUtils.makeInfallible(msg => {
     let { module, setupChild, args } = msg.data;
@@ -90,21 +85,21 @@ let chromeGlobal = this;
   addMessageListener("debug:setup-in-child", onSetupInChild);
 
   let onDisconnect = DevToolsUtils.makeInfallible(function (msg) {
     removeMessageListener("debug:disconnect", onDisconnect);
 
     // 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);
 
   let onInspect = DevToolsUtils.makeInfallible(function(msg) {
     // Store the node to be inspected in a global variable
     // (gInspectingNode). Later we'll fetch this variable again using
     // the findInspectingNode request over the remote debugging
--- a/toolkit/devtools/server/docs/lazy-actor-modules.md
+++ b/toolkit/devtools/server/docs/lazy-actor-modules.md
@@ -71,25 +71,25 @@ connected to the child process as parame
 
 ```js
 /**
  * E10S parent/child setup helpers
  */
 
 let gTrackedMessageManager = new Set();
 
-exports.setupParentProcess = function setupParentProcess({ mm, childID }) {
+exports.setupParentProcess = function setupParentProcess({ mm, prefix }) {
   if (gTrackedMessageManager.has(mm)) { return; }
   gTrackedMessageManager.add(mm);
 
   // listen for director-script requests from the child process
   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 }) {
     ...
   }
 ```
 
 The DebuggerServer emits "disconnected-from-child:CHILDID" events to give the actor modules
 the chance to cleanup their handlers registered on the disconnected message manager.
@@ -104,13 +104,13 @@ In the child process:
       to run the required setup
     - the actor module use the DebuggerServer.parentMessageManager.sendSyncMessage,
       DebuggerServer.parentMessageManager.addMessageListener helpers to send requests
       or to subscribe message handlers
       
 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)
--- a/toolkit/devtools/server/main.js
+++ b/toolkit/devtools/server/main.js
@@ -804,66 +804,68 @@ var DebuggerServer = {
 
   /**
    * Connect to a child process.
    *
    * @param object aConnection
    *        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
              .messageManager;
     mm.loadFrameScript("resource://gre/modules/devtools/server/child.js", false);
 
     let actor, childTransport;
     let prefix = aConnection.allocID("child");
-    let childID = null;
     let netMonitor = null;
 
     // provides hook to actor modules that need to exchange messages
     // between e10s parent and child processes
     let onSetupInParent = function (msg) {
       let { module, setupParent } = msg.json;
       let m, fn;
 
       try {
         m = require(module);
 
         if (!setupParent in m) {
           dumpn("ERROR: module '" + module + "' does not export '" + setupParent + "'");
           return false;
         }
 
-        m[setupParent]({ mm: mm, childID: childID });
+        m[setupParent]({ mm: mm, prefix: prefix });
 
         return true;
       } catch(e) {
         let error_msg = "exception during actor module setup running in the parent process: ";
         DevToolsUtils.reportException(error_msg + e);
         dumpn("ERROR: " + error_msg + " \n\t module: '" + module + "' \n\t setupParent: '" + setupParent + "'\n" +
               DevToolsUtils.safeErrorString(e));
         return false;
       }
     };
     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 = {
         onPacket: aConnection.send.bind(aConnection),
         onClosed: function () {}
       };
       childTransport.ready();
 
@@ -877,80 +879,75 @@ var DebuggerServer = {
       netMonitor = new NetworkMonitorManager(aFrame, actor.actor);
 
       events.emit(DebuggerServer, "new-child-process", { mm: mm });
 
       deferred.resolve(actor);
     }).bind(this);
     mm.addMessageListener("debug:actor", onActorCreated);
 
-    let onMessageManagerClose = DevToolsUtils.makeInfallible(function (subject, topic, data) {
-      if (subject == mm) {
-        Services.obs.removeObserver(onMessageManagerClose, topic);
-
-        // 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);
+    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 });
 
-          // ... 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 });
+        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 (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;
   },
 
   /**
    * Create a new debugger connection for the given transport. Called after
    * connectPipe(), from connectToParent, or from an incoming socket