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"]