1979546 - Subprocess call can deadlock on Windows when WaitForMultipleObjects receives 64+ events (=16+ concurrent programs)

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect, P1)

Description

[Rob Wu [:robwu]]

Bug 1971449 reports a scenario with the following characteristics:

• An extension calls sendNativeMessage, potentially many times. This launches a native application via the subprocess module internally.
  • Test extension and program: extension-poc.zip (preview without downloading: echo_host.cs via crxviewer)
• At some point a bug happens where the first message is readable by the program, but not the second chunk.
  • Log file: native-host.log at https://bugzilla.mozilla.org/show_bug.cgi?id=1971449#c9
• When the bug happens, native messaging hosts binaries accumulate in the process list. Could be as high as 2000 processes.

After auditing the relevant implementation in Firefox, I found a potential way for this to trigger this outcome.

• The NativeMessaging implementation always writes two chunks as part of any write. This passes through write in subprocess_common.sys.mjs to write in subprocess_win.worker.js.
  • The first write is immediately written to the process because buffer is initially empty (source: write, writeNext, writeBuffer).
  • The second write awaits the reported completion of the first write (source: onReady).
• The completion is reported via onReady(), via poll.
  • The events to poll are the set of 1 signal, processes and their pipes, computed in updatePollEvents.
  • onReady is called whenever a change to any pipe or process is observed through the WaitForMultipleObjects.
    • According to MSDN: WaitForMultipleObjects, "The maximum number of object handles is MAXIMUM_WAIT_OBJECTS".
    • MAXIMUM_WAIT_OBJECTS is typically 64 according to Is MAXIMUM_WAIT_OBJECTS really 64? - Stack Overflow
• In practice, this means that the maximum number of concurrent processes supported by the Subprocess module on Windows is 15:
  • 1 signal (always present, used to detect Firefox shutdown)
  • 15 processes
  • 45 pipes (stdin/stdout/stderr for each process - this could be lower if there are no pending reads or writes)
  • is 61 events out of the limit of 64.
  • If 16 or more processes are created concurrently, the number of events exceeds MAXIMUM_WAIT_OBJECTS and WaitForMultipleObjects will fail and therefore io.poll() will deadlock, because it is no longer able to be notified of changes to pipes and processes.

I created a unit test that successfully creates 15 processes, but fails at 16.

Comment 1

[Rob Wu [:robwu]]

The MSDN documentation was suggesting something very complicated.

I found this article that may be a simpler path towards dealing with the limit: https://devblogs.microsoft.com/oldnewthing/?p=110169

Comment 2

[Rob Wu [:robwu]]

I considered splitting the list of handles in chunks and using WaitForMultipleObjects on separate lists of handles. When there is any activity, this works reasonably well (we can set the timeout to 0). But if all pipes and processes are pending, we need to choose which 64 of them to wait for. While waiting for these, the thread is unaware of progress of the others. A way to overcome that is to lower the timeout, from 5000ms to something shorter, at the expense of waking up the subprocess thread more often. That's not ideal either.

