Remove flag --test-sandbox

Remove flag --test-sandbox

And associated code.

BUG=352392
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274846

[jam, 2014-05-15 19:54:28]

can you add mac/linux sandbox owners (off the top of my head, rsesek and jln) to
sign off on the changes to renderer_main_platform_delegate_mac.mm and
renderer_main_platform_delegate_linux.cc? i.e. i don't know if they're depending
on these files.

also, remove security_tests from gyp, its source files, and its references in
.isolate

[jam, 2014-05-15 19:54:58]

+rsesek,jln

[Robert Sesek, 2014-05-15 20:00:26]

There's more code for the Mac part of this removal, but you should check with
Jeremy about deleting it:

https://code.google.com/p/chromium/codesearch#search/&q=RendererSandboxTestsR...

[jeremy, 2014-05-18 11:26:53]

I'm fine removing these, since the real Mac sandbox tests are in
content/common/sandbox_mac_unittest_helper.mm and friends.

As Robert mentioend there are a bunch of other files for the Mac implementation
- could you please clean those up too -
chrome/test/security_tests/renderer_sandbox_tests_mac.mm and associated files.

[luken, 2014-05-19 17:29:59]

removed security_tests directory, removed references in .gyp and .isolate files.
PTAL, thanks.

[jln (very slow on Chromium), 2014-05-20 02:29:10]

https://codereview.chromium.org/285283003/diff/60001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_linux.cc (left):

https://codereview.chromium.org/285283003/diff/60001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_linux.cc:57: void
RendererMainPlatformDelegate::RunSandboxTests(bool no_sandbox) {
I really would like to keep this sanity check that the sandbox is working as
expected. The logic is really complicated and error-prone. Moreover, on Linux
we're dealing with a variety of kernels, libc versions etc that make life
harder.

Having a sanity check of the real status of the sandbox right before a renderer
starts is great.

For now, we could perhaps simply add this at the end of EnableSandbox() instead
(no need for |no_sandbox|)? I can then move it to InitializeSandbox() soon.

Are we sure we don't want to keep something similar on all platforms? Unit tests
will never have the same safety. This tests that someone didn't comment out
platform.EnableSandbox() by mistake as well.
How does Windows run sandboxing tests within a real renderer now? I have added a
comment to the bug as well.

[jln (very slow on Chromium), 2014-05-22 21:04:54]

Ohh and I had missed that you were deleting
chrome/test/security_tests/sandbox_browsertest_linux.cc

Let's keep this around please. If it's not longer being run on bots I should fix
this instead!

https://codereview.chromium.org/285283003/diff/60001/chrome/test/security_tes...
File chrome/test/security_tests/sandbox_browsertest_linux.cc (left):

https://codereview.chromium.org/285283003/diff/60001/chrome/test/security_tes...
chrome/test/security_tests/sandbox_browsertest_linux.cc:21:
IN_PROC_BROWSER_TEST_F(SandboxLinuxTest, MAYBE_SandboxStatus) {
Let's keep this around please.

[luken, 2014-05-28 19:19:39]

https://codereview.chromium.org/285283003/diff/60001/chrome/test/security_tes...
File chrome/test/security_tests/sandbox_browsertest_linux.cc (left):

https://codereview.chromium.org/285283003/diff/60001/chrome/test/security_tes...
chrome/test/security_tests/sandbox_browsertest_linux.cc:21:
IN_PROC_BROWSER_TEST_F(SandboxLinuxTest, MAYBE_SandboxStatus) {
On 2014/05/22 21:04:54, jln wrote:
> Let's keep this around please.
Can we move it to somewhere other than security_tests/? Otherwise this will the
the sole remaining file in that directory.

https://codereview.chromium.org/285283003/diff/60001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_linux.cc (left):

https://codereview.chromium.org/285283003/diff/60001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_linux.cc:57: void
RendererMainPlatformDelegate::RunSandboxTests(bool no_sandbox) {
On 2014/05/20 02:29:11, jln wrote:
> I really would like to keep this sanity check that the sandbox is working as
> expected. The logic is really complicated and error-prone. Moreover, on Linux
> we're dealing with a variety of kernels, libc versions etc that make life
> harder.
> 
> Having a sanity check of the real status of the sandbox right before a
renderer
> starts is great.
> 
> For now, we could perhaps simply add this at the end of EnableSandbox()
instead
> (no need for |no_sandbox|)? I can then move it to InitializeSandbox() soon.
> 
> Are we sure we don't want to keep something similar on all platforms? Unit
tests
> will never have the same safety. This tests that someone didn't comment out
> platform.EnableSandbox() by mistake as well.
> How does Windows run sandboxing tests within a real renderer now? I have added
a
> comment to the bug as well.

Done.

[luken, 2014-05-30 17:30:46]

Ping, PTAL, thanks!

[Robert Sesek, 2014-05-30 17:36:47]

LGTM

https://codereview.chromium.org/285283003/diff/80001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_mac.mm (right):

https://codereview.chromium.org/285283003/diff/80001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_mac.mm:18: #import
"content/public/common/injection_test_mac.h"
Remove.

[luken, 2014-05-30 17:43:21]

jln, can you PTAL too? Thanks!

https://codereview.chromium.org/285283003/diff/80001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_mac.mm (right):

https://codereview.chromium.org/285283003/diff/80001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_mac.mm:18: #import
"content/public/common/injection_test_mac.h"
On 2014/05/30 17:36:48, rsesek wrote:
> Remove.

Done.

[jam, 2014-06-02 06:45:21]

lgtm

[jln (very slow on Chromium), 2014-06-04 00:09:18]

As I mentioned before, I'm sad to see
chrome/test/security_tests/sandbox_browsertest_linux.cc go, as we wanted to add
more in-browser and in-renderer tests, not less.

This lgtm though, I can simply re-add this once we need it, in a new test
binary.

[luken, 2014-06-04 03:47:47]

The CQ bit was checked by luken@chromium.org

[commit-bot: I haz the power, 2014-06-04 03:48:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/luken@chromium.org/285283003/100001

[commit-bot: I haz the power, 2014-06-04 04:42:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-06-04 04:44:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 04:44:32]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[luken, 2014-06-04 16:38:26]

The CQ bit was checked by luken@chromium.org

[commit-bot: I haz the power, 2014-06-04 16:39:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/luken@chromium.org/285283003/100001

[commit-bot: I haz the power, 2014-06-04 16:42:06]

Change committed as 274846