Bug 1240912 - Add browser swap support to multiprocess actors. r=ochameau draft
authorJ. Ryan Stinnett <jryans@gmail.com>
Fri, 05 Aug 2016 19:03:12 -0500
changeset 402168 26b2f193a08e683d9dff466ad7d88db27c77a739
parent 402167 eec7e8b15d9baccc91105f08c526e1a2ed44912d
child 402169 3c797cae9e501d6a4dcbb288450221742a811e17
push id26614
push userbmo:jryans@gmail.com
push dateThu, 18 Aug 2016 01:07:09 +0000
reviewersochameau
bugs1240912
milestone51.0a1
Bug 1240912 - Add browser swap support to multiprocess actors. r=ochameau Watch for browser swap events and update the message manager as necessary for each of the actors that use the `setupInParent` multiprocess functionality. MozReview-Commit-ID: HPibSONSYk4
devtools/server/actors/director-registry.js
devtools/server/actors/storage.js
devtools/server/child.js
devtools/server/docs/actor-e10s-handling.md
devtools/server/main.js
devtools/shared/webconsole/network-monitor.js
devtools/shared/webconsole/server-logger-monitor.js
--- a/devtools/server/actors/director-registry.js
+++ b/devtools/server/actors/director-registry.js
@@ -105,55 +105,48 @@ const DirectorRegistry = exports.Directo
     gDirectorScripts = Object.create(null);
   }
 };
 
 /**
  * E10S parent/child setup helpers
  */
 
