win: Add WinMultiprocess for multiprocess Windows tests

win: Add WinMultiprocess for multiprocess Windows tests

As a prerequisite for https://codereview.chromium.org/1138923004/.

R=cpu@chromium.org, rsesek@chromium.org
TBR=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/44727e9c790ba51baeac9ac45f67ef6ddb6d61a5

[scottmg, 2015-05-21 18:58:43]

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

[scottmg, 2015-05-21 18:59:10]

Patchset #1 (id:1) has been deleted

[scottmg, 2015-05-21 19:08:04]

scottmg@chromium.org changed reviewers:
+ cpu@chromium.org

[scottmg, 2015-05-21 19:08:05]

I went back and forth a bit on the inheritance here (note that it doesn't derive
from Multiprocess or MultiprocessExec). In the end I decided this was simplest
because of the fundamental difference between fork() and how Windows creates
processes. The fine-grained division of labor of the parent classes doesn't make
any sense on Windows. The interface is the same insofar as it makes sense.

The previous Windows version of MultiprocessExec still exists and is used by
HTTPTransportTest. I think that's fine as-is for now, but given that that's the
only test that uses it, perhaps we could eventually switch it to just use
MachMultiprocess/WinMultiprocess and have the python subserver be a detail of
that test.

[cpu_(ooo_6.6-7.5), 2015-05-22 16:15:23]

lgtm with one issue

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:36: if (!args) {
needs a localfree(args)

alternatively we can do static num_args and args and keep that memory till the
end of time, that would be ok specially if we are calling GetSwitch a lot.

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:69: sscanf(switch_value.c_str(), "%x|%x",
&c2p_write, &p2c_read);
the way brettw made me do these things is with SplitString (base), I won't  do
that but I just mention it just in case.

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:104: std::wstring command_line;
104 can be in 107

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:124: &command_line[0],  // This cannot be constant,
per MSDN.
it will in some cases write a 0 to separate things.

[scottmg, 2015-05-25 19:04:57]

Thanks.

Robert, could you take a look too?

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:36: if (!args) {
On 2015/05/22 16:15:23, cpu wrote:
> needs a localfree(args)
> 
> alternatively we can do static num_args and args and keep that memory till the
> end of time, that would be ok specially if we are calling GetSwitch a lot.

Since this is test code only, I will stick to freeing it properly rather than
static. Done.

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:69: sscanf(switch_value.c_str(), "%x|%x",
&c2p_write, &p2c_read);
On 2015/05/22 16:15:23, cpu wrote:
> the way brettw made me do these things is with SplitString (base), I won't  do
> that but I just mention it just in case.

Probably better, done. (I thought I originally I would need more things, but
with only two split is better than scanf.)

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:104: std::wstring command_line;
On 2015/05/22 16:15:23, cpu wrote:
> 104 can be in 107

Done.

https://codereview.chromium.org/1151953002/diff/60001/test/win/win_multiproce...
test/win/win_multiprocess.cc:124: &command_line[0],  // This cannot be constant,
per MSDN.
On 2015/05/22 16:15:23, cpu wrote:
> it will in some cases write a 0 to separate things.

Acknowledged.

[Robert Sesek, 2015-05-26 15:48:13]

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
test/win/win_multiprocess.cc:96: exit(0);
Is it safe to use exit() and call exit-time-destructors, or should you call the
equivalent of _exit() ?

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
test/win/win_multiprocess.cc:163: }
In the POSIX version, Multiprocess then waits for the child to exit and it
collects the status code.

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
File test/win/win_multiprocess.h (right):

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
test/win/win_multiprocess.h:91: //! the child process to exit, as this is
handled by this class.
Is it?

[scottmg, 2015-05-26 18:44:29]

Thanks!

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
test/win/win_multiprocess.cc:96: exit(0);
On 2015/05/26 15:48:13, Robert Sesek wrote:
> Is it safe to use exit() and call exit-time-destructors, or should you call
the
> equivalent of _exit() ?

This will be a completely separate unrelated process on Windows, so I think it
makes sense to exit normally.

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
test/win/win_multiprocess.cc:163: }
On 2015/05/26 15:48:13, Robert Sesek wrote:
> In the POSIX version, Multiprocess then waits for the child to exit and it
> collects the status code.

