Bug 1323466 - Prevent loading webbrowser.js in child process. r=jryans draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Mon, 19 Dec 2016 03:14:12 -0800
changeset 455265 aa67e816b5f13f81994ba9722d964044d003815b
parent 455264 f03c98ec4ea7c6246cbf20dd7b8da73c3a56f9cb
child 455266 6e58c8747906cfbdea78dda7344e63aee00f35c2
push id40181
push userbmo:poirot.alex@gmail.com
push dateTue, 03 Jan 2017 11:03:25 +0000
reviewersjryans
bugs1323466
milestone53.0a1
Bug 1323466 - Prevent loading webbrowser.js in child process. r=jryans MozReview-Commit-ID: J1EmwxplNhA Make actor registration more explicit and documented. Each codepath depends on various set of actors, and it may be confused as we often register actors if the DebuggerServer wasn't initialized yet. But it is often already started by some other callsite... This changeset also converts childtab to being just a module and stop using DebuggerServer.addActors magic.
devtools/client/aboutdebugging/initializer.js
devtools/client/framework/ToolboxProcess.jsm
devtools/client/framework/target.js
devtools/server/actors/childtab.js
devtools/server/actors/webconsole.js
devtools/server/child.js
devtools/server/content-server.jsm
devtools/server/main.js
devtools/server/tests/unit/test_dbgglobal.js
devtools/server/tests/unit/test_register_actor.js
--- a/devtools/client/aboutdebugging/initializer.js
+++ b/devtools/client/aboutdebugging/initializer.js
@@ -28,19 +28,21 @@ const { createFactory } = require("devto
 const { render, unmountComponentAtNode } = require("devtools/client/shared/vendor/react-dom");
 
 const AboutDebuggingApp = createFactory(require("./components/aboutdebugging"));
 
 var AboutDebugging = {
   init() {
     if (!DebuggerServer.initialized) {
       DebuggerServer.init();
-      DebuggerServer.addBrowserActors();
     }
     DebuggerServer.allowChromeProcess = true;
+    // We want a full featured server for about:debugging. Especially the
+    // "browser actors" like addons.
+    DebuggerServer.registerActors({ root: true, browser: true, tab: true });
 
     this.client = new DebuggerClient(DebuggerServer.connectPipe());
 
     this.client.connect().then(() => {
       let client = this.client;
       let telemetry = new Telemetry();
 
       render(AboutDebuggingApp({ client, telemetry }),
--- a/devtools/client/framework/ToolboxProcess.jsm
+++ b/devtools/client/framework/ToolboxProcess.jsm
@@ -130,17 +130,20 @@ BrowserToolboxProcess.prototype = {
     let { DebuggerServer } = this.loader.require("devtools/server/main");
     this.debuggerServer = DebuggerServer;
     dumpn("Created a separate loader instance for the DebuggerServer.");
 
     // Forward interesting events.
     this.debuggerServer.on("connectionchange", this.emit);
 
     this.debuggerServer.init();
-    this.debuggerServer.addBrowserActors();
+    // We mainly need a root actor and tab actors for opening a toolbox, even
+    // against chrome/content/addon. But the "no auto hide" button uses the
+    // preference actor, so also register the browser actors.
+    this.debuggerServer.registerActors({ root: true, browser: true, tab: true });
     this.debuggerServer.allowChromeProcess = true;
     dumpn("initialized and added the browser actors for the DebuggerServer.");
 
     let chromeDebuggingPort =
       Services.prefs.getIntPref("devtools.debugger.chrome-debugging-port");
     let chromeDebuggingWebSocket =
       Services.prefs.getBoolPref("devtools.debugger.chrome-debugging-websocket");
     let listener = this.debuggerServer.createListener();
--- a/devtools/client/framework/target.js
+++ b/devtools/client/framework/target.js
@@ -381,18 +381,23 @@ TabTarget.prototype = {
 
     this._remote = defer();
 
     if (this.isLocalTab) {
       // Since a remote protocol connection will be made, let's start the
       // DebuggerServer here, once and for all tools.
       if (!DebuggerServer.initialized) {
         DebuggerServer.init();
-        DebuggerServer.addBrowserActors();
       }
+      // When connecting to a local tab, we only need the root actor.
+      // Then we are going to call DebuggerServer.connectToChild and talk
+      // directly with actors living in the child process.
+      // We also need browser actors for actor registry which enabled addons
+      // to register custom actors.
+      DebuggerServer.registerActors({ root: true, browser: true, tab: false });
 
       this._client = new DebuggerClient(DebuggerServer.connectPipe());
       // A local TabTarget will never perform chrome debugging.
       this._chrome = false;
     }
 
     this._setupRemoteListeners();
 
--- a/devtools/server/actors/childtab.js
+++ b/devtools/server/actors/childtab.js
@@ -75,8 +75,10 @@ ContentActor.prototype.exit = function (
 
 /**
  * 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());
 };
+
+exports.ContentActor = ContentActor;
--- a/devtools/server/actors/webconsole.js
+++ b/devtools/server/actors/webconsole.js
@@ -146,18 +146,17 @@ WebConsoleActor.prototype =
 
   /**
    * Boolean getter that tells if the parent actor is a ContentActor.
    *
    * @private
    * @type boolean
    */
   get _parentIsContentActor() {
-    return "ContentActor" in DebuggerServer &&
-            this.parentActor instanceof DebuggerServer.ContentActor;
+    return this.parentActor.constructor.name == "ContentActor";
   },
 
   /**
    * The window or sandbox we work with.
    * Note that even if it is named `window` it refers to the current
    * global we are debugging, which can be a Sandbox for addons
    * or browser content toolbox.
    *
--- a/devtools/server/child.js
+++ b/devtools/server/child.js
@@ -12,42 +12,39 @@ try {
   // Encapsulate in its own scope to allows loading this frame script more than once.
   (function () {
     const Cu = Components.utils;
     const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
 
     const DevToolsUtils = require("devtools/shared/DevToolsUtils");
     const { dumpn } = DevToolsUtils;
     const { DebuggerServer, ActorPool } = require("devtools/server/main");
+    const { ContentActor } = require("devtools/server/actors/childtab");
 
     if (!DebuggerServer.initialized) {
       DebuggerServer.init();
-      // For non-e10s mode, there is only one server instance, so be sure the browser
-      // actors get loaded.
-      DebuggerServer.addBrowserActors();
     }
-
-    // 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();
+    // We want a special server without any root actor and only tab actors.
+    // We are going to spawn a ContentActor instance in the next few lines,
+    // it is going to act like a root actor without being one.
+    DebuggerServer.registerActors({ root: false, browser: false, tab: true });
 
     let connections = new Map();
 
     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 actor = new ContentActor(conn, chromeGlobal, prefix);
       let actorPool = new ActorPool(conn);
       actorPool.addActor(actor);
       conn.addActorPool(actorPool);
 
       sendAsyncMessage("debug:actor", {actor: actor.form(), prefix: prefix});
     });
 
     addMessageListener("debug:connect", onConnect);
--- a/devtools/server/content-server.jsm
+++ b/devtools/server/content-server.jsm
@@ -27,22 +27,20 @@ function setupServer(mm) {
   // using it in the same process.
   gLoader = new DevToolsLoader();
   gLoader.invisibleToDebugger = true;
   let { DebuggerServer } = gLoader.require("devtools/server/main");
 
   if (!DebuggerServer.initialized) {
     DebuggerServer.init();
   }
-
-  // In case of apps being loaded in parent process, DebuggerServer is already
-  // initialized, but child specific actors are not registered.
-  // Otherwise, for child process, we need to load actors the first
-  // time we load child.js
-  DebuggerServer.addChildActors();
+  // For browser content toolbox, we do need a regular root actor and all tab
+  // actors, but don't need all the "browser actors" that are only useful when
+  // debugging the parent process via the browser toolbox.
+  DebuggerServer.registerActors({ browser: false, root: true, tab: true });
 
   // Clean up things when the client disconnects
   mm.addMessageListener("debug:content-process-destroy", function onDestroy() {
     mm.removeMessageListener("debug:content-process-destroy", onDestroy);
 
     DebuggerServer.destroy();
     gLoader.destroy();
     gLoader = null;
--- a/devtools/server/main.js
+++ b/devtools/server/main.js
@@ -166,16 +166,25 @@ var DebuggerServer = {
   chromeWindowType: null,
 
   /**
    * Allow debugging chrome of (parent or child) processes.
    */
   allowChromeProcess: false,
 
   /**
+   * We run a special server in child process whose main actor is an instance
+   * of ContentActor, but that isn't a root actor. Instead there is no root
+   * actor registered on DebuggerServer.
+   */
+  get rootlessServer() {
+    return !this.isModuleRegistered("devtools/server/actors/webbrowser");
+  },
+
+  /**
    * Initialize the debugger server.
    */
   init() {
     if (this.initialized) {
       return;
     }
 
     this._connections = {};
@@ -224,23 +233,57 @@ var DebuggerServer = {
   /**
    * Raises an exception if the server has not been properly initialized.
    */
   _checkInit() {
     if (!this._initialized) {
       throw new Error("DebuggerServer has not been initialized.");
     }
 
-    if (!this.createRootActor) {
+    if (!this.rootlessServer && !this.createRootActor) {
       throw new Error("Use DebuggerServer.addActors() to add a root actor " +
                       "implementation.");
     }
   },
 
   /**
+   * Register all type of actors. Only register the one that are not already
+   * registered.
+   *
+   * @param root boolean
+   *        Registers the root actor from webbrowser module, which is used to
+   *        connect to and fetch any other actor.
+   * @param browser boolean
+   *        Registers all the parent process actors useful for debugging the
+   *        runtime itself, like preferences and addons actors.
+   * @param tab boolean
+   *        Registers all the tab actors like console, script, ... all useful
+   *        for debugging a target context.
+   * @param windowType string
+   *        "windowtype" attribute of the main chrome windows. Used by some
+   *        actors to retrieve them.
+   */
+  registerActors({ root = true, browser = true, tab = true,
+                   windowType = "navigator:browser" }) {
+    this.chromeWindowType = windowType;
+
+    if (browser) {
+      this.addBrowserActors(windowType);
+    }
+
+    if (root) {
+      this.registerModule("devtools/server/actors/webbrowser");
+    }
+
+    if (tab) {
+      this.addTabActors();
+    }
+  },
+
+  /**
    * Load a subscript into the debugging global.
    *
    * @param url string A url that will be loaded as a subscript into the
    *        debugging global.  The user must load at least one script
    *        that implements a createRootActor() function to create the
    *        server's root actor.
    */
   addActors(url) {
@@ -279,17 +322,17 @@ var DebuggerServer = {
    *            registers a global actor instance, if true.
    *            A global actor has the root actor as its parent.
    *          - "tab"
    *            registers a tab actor instance, if true.
    *            A new actor will be created for each tab and each app.
    */
   registerModule(id, options) {
     if (id in gRegisteredModules) {
-      throw new Error("Tried to register a module twice: " + id + "\n");
+      return;
     }
 
     if (options) {
       // Lazy loaded actors
       let {prefix, constructor, type} = options;
       if (typeof (prefix) !== "string") {
         throw new Error(`Lazy actor definition for '${id}' requires a string ` +
                         `'prefix' option.`);
@@ -415,35 +458,16 @@ var DebuggerServer = {
     this.registerModule("devtools/server/actors/heap-snapshot-file", {
       prefix: "heapSnapshotFile",
       constructor: "HeapSnapshotFileActor",
       type: { global: true }
     });
   },
 
   /**
-   * Install tab actors in documents loaded in content childs
-   */
-  addChildActors() {
-    // In case of apps being loaded in parent process, DebuggerServer is already
-    // initialized and browser actors are already loaded,
-    // but childtab.js hasn't been loaded yet.
-    if (!DebuggerServer.tabActorFactories.hasOwnProperty("consoleActor")) {
-      this.addTabActors();
-    }
-    // But webbrowser.js and childtab.js aren't loaded from shell.js.
-    if (!this.isModuleRegistered("devtools/server/actors/webbrowser")) {
-      this.registerModule("devtools/server/actors/webbrowser");
-    }
-    if (!("ContentActor" in this)) {
-      this.addActors("resource://devtools/server/actors/childtab.js");
-    }
-  },
-
-  /**
    * Install tab actors.
    */
   addTabActors() {
     this.registerModule("devtools/server/actors/webconsole", {
       prefix: "console",
       constructor: "WebConsoleActor",
       type: { tab: true }
     });
--- a/devtools/server/tests/unit/test_dbgglobal.js
+++ b/devtools/server/tests/unit/test_dbgglobal.js
@@ -11,19 +11,16 @@ function run_test()
   check_except(DebuggerServer.closeAllListeners);
   check_except(DebuggerServer.connectPipe);
 
   // Allow incoming connections.
   DebuggerServer.init();
 
   // These should still fail because we haven't added a createRootActor
   // implementation yet.
-  check_except(function () {
-    DebuggerServer.createListener();
-  });
   check_except(DebuggerServer.closeAllListeners);
   check_except(DebuggerServer.connectPipe);
 
   DebuggerServer.registerModule("xpcshell-test/testactors");
 
   // Now they should work.
   DebuggerServer.createListener();
   DebuggerServer.closeAllListeners();
--- a/devtools/server/tests/unit/test_register_actor.js
+++ b/devtools/server/tests/unit/test_register_actor.js
@@ -27,22 +27,19 @@ function run_test()
 
 function test_deprecated_api() {
   // The xpcshell-test/ path maps to resource://test/
   DebuggerServer.registerModule("xpcshell-test/registertestactors-01");
   DebuggerServer.registerModule("xpcshell-test/registertestactors-02");
 
   check_actors(true);
 
-  check_except(() => {
-    DebuggerServer.registerModule("xpcshell-test/registertestactors-01");
-  });
-  check_except(() => {
-    DebuggerServer.registerModule("xpcshell-test/registertestactors-02");
-  });
+  // Calling registerModule again is just a no-op and doesn't throw
+  DebuggerServer.registerModule("xpcshell-test/registertestactors-01");
+  DebuggerServer.registerModule("xpcshell-test/registertestactors-02");
 
   DebuggerServer.unregisterModule("xpcshell-test/registertestactors-01");
   DebuggerServer.unregisterModule("xpcshell-test/registertestactors-02");
   check_actors(false);
 
   DebuggerServer.registerModule("xpcshell-test/registertestactors-01");
   DebuggerServer.registerModule("xpcshell-test/registertestactors-02");
   check_actors(true);