Bug 1783160 - [devtools] Use private fields and methods in BrowserToolboxLauncher. r=jdescottes.

Tests were using the internal `_dbgProcess` and `_dbgProfilePath` properties, which
we migrate to private properties.
We now pass those properties as part of the `run` event that those tests are
setting up, so we can run assertions on those object.
We also emit the event with `emitForTests` to make it clear those properties
are only accessed outside of the class from tests.
Finally, we take this as an opportunity to clean `browser_dbg-chrome-create.js`.
Since it was the only callsite using `onClose`, and given that we directly call
the `close` method, we can remove the `onClose` from the signature of `BrowserToolboxLauncher`.

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

devtools/client/debugger/test/mochitest/browser_dbg-chrome-create.js

@@ -13,82 +13,49 @@
 PromiseTestUtils.allowMatchingRejectionsGlobally(/File closed/);
 PromiseTestUtils.allowMatchingRejectionsGlobally(/NS_ERROR_FAILURE/);
 
+// This test can be slow to run
 requestLongerTimeout(5);
 
 const { BrowserToolboxLauncher } = ChromeUtils.import(
   "resource://devtools/client/framework/browser-toolbox/Launcher.jsm"
 );
-let gProcess = undefined;
 
 add_task(async function() {
-  // Windows XP and 8.1 test machines are terribly slow at this test.
   await pushPref("devtools.chrome.enabled", true);
   await pushPref("devtools.debugger.remote-enabled", true);
 
-  gProcess = await initChromeDebugger();
+  info("Call BrowserToolboxLauncher.init");
+  const [browserToolboxLauncher, process, profilePath] = await new Promise(
+    resolve => {
+      BrowserToolboxLauncher.init({
+        onRun: (btl, dbgProcess, dbgProfilePath) => {
+          info("Browser toolbox process started successfully.");
+          resolve([btl, dbgProcess, dbgProfilePath]);
+        },
+      });
+    }
+  );
 
-  ok(
-    gProcess._dbgProcess,
-    "The remote debugger process wasn't created properly!"
-  );
-  ok(
-    gProcess._dbgProcess.exitCode == null,
-    "The remote debugger process isn't running!"
-  );
+  ok(process, "The remote debugger process was created");
+  ok(process.exitCode == null, "The remote debugger process is running");
   is(
-    typeof gProcess._dbgProcess.pid,
+    typeof process.pid,
     "number",
-    "The remote debugger process doesn't have a pid (?!)"
-  );
-
-  info(`process location: ${gProcess._dbgProcess.location}`);
-  info(`process pid: ${gProcess._dbgProcess.pid}`);
-  info(`process name: ${gProcess._dbgProcess.processName}`);
-  info(`process sig: ${gProcess._dbgProcess.processSignature}`);
-
-  ok(
-    gProcess._dbgProfilePath,
-    "The remote debugger profile wasn't created properly!"
+    `The remote debugger process has a proper pid (${process.pid})`
   );
 
   is(
-    gProcess._dbgProfilePath,
+    profilePath,
     PathUtils.join(PathUtils.profileDir, "chrome_debugger_profile"),
-    "The remote debugger profile isn't where we expect it!"
+    `The remote debugger profile has the expected path`
   );
 
-  info(`profile path: ${gProcess._dbgProfilePath}`);
+  info("Close the browser toolbox");
+  await browserToolboxLauncher.close();
 
-  await gProcess.close();
-});
-
-function initChromeDebugger() {
-  info("Initializing a chrome debugger process.");
-  return new Promise(resolve => {
-    BrowserToolboxLauncher.init({
-      onClose,
-      onRun: _process => {
-        info("Browser toolbox process started successfully.");
-        resolve(_process);
-      },
-    });
-  });
-}
-
-function onClose() {
   is(
-    gProcess._dbgProcess.exitCode,
+    process.exitCode,
     Services.appinfo.OS == "WINNT" ? -9 : -15,
-    "The remote debugger process didn't die cleanly."
+    "The remote debugger process died cleanly"
   );
-
-  info(`process exit value: ${gProcess._dbgProcess.exitCode}`);
-
-  info(`profile path: ${gProcess._dbgProfilePath}`);
-
-  finish();
-}
-
-registerCleanupFunction(function() {
-  gProcess = null;
 });

