Enforce no path service has improper path.

Enforce no path service has improper path.

R=cpu@chromium.org,mark@chromium.org
BUG=168890
Committed: r178932

[cpu_(ooo_6.6-7.5), 2013-01-25 04:12:04]

lgtm

[M-A Ruel, 2013-01-25 13:29:59]

On 2013/01/25 04:12:04, cpu wrote:
> lgtm

Eh, thanks, I wanted to see if the try jobs would pass first.

Mark, can you do an OWNERS approval?

[Mark Mentovai, 2013-01-25 13:44:34]

{PATH,DIR}_{EXE,MODULE} can still have dots in them on the Mac (and I’m not keen
on “fixing” that because it will make certain operations less robust). This test
will fail in those cases, right?

[M-A Ruel, 2013-01-25 13:50:56]

On 2013/01/25 13:44:34, Mark Mentovai wrote:
> {PATH,DIR}_{EXE,MODULE} can still have dots in them on the Mac (and I’m not
keen
> on “fixing” that because it will make certain operations less robust). This
test
> will fail in those cases, right?

I'm just enforcing the current assumptions in the code with more unit testing
and not changing the behavior.

The way some functions in file_util work now fail if there's '..' in the path,
the calling code must make it absolute first if the security context allows
doing this. But many testing code path assumes the path returned by PathService
won't have '..' as a path component. This mostly happens in test isolation by
accident because of the way the tests are called versus with runtest.py.

[Mark Mentovai, 2013-01-25 13:55:01]

Right, I’m saying that there are cases where the path service will return paths
with dots because removing those dots would expose corner-case problems that are
worse than leaving the dots alone. Tests or other code that assume that
ReferencesParent will always be true for EXE and MODULE paths on the Mac are
incorrect.

The addition to this test is fine for all other path types.

[M-A Ruel, 2013-01-25 14:08:59]

On 2013/01/25 13:55:01, Mark Mentovai wrote:
> Right, I’m saying that there are cases where the path service will return
paths
> with dots because removing those dots would expose corner-case problems that
are
> worse than leaving the dots alone. Tests or other code that assume that
> ReferencesParent will always be true for EXE and MODULE paths on the Mac are
> incorrect.

Interesting, can you enlighten me about these cases?

[M-A Ruel, 2013-01-25 14:15:38]

On 2013/01/25 14:08:59, Marc-Antoine Ruel wrote:
> On 2013/01/25 13:55:01, Mark Mentovai wrote:
> > Right, I’m saying that there are cases where the path service will return
> paths
> > with dots because removing those dots would expose corner-case problems that
> are
> > worse than leaving the dots alone. Tests or other code that assume that
> > ReferencesParent will always be true for EXE and MODULE paths on the Mac are
> > incorrect.
> 
> Interesting, can you enlighten me about these cases?

In the meantime, I uploaded patchset #3 which limits the checks on OSX. I'd like
to add a comment explaining why.

[Mark Mentovai, 2013-01-25 14:18:39]

When Chrome or a test is started with dots in the path from the command line,
like

./unit_tests

developers are likely to start it, the EXE and MODULE paths will have those dots
in them.

If it was just for tests, it’d be fine to get rid of the dots by computing an
absolute path. That’s why I was OK with doing this for the DIR_SOURCE_ROOT thing
earlier this week: only tests use DIR_SOURCE_ROOT.

However, the EXE and MODULE paths are also used in production, in the field, on
user machines. The dots are correct, valid, legal paths that work properly as
long as nobody changes directory, which in the case of Chrome, nobody ever does.
Transforming these paths to absolute paths involves doing I/O and will slightly
delay startup (not that big a deal but we’re trying to keep startup time down)
but more importantly will produce pathnames that are not valid if they would be
too long. This is the corner-case problem.

When the choice is between “use the dots, they’ll work 100% of the time” and “do
some I/O to get a prettier absolute path that’ll only work 99.9% of the time,”
we chose the former.

[M-A Ruel, 2013-01-25 14:19:33]

And to clarify the goal, I'm trying to get rid of failures like this one:
http://build.chromium.org/p/chromium.swarm/builders/Mac%20Swarm%20Tests%20%28...

[21441:-1609108160:0124/140650:1737177492882752:ERROR:resource_bundle.cc(579)]
Failed to load
/Volumes/data/b/swarm_slave/swarm_tests/run_tha_testo5i612/chrome/../out/Release/resources.pak

which I suspect fails because of the '..'. Once this CL gets in, I'll see the
paths with '..' and will fix them.

[Mark Mentovai, 2013-01-25 14:19:33]

LGTM on patch set 3. You can add a comment based on my explanation if you like.
Thanks.

[Mark Mentovai, 2013-01-25 14:21:20]

Yup, I understand. Whoever’s invoking with those dots should be fixed.

[M-A Ruel, 2013-01-25 14:21:51]

On 2013/01/25 14:18:39, Mark Mentovai wrote:
> When Chrome or a test is started with dots in the path from the command line,
> like
> 
> ./unit_tests
> 
> developers are likely to start it, the EXE and MODULE paths will have those
dots
> in them.

Just to make sure we are on the same page; I only see one dot in there. One dot
is fine, it's '..' that is banned. So in this case if the resulting path is
/path/to/./ it is fine.

[M-A Ruel, 2013-01-25 14:30:26]

To easily reproduce:
ninja -C out/Release unit_tests
cd chrome
../out/Release/unit_tests --gtest_list_tests

[commit-bot: I haz the power, 2013-01-25 20:10:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/12035095/5017

[commit-bot: I haz the power, 2013-01-25 23:51:51]

Failed to apply the patch.
Sending        base/path_service_unittest.cc
Sending        chrome/chrome_tests_unit.gypi
Transmitting file data ..

[Nico, 2013-03-13 23:18:40]

https://chromiumcodereview.appspot.com/12035095/diff/5017/base/path_service_u...
File base/path_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/12035095/diff/5017/base/path_service_u...
base/path_service_unittest.cc:65: #if defined(OS_MAC)
It's OS_MAXOSX (https://codereview.chromium.org/12829005/)