Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931173003:
Implement the V2 sandbox in the process launcher. (Closed)
DescriptionImplement the V2 sandbox in the process launcher.
Implements the V2 sandbox in the process launcher, passing the
parameters and flags to Chrome Helper executable. The V2 sandbox is
currently a disabled by default feature.
BUG=689306
CQ-DEPEND=2916323004
Review-Url: https://codereview.chromium.org/2931173003
Cr-Commit-Position: refs/heads/master@{#482462}
Committed: https://chromium.googlesource.com/chromium/src/+/0f7a19296f354d7ed937a0d161c08638f542d919
Patch Set 1 #
Total comments: 6
Patch Set 2 : Re-organize into sandbox_parameters_mac.mm #Patch Set 3 : Fix deps #
Total comments: 4
Patch Set 4 : Add header guard #Patch Set 5 : Fix deps and review comments #Patch Set 6 : Fix name clash #Patch Set 7 : Fix name clash #Patch Set 8 : Fix command line setup #
Total comments: 2
Patch Set 9 : Implement the V2 sandbox in the process launcher. #
Total comments: 1
Patch Set 10 : Add comment explaining V2 sandbox #
Messages
Total messages: 64 (45 generated)
kerrnel@chromium.org changed reviewers: + rsesek@chromium.org
PTAL. Thanks, Greg
https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h File base/mac/bundle_locations.h (right): https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h... base/mac/bundle_locations.h:54: // Return the bundle identifier of the outer bundle. I'm not sure this addition is warranted. https://codereview.chromium.org/2931173003/diff/1/content/browser/child_proce... File content/browser/child_process_launcher_helper_mac.cc (right): https://codereview.chromium.org/2931173003/diff/1/content/browser/child_proce... content/browser/child_process_launcher_helper_mac.cc:65: base::Feature kV2SandboxFeature{"V2SandboxFeature", This should go in content_features.cc/h and needs a better name (e.g., "MacSandboxV2").
https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h File base/mac/bundle_locations.h (right): https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h... base/mac/bundle_locations.h:54: // Return the bundle identifier of the outer bundle. On 2017/06/12 21:13:54, Robert Sesek wrote: > I'm not sure this addition is warranted. I put this in there so .cc files can get the bundle identifier, which otherwise comes as NSString from an NSBundle. Do you prefer a different solution?
https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h File base/mac/bundle_locations.h (right): https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h... base/mac/bundle_locations.h:54: // Return the bundle identifier of the outer bundle. On 2017/06/12 21:15:51, Greg K wrote: > On 2017/06/12 21:13:54, Robert Sesek wrote: > > I'm not sure this addition is warranted. > > I put this in there so .cc files can get the bundle identifier, which otherwise > comes as NSString from an NSBundle. Do you prefer a different solution? Rename the file to .mm. We don't add things to base that have one caller.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h File base/mac/bundle_locations.h (right): https://codereview.chromium.org/2931173003/diff/1/base/mac/bundle_locations.h... base/mac/bundle_locations.h:54: // Return the bundle identifier of the outer bundle. On 2017/06/12 21:18:07, Robert Sesek wrote: > On 2017/06/12 21:15:51, Greg K wrote: > > On 2017/06/12 21:13:54, Robert Sesek wrote: > > > I'm not sure this addition is warranted. > > > > I put this in there so .cc files can get the bundle identifier, which > otherwise > > comes as NSString from an NSBundle. Do you prefer a different solution? > > Rename the file to .mm. We don't add things to base that have one caller. Done. https://codereview.chromium.org/2931173003/diff/1/content/browser/child_proce... File content/browser/child_process_launcher_helper_mac.cc (right): https://codereview.chromium.org/2931173003/diff/1/content/browser/child_proce... content/browser/child_process_launcher_helper_mac.cc:65: base::Feature kV2SandboxFeature{"V2SandboxFeature", On 2017/06/12 21:13:54, Robert Sesek wrote: > This should go in content_features.cc/h and needs a better name (e.g., > "MacSandboxV2"). Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Nice! https://codereview.chromium.org/2931173003/diff/40001/content/browser/child_p... File content/browser/child_process_launcher_helper_mac.cc (right): https://codereview.chromium.org/2931173003/diff/40001/content/browser/child_p... content/browser/child_process_launcher_helper_mac.cc:81: // base::CommandLine::AppendArguments messes up the arguments. How so? Is this a bug? https://codereview.chromium.org/2931173003/diff/40001/content/browser/sandbox... File content/browser/sandbox_parameters_mac.h (right): https://codereview.chromium.org/2931173003/diff/40001/content/browser/sandbox... content/browser/sandbox_parameters_mac.h:4: Need #include guards.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2931173003/diff/40001/content/browser/child_p... File content/browser/child_process_launcher_helper_mac.cc (right): https://codereview.chromium.org/2931173003/diff/40001/content/browser/child_p... content/browser/child_process_launcher_helper_mac.cc:81: // base::CommandLine::AppendArguments messes up the arguments. On 2017/06/13 19:05:08, Robert Sesek wrote: > How so? Is this a bug? This was actually a problem in the original CL that seems to work fine in the current CL, so I'll just drop the whole custom block. https://codereview.chromium.org/2931173003/diff/40001/content/browser/sandbox... File content/browser/sandbox_parameters_mac.h (right): https://codereview.chromium.org/2931173003/diff/40001/content/browser/sandbox... content/browser/sandbox_parameters_mac.h:4: On 2017/06/13 19:05:08, Robert Sesek wrote: > Need #include guards. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM! https://codereview.chromium.org/2931173003/diff/140001/content/browser/sandbo... File content/browser/sandbox_parameters_mac.h (right): https://codereview.chromium.org/2931173003/diff/140001/content/browser/sandbo... content/browser/sandbox_parameters_mac.h:6: #define CONTENT_BROWSER_SANDBOX_PARAMETERS_MAC_ nit: needs trailing _H_
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2931173003/diff/140001/content/browser/sandbo... File content/browser/sandbox_parameters_mac.h (right): https://codereview.chromium.org/2931173003/diff/140001/content/browser/sandbo... content/browser/sandbox_parameters_mac.h:6: #define CONTENT_BROWSER_SANDBOX_PARAMETERS_MAC_ On 2017/06/14 17:47:00, Robert Sesek wrote: > nit: needs trailing _H_ Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2931173003/#ps160001 (title: "Implement the V2 sandbox in the process launcher.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kerrnel@chromium.org changed reviewers: + avi@chromium.org, thakis@chromium.org
thakis@chromium.org: Please review changes in chrome/ avi@chromium.org: Please review changes in content/ Thanks, Greg
chrome/ rs-lgtm Avi's out until next week I think.
Description was changed from ========== Implement the V2 sandbox in the process launcher. Implements the V2 sandbox in the process launcher, passing the parameters and flags to Chrome Helper executable. The V2 sandbox is currently a disabled by default feature. BUG=689306 CQ-DEPEND=2916323004 ========== to ========== Implement the V2 sandbox in the process launcher. Implements the V2 sandbox in the process launcher, passing the parameters and flags to Chrome Helper executable. The V2 sandbox is currently a disabled by default feature. BUG=689306 CQ-DEPEND=2916323004 ==========
kerrnel@chromium.org changed reviewers: - avi@chromium.org
kerrnel@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in content/ Thanks, Greg
kerrnel@chromium.org changed reviewers: + creis@chromium.org - jochen@chromium.org
jochen is OOO, creis@ can you PTAL for OWNERS review of content/? Thanks, Greg
content/ LGTM with nit. https://codereview.chromium.org/2931173003/diff/160001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2931173003/diff/160001/content/public/common/... content/public/common/content_features.cc:383: const base::Feature kMacV2Sandbox{"MacV2Sandbox", Can you document a bit about the V2 sandbox (e.g., maybe something about what's new)?
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, creis@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2931173003/#ps180001 (title: "Add comment explaining V2 sandbox")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498520247934420,
"parent_rev": "80e7ad9bbb7e8d04e88b4793644c449b1c3d23e0", "commit_rev":
"0f7a19296f354d7ed937a0d161c08638f542d919"}
Message was sent while issue was closed.
Description was changed from ========== Implement the V2 sandbox in the process launcher. Implements the V2 sandbox in the process launcher, passing the parameters and flags to Chrome Helper executable. The V2 sandbox is currently a disabled by default feature. BUG=689306 CQ-DEPEND=2916323004 ========== to ========== Implement the V2 sandbox in the process launcher. Implements the V2 sandbox in the process launcher, passing the parameters and flags to Chrome Helper executable. The V2 sandbox is currently a disabled by default feature. BUG=689306 CQ-DEPEND=2916323004 Review-Url: https://codereview.chromium.org/2931173003 Cr-Commit-Position: refs/heads/master@{#482462} Committed: https://chromium.googlesource.com/chromium/src/+/0f7a19296f354d7ed937a0d161c0... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/0f7a19296f354d7ed937a0d161c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