-var gTrackedMessageManager = new Set();
-
-exports.setupParentProcess = function setupParentProcess({mm, prefix}) {
-  // prevents multiple subscriptions on the same messagemanager
-  if (gTrackedMessageManager.has(mm)) {
-    return;
-  }
-  gTrackedMessageManager.add(mm);
-
+exports.setupParentProcess = function setupParentProcess({ mm, prefix }) {
   // listen for director-script requests from the child process
-  mm.addMessageListener("debug:director-registry-request", handleChildRequest);
-
-  DebuggerServer.once("disconnected-from-child:" + prefix, handleMessageManagerDisconnected);
+  swapBrowser(mm);
 
   /* parent process helpers */
 
-  function handleMessageManagerDisconnected(evt, { mm: disconnected_mm }) {
-    // filter out not subscribed message managers
-    if (disconnected_mm !== mm || !gTrackedMessageManager.has(mm)) {
-      return;
-    }
-
-    gTrackedMessageManager.delete(mm);
-
-    // unregister for director-script requests handlers from the parent process (if any)
-    mm.removeMessageListener("debug:director-registry-request", handleChildRequest);
-  }
-
   function handleChildRequest(msg) {
     switch (msg.json.method) {
       case "get":
         return DirectorRegistry.get(msg.json.args[0]);
       case "list":
         return DirectorRegistry.list();
       default:
         console.error(ERR_DIRECTOR_PARENT_UNKNOWN_METHOD, msg.json.method);
         throw new Error(ERR_DIRECTOR_PARENT_UNKNOWN_METHOD);
     }
   }
+
+  function swapBrowser(newMM) {
+    if (mm) {
+      mm.removeMessageListener("debug:director-registry-request", handleChildRequest);
+    }
+    mm = newMM;
+    if (mm) {
+      mm.addMessageListener("debug:director-registry-request", handleChildRequest);
+    }
+  }
+
+  return {
+    onBrowserSwap: swapBrowser,
+    onDisconnected: () => swapBrowser(null),
+  };
 };
 
 /**
  * The DirectorRegistry Actor is a global actor which manages install/uninstall of
  * director scripts definitions.
  */
 const DirectorRegistryActor = exports.DirectorRegistryActor = protocol.ActorClassWithSpec(directorRegistrySpec, {
   /* init & destroy methods */
--- a/devtools/server/actors/storage.js
+++ b/devtools/server/actors/storage.js
@@ -13,18 +13,16 @@ const Services = require("Services");
 const promise = require("promise");
 const {isWindowIncluded} = require("devtools/shared/layout/utils");
 const specs = require("devtools/shared/specs/storage");
 const { Task } = require("devtools/shared/task");
 
 loader.lazyImporter(this, "OS", "resource://gre/modules/osfile.jsm");
 loader.lazyImporter(this, "Sqlite", "resource://gre/modules/Sqlite.jsm");
 
-var gTrackedMessageManager = new Map();
-
 // Maximum number of cookies/local storage key-value-pairs that can be sent
 // over the wire to the client in one request.
 const MAX_STORE_OBJECT_COUNT = 50;
 // Delay for the batch job that sends the accumulated update packets to the
 // client (ms).
 const BATCH_DELAY = 200;
 
 // MAX_COOKIE_EXPIRY should be 2^63-1, but JavaScript can't handle that
@@ -904,64 +902,62 @@ var cookieHelpers = {
     }
   },
 };
 
 /**
  * E10S parent/child setup helpers
  */
 
-exports.setupParentProcessForCookies = function ({mm, prefix}) {
+exports.setupParentProcessForCookies = function ({ mm, prefix }) {
   cookieHelpers.onCookieChanged =
     callChildProcess.bind(null, "onCookieChanged");
 
   // listen for director-script requests from the child process
-  mm.addMessageListener("debug:storage-cookie-request-parent",
-                        cookieHelpers.handleChildRequest);
-
-  DebuggerServer.once("disconnected-from-child:" + prefix,
-                      handleMessageManagerDisconnected);
-
-  gTrackedMessageManager.set("cookies", mm);
-
-  function handleMessageManagerDisconnected(evt, { mm: disconnectedMm }) {
-    // filter out not subscribed message managers
-    if (disconnectedMm !== mm || !gTrackedMessageManager.has("cookies")) {
-      return;
-    }
-
-    // Although "disconnected-from-child" implies that the child is already
-    // disconnected this is not the case. The disconnection takes place after
-    // this method has finished. This gives us chance to clean up items within
-    // the parent process e.g. observers.
-    cookieHelpers.removeCookieObservers();
-
-    gTrackedMessageManager.delete("cookies");
-
-    // unregister for director-script requests handlers from the parent process
-    // (if any)
-    mm.removeMessageListener("debug:storage-cookie-request-parent",
-                             cookieHelpers.handleChildRequest);
-  }
+  swapBrowser(mm);
 
   function callChildProcess(methodName, ...args) {
     if (methodName === "onCookieChanged") {
       args[0] = JSON.stringify(args[0]);
     }
 
     try {
       mm.sendAsyncMessage("debug:storage-cookie-request-child", {
         method: methodName,
         args: args
       });
     } catch (e) {
       // We may receive a NS_ERROR_NOT_INITIALIZED if the target window has
       // been closed. This can legitimately happen in between test runs.
     }
   }
+
+  function swapBrowser(newMM) {
+    if (mm) {
+      mm.removeMessageListener("debug:storage-cookie-request-parent",
+                               cookieHelpers.handleChildRequest);
+    }
+    mm = newMM;
+    if (mm) {
+      mm.addMessageListener("debug:storage-cookie-request-parent",
+                            cookieHelpers.handleChildRequest);
+    }
+  }
+
+  return {
+    onBrowserSwap: swapBrowser,
+    onDisconnected: () => {
+      // Although "disconnected-from-child" implies that the child is already
+      // disconnected this is not the case. The disconnection takes place after
+      // this method has finished. This gives us chance to clean up items within
+      // the parent process e.g. observers.
+      cookieHelpers.removeCookieObservers();
+      swapBrowser(null);
+    }
+  };
 };
 
 /**
  * Helper method to create the overriden object required in
  * StorageActors.createActor for Local Storage and Session Storage.
  * This method exists as both Local Storage and Session Storage have almost
  * identical actors.
  */
@@ -2122,39 +2118,36 @@ var indexedDBHelpers = {
     }
   }
 };
 
 /**
  * E10S parent/child setup helpers
  */
 