Done, and added expectation+test for non-zero exit code.

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
File test/win/win_multiprocess.h (right):

https://codereview.chromium.org/1151953002/diff/70001/test/win/win_multiproce...
test/win/win_multiprocess.h:91: //! the child process to exit, as this is
handled by this class.
On 2015/05/26 15:48:13, Robert Sesek wrote:
> Is it?

Done.

[erikwright (departed), 2015-05-26 19:21:57]

erikwright@chromium.org changed reviewers:
+ erikwright@chromium.org

[erikwright (departed), 2015-05-26 19:21:58]

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
test/win/win_multiprocess.cc:157: // Block until the child process has launched.
CreateProcess() returns
What can you do at this point that you couldn't do before reading this byte? You
have a process handle already. You can wait on it already. All I can see is that
you have shortened the window for a potential race condition.

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
File test/win/win_multiprocess.h (right):

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
test/win/win_multiprocess.h:97: //! This method must not use
`WaitForSingleObject()`-family call to wait for
Why _must_ this method not do this?

[scottmg, 2015-05-26 21:04:37]

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
test/win/win_multiprocess.cc:157: // Block until the child process has launched.
CreateProcess() returns
On 2015/05/26 19:21:58, erikwright wrote:
> What can you do at this point that you couldn't do before reading this byte?
You
> have a process handle already. You can wait on it already. All I can see is
that
> you have shortened the window for a potential race condition.

In the tests (the snapshot ones in the next CL) things like OpenProcess were
failing because CreateProcess returns immediately, before the Loader has
actually loaded the process.

We could say that the parent needs to take care of this (by waiting on the
process first or some other ipc), but in this test setup it seems like the
intention is to have the two sides up and ready to go, so it seems awkward to
require that in every test.

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
File test/win/win_multiprocess.h (right):

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
test/win/win_multiprocess.h:97: //! This method must not use
`WaitForSingleObject()`-family call to wait for
On 2015/05/26 19:21:58, erikwright wrote:
> Why _must_ this method not do this?

Good point, it could if it wanted to for some reason. Updated comment.

[erikwright (departed), 2015-05-27 17:03:55]

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
test/win/win_multiprocess.cc:157: // Block until the child process has launched.
CreateProcess() returns
On 2015/05/26 21:04:37, scottmg wrote:
> On 2015/05/26 19:21:58, erikwright wrote:
> > What can you do at this point that you couldn't do before reading this byte?
> You
> > have a process handle already. You can wait on it already. All I can see is
> that
> > you have shortened the window for a potential race condition.
> 
> In the tests (the snapshot ones in the next CL) things like OpenProcess were
> failing because CreateProcess returns immediately, before the Loader has
> actually loaded the process.
> 
> We could say that the parent needs to take care of this (by waiting on the
> process first or some other ipc), but in this test setup it seems like the
> intention is to have the two sides up and ready to go, so it seems awkward to
> require that in every test.

Why do you need to call OpenProcess? Is it because the permissions on
child_handle_ are insufficient?

Or is it just because the code under test expects a PID and opens its own
handle?

[scottmg, 2015-05-27 17:28:54]

On 2015/05/27 17:03:55, erikwright wrote:
>
https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
> File test/win/win_multiprocess.cc (right):
> 
>
https://codereview.chromium.org/1151953002/diff/90001/test/win/win_multiproce...
> test/win/win_multiprocess.cc:157: // Block until the child process has
launched.
> CreateProcess() returns
> On 2015/05/26 21:04:37, scottmg wrote:
> > On 2015/05/26 19:21:58, erikwright wrote:
> > > What can you do at this point that you couldn't do before reading this
byte?
> > You
> > > have a process handle already. You can wait on it already. All I can see
is
> > that
> > > you have shortened the window for a potential race condition.
> > 
> > In the tests (the snapshot ones in the next CL) things like OpenProcess were
> > failing because CreateProcess returns immediately, before the Loader has
> > actually loaded the process.
> > 
> > We could say that the parent needs to take care of this (by waiting on the
> > process first or some other ipc), but in this test setup it seems like the
> > intention is to have the two sides up and ready to go, so it seems awkward
to
> > require that in every test.
> 
> Why do you need to call OpenProcess? Is it because the permissions on
> child_handle_ are insufficient?
> 
> Or is it just because the code under test expects a PID and opens its own
> handle?

