Refactor the process creation code on POSIX platforms

Refactor the process creation code on POSIX platforms

This moved the process creation code to a class, and splits the
single function used before into several methods operation on instance
variables.

The refactor revealed at least one leaked file descriptor.

R=kustermann@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=43280

[Søren Gjesse, 2015-01-28 15:36:43]

Please take a look. The code is basically the same as before, but hopefully a
bit easier to follow.

[kustermann, 2015-01-29 10:20:33]

LGTM with comments

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:304: // This run in the original process.
run -> runs

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:315: // Notify child process to start.
Add a comment here
"this has to happen to delay exec() until RegisterProcess() was called, since
this ensures we're ready to accept the exit code"

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:320: perror("Failed sending notification message");
:-/

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:367: ASSERT(write_out_[1] == -1);
In the detached case write_out_[0] and write_out_[1] are -1, so you can assert
for this.

BUT: You cannot close(-1) them, right?

i.e. remove
 364       VOID_TEMP_FAILURE_RETRY(close(write_out_[0]));
 365       VOID_TEMP_FAILURE_RETRY(close(write_out_[1]));

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:437: if (TEMP_FAILURE_RETRY(dup2(write_out_[0],
STDIN_FILENO)) == -1) {
I like it: Marcro expansion in expressions which run while/do-while loops :)

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:595: delete[] program_environment_;
This is not a good idea. You free the memory, but you don't set it to NULL.

Please remove these two lines, the destructor of ProcessStarter will take care
of it :)

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:648: *os_error_message_ =
const_cast<char*>(no_message);
That is slightly suspicious.

Once a malloc'ed string is "returned" and once a "static const char*" (which
will probably end up in the read-only memory segment), which can of course never
be free()ed.

So either call sites don't call free() which means we might leak some memory in
error cases, or the program might crash in error conditions because of trying to
free() memory which wasn't malloc()ed first.

[Søren Gjesse, 2015-01-29 12:25:55]

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:304: // This run in the original process.
On 2015/01/29 10:20:33, kustermann wrote:
> run -> runs

Done.

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:315: // Notify child process to start.
On 2015/01/29 10:20:33, kustermann wrote:
> Add a comment here
> "this has to happen to delay exec() until RegisterProcess() was called, since
> this ensures we're ready to accept the exit code"

Done.

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:320: perror("Failed sending notification message");
On 2015/01/29 10:20:33, kustermann wrote:
> :-/

Changed to CleanupAndReturnError().

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:367: ASSERT(write_out_[1] == -1);
On 2015/01/29 10:20:33, kustermann wrote:
> In the detached case write_out_[0] and write_out_[1] are -1, so you can assert
> for this.
> 
> BUT: You cannot close(-1) them, right?
> 
> i.e. remove
>  364       VOID_TEMP_FAILURE_RETRY(close(write_out_[0]));
>  365       VOID_TEMP_FAILURE_RETRY(close(write_out_[1]));

Good catch. Removed.

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:437: if (TEMP_FAILURE_RETRY(dup2(write_out_[0],
STDIN_FILENO)) == -1) {
On 2015/01/29 10:20:33, kustermann wrote:
> I like it: Marcro expansion in expressions which run while/do-while loops :)

Acknowledged.

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:595: delete[] program_environment_;
On 2015/01/29 10:20:33, kustermann wrote:
> This is not a good idea. You free the memory, but you don't set it to NULL.
> 
> Please remove these two lines, the destructor of ProcessStarter will take care
> of it :)

Good catch, my mistake when I moved this to the destructor.

https://codereview.chromium.org/884143002/diff/60001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:648: *os_error_message_ =
const_cast<char*>(no_message);
On 2015/01/29 10:20:33, kustermann wrote:
> That is slightly suspicious.
> 
> Once a malloc'ed string is "returned" and once a "static const char*" (which
> will probably end up in the read-only memory segment), which can of course
never
> be free()ed.
> 
> So either call sites don't call free() which means we might leak some memory
in
> error cases, or the program might crash in error conditions because of trying
to
> free() memory which wasn't malloc()ed first.

Good point. We are probably hosed anyway if malloc fails. But changed it to
return NULL, and moved the constant to the caller.

[Søren Gjesse, 2015-01-29 12:28:07]

Thanks for the review.

[Søren Gjesse, 2015-01-29 13:06:48]

Committed patchset #7 (id:120001) manually as 43280 (presubmit successful).