1259852 - Deduplicate Linux/BSD/Mac environment handling in IPC LaunchApp

Status: RESOLVED FIXED

(Core :: IPC, defect, P2)

Description

[Jan Beich]

(In reply to Jed Davis [:jld] from comment bug 1233275 #0)
> Also, the process_util_bsd implementation of environment merging looks
> smaller and simpler than the process_util_linux one, so I'd suggest moving
> it into process_util_posix and using it in both.

process_util_linux.cc has lengthy B2G ifdefs which probably need to be split into a separate file.

Comment 1

[Andrew Overholt [:overholt]]

I'm putting backlog here but please don't let that affect how soon this gets done or let it be an indication of priority.

Comment 2

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

It's been a while since I made that comment, but I think I was just suggesting copying the code that handles environments, not switching completely over to posix_spawn.

But I've been poking at child process handling lately, so let's consider it anyway.

Linux doesn't have native posix_spawn, but glibc has an implementation that uses CLONE_VFORK.  It might be interesting to measure how it affects performance around child process startup; vfork blocks the entire parent process, but CoW'ing the entire address space isn't cheap either.

However, posix_spawn doesn't have a “close all other fds” feature, which is what base::LaunchApp is supposed to do.  OS X has the nonstandard extension POSIX_SPAWN_CLOEXEC_DEFAULT, at least on recent versions, which we're not using but upstream Chromium is (https://crbug.com/179923#c45); this is not supported by glibc.  Indeed, upstream Chromium seems to use posix_spawn only on OS X.  

What we *are* doing, in process_util_bsd.cc and process_util_mac.mm, has a race condition: another thread can create file descriptors after SetAllFDsToCloseOnExec and before the posix_spawn.  (Even if the other thread is trying to be well-behaved and set close-on-exec, it's not always possible to do that atomically without nonstandard features like pipe2() and SOCK_CLOEXEC, or standardized but relatively new features like F_DUPFD_CLOEXEC.)  See https://crbug.com/11174, but for Mac also see bug 592951.

Comment 3

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

I'm going to turn this bug back into what it was going to be, and file new bugs for the race conditions on the posix_spawn platforms.

Comment 4

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

Attachment: Bug 1259852 - Merge Linux/BSD/Mac child process environment handling. f=jbeich

This is mostly based on the BSD version, which in turn is more or less
the Mac version minus some race conditions.  The Linux version does
something similar, but more verbosely and (at least in my opinion) is
harder to follow.  Some changes have been made, mainly to use C++11
features like UniquePtr.

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

Comment 5

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

Comment on attachment 8915386 [details]
Bug 1259852 - Merge Linux/BSD/Mac child process environment handling.  f=jbeich

I could use some help testing the *BSD version of this.  I happen to have a FreeBSD 11.0 system that's probably large enough to build Firefox, which I was going to use to at least build-test this patch, but I ran into problems with the Rust dependency (see https://github.com/rust-lang/rust/issues/44433 for details).

Comment 6

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

Comment on attachment 8915386 [details]
Bug 1259852 - Merge Linux/BSD/Mac child process environment handling.  f=jbeich

https://reviewboard.mozilla.org/r/186576/#review193358

::: ipc/chromium/src/base/process_util_posix.cc:396
(Diff revision 1)
> +    entry += key_val.second;
> +    array[i] = strdup(entry.c_str());
> +    i++;
> +  }
> +  array[i] = nullptr;
> +  return mozilla::Move(array);

Is this Move necessary? My understanding is that a move will be used as long as copy elision doesn't happen. The Move will just prevent copy elision.

Comment 7

[Jan Beich]

Comment on attachment 8915386 [details]
Bug 1259852 - Merge Linux/BSD/Mac child process environment handling.  f=jbeich

With the patch applied both content and plugin processes still work fine on FreeBSD.

Comment 8

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

(In reply to Bill McCloskey (:billm) from comment #6)
> > +  return mozilla::Move(array);
> 
> Is this Move necessary? My understanding is that a move will be used as long
> as copy elision doesn't happen. The Move will just prevent copy elision.

Thanks for pointing that out.  I knew about the C++11 copy elision optimization and C++17's more Rust-like behavior (“guaranteed copy elision”), but I didn't know that C++11 would promote copies to moves like that.

Comment 9

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

Comment on attachment 8915386 [details]
Bug 1259852 - Merge Linux/BSD/Mac child process environment handling.  f=jbeich

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186576/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0567ed7f6074
Merge Linux/BSD/Mac child process environment handling. r=billm f=jbeich

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/0567ed7f6074