Add the ability to change directories before execing.

Add the ability to change directories before execing.

BUG=387037
Committed: https://crrev.com/3371bf55c80d011f2d654b6566a20937df3b20f1
Cr-Commit-Position: refs/heads/master@{#314294}

[rickyz (no longer on Chrome), 2015-01-30 23:19:27]

rickyz@chromium.org changed reviewers:
+ jln@chromium.org

[jln (very slow on Chromium), 2015-01-30 23:48:11]

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
File base/process/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
base/process/process_util_unittest.cc:1027: const char* kTmpDir = "/tmp";
Let's declare storage: const char kTmpDir[]

But you also need to change that since it won't work on Android (where you'd
need /data/local/tmp).

You should simply use GetTempDir()

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
base/process/process_util_unittest.cc:1038: base::LaunchOptions options;
I would change the CurrentDirectory to NOT the temp dir first.

[rickyz (no longer on Chrome), 2015-01-30 23:58:37]

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
File base/process/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
base/process/process_util_unittest.cc:1027: const char* kTmpDir = "/tmp";
On 2015/01/30 23:48:11, jln wrote:
> Let's declare storage: const char kTmpDir[]
> 
> But you also need to change that since it won't work on Android (where you'd
> need /data/local/tmp).
> 
> You should simply use GetTempDir()

Done, thanks for saving me a try failure :-)

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
base/process/process_util_unittest.cc:1038: base::LaunchOptions options;
On 2015/01/30 23:48:11, jln wrote:
> I would change the CurrentDirectory to NOT the temp dir first.

I made this change, and tests seem to pass, but are you sure that no other tests
running in a different thread might depend on a certain cwd?

[jln (very slow on Chromium), 2015-01-31 00:21:56]

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
File base/process/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/885423003/diff/1/base/process/process_...
base/process/process_util_unittest.cc:1038: base::LaunchOptions options;
On 2015/01/30 23:58:37, rickyz wrote:
> On 2015/01/30 23:48:11, jln wrote:
> > I would change the CurrentDirectory to NOT the temp dir first.
> 
> I made this change, and tests seem to pass, but are you sure that no other
tests
> running in a different thread might depend on a certain cwd?

Tests are never run concurrently in threads. What happens in that sometimes
threads from other tests linger and create flakiness. If we make such threads
crash here that would be fine and catch a reason for flakiness: they should not
exist.

https://chromiumcodereview.appspot.com/885423003/diff/20001/base/process/proc...
File base/process/process_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/885423003/diff/20001/base/process/proc...
base/process/process_util_unittest.cc:1054:
ASSERT_TRUE(base::SetCurrentDirectory(orig_cwd));
You could put this in a ClosureRunner instead, but it's probably not needed.

https://chromiumcodereview.appspot.com/885423003/diff/20001/base/process/proc...
base/process/process_util_unittest.cc:1059: options.current_directory =
base::FilePath("/etc/passwd");
How about /dev/null? I'm not sure /etc/passwd is guaranteed by POSIX (I'm pretty
sure it won't exist on Android too).

https://chromiumcodereview.appspot.com/885423003/diff/20001/base/process/proc...
base/process/process_util_unittest.cc:1066: EXPECT_EQ(-1, exit_code);
Let's do EXPECT_NE(kSuccess, ...) since we're relying on undocumented oddities
of CHECK/RAW_CHECK here.

[rickyz (no longer on Chrome), 2015-01-31 00:59:38]

https://codereview.chromium.org/885423003/diff/1/base/process/process_util_un...
File base/process/process_util_unittest.cc (right):

https://codereview.chromium.org/885423003/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:1038: base::LaunchOptions options;
On 2015/01/31 00:21:55, jln wrote:
> On 2015/01/30 23:58:37, rickyz wrote:
> > On 2015/01/30 23:48:11, jln wrote:
> > > I would change the CurrentDirectory to NOT the temp dir first.
> > 
> > I made this change, and tests seem to pass, but are you sure that no other
> tests
> > running in a different thread might depend on a certain cwd?
> 
> Tests are never run concurrently in threads. What happens in that sometimes
> threads from other tests linger and create flakiness. If we make such threads
> crash here that would be fine and catch a reason for flakiness: they should
not
> exist.

Removing this and adding a TODO as we discussed due to the SpawnChildWithOptions
relying on the current directory in this change.

