Bug 1982950 - Stop polling as soon as process exits a=RyanVM DONTBUILD

The patch to bug 1979546 refactored the logic that awaits the process
termination. The logic that processes process termination should stop
the poll loop() via updatePollEvents(). This did not happen, because
updatePollEvents() considered a process alive if it is in
`io.processes`, but the updatePollEvents() was not called when
`cleanupProcess` cleared the last reference.

Consequently, the poll loop continued forever if the process's pipes
closed before the process.

Original Revision: https://phabricator.services.mozilla.com/D261060

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

toolkit/modules/subprocess/subprocess_win.worker.js

@@ -235,6 +235,7 @@ class InputPipe extends Pipe {
       this.overlapped.address()
     );
 
+    // TODO bug 1983138: libc.winLastError should be ctypes.winLastError
     if (!ok && (!this.process.handle || libc.winLastError)) {
       this.onError();
     } else {
@@ -332,6 +333,7 @@ class OutputPipe extends Pipe {
       this.overlapped.address()
     );
 
+    // TODO bug 1983138: libc.winLastError should be ctypes.winLastError
     if (!ok && libc.winLastError) {
       this.onError();
     } else {
@@ -758,11 +760,18 @@ io = {
 
   updatePollEvents() {
     let shouldPoll = false;
-    if (this.processes.size) {
+    for (const process of this.processes.values()) {
       // As long as the process is alive, it may notify IOCP.
-      // When the process exits, we'll remove it from io.processes.
+      // When the process exits, process.handle is cleared by its wait(), which
-      shouldPoll = true;
+      // calls updatePollEvents(), but before it is removed from io.processes.
-    } else {
+      // To ensure that we immediately stop polling if it was the last process,
+      // check if process.handle is set.
+      if (process.handle) {
+        shouldPoll = true;
+        break;
+      }
+    }
+    if (!shouldPoll) {
       for (let pipe of this.pipes.values()) {
         if (pipe.hasPendingIO()) {
           shouldPoll = true;

toolkit/modules/subprocess/subprocess_worker_common.js

@@ -153,10 +153,17 @@ let requests = {
     });
   },
 
+  // For testing.
+  getIsPolling() {
+    return { data: io.polling };
+  },
+
+  // For testing.
   getOpenFiles() {
     return { data: new Set(io.pipes.keys()) };
   },
 
+  // For testing.
   getProcesses() {
     let data = new Map(
       Array.from(io.processes.values())
@@ -166,6 +173,7 @@ let requests = {
     return { data };
   },
 
+  // For testing.
   waitForNoProcesses() {
     return Promise.all(
       Array.from(io.processes.values(), proc => proc.awaitFinished())

toolkit/modules/subprocess/test/xpcshell/data_test_script.py

@@ -54,6 +54,10 @@ elif cmd == "ignore_sigterm":
             import time
 
             time.sleep(3600)
+elif cmd == "close_pipes_and_wait_for_stdin":
+    os.close(sys.stdout.fileno())
+    os.close(sys.stderr.fileno())
+    sys.stdin.buffer.read(1)
 elif cmd == "print":
     output(sys.argv[2], stream=sys.stdout, print_only=True)
     output(sys.argv[3], stream=sys.stderr, print_only=True)

toolkit/modules/subprocess/test/xpcshell/test_subprocess_polling.js

@@ -0,0 +1,62 @@
+"use strict";
+
+const { getSubprocessImplForTest } = ChromeUtils.importESModule(
+  "resource://gre/modules/Subprocess.sys.mjs"
+);
+
+let PYTHON;
+const TEST_SCRIPT = do_get_file("data_test_script.py").path;
+
+add_setup(async () => {
+  PYTHON = await Subprocess.pathSearch(Services.env.get("PYTHON"));
+});
+
+// When the last process exits, we should stop polling.
+// This is a regression test for bug 1982950
+add_task(async function test_polling_only_when_process_is_running() {
+  let worker = getSubprocessImplForTest().Process.getWorker();
+
+  equal(
+    await worker.call("getIsPolling", []),
+    false,
+    "Initially not polling before starting a program"
+  );
+
+  let proc = await Subprocess.call({
+    command: PYTHON,
+    arguments: ["-u", TEST_SCRIPT, "close_pipes_and_wait_for_stdin"],
+  });
+
+  equal(
+    await worker.call("getIsPolling", []),
+    true,
+    "Is polling while process is active"
+  );
+
+  // TODO bug 1983138: Re-enable this check once readString() resolves on error
+  // instead of timing out.
+  // // This verifies that read() returns when stdout is closed prematurely.
+  // equal(
+  //   await proc.stdout.readString(),
+  //   "",
+  //   "Test program should have closed stdout prematurely without stdout"
+  // );
+
+  equal(
+    await worker.call("getIsPolling", []),
+    true,
+    "Is still polling while process is active"
+  );
+
+  info("Closing stdin to trigger exit");
+  await proc.stdin.close();
+
+  let { exitCode } = await proc.wait();
+  equal(exitCode, 0, "Got expected exit code");
+
+  equal(
+    await worker.call("getIsPolling", []),
+    false,
+    "Not polling when last process has exited"
+  );
+});

toolkit/modules/subprocess/test/xpcshell/xpcshell.toml

@@ -27,3 +27,5 @@ skip-if = [
 ["test_subprocess_getEnvironment.js"]
 
 ["test_subprocess_pathSearch.js"]
+
+["test_subprocess_polling.js"]