Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(124)

Issue 831373002: Move ForkWithFlags from sandbox/ to base/ and plug it into LaunchProcess. (Closed)

Created:
5 years, 11 months ago by rickyz (no longer on Chrome)
Modified:
5 years, 11 months ago
CC:
chromium-reviews, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, jln+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Respond to comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+179 lines, -130 lines) Patch
M base/process/launch.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M base/process/launch_posix.cc View 1 chunk +11 lines, -1 line 0 comments Download
M base/process/process.h View 1 1 chunk +19 lines, -0 lines 0 comments Download
M base/process/process_linux.cc View 3 chunks +92 lines, -0 lines 0 comments Download
M base/process/process_unittest.cc View 1 2 chunks +45 lines, -0 lines 0 comments Download
M base/process/process_util_unittest.cc View 1 chunk +2 lines, -4 lines 0 comments Download
M content/zygote/zygote_linux.cc View 1 chunk +1 line, -1 line 0 comments Download
M sandbox/linux/services/syscall_wrappers.h View 1 chunk +0 lines, -12 lines 0 comments Download
M sandbox/linux/services/syscall_wrappers.cc View 3 chunks +6 lines, -83 lines 0 comments Download
M sandbox/linux/services/syscall_wrappers_unittest.cc View 1 chunk +0 lines, -29 lines 0 comments Download

Messages

Total messages: 19 (6 generated)
rickyz (no longer on Chrome)
5 years, 11 months ago (2015-01-05 22:20:16 UTC) #2
rickyz (no longer on Chrome)
5 years, 11 months ago (2015-01-05 22:45:41 UTC) #4
mdempsky
lgtm
5 years, 11 months ago (2015-01-05 22:57:04 UTC) #5
jln (very slow on Chromium)
Please explain in the message that this is mostly code moving from sandbox/ to base/ ...
5 years, 11 months ago (2015-01-06 00:22:31 UTC) #6
rickyz (no longer on Chrome)
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#newcode119 base/process/launch.h:119: // multiple threads are running. On 2015/01/06 00:22:31, jln ...
5 years, 11 months ago (2015-01-06 02:45:37 UTC) #7
rickyz (no longer on Chrome)
Hi, Julien suggested that you'd be a good reviewer for some base/process changes - mind ...
5 years, 11 months ago (2015-01-06 22:58:15 UTC) #10
rickyz (no longer on Chrome)
(whoever gets to it first)
5 years, 11 months ago (2015-01-06 23:14:58 UTC) #12
jln (very slow on Chromium)
lgtm Mark: Lei is swamped, so if you have a minute to look, that would ...
5 years, 11 months ago (2015-01-07 00:42:36 UTC) #13
Mark Mentovai
LGTM
5 years, 11 months ago (2015-01-07 18:01:01 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/831373002/20001
5 years, 11 months ago (2015-01-07 18:55:13 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 11 months ago (2015-01-07 19:10:33 UTC) #17
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/e12d6652ece2a3ab72bb05837cfd7f0b0b9ecf3a Cr-Commit-Position: refs/heads/master@{#310327}
5 years, 11 months ago (2015-01-07 19:13:07 UTC) #18
Marijn Kruisselbrink
5 years, 11 months ago (2015-01-07 20:30:04 UTC) #19
Message was sent while issue was closed.
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).

Powered by Google App Engine
This is Rietveld 408576698