https://codereview.chromium.org/885423003/diff/20001/base/process/process_uti...
File base/process/process_util_unittest.cc (right):

https://codereview.chromium.org/885423003/diff/20001/base/process/process_uti...
base/process/process_util_unittest.cc:1054:
ASSERT_TRUE(base::SetCurrentDirectory(orig_cwd));
On 2015/01/31 00:21:56, jln wrote:
> You could put this in a ClosureRunner instead, but it's probably not needed.

This ended up getting removed.

https://codereview.chromium.org/885423003/diff/20001/base/process/process_uti...
base/process/process_util_unittest.cc:1059: options.current_directory =
base::FilePath("/etc/passwd");
On 2015/01/31 00:21:56, jln wrote:
> How about /dev/null? I'm not sure /etc/passwd is guaranteed by POSIX (I'm
pretty
> sure it won't exist on Android too).

Done.

https://codereview.chromium.org/885423003/diff/20001/base/process/process_uti...
base/process/process_util_unittest.cc:1066: EXPECT_EQ(-1, exit_code);
On 2015/01/31 00:21:56, jln wrote:
> Let's do EXPECT_NE(kSuccess, ...) since we're relying on undocumented oddities
> of CHECK/RAW_CHECK here.

Done.

[jln (very slow on Chromium), 2015-02-03 02:07:53]

Paranoid nit.

https://chromiumcodereview.appspot.com/885423003/diff/60001/base/process/laun...
File base/process/launch_posix.cc (right):

https://chromiumcodereview.appspot.com/885423003/diff/60001/base/process/laun...
base/process/launch_posix.cc:520:
RAW_CHECK(chdir(options.current_directory.value().c_str()) == 0);
Sorry I didn't comment on that before: you need to do the c_str() conversion
prefork to be async safe.

[rickyz (no longer on Chrome), 2015-02-03 02:17:59]

https://codereview.chromium.org/885423003/diff/60001/base/process/launch_posi...
File base/process/launch_posix.cc (right):

https://codereview.chromium.org/885423003/diff/60001/base/process/launch_posi...
base/process/launch_posix.cc:520:
RAW_CHECK(chdir(options.current_directory.value().c_str()) == 0);
On 2015/02/03 02:07:53, jln wrote:
> Sorry I didn't comment on that before: you need to do the c_str() conversion
> prefork to be async safe.
Sure, I moved another c_str caller up as well.

[jln (very slow on Chromium), 2015-02-03 02:37:26]

lgtm with ident nit.

https://codereview.chromium.org/885423003/diff/80001/base/process/launch_posi...
File base/process/launch_posix.cc (right):

https://codereview.chromium.org/885423003/diff/80001/base/process/launch_posi...
base/process/launch_posix.cc:351: for (size_t i = 0; i < argv.size(); i++)
Wrong indent.

Nit: I also prefer {}, but your choice.

[rickyz (no longer on Chrome), 2015-02-03 02:44:44]

rickyz@chromium.org changed reviewers:
+ rvargas@chromium.org

[rickyz (no longer on Chrome), 2015-02-03 02:44:44]

Thanks, adding rvargas@ for base OWNERS.

https://codereview.chromium.org/885423003/diff/80001/base/process/launch_posi...
File base/process/launch_posix.cc (right):

https://codereview.chromium.org/885423003/diff/80001/base/process/launch_posi...
base/process/launch_posix.cc:351: for (size_t i = 0; i < argv.size(); i++)
On 2015/02/03 02:37:26, jln wrote:
> Wrong indent.
> 
> Nit: I also prefer {}, but your choice.

Oops, fixed - I prefer {} too.

[rvargas (doing something else), 2015-02-03 03:32:10]

RBSTMP LGTM

https://chromiumcodereview.appspot.com/885423003/diff/100001/base/process/lau...
File base/process/launch_posix.cc (right):

https://chromiumcodereview.appspot.com/885423003/diff/100001/base/process/lau...
base/process/launch_posix.cc:528: #if defined(OS_POSIX)
isn't this redundant?

[rickyz (no longer on Chrome), 2015-02-03 07:24:29]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-02-03 07:24:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/885423003/120001

[commit-bot: I haz the power, 2015-02-03 08:44:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-03 08:45:52]

Patchset 7 (id:??) landed as
https://crrev.com/3371bf55c80d011f2d654b6566a20937df3b20f1
Cr-Commit-Position: refs/heads/master@{#314294}