(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #1)

IPC has a not-quite-standard approach to this that works on the OSes we support: https://searchfox.org/mozilla-central/rev/c56f656febb1/ipc/chromium/src/base/process_util_posix.cc#124

That needs to be called between fork and exec, so it's not something that
can be translated directly into js-ctypes, but possibly the JS could call
into base::LaunchApp; see also bug 1406971.

If the only thing it takes is a JS accessible xpcom component that expose these lines, I could try and devote a few spare cycles to write one.

Also, if I read correctly that it's closing everything except stdin, stdout, stderr and the chrootserver file descriptor. Is this still the behavior that needs to be replicated?

Also, if the idea is to call this from JavaScript... aren't we going to have allocations and/or GCs in JavaScript between fork() and exec()? That can cause problems, right?

No, it can't be an XPCOM component. The code that needs to call it runs in a worker. And, yes, running JS code in between the fork and exec would be a problem, which is why the API needs to do both in a single operation. That's why we use posix_spawn currently.

(In reply to Kris Maglione [:kmag] from comment #6)

No, it can't be an XPCOM component. The code that needs to call it runs in a worker. And, yes, running JS code in between the fork and exec would be a problem, which is why the API needs to do both in a single operation. That's why we use posix_spawn currently.

Ok, so in this case, if I understand correctly, we cannot use https://searchfox.org/mozilla-central/rev/c56f656febb1/ipc/chromium/src/base/process_util_posix.cc#124, right?

I toyed with the idea of wrapping around fork() + exec() + closeUnnecessaryFDs(), and exposing this either through webidl or through js-ctypes, but there's still the risk that concurrent gc could run between fork() and exec(). If we could deactivate gc before fork() and reactivate it only only on the parent, though, I guess we could make it work.

My idea was to wrap base::LaunchApp (and the necessary subset of LaunchOptions) for use by JS, and have it call that the same way it currently calls posix_spawn. Currently Subprocess.jsm supports some features that LaunchApp doesn't (environmentAppend: false, workdir, disclaim; the first of these doesn't seem to be used), but they could be added.

I strongly advise against trying to run JS in the forked child of a multithreaded process. POSIX allows only async-signal-safe operations in that context (in particular: no locking or anything that could indirectly use locking), and we have some hacks so that malloc more or less works, but in general it's best to do as little as possible, and a JS interpreter is likely to cause problems.

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #8)

My idea was to wrap base::LaunchApp (and the necessary subset of LaunchOptions) for use by JS, and have it call that the same way it currently calls posix_spawn. Currently Subprocess.jsm supports some features that LaunchApp doesn't (environmentAppend: false, workdir, disclaim; the first of these doesn't seem to be used), but they could be added.

Does Subprocess.jsm have access to webidl-defined methods? If so, we could create a method ChromeUtils.launchApp.

Kris, are you still actively working on this / do we still need to fix this / how important is this?

Kicking this out of our triage for now, but leaving needinfo in case this is more important for some reason.

I'm not working on it. We do still need to do it. It's hard to say how important it is. We mark a lot of file descriptors as non-inheritable now, so it's not necessarily a huge issue. But there are still some we don't, and there's still a race between creating them and adding the CLOEXEC flag in a fair number of cases. So it's not exactly great as it is either.

Unexpected file descriptors that I can see with a quick look at lsof:

• Necko sockets
• Anything received over IPC, notably a lot of shared memory
• Various .xpi files and also the omnijars
• startupCache files from the profile

So there are still a number of problems here. (IPC should use MSG_CMSG_CLOEXEC, or set O_CLOEXEC nonatomically if that's not available; that one, at least, I know how to fix.)

I look at look at what Subprocess.jsm provides that base::LaunchApp currently doesn't:

• Replacing the environment instead of modifying the existing one
• The macOS-specific responsibility_spawnattrs_setdisclaim feature
• Setting the working directory

These are all easy enough to add. The other problem is how to expose them to JS; nsIProcess would need to add all of those, as well as the ability to assign fds to stdin/out/err (at least for the cases of pipes or /dev/null). Or maybe XPCOM isn't the right tool for that.

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #13)

These are all easy enough to add. The other problem is how to expose them to JS; nsIProcess would need to add all of those, as well as the ability to assign fds to stdin/out/err (at least for the cases of pipes or /dev/null). Or maybe XPCOM isn't the right tool for that.

nsIProcess has enough other problems that I'd rather start from scratch with something new rather than use it. We also can't use it for Subprocess.jsm since Subprocess needs to run in a worker thread for the sake of the IO work it does, so it can't use XPCOM. We could get rid of the JS part entirely, but I think that would be even more reason not to try to build on nsIProcess

Currently, Subprocess.jsm on Unix uses js-ctypes to call posix_spawn.
This has some issues, primarily that file descriptors are inherited by
the child process unless explicitly opted-out, which unfortunately a lot
of code doesn't do. This patch changes it to use IPC process launching,
where fd inheritance is opt-in, by:

1. Extending base::LaunchApp to handle a few features that Subprocess
   needs (setting the process's working directory, specifying the full
   environment, and the macOS disclaim flag)

2. Adding a WebIDL method to IOUtils to expose that function to JS
   (currently Unix-only; Windows could also be supported but it would
   probably want to use a different IDL type for strings, given that the
   OS APIs use 16-bit code units).

3. Replacing the part of Subprocess that invokes posix_spawn (and
   related functions) by calling that method; the rest of Subprocess's
   machinery to manage pipes and I/O is unchanged. (The Windows backend
   is also unchanged; I'm not aware of any functional issues there.)
   This results in some dead code, which is removed.

https://hg.mozilla.org/mozilla-central/rev/6be756552189