1400061 - Remove use of base::SetAllFDsToCloseOnExec from process_util_mac.mm

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

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

This is the other half of bug 1400042: removing the inherently racy base::SetAllFDsToCloseOnExec from how we launch child processes on Mac.

I suggested switching the other BSDs to fork/exec, but on OS X there's Mach weirdness we might run into (see https://crbug.com/179923), and there's also the cross-architecture NPAPI plugin support from bug 590057 and bug 592951 which I don't know if we still need.

(Also, fork() is inherently expensive and scales linearly with the forking process's virtual memory size; see https://crbug.com/179923#c35.  If posix_spawn can work, it's probably faster.)

Fortunately, Apple has a nonstandard extension, POSIX_SPAWN_CLOEXEC_DEFAULT, which apparently does exactly what we want.  https://crbug.com/179923#c31 says that “it 1) doesn't exist on 10.6 and 2) causes kernel panics on 10.7”, but according to https://support.mozilla.org/en-US/kb/firefox-osx our oldest supported version is 10.9.  Chromium seems to use it by default now.

So it might be enough to just throw in a posix_spawnattr_setflags and remove the SetAllFDsToCloseOnExec.

Comment 1

[(not currently active) Ted Mielczarek]

> and there's also the cross-architecture NPAPI plugin support from bug 590057 and bug 592951 which I don't know if we still need.

We dropped support for 32-bit OS X, so we don't need that anymore.

Comment 2

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

I was already poking at process_util_mac.mm for bug 1401790, and it was easier to fix this than avoid fixing it.

Comment 3

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

Attachment: Bug 1400061 - Stop using SetAllFDsToCloseOnExec when launching processes on OS X.

As its original comments indicate, SetAllFDsToCloseOnExec has an
unavoidable race condition if another thread creates file descriptors
during launch.  Instead, use POSIX_SPAWN_CLOEXEC_DEFAULT, which is an
Apple-specific extension to posix_spawn that accomplished the desired
effect atomically.

This patch also introduces some RAII to simplify cleanup in error cases.

Review commit: https://reviewboard.mozilla.org/r/188546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188546/

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8917596 [details]
Bug 1400061 - Stop using SetAllFDsToCloseOnExec when launching processes on OS X.

https://reviewboard.mozilla.org/r/188546/#review195698

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fc1873ffe07
Stop using SetAllFDsToCloseOnExec when launching processes on OS X. r=billm

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7fc1873ffe07