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. 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
2022-08-08 06:15:42 +00:00 · 2022-08-08 06:15:42 +00:00 · 45a749c7be
--- a/devtools/client/debugger/test/mochitest/browser_dbg-chrome-create.js
+++ b/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;
 });
--- a/devtools/client/framework/browser-toolbox/Launcher.jsm
+++ b/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.emit("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;
  }
 }

--- a/devtools/client/framework/browser-toolbox/test/helpers-browser-toolbox.js
+++ b/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