Bug 1521052 - Destroy DebuggerServer in the content process when the last connection drops. r=jdescottes
authorAlexandre Poirot <poirot.alex@gmail.com>
Wed, 23 Jan 2019 14:46:11 +0000
changeset 515105 c68ab257f8c8f0023ad3dbb436de6fb49bd54317
parent 515104 b4cd1ef4baf051d5f9e933581bb3187059c02289
child 515106 375bd8800b2d5b759453308e6115406decd20df8
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes
bugs1521052
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
devtools/client/shared/test/test-actor-registry.js
devtools/client/webide/test/test_autoconnect_runtime.html
devtools/client/webide/test/test_autoselect_project.html
devtools/server/startup/frame.js
devtools/server/tests/browser/browser.ini
devtools/server/tests/browser/browser_debugger_server.js
devtools/server/tests/mochitest/test_connectToFrame.html
--- a/devtools/client/shared/test/test-actor-registry.js
+++ b/devtools/client/shared/test/test-actor-registry.js
@@ -60,16 +60,21 @@
     // We need to spawn a client instance,
     // but for that we have to first ensure a server is running
     DebuggerServer.init();
     DebuggerServer.registerAllActors();
     const client = new DebuggerClient(DebuggerServer.connectPipe());
 
     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);
 
     return getTestActor(client, tab);
   };
 
   // Fetch the content of a URI
   const request = function(uri) {
--- a/devtools/client/webide/test/test_autoconnect_runtime.html
+++ b/devtools/client/webide/test/test_autoconnect_runtime.html
@@ -62,16 +62,20 @@
           connectionsChanged = waitForConnectionChange("closed", 2);
 
           await nextTick();
           await closeWebIDE(win);
 
           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();
 
           win.AppManager.runtimeList.usb.push(fakeRuntime);
           win.AppManager.update("runtime-list");
 
           await waitForUpdate(win, "runtime-targets");
--- a/devtools/client/webide/test/test_autoselect_project.html
+++ b/devtools/client/webide/test/test_autoselect_project.html
@@ -56,16 +56,20 @@
           connectionsChanged = waitForConnectionChange("closed", 2);
 
           await nextTick();
           await closeWebIDE(win);
 
           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
           win = await openWebIDE();
 
           docRuntime = getRuntimeDocument(win);
 
           panelNode = docRuntime.querySelector("#runtime-panel");
--- a/devtools/server/startup/frame.js
+++ b/devtools/server/startup/frame.js
@@ -117,12 +117,25 @@ try {
     // messageManager connection goes away.  Watching for "unload" here ensures we close
     // any connections when the frame is unloaded.
     addEventListener("unload", () => {
       for (const conn of connections.values()) {
         conn.close();
       }
       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`);
 }
--- a/devtools/server/tests/browser/browser.ini
+++ b/devtools/server/tests/browser/browser.ini
@@ -67,16 +67,17 @@ skip-if = e10s # Bug 1183605 - devtools/
 [browser_canvasframe_helper_03.js]
 skip-if = e10s # Bug 1183605 - devtools/server/tests/browser/ tests are still disabled in E10S
 [browser_canvasframe_helper_04.js]
 skip-if = e10s # Bug 1183605 - devtools/server/tests/browser/ tests are still disabled in E10S
 [browser_canvasframe_helper_05.js]
 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]
 [browser_inspector-mutations-frameload.js]
 [browser_inspector-release.js]
 [browser_inspector-remove.js]
 [browser_inspector-retain.js]
 [browser_inspector-search.js]
new file mode 100644
--- /dev/null
+++ b/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();
+}
--- a/devtools/server/tests/mochitest/test_connectToFrame.html
+++ b/devtools/server/tests/mochitest/test_connectToFrame.html
@@ -75,57 +75,70 @@ function runTests() {
   }, false);
 
   // Instantiate a minimal server
   DebuggerServer.init();
   if (!DebuggerServer.createRootActor) {
     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...
+    await client.request({
+      to: actor.testActor,
+      type: "hello",
+    });
 
-      // 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();
+    // Connect a second client in parallel to asser that it received a distinct set of
+    // target actors
+    await 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
-        mm.addMessageListener("test-actor-destroyed", function listener() {
-          mm.removeMessageListener("test-actor-destroyed", listener);
-          ok(true, "Actor is cleaned up");
-
-          secondClient(actor.testActor);
-        });
+    // 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() {
     DebuggerServer.destroy();
     iframe.remove();
     SimpleTest.finish();
   }