1752638 - Fork server: remoting for child process exit handling

Status: NEW

(Core :: IPC, task)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Currently, the fork server ignores child processes exiting, and the parent process needs to guess whether the process exited by probing its pid, which can be wrong if the pid is reused (in the worst case, this could mean that we kill -9 an unrelated process). This also doesn't allow seeing the child process's exit status; see also bug 1752625.

Currently, the fork server's MiniTransceiver requires messages to alternate between the parent sending and the parent receiving. I'd suggest removing that, and instead handling synchronous calls by having the parent send the fork server one end of a socketpair or pipe for that message's reply, like the Linux sandbox broker and the crash reporter. The socket should also be changed from SOCK_STREAM to SOCK_SEQPACKET to allow sending atomically from any thread; note that it currently doesn't handle short writes anyway.

The resulting protocol would look like this:

• [parent→forkserver, sync] LaunchApp; takes various parameters, returns pid
• [parent→forkserver, sync] WaitPid; takes pid and whether to block, returns status or error
• [forkserver→parent, async] SigChld; this will need to be connected to ProcessWatcher (which, after bug 1658072, will use the pipe-to-self trick in our code instead of libevent, so writing a byte to that pipe would work).

The remote waitpid being synchronous and done only at the parent's request (not automatically when the SIGCHLD is received) is subtle: before we call it, we know the pid is still in use and can't be reused; afterwards, we know it's not. This lets us (where “us” includes my ProcessWatcher rewrite) safely index child process information by pid and know that there can't be duplicates as long as we remove any information for a process before doing the waitpid.

For the SIGCHLD notifications, the forkserver socket will need to be connected to an event loop; the IPC I/O thread will work. Alternately, the write end of the ProcessWatcher signal pipe could be sent to the fork server process so it can wake up the ProcessWatcher directly; that's kind of hacky but might be simpler. Also, there's going to be some complication as far as when to waitpid a child directly vs. calling the fork server; one possibility is that, for a fork server child, waitpiding it directly will always fail with ECHILD (we know that the pid hasn't been freed for reuse yet), so it could just always try to wait directly and then fall back to messaging the fork server in that case.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

We probably don't want to ship the forkserver in release before we fix this.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1752638 - WORK IN PROGRESS - MiniTransceiver/protocol changes

This patch has my half-finished work making some changes to how fork
server communications works. Before it used a stream socket and always
had matched pairs of synchronous request/reply with no interruptions.
This changes the main protocol to use a sequential-packet socket with
asynchronous messages in (eventually) both directions; ones that need
a reply (currently the fork request, later also the wait request) will
attach a socket for that, like what we do in the Linux sandbox and crash
reporter.

That also allows requests to be sent from any thread in the main process
without locking, although that will probably need threadsafe refcouting
for safety. The attached sockets can be stream sockets, e.g. so that
the arg/env strings could exceed the size of the socket buffer if
needed.

What is not done here is to actually implement the new messages.
Remoting waitid should be relatively straightforward, along similar
lines to how the fork request works after this patch. Sending SIGCHLD
back is more interesting: it's possible to sendmsg directly from the
signal handler, but the message would have to be pre-serialized, because
malloc is not safe there; it's also possible to use the pipe-to-self
idiom. In the main process, connecting those messages to the I/O thread
event loop will also need some work.

There is also some subtlety about child processes outliving the fork
server -- currently we intentionally shut it down while child processes
are still running, and it will always be possible for it to crash, so
we'll need some kind of best-effort fallback there.

There are also some FIXME comments in this patch.

And, integration with ProcessWatcher should be aware of D141309.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

There's a simpler way to do this, pointed out by Nika on Matrix a while back. The simpler version is:

• The fork server IPC protocol continues to have only sync messages from the parent, but probably with a mutex around the parent side to allow use from other threads.
• There are now two RPC calls the fork server accepts: launch and waitpid
• There's a separate pipe for SIGCHLD events: the fork server's signal handler writes a byte to it directly, and the parent process polls the other end for reading (like the current use of pipe-to-self, but pipe-to-other).

One potential issue here is that if we have n child processes in the process of exiting and then they all exit one by one, we'll make O(n^2) RPC calls to the fork server, which could be expensive. (But maybe not; the constant factor could be relatively low and this only counts child processes after the GeckoChildProcessHost is destroyed but before OS-level process exit.) Also, a remote waitpid request could block the calling thread (the I/O thread?) for several milliseconds while the fork server clones itself (unfortunately, Linux doesn't heavily optimize the case of forking a process with many memory mappings but very little changed since the last fork, possibly as a tradeoff against more common use cases). To avoid this, the advanced version of this idea adds:

• The fork server keeps track of all running child processes; alternately, the parent process can register interest in them when ProcessWatcher starts watching.
• On SIGCHLD, the fork server will waitid with WNOWAIT on each child process, and write the pid of each one that exited to the pipe; ProcessWatcher would be adjusted accordingly.
• The actual waitpid to collect the process should still be a sync IPC, so that we don't have pids being reused when the parent process still thinks it's a running child process. This is important because we're trying to remove the potential for annoying edge cases like accidentally SIGKILLing the wrong process.

Basically this moves more of ProcessWatcher's logic into the fork server itself, and also duplicates some of the state, so it's more complicated and there are some subtleties.