Reorganize Multiprocess and implement for Windows

Reorganize Multiprocess and implement for Windows

- Various "FD" to "Handle"
- Existing Multiprocess implementation moves to _posix.
- Stub implementation for _win.

At the moment, multiprocess_exec_win.cc contains implementations of both
Multiprocess methods and MultiprocessExec functions. This will need more
work in the future, but reflects the idea that all tests should be in
terms of MultiprocessExec eventually.

Currently, this works sufficiently to have util_test succeed (including
multiprocess_exec_test, and the recently ported HTTPTransport tests.)

R=mark@chromium.org
BUG=crashpad:1, crashpad:7
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/892c29e8ba3dbdd234d69456855367a2d9af85cf

[scottmg, 2015-01-27 01:37:47]

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_test_child.cc (left):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_test_child.cc:158: // Make sure there's nothing open
other than stdin, stdout, and stderr.
This didn't work at all, once I finally got the binaries running. It's flaky,
and there's a ton of other non-file handles open. Need to determine what we want
to test here, and do it in some other way.

[Mark Mentovai, 2015-01-28 19:11:24]

Does this supersede https://codereview.chromium.org/853853002/?

[Mark Mentovai, 2015-01-28 19:12:29]

Never mind, I see that it does. Gmail orphaned the message and it wasn’t
immediately apparent.

[Mark Mentovai, 2015-01-28 19:58:28]

OK, let’s get this unblocked now!

https://codereview.chromium.org/880763002/diff/40001/compat/win/sys/types.h
File compat/win/sys/types.h (right):

https://codereview.chromium.org/880763002/diff/40001/compat/win/sys/types.h#n...
compat/win/sys/types.h:27: typedef unsigned long pid_t;
I kind of hope that this isn’t necessary in the long run. Leave a TODO to review
this in the future. Or, instead of adding this, #ifdef out the declaration of
Multiprocess::ChildPID() on Windows. (You left it declared but didn’t implement
it.)

https://codereview.chromium.org/880763002/diff/40001/snapshot/mac/process_rea...
File snapshot/mac/process_reader_test.cc (right):

https://codereview.chromium.org/880763002/diff/40001/snapshot/mac/process_rea...
snapshot/mac/process_reader_test.cc:102: int write_fd = WritePipeHandle();
This is in platform-specific code, so the int is kinda OK, but it’s better to
use the abstraction and call it “FileHandle write_handle” now. The same pattern
recurs elsewhere in this file.

