Bug 1521052 - Destroy DebuggerServer in the content process when the last connection drops. r=jdescottes

We never really tried to cleanup the DebuggerServer and so a few tests require some tweaks
to acknowledge that once the last connection drop (typically, we close the toolbox or target),
the server is destroyed and dynamically registered actors are also destroyed.

I think it is great to consider that everything is cleaned up as we may followup to destroy
the whole loader.

Depends on D16961

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

--HG--
extra : moz-landing-system : lando

devtools/client/shared/test/test-actor-registry.js

@@ -65,6 +65,11 @@
 
     await client.connect();
 
+    // Force connecting to the tab so that the actor is registered in the tab.
+    // Calling `getTab` will spawn a DebuggerServer and ActorRegistry in the content
+    // process.
+    await client.mainRoot.getTab({tab});
+
     // We also need to make sure the test actor is registered on the server.
     await exports.registerTestActor(client);
 

devtools/client/webide/test/test_autoconnect_runtime.html

@@ -67,6 +67,10 @@
           await connectionsChanged;
           is(Object.keys(DebuggerServer._connections).length, 0, "Disconnected");
 
+          // The DebuggerServer is destroyed when closing WebIDE, so re-initialize it.
+          DebuggerServer.init();
+          DebuggerServer.registerAllActors();
+
           connectionsChanged = waitForConnectionChange("opened", 2);
 
           win = await openWebIDE();

devtools/client/webide/test/test_autoselect_project.html

@@ -61,6 +61,10 @@
           await connectionsChanged;
           is(Object.keys(DebuggerServer._connections).length, 0, "Disconnected");
 
+          // The DebuggerServer is destroyed when closing WebIDE, so re-initialize it.
+          DebuggerServer.init();
+          DebuggerServer.registerAllActors();
+
           connectionsChanged = waitForConnectionChange("opened", 2);
 
           // Re-open, should reselect main process after connection

devtools/server/startup/frame.js

@@ -122,6 +122,19 @@ try {
       }
       connections.clear();
     });