I learned about the concept of I/O Completion Ports (IOCP) (https://learn.microsoft.com/en-us/windows/win32/fileio/i-o-completion-ports), which seems to fit the problem space here. We need the ability to be notified of file read/write completion, process termination and a way to unblock the waiting worker thread so it can process messages from the parent thread (Subprocess.sys.mjs).

IOCP appears to provide the desired functionality:

• By design support an arbitrary number of file handles, with ReadFile and WriteFile functions having first-class support that are already in use by the subprocess module.
• Process termination can be detected: https://stackoverflow.com/questions/17724859/detecting-exit-failure-of-child-processes-using-iocp-c-windows
• PostQueuedCompletionStatus (MSDN can be used to immediately unblock a waiting GetQueuedCompletionStatus call.

I'll give a try at implementing this.

Comment 3

[Rob Wu [:robwu]]

For reference, the new library symbols from kernel32.dll that I will be importing in my patch.

• CreateIoCompletionPort - https://learn.microsoft.com/en-us/windows/win32/fileio/createiocompletionport
• GetQueuedCompletionStatus - https://learn.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-getqueuedcompletionstatus
• PostQueuedCompletionStatus - https://learn.microsoft.com/en-us/windows/win32/fileio/postqueuedcompletionstatus
• JOBOBJECT_ASSOCIATE_COMPLETION_PORT - https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-jobobject_associate_completion_port
• JobObjectAssociateCompletionPortInformation - a constant from the already-declared SetInformationJobObject - https://learn.microsoft.com/en-us/windows/win32/api/jobapi2/nf-jobapi2-setinformationjobobject

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1979546 - Replace WaitForMultipleObjects with IOCP in Subprocess

The current WaitForMultipleObjects-based implementation causes a
deadlock if 16 or more processes are spawned, as explained in the bug.

This patch replaces the use of events and WaitForMultipleObjects with
I/O Completion Ports (IOCP) as the mechanism to monitor I/O progress
and process terminations. This is not subject to the
MAXIMUM_WAIT_OBJECTS (64) limit of WaitForMultipleObjects.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/abd041fb3165
https://hg.mozilla.org/integration/autoland/rev/561affffd311
Replace WaitForMultipleObjects with IOCP in Subprocess r=win-reviewers,extension-reviewers,rpl,gstoll

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c5edae39a0a8
https://hg.mozilla.org/integration/autoland/rev/d46bbed8e543
Revert "Bug 1979546 - Replace WaitForMultipleObjects with IOCP in Subprocess r=win-reviewers,extension-reviewers,rpl,gstoll" for causing devtools failures.

Comment 7

[Serban Stanca [:SerbanS]]

Reverted this because it was causing devtools failures.

• Revert link
• Push with failures
• Failure Log
• Failure line: SUMMARY: AddressSanitizer: breakpoint /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:55 in AnnotateMozCrashReason

Please also check these mochitests failures.

• Failure Log

Comment 8

[Rob Wu [:robwu]]

This is an issue in the test harness, I'll fix it in https://bugzilla.mozilla.org/show_bug.cgi?id=1982170#c1 and then reland.

Note that it's worth mentioning that I believe that there is a pre-existing issue that causes the timer to be scheduled. It is the third bullet point of my list of unresolved todos that I collected while looking into this bug:

• read: libc.winLastError should be ctypes.winLastError https://searchfox.org/mozilla-central/rev/fcc0cfc7f5b1df957ae29f6b2ccfeb6df47b6be8/toolkit/modules/subprocess/subprocess_win.worker.js#193
• write: libc.winLastError should be ctypes.winLastError https://searchfox.org/mozilla-central/rev/fcc0cfc7f5b1df957ae29f6b2ccfeb6df47b6be8/toolkit/modules/subprocess/subprocess_win.worker.js#290
• loop may loop twice (and does so in my unit test): loop() from init, if run after updatePollEvents() schedules, then two loops are running repeatedly: https://searchfox.org/mozilla-central/rev/fcc0cfc7f5b1df957ae29f6b2ccfeb6df47b6be8/toolkit/modules/subprocess/subprocess_win.worker.js#686

I intended to fix these separately in a follow-up because they are pre-existing issues that have nothing to do with the bug reported here.

Comment 9

[Rob Wu [:robwu]]

Removing bug 1982170 as a dependency; Although the specific crash is indeed a test-only issue, the fact that a timer was still running was indeed an issue with the patch. I fixed it and don't see the previously failing tests don't fail any more in the try push.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/90a9d079dc47
https://hg.mozilla.org/integration/autoland/rev/ec77192e6f06
Replace WaitForMultipleObjects with IOCP in Subprocess r=win-reviewers,extension-reviewers,rpl,gstoll

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ec77192e6f06

Comment 12

[Rob Wu [:robwu]]

Attachment: Bug 1979546 - Replace WaitForMultipleObjects with IOCP in Subprocess

The current WaitForMultipleObjects-based implementation causes a
deadlock if 16 or more processes are spawned, as explained in the bug.

This patch replaces the use of events and WaitForMultipleObjects with
I/O Completion Ports (IOCP) as the mechanism to monitor I/O progress
and process terminations. This is not subject to the
MAXIMUM_WAIT_OBJECTS (64) limit of WaitForMultipleObjects.

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

Comment 13

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Windows-only; When the Subprocess.sys.mjs module has 16 processes or more concurrent processes, the subprocess worker that manages them loses its ability to monitor the state of the processes and stdin/stdout/stderr pipes that it manages. The Subprocess module is used by DevTools (Browser Toolbox) and WebExtensions (Native Messaging), and we have received a report of extensions being broken for users (bug 1971449), with gigabytes of memory usage and over 1000 processes spawned.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: QA not needed; any coverage that we'd get is already fully covered by unit tests.
• Risk associated with taking this patch: medium
• Explanation of risk level: Low, barely Medium risk. Medium because this is a significant but necessary refactor of the mechanism to monitor state changes to processes and pipes in the Subprocess.sys.mjs module. The risk is managed by the patches including extensive test coverage (that demonstrated the deadlock before, and verifies the fix after), and the patches has been on the train to release (143) without reported regressions or breakage. Furthermore, the changes only affect Subprocess.sys.mjs consumers (DevTools Browser Toolbox, and WebExtensions Native Messaging), so it does not impact Firefox users who do not use these features.
• String changes made/needed: None
• Is Android affected?: no

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/8c4662b634a9