Make second param for TestChildProcess optional.

Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
TEST=sbox_integration_tests on Windows 10.0.10586

Committed: https://crrev.com/f8a5a575c7e5109a353dcd71240c53fa484537aa
Cr-Commit-Position: refs/heads/master@{#384671}

[Will Harris, 2016-03-31 21:21:50]

Description was changed from

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
==========

to

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
==========

[Will Harris, 2016-03-31 21:21:51]

wfh@chromium.org changed reviewers:
+ pennymac@chromium.org

[Will Harris, 2016-03-31 21:22:11]

Description was changed from

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
==========

to

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
TEST=sbox_integration_tests on Windows 10.0.10586
==========

[Will Harris, 2016-03-31 21:23:14]

PTAL

[penny, 2016-04-01 17:20:31]

Hi Will,

Sorry, I've added some excessive input - I never really looked at the first fix
CL you landed.

Thanks for fixing the job message bug, and adding new tests (I'm a big fan of
that)!

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:208: // with a given command line.
The second optional parameter, if set to non-zero
<whisper>comma after 'non-zero'?</whisper>

:)

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:717: TEST(ProcessMitigationsTest,
CheckChildProcessSuccessAbnormalExit) {
In terms of process_mitigations_test.cc, this new test appears to behave exactly
the same as CheckChildProcessSuccess - they both result in an
SBOX_TEST_SUCCEEDED return from TestChildProcess.

I think I know that this test is actually meant to test the job messages Chrome
receives from the OS (ref crbug/584753), and that a "failure case" would show up
as a DCHECK in broker_services.cc, TargetEventsThread() (where we handle job
messages).

I'd recommend adding a detailed header & comment above this test, explaining
what it's testing, and point to the code where the DCHECK would happen in a
failure case (or link to the bug I mentioned above).  It would make it a lot
easier to understand what this test is actually doing, and why.

This is not really a test of our mitigations that prevent child process
creation.  It's a great new test to ensure we're handling job messages properly
- messages that we now know differ between versions of Windows.  In some ways,
this test belongs with any other Job-related tests (if they exist), but sbox
test environment is uniquely excellent for this test case, so I'm happy with
leaving it here (with a detailed comment).

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:741: TEST(ProcessMitigationsTest,
CheckChildProcessFailureAbnormalExit) {
I'm fairly sure this new test has the exact same run-time flow as
CheckChildProcessFailure.  There is no abnormal exit involved here, as the
actual launch of the process in TestChildProcess function fails due to our
sandbox settings (JOB level).  You don't get to terminate the child process with
any abnormal failure, because it never got launched.  So these two process
failure tests are identical.  I'd recommend removing this one.

[Will Harris, 2016-04-01 18:23:52]

PTAL

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:208: // with a given command line.
The second optional parameter, if set to non-zero
On 2016/04/01 17:20:30, penny wrote:
> <whisper>comma after 'non-zero'?</whisper>
> 
> :)

actually it had a comma but it went over 80chars so I removed it. sigh. I'll
just add a new line... actually I'll just change the comment. and the code.

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:717: TEST(ProcessMitigationsTest,
CheckChildProcessSuccessAbnormalExit) {
On 2016/04/01 17:20:30, penny wrote:
> In terms of process_mitigations_test.cc, this new test appears to behave
exactly
> the same as CheckChildProcessSuccess - they both result in an
> SBOX_TEST_SUCCEEDED return from TestChildProcess.
> 
> I think I know that this test is actually meant to test the job messages
Chrome
> receives from the OS (ref crbug/584753), and that a "failure case" would show
up
> as a DCHECK in broker_services.cc, TargetEventsThread() (where we handle job
> messages).
> 
> I'd recommend adding a detailed header & comment above this test, explaining
> what it's testing, and point to the code where the DCHECK would happen in a
> failure case (or link to the bug I mentioned above).  It would make it a lot
> easier to understand what this test is actually doing, and why.
> 
> This is not really a test of our mitigations that prevent child process
> creation.  It's a great new test to ensure we're handling job messages
properly
> - messages that we now know differ between versions of Windows.  In some ways,
> this test belongs with any other Job-related tests (if they exist), but sbox
> test environment is uniquely excellent for this test case, so I'm happy with
> leaving it here (with a detailed comment).

yes good point this should maybe go into job tests but those aren't integration
tests (i.e. they don't spawn child processes) as they are in sbox_unittests, so
I think it's okay to leave them here.

Regarding comments, we don't usually link bugs in code because it's always easy
(in theory) to look at commit logs and commits will link to the bugs, otherwise
there would be crbug links everywhere in the code... I suppose that's just
chromium style.

I've added a more detailed comment.

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:741: TEST(ProcessMitigationsTest,
CheckChildProcessFailureAbnormalExit) {
On 2016/04/01 17:20:30, penny wrote:
> I'm fairly sure this new test has the exact same run-time flow as
> CheckChildProcessFailure.  There is no abnormal exit involved here, as the
> actual launch of the process in TestChildProcess function fails due to our
> sandbox settings (JOB level).  You don't get to terminate the child process
with
> any abnormal failure, because it never got launched.  So these two process
> failure tests are identical.  I'd recommend removing this one.

Done. Removed.

[penny, 2016-04-01 18:35:36]

LGTM

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:208: // with a given command line.
The second optional parameter, if set to non-zero
On 2016/04/01 18:23:52, Will Harris wrote:
> On 2016/04/01 17:20:30, penny wrote:
> > <whisper>comma after 'non-zero'?</whisper>
> > 
> > :)
> 
> actually it had a comma but it went over 80chars so I removed it. sigh. I'll
> just add a new line... actually I'll just change the comment. and the code.

ROFL

[Will Harris, 2016-04-01 18:53:45]

On 2016/04/01 18:35:36, penny wrote:
> LGTM
> 
>
https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
> File sandbox/win/src/process_mitigations_test.cc (right):
> 
>
https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
> sandbox/win/src/process_mitigations_test.cc:208: // with a given command line.
> The second optional parameter, if set to non-zero
> On 2016/04/01 18:23:52, Will Harris wrote:
> > On 2016/04/01 17:20:30, penny wrote:
> > > <whisper>comma after 'non-zero'?</whisper>
> > > 
> > > :)
> > 
> > actually it had a comma but it went over 80chars so I removed it. sigh. I'll
> > just add a new line... actually I'll just change the comment. and the code.
> 
> ROFL

Argh the commit I just lgtmed now means I have to rebase.  Arrghh

[Will Harris, 2016-04-01 19:14:37]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-04-01 19:14:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
pennymac@chromium.org
Link to the patchset: https://codereview.chromium.org/1847213002/#ps40001
(title: "rebase")

[Will Harris, 2016-04-01 19:15:01]

On 2016/04/01 18:53:45, Will Harris wrote:
> On 2016/04/01 18:35:36, penny wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
> > File sandbox/win/src/process_mitigations_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1847213002/diff/1/sandbox/win/src/process_mit...
> > sandbox/win/src/process_mitigations_test.cc:208: // with a given command
line.
> > The second optional parameter, if set to non-zero
> > On 2016/04/01 18:23:52, Will Harris wrote:
> > > On 2016/04/01 17:20:30, penny wrote:
> > > > <whisper>comma after 'non-zero'?</whisper>
> > > > 
> > > > :)
> > > 
> > > actually it had a comma but it went over 80chars so I removed it. sigh.
I'll
> > > just add a new line... actually I'll just change the comment. and the
code.
> > 
> > ROFL
> 
> Argh the commit I just lgtmed now means I have to rebase.  Arrghh

hmm didn't need a rebase, looks like a flake, committing it again.

[commit-bot: I haz the power, 2016-04-01 19:15:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847213002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847213002/40001

[commit-bot: I haz the power, 2016-04-01 20:06:23]

Description was changed from

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
TEST=sbox_integration_tests on Windows 10.0.10586
==========

to

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
TEST=sbox_integration_tests on Windows 10.0.10586
==========

[commit-bot: I haz the power, 2016-04-01 20:06:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-01 20:07:43]

Description was changed from

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
TEST=sbox_integration_tests on Windows 10.0.10586
==========

to

==========
Make second param for TestChildProcess optional.

This fixes a regression intoduced in crrev.com/1826223004 as the
TestChildProcess function is used by several other tests.

BUG=599545
TEST=sbox_integration_tests on Windows 10.0.10586

Committed: https://crrev.com/f8a5a575c7e5109a353dcd71240c53fa484537aa
Cr-Commit-Position: refs/heads/master@{#384671}
==========

[commit-bot: I haz the power, 2016-04-01 20:07:44]

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

[Will Harris, 2016-04-01 20:14:53]

On 2016/04/01 20:07:44, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/f8a5a575c7e5109a353dcd71240c53fa484537aa
> Cr-Commit-Position: refs/heads/master@{#384671}

also passed on 10586 -
https://chromium-swarm.appspot.com/user/task/2dec1a2c4d7f5510