https://codereview.chromium.org/880763002/diff/40001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:47: if (!success && GetLastError() ==
ERROR_BROKEN_PIPE) {
A light restructuring to keep rechecking the same things over and over again
would clarify this a bit.

  if (!success) {
    // Alternatively: const DWORD error = GetLastError();
    if (GetLastError() == ERROR_BROKEN_PIPE) {
    } else if (GetLastError() != ERROR_MORE_DATA) {
    }
  } else if (bytes_read == 0 && GetFileType(file) != FILE_TYPE_PIPE) {
  }

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h
File util/test/multiprocess.h (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h...
util/test/multiprocess.h:149: internal::MultiprocessInfo* info() { return info_;
}
This should be const.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec.h (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec.h:69: std::wstring argv_;
The “v” isn’t really right if it’s just a string. And it looks like this has the
whole command line, not just the arguments. command_line_ is a better name.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_posix.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_posix.cc:75: int read_fd = ReadPipeHandle();
FileHandle read_handle. Line 87 too.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_test_child.cc (left):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_test_child.cc:158: // Make sure there's nothing open
other than stdin, stdout, and stderr.
scottmg wrote:
> This didn't work at all, once I finally got the binaries running. It's flaky,
> and there's a ton of other non-file handles open. Need to determine what we
want
> to test here, and do it in some other way.

Let’s leave a TODO here, then.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_win.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:26: void ArgvQuote(const std::wstring&
argument, std::wstring* command_line) {
It’s not obvious that this is additive. Add a comment, rename the function, or
both.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:28: if (argument.empty() == false &&
Adapt to local style: !argument.empty(), std::wstring::npos.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:32: // Ref:
http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/every...
Promote this to the function-level comment?

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:32: // Ref:
http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/every...
I’m glad you did this correctly. Sloppy quoting is a pet peeve of mine.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:35: unsigned backslash_count = 0;
size_t

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:74: Multiprocess::Multiprocess()
There should be a comment somewhere (maybe here, maybe in a header) that
Multiprocess on Windows isn’t the same as on POSIX, and that it just exists to
support MultiprocessExec.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:82: RunChild();
To that end, this whole function is kind of weird. What’s going on here?

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:108: 
You haven’t implemented ChildPID() (but you probably don’t want to, see
elsewhere). You also haven’t implemented CloseReadPipe() or CloseWritePipe().
Those two ought to be implemented.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:155: set_info(new
internal::MultiprocessInfo);
() to be explicit.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_p...
File util/test/multiprocess_posix_test.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_p...
util/test/multiprocess_posix_test.cc:39: int read_fd = ReadPipeHandle();
Same pattern here and on line 57.

[scottmg, 2015-01-28 22:10:10]

Patchset #4 (id:60001) has been deleted

[scottmg, 2015-01-28 22:14:58]

Thanks!

https://codereview.chromium.org/880763002/diff/40001/compat/win/sys/types.h
File compat/win/sys/types.h (right):

https://codereview.chromium.org/880763002/diff/40001/compat/win/sys/types.h#n...
compat/win/sys/types.h:27: typedef unsigned long pid_t;
On 2015/01/28 19:58:27, Mark Mentovai wrote:
> I kind of hope that this isn’t necessary in the long run. Leave a TODO to
review
> this in the future. Or, instead of adding this, #ifdef out the declaration of
> Multiprocess::ChildPID() on Windows. (You left it declared but didn’t
implement
> it.)

Avoiding the declaration seems clearer, done.

https://codereview.chromium.org/880763002/diff/40001/snapshot/mac/process_rea...
File snapshot/mac/process_reader_test.cc (right):

https://codereview.chromium.org/880763002/diff/40001/snapshot/mac/process_rea...
snapshot/mac/process_reader_test.cc:102: int write_fd = WritePipeHandle();
On 2015/01/28 19:58:27, Mark Mentovai wrote:
> This is in platform-specific code, so the int is kinda OK, but it’s better to
> use the abstraction and call it “FileHandle write_handle” now. The same
pattern
> recurs elsewhere in this file.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:47: if (!success && GetLastError() ==
ERROR_BROKEN_PIPE) {
On 2015/01/28 19:58:27, Mark Mentovai wrote:
> A light restructuring to keep rechecking the same things over and over again
> would clarify this a bit.
> 
>   if (!success) {
>     // Alternatively: const DWORD error = GetLastError();
>     if (GetLastError() == ERROR_BROKEN_PIPE) {
>     } else if (GetLastError() != ERROR_MORE_DATA) {
>     }
>   } else if (bytes_read == 0 && GetFileType(file) != FILE_TYPE_PIPE) {
>   }

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h
File util/test/multiprocess.h (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h...
util/test/multiprocess.h:149: internal::MultiprocessInfo* info() { return info_;
}
On 2015/01/28 19:58:27, Mark Mentovai wrote:
> This should be const.

Done. It still has to return a non-const* though as MultiprocessExec::PreFork is
modifying it.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec.h (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec.h:69: std::wstring argv_;
On 2015/01/28 19:58:27, Mark Mentovai wrote:
> The “v” isn’t really right if it’s just a string. And it looks like this has
the
> whole command line, not just the arguments. command_line_ is a better name.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_posix.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_posix.cc:75: int read_fd = ReadPipeHandle();
On 2015/01/28 19:58:27, Mark Mentovai wrote:
> FileHandle read_handle. Line 87 too.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_test_child.cc (left):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_test_child.cc:158: // Make sure there's nothing open
other than stdin, stdout, and stderr.
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> scottmg wrote:
> > This didn't work at all, once I finally got the binaries running. It's
flaky,
> > and there's a ton of other non-file handles open. Need to determine what we
> want
> > to test here, and do it in some other way.
> 
> Let’s leave a TODO here, then.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_win.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:26: void ArgvQuote(const std::wstring&
argument, std::wstring* command_line) {
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> It’s not obvious that this is additive. Add a comment, rename the function, or
> both.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:28: if (argument.empty() == false &&
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> Adapt to local style: !argument.empty(), std::wstring::npos.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:32: // Ref:
http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/every...
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> Promote this to the function-level comment?

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:35: unsigned backslash_count = 0;
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> size_t

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:74: Multiprocess::Multiprocess()
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> There should be a comment somewhere (maybe here, maybe in a header) that
> Multiprocess on Windows isn’t the same as on POSIX, and that it just exists to
> support MultiprocessExec.

Done in header.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:82: RunChild();
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> To that end, this whole function is kind of weird. What’s going on here?

It's setting up and spawning the child, then running the parent actions in this
process and then joining with the child to clean up. I added comments to that
effect.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:108: 
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> You haven’t implemented ChildPID() (but you probably don’t want to, see
> elsewhere). You also haven’t implemented CloseReadPipe() or CloseWritePipe().
> Those two ought to be implemented.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:155: set_info(new
internal::MultiprocessInfo);
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> () to be explicit.

Done.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_p...
File util/test/multiprocess_posix_test.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_p...
util/test/multiprocess_posix_test.cc:39: int read_fd = ReadPipeHandle();
On 2015/01/28 19:58:28, Mark Mentovai wrote:
> Same pattern here and on line 57.

Done.

[Mark Mentovai, 2015-01-28 22:32:54]

LGTM.

Not necessary for this change, but for later: Ultimately, it might be best to
split Multiprocess into the interface portion dealing with termination and pipe
handles and distinct (POSIX-only) portion dealing with fork()-no-exec().
MultiprocessExec could then inherit from the former on Windows without having to
cope with the baggage of the latter. What do you think?

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h
File util/test/multiprocess.h (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h...
util/test/multiprocess.h:149: internal::MultiprocessInfo* info() { return info_;
}
scottmg wrote:
> On 2015/01/28 19:58:27, Mark Mentovai wrote:
> > This should be const.
> 
> Done. It still has to return a non-const* though as MultiprocessExec::PreFork
is
> modifying it.

Oh, I wrote this comment as I was reading from top to bottom and meant to take
it out after I saw how you were using it. No, I don’t think this should be const
at all.

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
File util/test/multiprocess_exec_win.cc (right):

https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
util/test/multiprocess_exec_win.cc:82: RunChild();
scottmg wrote:
> On 2015/01/28 19:58:28, Mark Mentovai wrote:
> > To that end, this whole function is kind of weird. What’s going on here?
> 
> It's setting up and spawning the child, then running the parent actions in
this
> process and then joining with the child to clean up. I added comments to that
> effect.

It looked like it was running the child actions in this process too. I was
confused by Multiprocess and MultiprocessExec being in the same file. The new
comment helps.

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
File util/test/multiprocess_exec.h (right):

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
util/test/multiprocess_exec.h:68: #else
#elif defined(OS_WIN)

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
File util/test/multiprocess_exec_posix.cc (right):

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
util/test/multiprocess_exec_posix.cc:19: #include <stdlib.h>
This needs <unistd.h> now too.

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
File util/test/multiprocess_posix.cc (right):

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
util/test/multiprocess_posix.cc:19: #include <sys/wait.h>
This needs <unistd.h> now that you’ve removed it from the header.

[scottmg, 2015-01-28 22:44:05]

On 2015/01/28 22:32:54, Mark Mentovai wrote:
> LGTM.
> 
> Not necessary for this change, but for later: Ultimately, it might be best to
> split Multiprocess into the interface portion dealing with termination and
pipe
> handles and distinct (POSIX-only) portion dealing with fork()-no-exec().
> MultiprocessExec could then inherit from the former on Windows without having
to
> cope with the baggage of the latter. What do you think?
> 
> https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h
> File util/test/multiprocess.h (right):
> 
>
https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h...
> util/test/multiprocess.h:149: internal::MultiprocessInfo* info() { return
info_;
> }
> scottmg wrote:
> > On 2015/01/28 19:58:27, Mark Mentovai wrote:
> > > This should be const.
> > 
> > Done. It still has to return a non-const* though as
MultiprocessExec::PreFork
> is
> > modifying it.
> 
> Oh, I wrote this comment as I was reading from top to bottom and meant to take
> it out after I saw how you were using it. No, I don’t think this should be
const
> at all.
> 
>
https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
> File util/test/multiprocess_exec_win.cc (right):
> 
>
https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
> util/test/multiprocess_exec_win.cc:82: RunChild();
> scottmg wrote:
> > On 2015/01/28 19:58:28, Mark Mentovai wrote:
> > > To that end, this whole function is kind of weird. What’s going on here?
> > 
> > It's setting up and spawning the child, then running the parent actions in
> this
> > process and then joining with the child to clean up. I added comments to
that
> > effect.
> 
> It looked like it was running the child actions in this process too. I was
> confused by Multiprocess and MultiprocessExec being in the same file. The new
> comment helps.
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> File util/test/multiprocess_exec.h (right):
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> util/test/multiprocess_exec.h:68: #else
> #elif defined(OS_WIN)
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> File util/test/multiprocess_exec_posix.cc (right):
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> util/test/multiprocess_exec_posix.cc:19: #include <stdlib.h>
> This needs <unistd.h> now too.
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
> File util/test/multiprocess_posix.cc (right):
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
> util/test/multiprocess_posix.cc:19: #include <sys/wait.h>
> This needs <unistd.h> now that you’ve removed it from the header.

[scottmg, 2015-01-28 22:48:20]

On 2015/01/28 22:32:54, Mark Mentovai wrote:
> LGTM.
> 
> Not necessary for this change, but for later: Ultimately, it might be best to
> split Multiprocess into the interface portion dealing with termination and
pipe
> handles and distinct (POSIX-only) portion dealing with fork()-no-exec().
> MultiprocessExec could then inherit from the former on Windows without having
to
> cope with the baggage of the latter. What do you think?

(Oops, my mouse is on the fritz)

Yes, I think that sounds good. I was thinking maybe less inheritance would be
better, as it seems a bit rigid currently. Something like having the
Multiprocess interface to present what the tests want, but having the
implementation mostly in utility functions/classes and then MultiprocessNoFork,
MultiprocessMach, and MultiprocessExec all direct derivations of Multiprocess
rather than two level. But I'm not sure exactly how that would look yet, mostly
just an idea.

> 
> https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h
> File util/test/multiprocess.h (right):
> 
>
https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess.h...
> util/test/multiprocess.h:149: internal::MultiprocessInfo* info() { return
info_;
> }
> scottmg wrote:
> > On 2015/01/28 19:58:27, Mark Mentovai wrote:
> > > This should be const.
> > 
> > Done. It still has to return a non-const* though as
MultiprocessExec::PreFork
> is
> > modifying it.
> 
> Oh, I wrote this comment as I was reading from top to bottom and meant to take
> it out after I saw how you were using it. No, I don’t think this should be
const
> at all.
> 
>
https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
> File util/test/multiprocess_exec_win.cc (right):
> 
>
https://codereview.chromium.org/880763002/diff/40001/util/test/multiprocess_e...
> util/test/multiprocess_exec_win.cc:82: RunChild();
> scottmg wrote:
> > On 2015/01/28 19:58:28, Mark Mentovai wrote:
> > > To that end, this whole function is kind of weird. What’s going on here?
> > 
> > It's setting up and spawning the child, then running the parent actions in
> this
> > process and then joining with the child to clean up. I added comments to
that
> > effect.
> 
> It looked like it was running the child actions in this process too. I was
> confused by Multiprocess and MultiprocessExec being in the same file. The new
> comment helps.
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> File util/test/multiprocess_exec.h (right):
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> util/test/multiprocess_exec.h:68: #else
> #elif defined(OS_WIN)
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> File util/test/multiprocess_exec_posix.cc (right):
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
> util/test/multiprocess_exec_posix.cc:19: #include <stdlib.h>
> This needs <unistd.h> now too.
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
> File util/test/multiprocess_posix.cc (right):
> 
>
https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
> util/test/multiprocess_posix.cc:19: #include <sys/wait.h>
> This needs <unistd.h> now that you’ve removed it from the header.

[scottmg, 2015-01-28 22:48:26]

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
File util/test/multiprocess_exec.h (right):

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
util/test/multiprocess_exec.h:68: #else
On 2015/01/28 22:32:54, Mark Mentovai wrote:
> #elif defined(OS_WIN)

Done.

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
File util/test/multiprocess_exec_posix.cc (right):

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_e...
util/test/multiprocess_exec_posix.cc:19: #include <stdlib.h>
On 2015/01/28 22:32:54, Mark Mentovai wrote:
> This needs <unistd.h> now too.

Done.

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
File util/test/multiprocess_posix.cc (right):

https://codereview.chromium.org/880763002/diff/80001/util/test/multiprocess_p...
util/test/multiprocess_posix.cc:19: #include <sys/wait.h>
On 2015/01/28 22:32:54, Mark Mentovai wrote:
> This needs <unistd.h> now that you’ve removed it from the header.

Done.

[scottmg, 2015-01-28 22:49:49]

Committed patchset #5 (id:100001) manually as
892c29e8ba3dbdd234d69456855367a2d9af85cf (presubmit successful).