Bug 1772146 - [devtools] Automatically destroy the DevToolsServer once the last connection drops. r=jdescottes

Do that except when keepAlive attribute is set to true for server
listening for many incoming client connections.
This is the case when starting server via --start-debugger-server
or when starting the server from the mobile browser.

Differential Revision: https://phabricator.services.mozilla.com/D147876

devtools/server/actors/targets/content-process.js

@@ -219,6 +219,8 @@ const ContentProcessTargetActor = TargetActorMixin(
       }
       Resources.unwatchAllResources(this);
 
+      this.emit("destroyed");
+
       Actor.prototype.destroy.call(this);
 
       if (this.threadActor) {

devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm

@@ -330,10 +330,6 @@ class DevToolsFrameChild extends JSWindowActorChild {
 
     DevToolsServer.init();
 
-    // Ask DevToolsServer to automatically destroy itself once the last
-    // connection is closed.
-    DevToolsServer.autoDestroy = true;
-
     // We want a special server without any root actor and only target-scoped actors.
     // We are going to spawn a WindowGlobalTargetActor instance in the next few lines,
     // it is going to act like a root actor without being one.

devtools/server/connectors/js-window-actor/DevToolsWorkerChild.jsm

@@ -58,8 +58,6 @@ class DevToolsWorkerChild extends JSWindowActorChild {
     //   See WatcherRegistry.getSessionData to see the full list of properties.
     this._connections = new Map();
 
-    this._onConnectionChange = this._onConnectionChange.bind(this);
-
     EventEmitter.decorate(this);
   }
 
@@ -283,7 +281,6 @@ class DevToolsWorkerChild extends JSWindowActorChild {
     // We are going to spawn a WorkerTargetActor instance in the next few lines,
     // it is going to act like a root actor without being one.
     DevToolsServer.registerActors({ target: true });
-    DevToolsServer.on("connectionchange", this._onConnectionChange);
 
     const connection = DevToolsServer.connectToParentWindowActor(
       this,
@@ -413,30 +410,6 @@ class DevToolsWorkerChild extends JSWindowActorChild {
     watcherConnectionData.connection.close();
   }
 
-  /**
-   * Destroy the server once its last connection closes. Note that multiple
-   * worker scripts may be running in parallel and reuse the same server.
-   */
-  _onConnectionChange() {
-    const { DevToolsServer } = lazy.Loader.require(
-      "devtools/server/devtools-server"
-    );
-
-    // Only destroy the server if there is no more connections to it. It may be
-    // used to debug another tab running in the same process.
-    if (DevToolsServer.hasConnection() || DevToolsServer.keepAlive) {
-      return;
-    }
-
-    if (this._destroyed) {
-      return;
-    }
-    this._destroyed = true;
-
-    DevToolsServer.off("connectionchange", this._onConnectionChange);
-    DevToolsServer.destroy();
-  }
-
   async sendPacket(packet, prefix) {
     return this.sendAsyncMessage("DevToolsWorkerChild:packet", {
       packet,

devtools/server/devtools-server.js

@@ -100,12 +100,6 @@ var DevToolsServer = {
    */
   keepAlive: false,
 
-  /**
-   * Set by DevToolsFrame JSWindowActor in order to automatically destroy
-   * the server once the last connection is removed. This is ignored if keepAlive is set to true.
-   */
-  autoDestroy: false,
-
   /**
    * We run a special server in child process whose main actor is an instance
    * of WindowGlobalTargetActor, but that isn't a root actor. Instead there is no root
@@ -425,9 +419,13 @@ var DevToolsServer = {
     delete this._connections[connection.prefix];
     this.emit("connectionchange", "closed", connection);
 
-    // Destroy the server once its last connection closes. Note that multiple
-    // JSWindowActor may be running in parallel and reuse the same server.
-    if (!this.autoDestroy || this.hasConnection() || this.keepAlive) {
+    // If keepAlive isn't explicitely set to true, destroy the server once its
+    // last connection closes. Multiple JSWindowActor may use the same DevToolsServer
+    // and in this case, let the server destroy itself once the last connection closes.
+    // Otherwise we set keepAlive to true when starting a listening server, receiving
+    // client connections. Typically when running server on phones, or on desktop
+    // via `--start-debugger-server`.
+    if (this.hasConnection() || this.keepAlive) {
       return;
     }
 

devtools/server/startup/content-process.jsm

@@ -16,27 +16,26 @@
 /* exported initContentProcessTarget */
 const EXPORTED_SYMBOLS = ["initContentProcessTarget"];
 
-let gLoader;
-
-function setupServer(mm) {
-  // Prevent spawning multiple server per process, even if the caller call us
-  // multiple times
-  if (gLoader) {
-    return gLoader;
-  }
+function initContentProcessTarget(msg) {
+  const mm = msg.target;
+  const prefix = msg.data.prefix;
+  const watcherActorID = msg.data.watcherActorID;
 
   // Lazy load Loader.jsm to prevent loading any devtools dependency too early.
-  const { DevToolsLoader } = ChromeUtils.import(
-    "resource://devtools/shared/loader/Loader.jsm"
-  );
+  const {
+    useDistinctSystemPrincipalLoader,
+    releaseDistinctSystemPrincipalLoader,
+  } = ChromeUtils.import("resource://devtools/shared/loader/Loader.jsm");
+
+  // Use a unique object to identify this one usage of the loader
+  const loaderRequester = {};
 
   // Init a custom, invisible DevToolsServer, in order to not pollute the
   // debugger with all devtools modules, nor break the debugger itself with
   // using it in the same process.
-  gLoader = new DevToolsLoader({
-    invisibleToDebugger: true,
-  });
-  const { DevToolsServer } = gLoader.require("devtools/server/devtools-server");
+  const loader = useDistinctSystemPrincipalLoader(loaderRequester);
+
+  const { DevToolsServer } = loader.require("devtools/server/devtools-server");
 
   DevToolsServer.init();
   // For browser content toolbox, we do need a regular root actor and all tab
@@ -44,36 +43,8 @@ function setupServer(mm) {
   // debugging the parent process via the browser toolbox.
   DevToolsServer.registerActors({ root: true, target: true });
 
-  // Destroy the server once its last connection closes. Note that multiple frame
-  // scripts may be running in parallel and reuse the same server.
-  function destroyServer() {
-    // Only destroy the server if there is no more connections to it. It may be used
-    // to debug the same process from another client.
-    if (DevToolsServer.hasConnection()) {
-      return;
-    }
-    DevToolsServer.off("connectionchange", destroyServer);
-
-    DevToolsServer.destroy();
-    gLoader.destroy();
-    gLoader = null;
-  }
-  DevToolsServer.on("connectionchange", destroyServer);
-
-  return gLoader;
-}
-
-function initContentProcessTarget(msg) {
-  const mm = msg.target;
-  const prefix = msg.data.prefix;
-  const watcherActorID = msg.data.watcherActorID;
-
-  // Setup a server if none started yet
-  const loader = setupServer(mm);
-
   // Connect both parent/child processes devtools servers RDP via message
   // managers
-  const { DevToolsServer } = loader.require("devtools/server/devtools-server");
   const conn = DevToolsServer.connectToParent(prefix, mm);
   conn.parentMessageManager = mm;
 
@@ -106,6 +77,13 @@ function initContentProcessTarget(msg) {
     // pools.
     conn.close();
   });
+
+  // Destroy the related loader when the target is destroyed
+  // and we were the last user of the special loader
+  actor.once("destroyed", () => {
+    releaseDistinctSystemPrincipalLoader(loaderRequester);
+  });
+
   return {
     actor,
     connection: conn,

devtools/server/startup/frame.js

@@ -26,12 +26,10 @@ try {
     let loader,
       customLoader = false;
     if (content.document.nodePrincipal.isSystemPrincipal) {
-      const { DevToolsLoader } = ChromeUtils.import(
+      const { useDistinctSystemPrincipalLoader } = ChromeUtils.import(
         "resource://devtools/shared/loader/Loader.jsm"
       );
-      loader = new DevToolsLoader({
-        invisibleToDebugger: true,
-      });
+      loader = useDistinctSystemPrincipalLoader(chromeGlobal);
       customLoader = true;
     } else {
       // Otherwise, use the shared loader.
@@ -169,21 +167,23 @@ try {
 
     // Destroy the server once its last connection closes. Note that multiple frame
     // scripts may be running in parallel and reuse the same server.
-    function destroyServer() {
+    function destroyLoader() {
       // Only destroy the server if there is no more connections to it. It may be used
       // to debug another tab running in the same process.
       if (DevToolsServer.hasConnection() || DevToolsServer.keepAlive) {
         return;
       }
-      DevToolsServer.off("connectionchange", destroyServer);
-      DevToolsServer.destroy();
+      DevToolsServer.off("connectionchange", destroyLoader);
 
       // When debugging chrome pages, we initialized a dedicated loader, also destroy it
       if (customLoader) {
-        loader.destroy();
+        const { releaseDistinctSystemPrincipalLoader } = ChromeUtils.import(
+          "resource://devtools/shared/loader/Loader.jsm"
+        );
+        releaseDistinctSystemPrincipalLoader(chromeGlobal);
       }
     }
-    DevToolsServer.on("connectionchange", destroyServer);
+    DevToolsServer.on("connectionchange", destroyLoader);
   })();
 } catch (e) {
   dump(`Exception in DevTools frame startup: ${e}\n`);

devtools/server/startup/worker.js

@@ -52,7 +52,6 @@ const { WorkerTargetActor } = worker.require(
 );
 const { DevToolsServer } = worker.require("devtools/server/devtools-server");
 
-DevToolsServer.init();
 DevToolsServer.createRootActor = function() {
   throw new Error("Should never get here!");
 };
@@ -68,6 +67,11 @@ this.addEventListener("message", async function(event) {
     case "connect":
       const { forwardingPrefix } = packet;
 
+      // Force initializing the server each time on connect
+      // as it may have been destroyed by a previous, now closed toolbox.
+      // Once the last connection drops, the server auto destroy itself.
+      DevToolsServer.init();
+
       // Step 3: Create a connection to the parent.
       const connection = DevToolsServer.connectToParent(forwardingPrefix, this);
 

devtools/server/tests/browser/browser_debugger_server.js

@@ -52,12 +52,10 @@ async function testDevToolsServerInitialized() {
 
   await commands.destroy();
 
-  // Disconnecting the client will remove all connections from both server,
-  // in parent and content process. But only the one in the content process will be
-  // destroyed.
+  // Disconnecting the client will remove all connections from both server, in parent and content process.
   ok(
-    DevToolsServer.initialized,
-    "Destroying the commands doesn't destroy the DevToolsServer in the parent process"
+    !DevToolsServer.initialized,
+    "Destroying the commands destroys the DevToolsServer in the parent process"
   );
   await assertServerInitialized(
     browser,
@@ -89,6 +87,7 @@ async function testDevToolsServerKeepAlive() {
   );
 
   info("Set DevToolsServer.keepAlive to true in the content process");
+  DevToolsServer.keepAlive = true;
   await setContentServerKeepAlive(browser, true);
 
   info("Destroy the commands, the content server should be kept alive");
@@ -100,7 +99,13 @@ async function testDevToolsServerKeepAlive() {
     "Server still running in content process"
   );
 
+  ok(
+    DevToolsServer.initialized,
+    "Destroying the commands never destroys the DevToolsServer in the parent process when keepAlive is true"
+  );
+
   info("Set DevToolsServer.keepAlive back to false");
+  DevToolsServer.keepAlive = false;
   await setContentServerKeepAlive(browser, false);
 
   info("Create and destroy a commands again");
@@ -115,6 +120,11 @@ async function testDevToolsServerKeepAlive() {
     "Server stopped in content process"
   );
 
+  ok(
+    !DevToolsServer.initialized,
+    "When turning keepAlive to false, the server in the parent process is destroyed"
+  );
+
   gBrowser.removeCurrentTab();
   DevToolsServer.destroy();
 }

devtools/server/tests/xpcshell/test_register_actor.js

@@ -5,6 +5,7 @@
 
 function run_test() {
   // Allow incoming connections.
+  DevToolsServer.keepAlive = true;
   DevToolsServer.init();
   DevToolsServer.registerAllActors();
 

devtools/shared/transport/tests/xpcshell/head_dbg.js

@@ -101,6 +101,8 @@ function initTestDevToolsServer() {
   DevToolsServer.setRootActor(createRootActor);
   // Allow incoming connections.
   DevToolsServer.init();
+  // Avoid the server from being destroyed when the last connection closes
+  DevToolsServer.keepAlive = true;
 }
 
 /**

devtools/startup/DevToolsStartup.jsm

@@ -974,9 +974,10 @@ DevToolsStartup.prototype = {
       portOrPath = Number(port) ? port : defaultPort;
     }
 
-    const { DevToolsLoader } = ChromeUtils.import(
-      "resource://devtools/shared/loader/Loader.jsm"
-    );
+    const {
+      useDistinctSystemPrincipalLoader,
+      releaseDistinctSystemPrincipalLoader,
+    } = ChromeUtils.import("resource://devtools/shared/loader/Loader.jsm");
 
     try {
       // Create a separate loader instance, so that we can be sure to receive
@@ -985,9 +986,7 @@ DevToolsStartup.prototype = {
       // actors and DebuggingServer itself, especially since we can mark
       // serverLoader as invisible to the debugger (unlike the usual loader
       // settings).
-      const serverLoader = new DevToolsLoader({
-        invisibleToDebugger: true,
-      });
+      const serverLoader = useDistinctSystemPrincipalLoader(this);
       const { DevToolsServer: devToolsServer } = serverLoader.require(
         "devtools/server/devtools-server"
       );
@@ -995,6 +994,11 @@ DevToolsStartup.prototype = {
         "devtools/shared/security/socket"
       );
       devToolsServer.init();
+
+      // Force the server to be kept running when the last connection closes.
+      // So that another client can connect after the previous one is disconnected.
+      devToolsServer.keepAlive = true;
+
       devToolsServer.registerAllActors();
       devToolsServer.allowChromeProcess = true;
       const socketOptions = { portOrPath, webSocket };
@@ -1013,7 +1017,7 @@ DevToolsStartup.prototype = {
         if (devToolsServer) {
           devToolsServer.destroy();
         }
-        serverLoader.destroy();
+        releaseDistinctSystemPrincipalLoader(this);
       };
       Services.obs.addObserver(close, "quit-application");
     } catch (e) {