-exports.setupParentProcessForIndexedDB = function ({mm, prefix}) {
+exports.setupParentProcessForIndexedDB = function ({ mm, prefix }) {
   // listen for director-script requests from the child process
-  mm.addMessageListener("debug:storage-indexedDB-request-parent",
-                        indexedDBHelpers.handleChildRequest);
-
-  DebuggerServer.once("disconnected-from-child:" + prefix,
-                      handleMessageManagerDisconnected);
-
-  gTrackedMessageManager.set("indexedDB", mm);
+  swapBrowser(mm);
 
-  function handleMessageManagerDisconnected(evt, { mm: disconnectedMm }) {
-    // filter out not subscribed message managers
-    if (disconnectedMm !== mm || !gTrackedMessageManager.has("indexedDB")) {
-      return;
+  function swapBrowser(newMM) {
+    if (mm) {
+      mm.removeMessageListener("debug:storage-indexedDB-request-parent",
+                               indexedDBHelpers.handleChildRequest);
     }
+    mm = newMM;
+    if (mm) {
+      mm.addMessageListener("debug:storage-indexedDB-request-parent",
+                            indexedDBHelpers.handleChildRequest);
+    }
+  }
 
-    gTrackedMessageManager.delete("indexedDB");
-
-    // unregister for director-script requests handlers from the parent process
-    // (if any)
-    mm.removeMessageListener("debug:storage-indexedDB-request-parent",
-                             indexedDBHelpers.handleChildRequest);
-  }
+  return {
+    onBrowserSwap: swapBrowser,
+    onDisconnected: () => swapBrowser(null),
+  };
 };
 
 /**
  * The main Storage Actor.
  */
 let StorageActor = protocol.ActorClassWithSpec(specs.storageSpec, {
   typeName: "storage",
 
--- a/devtools/server/child.js
+++ b/devtools/server/child.js
@@ -57,17 +57,17 @@ try {
     let onSetupInChild = DevToolsUtils.makeInfallible(msg => {
       let { module, setupChild, args } = msg.data;
       let m;
 
       try {
         m = require(module);
 
         if (!setupChild in m) {
-          dumpn(`ERROR: module '${module}' does not export 'setupChild'`);
+          dumpn(`ERROR: module '${module}' does not export '${setupChild}'`);
           return false;
         }
 
         m[setupChild].apply(m, args);
       } catch (e) {
         let errorMessage =
           "Exception during actor module setup running in the child process: ";
         DevToolsUtils.reportException(errorMessage + e);
--- a/devtools/server/docs/actor-e10s-handling.md
+++ b/devtools/server/docs/actor-e10s-handling.md
@@ -39,66 +39,68 @@ The `setupChildProcess` helper defined a
 
 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:
 
 ```
-let gTrackedMessageManager = new Set();
 exports.setupParentProcess = function setupParentProcess({ mm, prefix }) {
-  // Prevent multiple subscriptions on the same messagemanager.
-  if (gTrackedMessageManager.has(mm)) { return; }
-  gTrackedMessageManager.add(mm);
-
   // Start listening for messages from the actor in the child process.
-  mm.addMessageListener("debug:some-message-name", handleChildRequest);
+  swapBrowser(mm);
 
   function handleChildRequest(msg) {
     switch (msg.json.method) {
       case "get":
         return doGetInParentProcess(msg.json.args[0]);
         break;
       case "list":
         return doListInParentProcess();
         break;
       default:
         console.error("Unknown method name", msg.json.method);
         throw new Error("Unknown method name");
     }
   }
 
-  // Listen to the disconnection message to clean-up.
-  DebuggerServer.once("disconnected-from-child:" + prefix, handleMessageManagerDisconnected);
+  function swapBrowser(newMM) {
+    if (mm) {
+      // Remove listener from old message manager
+      mm.removeMessageListener("debug:some-message-name", handleChildRequest);
+    }
+    // Switch to the new message manager for future use
+    // Note: Make sure that any other functions also use the new reference.
+    mm = newMM;
+    if (mm) {
+      // Add listener to new message manager
+      mm.addMessageListener("debug:some-message-name", handleChildRequest);
+    }
+  }
 
-  function handleMessageManagerDisconnected(evt, { mm: disconnected_mm }) {
-    // filter out not subscribed message managers
-    if (disconnected_mm !== mm || !gTrackedMessageManager.has(mm)) {
-      return;
-    }
-
-    gTrackedMessageManager.delete(mm);
-
-    // unregister for director-script requests handlers from the parent process (if any)
-    mm.removeMessageListener("debug:director-registry-request", handleChildRequest);
-  }
+  return {
+    onBrowserSwap: swapBrowser,
+    onDisconnected: () => swapBrowser(null),
+  };
+};
 ```
 
-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.
+The server will call the `onDisconnected` method returned by the parent process setup flow to give the actor modules the chance to cleanup their handlers registered on the disconnected message manager.
+
+The server will call the `onBrowserSwap` method returned by the parent process setup flow to notify actor modules when the message manager for the target frame has changed.  The parent process code should remove any message listeners from the previous message manager and add them to the new one.
 
 ## 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 `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 `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.
+* the `setupParent` function then uses the mm to subscribe the message manager events,
+* the `setupParent` function returns an object with `onDisconnected` and `onBrowserSwap` methods which the server can use to notify the module of various lifecycle events
--- a/devtools/server/main.js
+++ b/devtools/server/main.js
@@ -1006,35 +1006,36 @@ var DebuggerServer = {
 
     let actor, childTransport;
     let prefix = connection.allocID("child");
     // Compute the same prefix that's used by DebuggerServerConnection
     let connPrefix = prefix + "/";
 
     // provides hook to actor modules that need to exchange messages
     // between e10s parent and child processes
+    let parentModules = [];
     let onSetupInParent = function (msg) {
       // We may have multiple connectToChild instance running for the same tab
       // and need to filter the messages.
       if (msg.json.prefix != connPrefix) {
         return false;
       }
 
       let { module, setupParent } = msg.json;
       let m;
 
       try {
         m = require(module);
 
         if (!setupParent in m) {
-          dumpn(`ERROR: module '${module}' does not export 'setupParent'`);
+          dumpn(`ERROR: module '${module}' does not export '${setupParent}'`);
           return false;
         }
 
-        m[setupParent]({ mm, prefix: connPrefix });
+        parentModules.push(m[setupParent]({ mm, prefix: connPrefix }));
 
         return true;
       } catch (e) {
         let errorMessage =
           "Exception during actor module setup running in the parent process: ";
         DevToolsUtils.reportException(errorMessage + e);
         dumpn(`ERROR: ${errorMessage}\n\t module: '${module}'\n\t ` +
               `setupParent: '${setupParent}'\n${DevToolsUtils.safeErrorString(e)}`);
@@ -1071,24 +1072,38 @@ var DebuggerServer = {
       // Update frame and mm to point to the new browser frame
       frame = newFrame;
       // Get messageManager from XUL browser (which might be a specialized tunnel for RDM)
       // or else fallback to asking the frameLoader itself.
       mm = frame.messageManager || frame.frameLoader.messageManager;
       // Add listeners to new frame and mm
       trackMessageManager();
 
+      // provides hook to actor modules that need to exchange messages
+      // between e10s parent and child processes
+      parentModules.forEach(mod => {
+        if (mod.onBrowserSwap) {
+          mod.onBrowserSwap(mm);
+        }
+      });
+
       if (childTransport) {
         childTransport.swapBrowser(mm);
       }
     };
 
     let destroy = DevToolsUtils.makeInfallible(function () {
       // provides hook to actor modules that need to exchange messages
       // between e10s parent and child processes
+      parentModules.forEach(mod => {
+        if (mod.onDisconnected) {
+          mod.onDisconnected();
+        }
+      });
+      // TODO: Remove this deprecated path once it's no longer needed by add-ons.
       DebuggerServer.emit("disconnected-from-child:" + connPrefix,
                           { mm, prefix: connPrefix });
 
       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;
--- a/devtools/shared/webconsole/network-monitor.js
+++ b/devtools/shared/webconsole/network-monitor.js
@@ -1563,47 +1563,61 @@ NetworkEventActorProxy.prototype = {
 })();
 
 /**
  * This is triggered by the child calling `setupInParent` when the child's network monitor
  * is starting up.  This initializes the parent process side of the monitoring.
  */
 function setupParentProcess({ mm, prefix }) {
   let networkMonitor = new NetworkMonitorParent(mm, prefix);
-  DebuggerServer.once("disconnected-from-child:" + prefix, () => {
-    networkMonitor.destroy();
-    networkMonitor = null;
-  });
+  return {
+    onBrowserSwap: newMM => networkMonitor.swapBrowser(newMM),
+    onDisconnected: () => {
+      networkMonitor.destroy();
+      networkMonitor = null;
+    }
+  };
 }
 
 exports.setupParentProcess = setupParentProcess;
 
 /**
  * The NetworkMonitorParent runs in the parent process and uses the message manager to
  * listen for requests from the child process to start/stop the network monitor.  Most
  * request data is only available from the parent process, so that's why the network
  * monitor needs to run there when debugging tabs that are in the child.
  *
  * @param nsIMessageManager mm
  *        The message manager for the browser we're filtering on.
  * @param string prefix
  *        The RDP connection prefix that uniquely identifies the connection.
  */
 function NetworkMonitorParent(mm, prefix) {
-  this.messageManager = mm;
   this.onNetMonitorMessage = this.onNetMonitorMessage.bind(this);
   this.onNetworkEvent = this.onNetworkEvent.bind(this);
-
-  mm.addMessageListener("debug:netmonitor", this.onNetMonitorMessage);
+  this.swapBrowser(mm);
 }
 
 NetworkMonitorParent.prototype = {
   netMonitor: null,
   messageManager: null,
 
+  swapBrowser(mm) {
+    if (this.messageManager) {
+      let oldMM = this.messageManager;
+      oldMM.removeMessageListener("debug:netmonitor", this.onNetMonitorMessage);
+      oldMM.removeMessageListener(this.frameUpdateMessage, this);
+    }
+    this.messageManager = mm;
+    if (mm) {
+      mm.addMessageListener("debug:netmonitor", this.onNetMonitorMessage);
+      mm.addMessageListener(this.frameUpdateMessage, this);
+    }
+  },
+
   /**
    * Handler for "debug:netmonitor" messages received through the message manager
    * from the content process.
    *
    * @param object msg
    *        Message from the content.
    */
   onNetMonitorMessage: DevToolsUtils.makeInfallible(function (msg) {
@@ -1653,21 +1667,17 @@ NetworkMonitorParent.prototype = {
    *         A NetworkEventActorProxy instance which is notified when further
    *         data about the request is available.
    */
   onNetworkEvent: DevToolsUtils.makeInfallible(function (event) {
     return new NetworkEventActorProxy(this.messageManager).init(event);
   }),
 
   destroy: function () {
-    if (this.messageManager) {
-      let mm = this.messageManager;
-      this.messageManager = null;
-      mm.removeMessageListener("debug:netmonitor", this.onNetMonitorMessage);
-    }
+    this.swapBrowser(null);
     this.frames = null;
 
     if (this.netMonitor) {
       this.netMonitor.destroy();
       this.netMonitor = null;
     }
   },
 };
--- a/devtools/shared/webconsole/server-logger-monitor.js
+++ b/devtools/shared/webconsole/server-logger-monitor.js
@@ -36,71 +36,49 @@ const acceptableHeaders = ["x-chromelogg
  * Read more about the architecture:
  * https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/server/docs/actor-e10s-handling.md
  */
 var ServerLoggerMonitor = {
   // Initialization
 
   initialize: function () {
     this.onChildMessage = this.onChildMessage.bind(this);
-    this.onDisconnectChild = this.onDisconnectChild.bind(this);
     this.onExamineResponse = this.onExamineResponse.bind(this);
 
-    // Set of tracked message managers.
-    this.messageManagers = new Set();
-
     // Set of registered child frames (loggers).
     this.targets = new Set();
   },
 
   // Parent Child Relationship
 
-  attach: makeInfallible(function ({mm, prefix}) {
-    let size = this.messageManagers.size;
-
-    trace.log("ServerLoggerMonitor.attach; ", size, arguments);
+  attach: makeInfallible(function ({ mm, prefix }) {
+    trace.log("ServerLoggerMonitor.attach; ", arguments);
 
-    if (this.messageManagers.has(mm)) {
-      return;
-    }
-
-    this.messageManagers.add(mm);
+    let swapBrowser = newMM => {
+      if (mm) {
+        mm.removeMessageListener("debug:server-logger", this.onChildMessage);
+      }
+      mm = newMM;
+      if (mm) {
+        mm.addMessageListener("debug:server-logger", this.onChildMessage);
+      }
+    };
 
     // Start listening for messages from the {@ServerLogger} actor
     // living in the child process.
-    mm.addMessageListener("debug:server-logger", this.onChildMessage);
-
-    // Listen to the disconnection message to clean-up.
-    DebuggerServer.once("disconnected-from-child:" + prefix,
-      this.onDisconnectChild);
-  }),
-
-  detach: function (mm) {
-    let size = this.messageManagers.size;
-
-    trace.log("ServerLoggerMonitor.detach; ", size);
+    swapBrowser(mm);
 
-    // Unregister message listeners
-    mm.removeMessageListener("debug:server-logger", this.onChildMessage);
-  },
-
-  onDisconnectChild: function (event, mm) {
-    let size = this.messageManagers.size;
-
-    trace.log("ServerLoggerMonitor.onDisconnectChild; ",
-      size, arguments);
-
-    if (!this.messageManagers.has(mm)) {
-      return;
-    }
-
-    this.detach(mm);
-
-    this.messageManagers.delete(mm);
-  },
+    return {
+      onBrowserSwap: swapBrowser,
+      onDisconnected: () => {
+        trace.log("ServerLoggerMonitor.onDisconnectChild; ", arguments);
+        swapBrowser(null);
+      }
+    };
+  }),
 
   // Child Message Handling
 
   onChildMessage: function (msg) {
     let method = msg.data.method;
 
     trace.log("ServerLoggerMonitor.onChildMessage; ", method, msg);
 
@@ -199,16 +177,16 @@ var ServerLoggerMonitor = {
       headers.length, ", ", headers);
   }),
 };
 
 /**
  * Executed automatically by the framework.
  */
 function setupParentProcess(event) {
-  ServerLoggerMonitor.attach(event);
+  return ServerLoggerMonitor.attach(event);
 }
 
 // Monitor initialization.
 ServerLoggerMonitor.initialize();
 
 // Exports from this module
 exports.setupParentProcess = setupParentProcess;