From d01e2df6064aae16d9593e7b1612b7ce546e3f6d Mon Sep 17 00:00:00 2001 From: Rob Wu Date: Fri, 26 Sep 2025 14:46:13 +0000 Subject: [PATCH] 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 --- .../subprocess/subprocess_win.worker.js | 17 +++-- .../subprocess/subprocess_worker_common.js | 8 +++ .../test/xpcshell/data_test_script.py | 4 ++ .../test/xpcshell/test_subprocess_polling.js | 62 +++++++++++++++++++ .../subprocess/test/xpcshell/xpcshell.toml | 2 + 5 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 toolkit/modules/subprocess/test/xpcshell/test_subprocess_polling.js diff --git a/toolkit/modules/subprocess/subprocess_win.worker.js b/toolkit/modules/subprocess/subprocess_win.worker.js index 8363986bc259..22c300bb2a6d 100644 --- a/toolkit/modules/subprocess/subprocess_win.worker.js +++ b/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. - shouldPoll = true; - } else { + // When the process exits, process.handle is cleared by its wait(), which + // calls updatePollEvents(), but before it is removed from io.processes. + // 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; diff --git a/toolkit/modules/subprocess/subprocess_worker_common.js b/toolkit/modules/subprocess/subprocess_worker_common.js index ba9c5ce7773a..00ed0bcdc460 100644 --- a/toolkit/modules/subprocess/subprocess_worker_common.js +++ b/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()) diff --git a/toolkit/modules/subprocess/test/xpcshell/data_test_script.py b/toolkit/modules/subprocess/test/xpcshell/data_test_script.py index e1f5f5de93c3..ed2a86e7b74a 100644 --- a/toolkit/modules/subprocess/test/xpcshell/data_test_script.py +++ b/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) diff --git a/toolkit/modules/subprocess/test/xpcshell/test_subprocess_polling.js b/toolkit/modules/subprocess/test/xpcshell/test_subprocess_polling.js new file mode 100644 index 000000000000..81afe9ec956a --- /dev/null +++ b/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" + ); +}); diff --git a/toolkit/modules/subprocess/test/xpcshell/xpcshell.toml b/toolkit/modules/subprocess/test/xpcshell/xpcshell.toml index c0f67d9afb2b..1f3821c167e6 100644 --- a/toolkit/modules/subprocess/test/xpcshell/xpcshell.toml +++ b/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"]