Move ForkWithFlags from sandbox/ to base/ and plug it into LaunchProcess.

Move ForkWithFlags from sandbox/ to base/ and plug it into LaunchProcess.

ForkWithFlags is a wrapper around the clone syscall that uses the libc
clone wrapper, and thus updates the libc's pid cache if it has one
(using sys_clone directly does not update the pid cache, so getpid may
return an incorrect result in the child). This exposes the ability to
set clone flags, which is needed to use Linux namespaces.

BUG=312380
Committed: https://crrev.com/e12d6652ece2a3ab72bb05837cfd7f0b0b9ecf3a
Cr-Commit-Position: refs/heads/master@{#310327}

[rickyz (no longer on Chrome), 2015-01-05 22:20:16]

rickyz@chromium.org changed reviewers:
+ jln@chromium.org

[rickyz (no longer on Chrome), 2015-01-05 22:45:41]

rickyz@chromium.org changed reviewers:
+ mdempsky@chromium.org

[mdempsky, 2015-01-05 22:57:04]

lgtm

[jln (very slow on Chromium), 2015-01-06 00:22:31]

Please explain in the message that this is mostly code moving from sandbox/ to
base/

https://chromiumcodereview.appspot.com/831373002/diff/1/base/process/launch.h
File base/process/launch.h (right):

https://chromiumcodereview.appspot.com/831373002/diff/1/base/process/launch.h...
base/process/launch.h:119: // multiple threads are running.
1. Explain that the clone flags may not include a signal handlers (and that it
will always be SIGCHLD.

2. This warning is not at the right place and is not correct: even with
clone_flags == 0, it's not safe to fork from a multithreaded environment. But
we're going through hoops to make that sure that LaunchProcess() is written with
care so that the fork() + execve() sequence is safe. I don't think a warning
belongs here.

https://chromiumcodereview.appspot.com/831373002/diff/1/base/process/process.h
File base/process/process.h (right):

https://chromiumcodereview.appspot.com/831373002/diff/1/base/process/process....
base/process/process.h:125: BASE_EXPORT pid_t ForkWithFlags(unsigned long flags,
pid_t* ptid, pid_t* ctid);
It may be useful to warn about the usual fork() issues, such as the fact that
forking with threads requires a lot of care (and typically doing an execve()
soon).

https://chromiumcodereview.appspot.com/831373002/diff/1/base/process/process_...
File base/process/process_unittest.cc (right):

https://chromiumcodereview.appspot.com/831373002/diff/1/base/process/process_...
base/process/process_unittest.cc:218: pid_t pid = syscall(__NR_getpid);
const

https://chromiumcodereview.appspot.com/831373002/diff/1/base/process/process_...
base/process/process_unittest.cc:244: int exit_code = kDummyExitCode;
Why not kFail?

[rickyz (no longer on Chrome), 2015-01-06 02:45:37]

https://codereview.chromium.org/831373002/diff/1/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/831373002/diff/1/base/process/launch.h#newcod...
base/process/launch.h:119: // multiple threads are running.
On 2015/01/06 00:22:31, jln wrote:
> 1. Explain that the clone flags may not include a signal handlers (and that it
> will always be SIGCHLD.
> 
> 2. This warning is not at the right place and is not correct: even with
> clone_flags == 0, it's not safe to fork from a multithreaded environment. But
> we're going through hoops to make that sure that LaunchProcess() is written
with
> care so that the fork() + execve() sequence is safe. I don't think a warning
> belongs here.

Done.

https://codereview.chromium.org/831373002/diff/1/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/831373002/diff/1/base/process/process.h#newco...
base/process/process.h:125: BASE_EXPORT pid_t ForkWithFlags(unsigned long flags,
pid_t* ptid, pid_t* ctid);
On 2015/01/06 00:22:31, jln wrote:
> It may be useful to warn about the usual fork() issues, such as the fact that
> forking with threads requires a lot of care (and typically doing an execve()
> soon).

Done.

https://codereview.chromium.org/831373002/diff/1/base/process/process_unittes...
File base/process/process_unittest.cc (right):

https://codereview.chromium.org/831373002/diff/1/base/process/process_unittes...
base/process/process_unittest.cc:218: pid_t pid = syscall(__NR_getpid);
On 2015/01/06 00:22:31, jln wrote:
> const

Done.

https://codereview.chromium.org/831373002/diff/1/base/process/process_unittes...
base/process/process_unittest.cc:244: int exit_code = kDummyExitCode;
On 2015/01/06 00:22:31, jln wrote:
> Why not kFail?

Done.

[rickyz (no longer on Chrome), 2015-01-06 21:25:30]

Patchset #3 (id:40001) has been deleted

[rickyz (no longer on Chrome), 2015-01-06 22:58:14]

rickyz@chromium.org changed reviewers:
+ thestig@chromium.org

[rickyz (no longer on Chrome), 2015-01-06 22:58:15]

Hi, Julien suggested that you'd be a good reviewer for some base/process changes
- mind taking a look?

[rickyz (no longer on Chrome), 2015-01-06 23:14:57]

rickyz@chromium.org changed reviewers:
+ mark@chromium.org

[rickyz (no longer on Chrome), 2015-01-06 23:14:58]

(whoever gets to it first)

[jln (very slow on Chromium), 2015-01-07 00:42:36]

lgtm

Mark: Lei is swamped, so if you have a minute to look, that would be great.

[Mark Mentovai, 2015-01-07 18:01:01]

LGTM

[rickyz (no longer on Chrome), 2015-01-07 18:54:06]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-01-07 18:55:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/831373002/20001

[commit-bot: I haz the power, 2015-01-07 19:10:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-07 19:13:07]

Patchset 2 (id:??) landed as
https://crrev.com/e12d6652ece2a3ab72bb05837cfd7f0b0b9ecf3a
Cr-Commit-Position: refs/heads/master@{#310327}

[Marijn Kruisselbrink, 2015-01-07 20:30:04]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/842703002/ by mek@chromium.org.

The reason for reverting is: PrcessTest.CloneWithFlags fails on Linux ChromiumOS
Ozone Tests (1):
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

ProcessTest.CloneFlags (run #1):
[ RUN      ] ProcessTest.CloneFlags
../../base/process/process_unittest.cc:241: Failure
Value of: process.IsValid()
  Actual: false
Expected: true
[  FAILED  ] ProcessTest.CloneFlags (0 ms).