Closed Bug 1259852 Opened 4 years ago Closed 2 years ago

Deduplicate Linux/BSD/Mac environment handling in IPC LaunchApp

Categories

(Core :: IPC, defect, P2)

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox48 --- affected
firefox58 --- fixed

People

(Reporter: jbeich, Assigned: jld)

References

Details

(Whiteboard: btpp-backlog)

Attachments

(1 file)

(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.
Depends on: 1233275
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.
Whiteboard: btpp-backlog
Priority: -- → P3
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.
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.
Summary: Migrate ipc/chromium/src/base/process_util_linux.cc to posix_spawn() → Deduplicate Linux/BSD/Mac environment handling in IPC LaunchApp
Assignee: nobody → jld
Priority: P3 → P2
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).
Attachment #8915386 - Flags: review?(jbeich)
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.
Attachment #8915386 - Flags: review?(wmccloskey) → review+
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.
Attachment #8915386 - Flags: feedback+
(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.
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0567ed7f6074
Merge Linux/BSD/Mac child process environment handling. r=billm f=jbeich
https://hg.mozilla.org/mozilla-central/rev/0567ed7f6074
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.