devtools/client/framework/browser-toolbox/Launcher.jsm

@@ -58,7 +58,6 @@ const processes = new Set();
 
 /**
  * @typedef {Object} BrowserToolboxLauncherArgs
- * @property {function} onClose - A function called when the process stops running.
  * @property {function} onRun - A function called when the process starts running.
  * @property {boolean} overwritePreferences - Set to force overwriting the toolbox
  *                     profile's preferences with the current set of preferences.
@@ -91,28 +90,32 @@ class BrowserToolboxLauncher extends EventEmitter {
     return processes.size !== 0;
   }
 
+  #closed;
+  #devToolsServer;
+  #dbgProfilePath;
+  #dbgProcess;
+  #listener;
+  #loader;
+  #port;
+  #telemetry = new Telemetry();
+
   /**
    * Constructor for creating a process that will hold a chrome toolbox.
    *
    * @param {...BrowserToolboxLauncherArgs} args
    */
-  constructor({ onClose, onRun, overwritePreferences } = {}) {
+  constructor({ onRun, overwritePreferences } = {}) {
     super();
 
-    if (onClose) {
-      this.once("close", onClose);
-    }
     if (onRun) {
       this.once("run", onRun);
     }
 
-    this._telemetry = new Telemetry();
-
     this.close = this.close.bind(this);
     Services.obs.addObserver(this.close, "quit-application");
-    this._initServer();
-    this._initProfile(overwritePreferences);
-    this._create();
+    this.#initServer();
+    this.#initProfile(overwritePreferences);
+    this.#create();
 
     processes.add(this);
   }
@@ -120,8 +123,8 @@ class BrowserToolboxLauncher extends EventEmitter {
   /**
    * Initializes the devtools server.
    */
-  _initServer() {
-    if (this.devToolsServer) {
+  #initServer() {
+    if (this.#devToolsServer) {
       dumpn("The chrome toolbox server is already running.");
       return;
     }
@@ -133,22 +136,22 @@ class BrowserToolboxLauncher extends EventEmitter {
     // This allows us to safely use the tools against even the actors and
     // DebuggingServer itself, especially since we can mark this loader as
     // invisible to the debugger (unlike the usual loader settings).
-    this.loader = useDistinctSystemPrincipalLoader(this);
-    const { DevToolsServer } = this.loader.require(
+    this.#loader = useDistinctSystemPrincipalLoader(this);
+    const { DevToolsServer } = this.#loader.require(
       "devtools/server/devtools-server"
     );
-    const { SocketListener } = this.loader.require(
+    const { SocketListener } = this.#loader.require(
       "devtools/shared/security/socket"
     );
-    this.devToolsServer = DevToolsServer;
+    this.#devToolsServer = DevToolsServer;
     dumpn("Created a separate loader instance for the DevToolsServer.");
 
-    this.devToolsServer.init();
+    this.#devToolsServer.init();
     // We mainly need a root actor and target actors for opening a toolbox, even
     // against chrome/content. But the "no auto hide" button uses the
     // preference actor, so also register the browser actors.
-    this.devToolsServer.registerAllActors();
-    this.devToolsServer.allowChromeProcess = true;
+    this.#devToolsServer.registerAllActors();
+    this.#devToolsServer.allowChromeProcess = true;
     dumpn("initialized and added the browser actors for the DevToolsServer.");
 
     const bts = Cc["@mozilla.org/backgroundtasks;1"]?.getService(
@@ -157,10 +160,10 @@ class BrowserToolboxLauncher extends EventEmitter {
     if (bts?.isBackgroundTaskMode) {
       // A special root actor, just for background tasks invoked with
       // `--backgroundtask TASK --jsdebugger`.
-      const { createRootActor } = this.loader.require(
+      const { createRootActor } = this.#loader.require(
         "resource://gre/modules/backgroundtasks/dbg-actors.js"
       );
-      this.devToolsServer.setRootActor(createRootActor);
+      this.#devToolsServer.setRootActor(createRootActor);
     }
 
     const chromeDebuggingWebSocket = Services.prefs.getBoolPref(
@@ -171,25 +174,25 @@ class BrowserToolboxLauncher extends EventEmitter {
       portOrPath: -1,
       webSocket: chromeDebuggingWebSocket,
     };
-    const listener = new SocketListener(this.devToolsServer, socketOptions);
+    const listener = new SocketListener(this.#devToolsServer, socketOptions);
     listener.open();
-    this.listener = listener;
-    this.port = listener.port;
+    this.#listener = listener;
+    this.#port = listener.port;
 
-    if (!this.port) {
+    if (!this.#port) {
       throw new Error("No devtools server port");
     }
 
     dumpn("Finished initializing the chrome toolbox server.");
     dump(
-      `DevTools Server for Browser Toolbox listening on port: ${this.port}\n`
+      `DevTools Server for Browser Toolbox listening on port: ${this.#port}\n`
     );
   }
 
   /**
    * Initializes a profile for the remote debugger process.
    */
-  _initProfile(overwritePreferences) {
+  #initProfile(overwritePreferences) {
     dumpn("Initializing the chrome toolbox user profile.");
 
     const bts = Cc["@mozilla.org/backgroundtasks;1"]?.getService(
@@ -227,7 +230,7 @@ class BrowserToolboxLauncher extends EventEmitter {
     } catch (ex) {
       if (ex.result === Cr.NS_ERROR_FILE_ALREADY_EXISTS) {
         if (!overwritePreferences) {
-          this._dbgProfilePath = debuggingProfileDir.path;
+          this.#dbgProfilePath = debuggingProfileDir.path;
           return;
         }
         // Fall through and copy the current set of prefs to the profile.
@@ -238,7 +241,7 @@ class BrowserToolboxLauncher extends EventEmitter {
       }
     }
 
-    this._dbgProfilePath = debuggingProfileDir.path;
+    this.#dbgProfilePath = debuggingProfileDir.path;
 
     // We would like to copy prefs into this new profile...
     const prefsFile = debuggingProfileDir.clone();
@@ -278,18 +281,18 @@ class BrowserToolboxLauncher extends EventEmitter {
 
     dumpn(
       "Finished creating the chrome toolbox user profile at: " +
-        this._dbgProfilePath
+        this.#dbgProfilePath
     );
   }
 
   /**
    * Creates and initializes the profile & process for the remote debugger.
    */
-  _create() {
+  #create() {
     dumpn("Initializing chrome debugging process.");
 
     let command = Services.dirsvc.get("XREExeF", Ci.nsIFile).path;
-    let profilePath = this._dbgProfilePath;
+    let profilePath = this.#dbgProfilePath;
 
     // MOZ_BROWSER_TOOLBOX_BINARY is an absolute file path to a custom firefox binary.
     // This is especially useful when debugging debug builds which are really slow
@@ -339,7 +342,7 @@ class BrowserToolboxLauncher extends EventEmitter {
       // Disable safe mode for the new process in case this was opened via the
       // keyboard shortcut.
       MOZ_DISABLE_SAFE_MODE_KEY: "1",
-      MOZ_BROWSER_TOOLBOX_PORT: String(this.port),
+      MOZ_BROWSER_TOOLBOX_PORT: String(this.#port),
       MOZ_HEADLESS: null,
       // Never enable Marionette for the new process.
       MOZ_MARIONETTE: null,
@@ -367,7 +370,7 @@ class BrowserToolboxLauncher extends EventEmitter {
     }
 
     dump(`Starting Browser Toolbox ${command} ${args.join(" ")}\n`);
-    this._dbgProcessPromise = Subprocess.call({
+    Subprocess.call({
       command,
       arguments: args,
       environmentAppend: true,
@@ -375,14 +378,14 @@ class BrowserToolboxLauncher extends EventEmitter {
       environment,
     }).then(
       proc => {
-        this._dbgProcess = proc;
+        this.#dbgProcess = proc;
 
         // jsbrowserdebugger is not connected with a toolbox so we pass -1 as the
         // toolbox session id.
-        this._telemetry.toolOpened("jsbrowserdebugger", -1, this);
+        this.#telemetry.toolOpened("jsbrowserdebugger", -1, this);
 
         dumpn("Chrome toolbox is now running...");
-        this.emit("run", this);
+        this.emitForTests("run", this, proc, this.#dbgProfilePath);
 
         proc.stdin.close();
         const dumpPipe = async pipe => {
@@ -422,11 +425,11 @@ class BrowserToolboxLauncher extends EventEmitter {
    * Closes the remote debugging server and kills the toolbox process.
    */
   async close() {
-    if (this.closed) {
+    if (this.#closed) {
       return;
     }
 
-    this.closed = true;
+    this.#closed = true;
 
     dumpn("Cleaning up the chrome debugging process.");
 
@@ -434,32 +437,31 @@ class BrowserToolboxLauncher extends EventEmitter {
 
     // We tear down before killing the browser toolbox process to avoid leaking
     // socket connection objects.
-    if (this.listener) {
-      this.listener.close();
+    if (this.#listener) {
+      this.#listener.close();
     }
 
     // Note that the DevToolsServer can be shared with the DevToolsServer
     // spawned by DevToolsFrameChild. We shouldn't destroy it from here.
     // Instead we should let it auto-destroy itself once the last connection is closed.
-    this.devToolsServer = null;
+    this.#devToolsServer = null;
 
-    this._dbgProcess.stdout.close();
-    await this._dbgProcess.kill();
+    this.#dbgProcess.stdout.close();
+    await this.#dbgProcess.kill();
 
     // jsbrowserdebugger is not connected with a toolbox so we pass -1 as the
     // toolbox session id.
-    this._telemetry.toolClosed("jsbrowserdebugger", -1, this);
+    this.#telemetry.toolClosed("jsbrowserdebugger", -1, this);
 
     dumpn("Chrome toolbox is now closed...");
-    this.emit("close", this);
     processes.delete(this);
 
-    this._dbgProcess = null;
-    if (this.loader) {
+    this.#dbgProcess = null;
+    if (this.#loader) {
       releaseDistinctSystemPrincipalLoader(this);
     }
-    this.loader = null;
-    this._telemetry = null;
+    this.#loader = null;
+    this.#telemetry = null;
   }
 }
 

devtools/client/framework/browser-toolbox/test/helpers-browser-toolbox.js

@@ -61,9 +61,13 @@ async function initBrowserToolboxTask({
   ).PromiseTestUtils.allowMatchingRejectionsGlobally(/File closed/);
 
   let process;
+  let dbgProcess;
   if (!existingProcessClose) {
-    process = await new Promise(onRun => {
-      BrowserToolboxLauncher.init({ onRun, overwritePreferences: true });
+    [process, dbgProcess] = await new Promise(resolve => {
+      BrowserToolboxLauncher.init({
+        onRun: (_process, _dbgProcess) => resolve([_process, _dbgProcess]),
+        overwritePreferences: true,
+      });
     });
     ok(true, "Browser toolbox started");
     is(
@@ -210,7 +214,7 @@ async function initBrowserToolboxTask({
 
     const closePromise = existingProcessClose
       ? existingProcessClose()
-      : process._dbgProcess.wait();
+      : dbgProcess.wait();
     evaluateExpression("gToolbox.destroy()").catch(e => {
       // Ignore connection close as the toolbox destroy may destroy
       // everything quickly enough so that evaluate request is still pending