Bug 1181100 - Fix actors being registered dynamically when closing the first connected tab. r=jryans,mratcliffe a=ritu
authorAlexandre Poirot <poirot.alex@gmail.com>
Mon, 03 Aug 2015 07:52:42 -0700
changeset 291463 d4bf2f467ce9a65322367d6441b90594ff9d9d7b
parent 291462 12a8098d19453f001f7b8e54d8664a6e736ab450
child 291464 9a2b2c73fe26368e9ed44c6ea6799206e6a64ec8
push id5246
push usermozilla@noorenberghe.ca
push dateWed, 09 Sep 2015 21:17:14 +0000
reviewersjryans, mratcliffe, ritu
bugs1181100
milestone41.0
Bug 1181100 - Fix actors being registered dynamically when closing the first connected tab. r=jryans,mratcliffe a=ritu
toolkit/devtools/server/actors/storage.js
toolkit/devtools/server/child.js
toolkit/devtools/server/docs/actor-e10s-handling.md
toolkit/devtools/server/main.js
toolkit/devtools/server/tests/mochitest/chrome.ini
toolkit/devtools/server/tests/mochitest/setup-in-child.js
toolkit/devtools/server/tests/mochitest/setup-in-parent.js
toolkit/devtools/server/tests/mochitest/test_setupInParentChild.html
toolkit/devtools/server/tests/unit/head_dbg.js
--- a/toolkit/devtools/server/actors/storage.js
+++ b/toolkit/devtools/server/actors/storage.js
@@ -642,19 +642,19 @@ StorageActors.createActor({
     if (!DebuggerServer.isInChildProcess) {
       this.getCookiesFromHost = cookieHelpers.getCookiesFromHost;
       this.addCookieObservers = cookieHelpers.addCookieObservers;
       this.removeCookieObservers = cookieHelpers.removeCookieObservers;
       return;
     }
 
     const { sendSyncMessage, addMessageListener } =
-      DebuggerServer.parentMessageManager;
+      this.conn.parentMessageManager;
 
-    DebuggerServer.setupInParent({
+    this.conn.setupInParent({
       module: "devtools/server/actors/storage",
       setupParent: "setupParentProcessForCookies"
     });
 
     this.getCookiesFromHost =
       callParentProcess.bind(null, "getCookiesFromHost");
     this.addCookieObservers =
       callParentProcess.bind(null, "addCookieObservers");
@@ -737,37 +737,36 @@ let cookieHelpers = {
         let host = msg.data.args[0];
         let cookies = cookieHelpers.getCookiesFromHost(host);
         return JSON.stringify(cookies);
       case "addCookieObservers":
         return cookieHelpers.addCookieObservers();
         break;
       case "removeCookieObservers":
         return cookieHelpers.removeCookieObservers();
-        return null;
       default:
         console.error("ERR_DIRECTOR_PARENT_UNKNOWN_METHOD", msg.json.method);
         throw new Error("ERR_DIRECTOR_PARENT_UNKNOWN_METHOD");
     }
   },
 };
 
 /**
  * E10S parent/child setup helpers
  */
 
-exports.setupParentProcessForCookies = function({mm, childID}) {
+exports.setupParentProcessForCookies = function({mm, prefix}) {
   cookieHelpers.onCookieChanged =
     callChildProcess.bind(null, "onCookieChanged");
 
   // listen for director-script requests from the child process
   mm.addMessageListener("storage:storage-cookie-request-parent",
                         cookieHelpers.handleChildRequest);
 
-  DebuggerServer.once("disconnected-from-child:" + childID,
+  DebuggerServer.once("disconnected-from-child:" + prefix,
                       handleMessageManagerDisconnected);
 
   gTrackedMessageManager.set("cookies", mm);
 
   function handleMessageManagerDisconnected(evt, { mm: disconnected_mm }) {
     // filter out not subscribed message managers
     if (disconnected_mm !== mm || !gTrackedMessageManager.has(mm)) {
       return;
@@ -1026,16 +1025,19 @@ DatabaseMetadata.prototype = {
 };
 
 StorageActors.createActor({
   typeName: "indexedDB",
   storeObjectType: "idbstoreobject"
 }, {
   initialize: function(storageActor) {
     protocol.Actor.prototype.initialize.call(this, null);
+
+    this.storageActor = storageActor;
+
     this.maybeSetupChildProcess();
 
     this.objectsSize = {};
     this.storageActor = storageActor;
     this.onWindowReady = this.onWindowReady.bind(this);
     this.onWindowDestroyed = this.onWindowDestroyed.bind(this);
 
     events.on(this.storageActor, "window-ready", this.onWindowReady);
@@ -1224,19 +1226,19 @@ StorageActors.createActor({
       this.getValuesForHost = indexedDBHelpers.getValuesForHost;
       this.getObjectStoreData = indexedDBHelpers.getObjectStoreData;
       this.patchMetadataMapsAndProtos =
         indexedDBHelpers.patchMetadataMapsAndProtos;
       return;
     }
 
     const { sendSyncMessage, addMessageListener } =
-      DebuggerServer.parentMessageManager;
+      this.conn.parentMessageManager;
 
-    DebuggerServer.setupInParent({
+    this.conn.setupInParent({
       module: "devtools/server/actors/storage",
       setupParent: "setupParentProcessForIndexedDB"
     });
 
     this.getDBMetaData =
       callParentProcessAsync.bind(null, "getDBMetaData");
     this.getDBNamesForHost =
       callParentProcessAsync.bind(null, "getDBNamesForHost");
@@ -1590,22 +1592,22 @@ let indexedDBHelpers = {
     }
   }
 };
 
 /**
  * E10S parent/child setup helpers
  */
 
-exports.setupParentProcessForIndexedDB = function({mm, childID}) {
+exports.setupParentProcessForIndexedDB = function({mm, prefix}) {
   // listen for director-script requests from the child process
   mm.addMessageListener("storage:storage-indexedDB-request-parent",
                         indexedDBHelpers.handleChildRequest);
 
-  DebuggerServer.once("disconnected-from-child:" + childID,
+  DebuggerServer.once("disconnected-from-child:" + prefix,
                       handleMessageManagerDisconnected);
 
   gTrackedMessageManager.set("indexedDB", mm);
 
   function handleMessageManagerDisconnected(evt, { mm: disconnected_mm }) {
     // filter out not subscribed message managers
     if (disconnected_mm !== mm || !gTrackedMessageManager.has(mm)) {
       return;
--- a/toolkit/devtools/server/child.js
+++ b/toolkit/devtools/server/child.js
@@ -12,24 +12,21 @@ 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", {});
 
+  // Note that this frame script may be evaluated in non-e10s build
+  // In such case, DebuggerServer is already going to be initialized.
   if (!DebuggerServer.initialized) {
     DebuggerServer.init();
-
-    // message manager helpers provided for actor module parent/child message exchange
-    DebuggerServer.parentMessageManager = {
-      sendSyncMessage: sendSyncMessage,
-      addMessageListener: addMessageListener
-    };
+    DebuggerServer.isInChildProcess = true;
   }
 
   // In case of apps being loaded in parent process, DebuggerServer is already
   // initialized, but child specific actors are not registered.
   // Otherwise, for apps in child process, we need to load actors the first
   // time we load child.js
   DebuggerServer.addChildActors();
 
@@ -37,16 +34,17 @@ let chromeGlobal = this;
 
   let onConnect = DevToolsUtils.makeInfallible(function (msg) {
     removeMessageListener("debug:connect", onConnect);
 
     let mm = msg.target;
     let prefix = msg.data.prefix;
 
     let conn = DebuggerServer.connectToParent(prefix, mm);
+    conn.parentMessageManager = mm;
     connections.set(prefix, conn);
 
     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(), prefix: prefix});
--- a/toolkit/devtools/server/docs/actor-e10s-handling.md
+++ b/toolkit/devtools/server/docs/actor-e10s-handling.md
@@ -20,25 +20,27 @@ E.g. in the **director-registry**:
 
   // Setup the child<->parent communication only if the actor module
   // is running in a child process.
   if (DebuggerServer.isInChildProcess) {
     setupChildProcess();
   }
 
   function setupChildProcess() {
-    DebuggerServer.setupInParent({
+    // `setupInParent`  is defined on DebuggerServerConnection,
+    // your actor receive a reference to one instance in its constructor.
+    conn.setupInParent({
       module: "devtools/server/actors/director-registry",
       setupParent: "setupParentProcess"
     });
     // ...
   }
 ```
 
-The `setupChildProcess` helper defined and used in the previous example uses the `DebuggerServer.setupInParent` to run a given setup function in the parent process Debugger Server, e.g. in the **director-registry** module.
+The `setupChildProcess` helper defined and used in the previous example uses the `DebuggerServerConnection.setupInParent` to run a given setup function in the parent process Debugger Server, e.g. in the **director-registry** module.
 
 With this, the `DebuggerServer` running in the parent process will require the requested module (**director-registry**) and call its `setupParentProcess` function (which should be exported on the module).
 
 The `setupParentProcess` function will receive a parameter that contains a reference to the **MessageManager** and a prefix that should be used to send/receive messages between the child and parent processes.
 
 See below an example implementation of a `setupParent` function in the parent process:
 
 ```
@@ -76,27 +78,27 @@ exports.setupParentProcess = function se
 
     gTrackedMessageManager.delete(mm);
 
     // unregister for director-script requests handlers from the parent process (if any)
     mm.removeMessageListener("debug:director-registry-request", handleChildRequest);
   }
 ```
 
-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.
+The `DebuggerServer` emits "disconnected-from-child:PREFIX" events to give the actor modules the chance to cleanup their handlers registered on the disconnected message manager.
 
 ## Summary of the setup flow
 
 In the child process:
 
 * The `DebuggerServer` loads an actor module,
 * the actor module checks `DebuggerServer.isInChildProcess` to know whether it runs in a child process or not,
-* the actor module then uses the `DebuggerServer.setupInParent` helper to start setting up a parent-process counterpart,
-* the `DebuggerServer.setupInParent` helper asks the parent process to run the required module's setup function,
-* the actor module uses the `DebuggerServer.parentMessageManager.sendSyncMessage` and `DebuggerServer.parentMessageManager.addMessageListener` helpers to send or listen to message.
+* the actor module then uses the `DebuggerServerConnection.setupInParent` helper to start setting up a parent-process counterpart,
+* the `DebuggerServerConnection.setupInParent` helper asks the parent process to run the required module's setup function,
+* the actor module uses the `DebuggerServerConnection.parentMessageManager.sendSyncMessage` and `DebuggerServerConnection.parentMessageManager.addMessageListener` helpers to send or listen to message.
 
 In the parent process:
 
-* The DebuggerServer receives the `DebuggerServer.setupInParent` request,
+* The DebuggerServer receives the `DebuggerServerConnection.setupInParent` request,
 * tries to load the required module,
 * tries to call the `module[setupParent]` function with the frame message manager and the prefix as parameters `{ mm, prefix }`,
 * the `setupParent` function then uses the mm to subscribe the messagemanager events,
-* the `setupParent` function also uses the DebuggerServer object to subscribe *once* to the `"disconnected-from-child:PREFIX"` event to unsubscribe from messagemanager events.
\ No newline at end of file
+* the `setupParent` function also uses the DebuggerServer object to subscribe *once* to the `"disconnected-from-child:PREFIX"` event to unsubscribe from messagemanager events.
--- a/toolkit/devtools/server/main.js
+++ b/toolkit/devtools/server/main.js
@@ -878,73 +878,51 @@ var DebuggerServer = {
         }
       };
       aDbg.addListener(listener);
     });
   },
 
   /**
    * Check if the caller is running in a content child process.
+   * (Eventually set by child.js)
    *
    * @return boolean
    *         true if the caller is running in a content
    */
-  get isInChildProcess() {
-    return !!this.parentMessageManager;
-  },
+  isInChildProcess: false,
 
   /**
    * In a chrome parent process, ask all content child processes
    * to execute a given module setup helper.
    *
    * @param module
    *        The module to be required
    * @param setupChild
    *        The name of the setup helper exported by the above module
    *        (setup helper signature: function ({mm}) { ... })
    */
   setupInChild: function({ module, setupChild, args }) {
     if (this.isInChildProcess) {
       return;
     }
 
-    const gMessageManager = Cc["@mozilla.org/globalmessagemanager;1"].
-      getService(Ci.nsIMessageListenerManager);
-
-    gMessageManager.broadcastAsyncMessage("debug:setup-in-child", {
-      module: module,
-      setupChild: setupChild,
-      args: args,
+    this._childMessageManagers.forEach(mm => {
+      mm.sendAsyncMessage("debug:setup-in-child", {
+        module: module,
+        setupChild: setupChild,
+        args: args,
+      });
     });
   },
 
   /**
-   * In a content child process, ask the DebuggerServer in the parent process
-   * to execute a given module setup helper.
-   *
-   * @param module
-   *        The module to be required
-   * @param setupParent
-   *        The name of the setup helper exported by the above module
-   *        (setup helper signature: function ({mm}) { ... })
-   * @return boolean
-   *         true if the setup helper returned successfully
+   * Live list of all currenctly attached child's message managers.
    */
-  setupInParent: function({ module, setupParent }) {
-    if (!this.isInChildProcess) {
-      return false;
-    }
-
-    let { sendSyncMessage } = DebuggerServer.parentMessageManager;
-
-    return sendSyncMessage("debug:setup-in-parent", {
-      module: module,
-      setupParent: setupParent
-    });
-  },
+  _childMessageManagers: new Set(),
 
   /**
    * 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.
@@ -957,16 +935,17 @@ var DebuggerServer = {
    *         established.
    */
   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);
+    this._childMessageManagers.add(mm);
 
     let actor, childTransport;
     let prefix = aConnection.allocID("child");
     let netMonitor = null;
 
     // provides hook to actor modules that need to exchange messages
     // between e10s parent and child processes
     let onSetupInParent = function (msg) {
@@ -1062,16 +1041,18 @@ var DebuggerServer = {
 
       // 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);
+
+      DebuggerServer._childMessageManagers.delete(mm);
     });
 
     // Listen for app process exit
     let onMessageManagerClose = function (subject, topic, data) {
       if (subject == mm) {
         destroy();
       }
     };
@@ -1333,16 +1314,23 @@ function DebuggerServerConnection(aPrefi
 
 DebuggerServerConnection.prototype = {
   _prefix: null,
   get prefix() { return this._prefix },
 
   _transport: null,
   get transport() { return this._transport },
 
+  /**
+   * Message manager used to communicate with the parent process,
+   * set by child.js. Is only defined for connections instantiated
+   * within a child process.
+   */
+  parentMessageManager: null,
+
   close: function() {
     this._transport.close();
   },
 
   send: function DSC_send(aPacket) {
     this.transport.send(aPacket);
   },
 
@@ -1721,10 +1709,35 @@ DebuggerServerConnection.prototype = {
 
   /*
    * Debugging helper for inspecting the state of an actor pool.
    */
   _dumpPool: function DSC_dumpPools(aPool) {
     dumpn("/-------------------- dumping pool:");
     dumpn("--------------------- actorPool actors: " +
           uneval(Object.keys(aPool._actors)));
-  }
+  },
+
+  /**
+   * In a content child process, ask the DebuggerServer in the parent process
+   * to execute a given module setup helper.
+   *
+   * @param module
+   *        The module to be required
+   * @param setupParent
+   *        The name of the setup helper exported by the above module
+   *        (setup helper signature: function ({mm}) { ... })
+   * @return boolean
+   *         true if the setup helper returned successfully
+   */
+  setupInParent: function({ conn, module, setupParent }) {
+    if (!this.parentMessageManager) {
+      return false;
+    }
+
+    let { sendSyncMessage } = this.parentMessageManager;
+
+    return sendSyncMessage("debug:setup-in-parent", {
+      module: module,
+      setupParent: setupParent
+    });
+  },
 };
--- a/toolkit/devtools/server/tests/mochitest/chrome.ini
+++ b/toolkit/devtools/server/tests/mochitest/chrome.ini
@@ -11,16 +11,18 @@ support-files =
   inspector-helpers.js
   inspector-styles-data.css
   inspector-styles-data.html
   inspector-traversal-data.html
   large-image.jpg
   memory-helpers.js
   nonchrome_unsafeDereference.html
   small-image.gif
+  setup-in-child.js
+  setup-in-parent.js
 
 [test_connection-manager.html]
 skip-if = buildapp == 'mulet'
 [test_connectToChild.html]
 skip-if = buildapp == 'mulet'
 [test_css-logic.html]
 [test_css-logic-inheritance.html]
 [test_css-logic-media-queries.html]
@@ -80,15 +82,16 @@ skip-if = buildapp == 'mulet'
 [test_memory_attach_02.html]
 [test_memory_census.html]
 [test_memory_gc_01.html]
 [test_memory_gc_events.html]
 [test_preference.html]
 [test_registerActor.html]
 [test_SaveHeapSnapshot.html]
 [test_settings.html]
+[test_setupInParentChild.html]
 [test_styles-applied.html]
 [test_styles-computed.html]
 [test_styles-layout.html]
 [test_styles-matched.html]
 [test_styles-modify.html]
 [test_styles-svg.html]
 [test_unsafeDereference.html]
new file mode 100644
--- /dev/null
+++ b/toolkit/devtools/server/tests/mochitest/setup-in-child.js
@@ -0,0 +1,20 @@
+const {Cc, Ci} = require("chrome");
+const cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"].
+  getService(Ci.nsIMessageListenerManager);
+const { DebuggerServer } = require("devtools/server/main");
+
+exports.setupChild = function (a, b, c) {
+  cpmm.sendAsyncMessage("test:setupChild", [a, b, c]);
+}
+
+exports.callParent = function () {
+  // Hack! Fetch DebuggerServerConnection objects directly within DebuggerServer guts.
+  for (let id in DebuggerServer._connections) {
+    let conn = DebuggerServer._connections[id];
+    conn.setupInParent({
+      module: "chrome://mochitests/content/chrome/toolkit/devtools/server/tests/mochitest/setup-in-parent.js",
+      setupParent: "setupParent",
+      args: [{one: true}, 2, "three"]
+    });
+  }
+}
new file mode 100644
--- /dev/null
+++ b/toolkit/devtools/server/tests/mochitest/setup-in-parent.js
@@ -0,0 +1,10 @@
+let {Ci} = require("chrome");
+let Services = require("Services");
+
+exports.setupParent = function ({mm, prefix}) {
+  let args = [
+    !!mm.QueryInterface(Ci.nsIMessageSender),
+    prefix
+  ];
+  Services.obs.notifyObservers(null, "test:setupParent", JSON.stringify(args));
+}
new file mode 100644
--- /dev/null
+++ b/toolkit/devtools/server/tests/mochitest/test_setupInParentChild.html
@@ -0,0 +1,109 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+Bug 1181100 - Test DebuggerServerConnection.setupInParent and DebuggerServer.setupInChild
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Mozilla Bug</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+</head>
+<body>
+<pre id="test">
+<script type="application/javascript;version=1.8">
+
+let Cu = Components.utils;
+let Cc = Components.classes;
+let Ci = Components.interfaces;
+
+Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/devtools/dbg-client.jsm");
+Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
+
+window.onload = function() {
+  SimpleTest.waitForExplicitFinish();
+
+  SpecialPowers.pushPrefEnv({
+    "set": [
+      // Always log packets when running tests.
+      ["devtools.debugger.log", true],
+      ["dom.mozBrowserFramesEnabled", true]
+    ]
+  }, runTests);
+}
+
+function runTests() {
+  // Create a minimal iframe with a message manager
+  let iframe = document.createElement("iframe");
+  iframe.mozbrowser = true;
+  document.body.appendChild(iframe);
+
+  let mm = iframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.messageManager;
+
+  // Instantiate a minimal server
+  if (!DebuggerServer.initialized) {
+    DebuggerServer.init();
+  }
+  if (!DebuggerServer.createRootActor) {
+    DebuggerServer.addBrowserActors();
+  }
+
+  // Fake a connection to an iframe
+  let transport = DebuggerServer.connectPipe();
+  let conn = transport._serverConnection;
+  let client = new DebuggerClient(transport);
+
+  // Wait for a response from setupInChild
+  const ppmm = Cc["@mozilla.org/parentprocessmessagemanager;1"]
+                 .getService(Ci.nsIMessageListenerManager);
+  let onChild = msg => {
+    ppmm.removeMessageListener("test:setupChild", onChild);
+    let args = msg.json;
+
+    is(args[0], 1, "Got first numeric argument");
+    is(args[1], "two", "Got second string argument");
+    is(args[2].three, true, "Got last JSON argument");
+
+    // Ask the child to call setupInParent
+    DebuggerServer.setupInChild({
+      module: "chrome://mochitests/content/chrome/toolkit/devtools/server/tests/mochitest/setup-in-child.js",
+      setupChild: "callParent"
+    });
+  };
+  ppmm.addMessageListener("test:setupChild", onChild);
+
+  // Wait also for a reponse from setupInParent called from setup-in-child.js
+  let onParent = (_, topic, args) => {
+    Services.obs.removeObserver(onParent, "test:setupParent", false);
+    args = JSON.parse(args);
+
+    is(args[0], true, "Got `mm` argument, a message manager");
+    ok(args[1].match(/server\d+.conn\d+.child\d+/), "Got `prefix` argument");
+
+    cleanup();
+  };
+  Services.obs.addObserver(onParent, "test:setupParent", false);
+
+  // Instanciate e10s machinery and call setupInChild
+  DebuggerServer.connectToChild(conn, iframe).then(actor => {
+    DebuggerServer.setupInChild({
+      module: "chrome://mochitests/content/chrome/toolkit/devtools/server/tests/mochitest/setup-in-child.js",
+      setupChild: "setupChild",
+      args: [1, "two", {three: true}]
+    });
+  });
+
+  function cleanup() {
+    client.close(function () {
+      DebuggerServer.destroy();
+      iframe.remove();
+      SimpleTest.finish()
+    });
+  }
+
+}
+</script>
+</pre>
+</body>
+</html>
--- a/toolkit/devtools/server/tests/unit/head_dbg.js
+++ b/toolkit/devtools/server/tests/unit/head_dbg.js
@@ -325,16 +325,17 @@ function initTestTracerServer(aServer = 
   });
   // Allow incoming connections.
   aServer.init(function () { return true; });
 }
 
 function finishClient(aClient)
 {
   aClient.close(function() {
+    DebuggerServer.destroy();
     do_test_finished();
   });
 }
 
 // Create a server, connect to it and fetch tab actors for the parent process;
 // pass |aCallback| the debugger client and tab actor form with all actor IDs.
 function get_chrome_actors(callback)
 {