+
+    // 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 another tab running in the same process.
+      if (DebuggerServer.hasConnection()) {
+        return;
+      }
+      DebuggerServer.off("connectionchange", destroyServer);
+      DebuggerServer.destroy();
+    }
+    DebuggerServer.on("connectionchange", destroyServer);
   })();
 } catch (e) {
   dump(`Exception in DevTools frame startup: ${e}\n`);

devtools/server/tests/browser/browser.ini

@@ -72,6 +72,7 @@ skip-if = e10s # Bug 1183605 - devtools/server/tests/browser/ tests are still di
 skip-if = e10s # Bug 1183605 - devtools/server/tests/browser/ tests are still disabled in E10S
 [browser_canvasframe_helper_06.js]
 skip-if = e10s # Bug 1183605 - devtools/server/tests/browser/ tests are still disabled in E10S
+[browser_debugger_server.js]
 [browser_inspector-anonymous.js]
 [browser_inspector-insert.js]
 [browser_inspector-mutations-childlist.js]

devtools/server/tests/browser/browser_debugger_server.js

@@ -0,0 +1,59 @@
+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test basic features of DebuggerServer
+
+add_task(async function() {
+  // When running some other tests before, they may not destroy the main server.
+  // Do it manually before running our tests.
+  if (DebuggerServer.initialized) {
+    DebuggerServer.destroy();
+  }
+
+  await testDebuggerServerInitialized();
+});
+
+async function testDebuggerServerInitialized() {
+  const browser = await addTab("data:text/html;charset=utf-8,foo");
+  const tab = gBrowser.getTabForBrowser(browser);
+
+  ok(!DebuggerServer.initialized,
+    "By default, the DebuggerServer isn't initialized in parent process");
+  await ContentTask.spawn(browser, null, function() {
+    const {require} = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
+    const {DebuggerServer} = require("devtools/server/main");
+    ok(!DebuggerServer.initialized,
+      "By default, the DebuggerServer isn't initialized not in content process");
+  });
+
+  const target = await TargetFactory.forTab(tab);
+
+  ok(DebuggerServer.initialized,
+    "TargetFactory.forTab will initialize the DebuggerServer in parent process");
+  await ContentTask.spawn(browser, null, function() {
+    const {require} = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
+    const {DebuggerServer} = require("devtools/server/main");
+    ok(DebuggerServer.initialized,
+      "TargetFactory.forTab will initialize the DebuggerServer in content process");
+  });
+
+  await target.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.
+  ok(DebuggerServer.initialized,
+    "Destroying the target doesn't destroy the DebuggerServer in the parent process");
+  await ContentTask.spawn(browser, null, function() {
+    const {require} = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
+    const {DebuggerServer} = require("devtools/server/main");
+    ok(!DebuggerServer.initialized,
+      "But destroying the target ends up destroying the DebuggerServer in the content" +
+      " process");
+  });
+
+  gBrowser.removeCurrentTab();
+}

devtools/server/tests/mochitest/test_connectToFrame.html

@@ -80,47 +80,60 @@ function runTests() {
     DebuggerServer.registerAllActors();
   }
 
-  function firstClient() {
+  async function firstClient() {
     // Fake a first connection to an iframe
     const transport = DebuggerServer.connectPipe();
     const conn = transport._serverConnection;
     const client = new DebuggerClient(transport);
-    DebuggerServer.connectToFrame(conn, iframe).then(actor => {
-      ok(actor.testActor, "Got the test actor");
+    const actor = await DebuggerServer.connectToFrame(conn, iframe);
+    ok(actor.testActor, "Got the test actor");
 
-      // Ensure sending at least one request to our actor,
-      // otherwise it won't be instantiated, nor be destroyed...
-      client.request({
-        to: actor.testActor,
-        type: "hello",
-      }, function(response) {
-        // Then close the client. That should end up cleaning our test actor
-        client.close();
+    // Ensure sending at least one request to our actor,
+    // otherwise it won't be instantiated, nor be destroyed...
+    await client.request({
+      to: actor.testActor,
+      type: "hello",
+    });
 
-        // Ensure that our test actor got cleaned up;
-        // its destroy method should be called
-        mm.addMessageListener("test-actor-destroyed", function listener() {
-          mm.removeMessageListener("test-actor-destroyed", listener);
-          ok(true, "Actor is cleaned up");
+    // Connect a second client in parallel to asser that it received a distinct set of
+    // target actors
+    await secondClient(actor.testActor);
 
-          secondClient(actor.testActor);
-        });
+    ok(DebuggerServer.initialized,
+      "DebuggerServer isn't destroyed until all clients are disconnected");
+
+    // Ensure that our test actor got cleaned up;
+    // its destroy method should be called
+    const onActorDestroyed = new Promise(resolve => {
+      mm.addMessageListener("test-actor-destroyed", function listener() {
+        mm.removeMessageListener("test-actor-destroyed", listener);
+        ok(true, "Actor is cleaned up");
+        resolve();
       });
     });
+
+    // Then close the client. That should end up cleaning our test actor
+    await client.close();
+
+    await onActorDestroyed;
+
+    // This test loads a frame in the parent process, so that we end up sharing the same
+    // DebuggerServer instance
+    ok(!DebuggerServer.initialized,
+      "DebuggerServer is destroyed when all clients are disconnected");
+    cleanup();
   }
 
-  function secondClient(firstActor) {
+  async function secondClient(firstActor) {
     // Then fake a second one, that should spawn a new set of target-scoped actors
     const transport = DebuggerServer.connectPipe();
     const conn = transport._serverConnection;
     const client = new DebuggerClient(transport);
-    DebuggerServer.connectToFrame(conn, iframe).then(actor => {
-      ok(actor.testActor, "Got a test actor for the second connection");
-      isnot(actor.testActor, firstActor,
-            "We get different actor instances between two connections");
-
-      client.close(cleanup);
-    });
+    const actor = await DebuggerServer.connectToFrame(conn, iframe);
+    ok(actor.testActor, "Got a test actor for the second connection");
+    isnot(actor.testActor, firstActor,
+          "We get different actor instances between two connections");
+    return client.close();
   }
 
   function cleanup() {