It's not just OpenProcess, the tests need to e.g. ReadProcessMemory, see
https://codereview.chromium.org/1138923004/. In general it just seems like the
child should already be ready for access by the time the parent code runs, which
is the goal of the synchronization in the test harness.

[scottmg, 2015-05-27 21:52:54]

Anyone have any further concerns? I'd like to get the snapshot stuff done.

[Robert Sesek, 2015-05-28 15:27:58]

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
test/win/win_multiprocess.cc:76: pipe_p2c_write_(),
child_handle_(),

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
test/win/win_multiprocess.cc:176: if (exit_code != exit_code_)
Why not ASSERT_EQ(exit_code_, exit_code) ?

The POSIX impl doesn't do that because it has to parse out the waitpid status to
differentiate between signaled and exited.

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
File test/win/win_multiprocess_test.cc (right):

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
test/win/win_multiprocess_test.cc:26: TestWinMultiprocess(unsigned int
exit_code)
explicit

[scottmg, 2015-05-28 15:38:19]

thanks

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
File test/win/win_multiprocess.cc (right):

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
test/win/win_multiprocess.cc:76: pipe_p2c_write_(),
On 2015/05/28 15:27:57, Robert Sesek wrote:
> child_handle_(),

Done.

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
test/win/win_multiprocess.cc:176: if (exit_code != exit_code_)
On 2015/05/28 15:27:57, Robert Sesek wrote:
> Why not ASSERT_EQ(exit_code_, exit_code) ?
> 
> The POSIX impl doesn't do that because it has to parse out the waitpid status
to
> differentiate between signaled and exited.

Done.

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
File test/win/win_multiprocess_test.cc (right):

https://codereview.chromium.org/1151953002/diff/110001/test/win/win_multiproc...
test/win/win_multiprocess_test.cc:26: TestWinMultiprocess(unsigned int
exit_code)
On 2015/05/28 15:27:57, Robert Sesek wrote:
> explicit

Done.

[Robert Sesek, 2015-05-28 15:59:00]

LGTM

[scottmg, 2015-05-28 16:04:30]

Committed patchset #8 (id:150001) manually as
44727e9c790ba51baeac9ac45f67ef6ddb6d61a5 (presubmit successful).

[erikwright (departed), 2015-06-01 15:35:17]

https://codereview.chromium.org/1151953002/diff/150001/test/win/win_multiproc...
File test/win/win_multiprocess.h (right):

https://codereview.chromium.org/1151953002/diff/150001/test/win/win_multiproc...
test/win/win_multiprocess.h:105: //! Test failures should be reported via gtest:
`EXPECT_*()`, `ASSERT_*()`,
Is this true? What happens if a child process records a failed assertion or
expectation?

[scottmg, 2015-06-01 17:28:44]

On 2015/06/01 15:35:17, erikwright wrote:
>
https://codereview.chromium.org/1151953002/diff/150001/test/win/win_multiproc...
> File test/win/win_multiprocess.h (right):
> 
>
https://codereview.chromium.org/1151953002/diff/150001/test/win/win_multiproc...
> test/win/win_multiprocess.h:105: //! Test failures should be reported via
gtest:
> `EXPECT_*()`, `ASSERT_*()`,
> Is this true? What happens if a child process records a failed assertion or
> expectation?

Oh, you're right! I vaguely assumed the macros would exit before returning, but
clearly not. Fix and test here: https://codereview.chromium.org/1164683005