Bug 1528276 - Do not destroy the DebuggerServer in non-e10s when last frame connection is closed r=ochameau
☠☠ backed out by 6e4eb864eeb6 ☠ ☠
authorJulian Descottes <jdescottes@mozilla.com>
Tue, 26 Feb 2019 16:38:30 +0000
changeset 519058 c300840573f5548de1b1aeb68c6e60ef1d1487bc
parent 519057 27e91c8601ec93e4c9212bf0525ee38619b80dc8
child 519059 53b94170e1003ab02c3562e9dde8661d1578aec2
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersochameau
bugs1528276, 1521052
milestone67.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 1528276 - Do not destroy the DebuggerServer in non-e10s when last frame connection is closed r=ochameau When reviewing https://bugzilla.mozilla.org/show_bug.cgi?id=1521052 I did not think about Firefox for Android which is not using e10s. This means the main DebuggerServer will be killed when there are no connections left. Happy to discuss more about the preferred solution. This is a regression in 66 and I hope to uplift a fix for this. Differential Revision: https://phabricator.services.mozilla.com/D20830
devtools/server/startup/frame.js
devtools/server/tests/mochitest/test_connectToFrame.html
--- a/devtools/server/startup/frame.js
+++ b/devtools/server/startup/frame.js
@@ -31,16 +31,17 @@ try {
       loader.invisibleToDebugger = true;
       customLoader = true;
     } else {
       // Otherwise, use the shared loader.
       loader = ChromeUtils.import("resource://devtools/shared/Loader.jsm");
     }
     const { require } = loader;
 
+    const Services = require("Services");
     const DevToolsUtils = require("devtools/shared/DevToolsUtils");
     const { dumpn } = DevToolsUtils;
     const { DebuggerServer } = require("devtools/server/main");
     const { ActorPool } = require("devtools/server/actors/common");
 
     DebuggerServer.init();
     // We want a special server without any root actor and only target-scoped actors.
     // We are going to spawn a FrameTargetActor instance in the next few lines,
@@ -138,19 +139,24 @@ try {
         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() {
+      const isContentProcessServer =
+        Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_CONTENT;
       // 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()) {
+      // Also check if this server is running in a content process. For Firefox running
+      // with e10s disabled (eg Firefox for Android) the DebuggerServer should not be
+      // stopped when all connections are closed.
+      if (DebuggerServer.hasConnection() || !isContentProcessServer) {
         return;
       }
       DebuggerServer.off("connectionchange", destroyServer);
       DebuggerServer.destroy();
 
       // When debugging chrome pages, we initialized a dedicated loader, also destroy it
       if (customLoader) {
         loader.destroy();
--- a/devtools/server/tests/mochitest/test_connectToFrame.html
+++ b/devtools/server/tests/mochitest/test_connectToFrame.html
@@ -95,17 +95,17 @@ function runTests() {
       type: "hello",
     });
 
     // 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");
+      "DebuggerServer is still alive after disconnecting one client");
 
     // 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();
@@ -113,19 +113,19 @@ function runTests() {
     });
 
     // 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");
+    // DebuggerServer instance.
+    ok(DebuggerServer.initialized, "Shared DebuggerServer in the parent process is " +
+      "still alive after all clients are disconnected");
     cleanup();
   }
 
   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);