Make all the files mapped in when running base_unittests read only.

Make all the files mapped in when running base_unittests read only.

This means no file can be opened for write during the test run.

Make CopyFileUnsafe() reset the RO bit on Windows.
Add a tests that confirms the current CopyFile() behavior:
- On Windows, CopyFile() copies the ACL but now strips the READONLY bit.
- On OSX, CopyFile() copies the ACL.
- On anything else, ACL is not copied.

Rationale:
On anything-but-Windows, deleting a file require write access on the directory.
On Windows, deleting a file require not having the RO bit on the file.
CopyFile() affects the file but not the directory.

On isolated testing, the read only bit will be set on the file being copied,
causing the test to fail to delete the files.

This has wide implications in the unit tests. CopyFile() is mostly (but not
exclusively) used in unit tests.

R=thakis@chromium.org, vadimsh@chromium.org
BUG=116251
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244947

[M-A Ruel, 2014-01-13 18:32:52]

It passed on linux and OSX. Waiting for Windows result. If it passes Windows,
it'll be committed as-is once you approved it and I'll send a PSA to
chromium-dev.

Note that the tests can create files, but not modify files. It's an important
property to be able to run the same test twice or to not corrupt the isolated
cache.

[M-A Ruel, 2014-01-13 19:12:56]

On 2014/01/13 18:32:52, M-A Ruel wrote:
> It passed on linux and OSX. Waiting for Windows result. If it passes Windows,
> it'll be committed as-is once you approved it and I'll send a PSA to
> chromium-dev.
> 
> Note that the tests can create files, but not modify files. It's an important
> property to be able to run the same test twice or to not corrupt the isolated
> cache.

JsonPrefStoreTest.BasicAsync did fail on Windows. I'll sync there and
investigate. I've created CLs for the other tests so I can run try jobs. I'll
send request for reviews once they are working.

[Vadim Sh., 2014-01-13 19:18:29]

On 2014/01/13 18:32:52, M-A Ruel wrote:
> It passed on linux and OSX. Waiting for Windows result. If it passes Windows,
> it'll be committed as-is once you approved it and I'll send a PSA to
> chromium-dev.
> 
> Note that the tests can create files, but not modify files. It's an important
> property to be able to run the same test twice or to not corrupt the isolated
> cache.

Looks like it fails on win swarm for real... Also don't miss
android_dbg_triggered_tests failure, it's also related to isolate.

[M-A Ruel, 2014-01-14 00:13:46]

On 2014/01/13 19:18:29, Vadim Sh. wrote:
> On 2014/01/13 18:32:52, M-A Ruel wrote:
> > It passed on linux and OSX. Waiting for Windows result. If it passes
Windows,
> > it'll be committed as-is once you approved it and I'll send a PSA to
> > chromium-dev.
> > 
> > Note that the tests can create files, but not modify files. It's an
important
> > property to be able to run the same test twice or to not corrupt the
isolated
> > cache.
> 
> Looks like it fails on win swarm for real... Also don't miss
> android_dbg_triggered_tests failure, it's also related to isolate.

+Nico for OWNERS in base/

I had to change the CopyFile on Windows, as it has a different semantic than the
version on POSIX but similar to OSX. On the other hand, copying the security
descriptor on OSX is harmless but it is on Windows.

https://chromium.googlesource.com/chromium/src/+/master/base/file_util_posix.cc
Line 918, no security descriptor is copied

https://chromium.googlesource.com/chromium/src/+/master/base/file_util_mac.mm
Line 19
https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag...
security descriptors are copied.

The big difference is file deletion. On anything-but-windows, a user has to have
write access to the directory to be able to delete the file.
On Windows, the read-only bit inhibits file deletion, even if the user has write
access to the file in the SECURITY_DESCRIPTOR.

In practice, copying the file attribute caused test errors when a test copies a
test data file in a temporary directory and then tries to modify it. This is
only problematic on Windows.
http://build.chromium.org/p/tryserver.chromium/builders/win_swarm_triggered/b...
[ RUN      ] JsonPrefStoreTest.Basic
e:\b\build\slave\win\build\src\base\prefs\json_pref_store_unittest.cc(154):
error: Value of: TextContentsEqual(golden_output_file, output_file)
  Actual: false
Expected: true
[  FAILED  ] JsonPrefStoreTest.Basic (16 ms)

[Nico, 2014-01-14 02:30:33]

OS X also copies the the acl bits (see http://www.manpagez.com/man/3/copyfile/ ,
we use COPYFILE_ALL).

Having this consistent across platforms makes sense though – maybe add a test
checking for this, to make sure you're successful in making this consistent?

https://codereview.chromium.org/136693004/diff/100001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/136693004/diff/100001/base/file_util_win.cc#n...
base/file_util_win.cc:737: if (0 == ::CopyFile(from_path.value().c_str(), dest,
false)) {
nit: `::CopyFile(...) == 0`; no yoda conditions in chrome

https://codereview.chromium.org/136693004/diff/100001/base/file_util_win.cc#n...
base/file_util_win.cc:742: if (attrs==INVALID_FILE_ATTRIBUTES) {
nit: spaces around ==

[M-A Ruel, 2014-01-14 14:59:38]

On 2014/01/14 02:30:33, Nico wrote:
> OS X also copies the the acl bits (see http://www.manpagez.com/man/3/copyfile/
,
> we use COPYFILE_ALL).

I had referenced the doc in my previous message.

> Having this consistent across platforms makes sense though – maybe add a test
> checking for this, to make sure you're successful in making this consistent?

Done. I tested it manually on OSX, Ubuntu and Windows 7.

https://codereview.chromium.org/136693004/diff/100001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/136693004/diff/100001/base/file_util_win.cc#n...
base/file_util_win.cc:737: if (0 == ::CopyFile(from_path.value().c_str(), dest,
false)) {
On 2014/01/14 02:30:33, Nico wrote:
> nit: `::CopyFile(...) == 0`; no yoda conditions in chrome

Done.

https://codereview.chromium.org/136693004/diff/100001/base/file_util_win.cc#n...
base/file_util_win.cc:742: if (attrs==INVALID_FILE_ATTRIBUTES) {
On 2014/01/14 02:30:33, Nico wrote:
> nit: spaces around ==

Done.

https://codereview.chromium.org/136693004/diff/190001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

https://codereview.chromium.org/136693004/diff/190001/base/file_util_unittest...
base/file_util_unittest.cc:1442: // On OSX, file mode is copied.
Note that I am not changing this behavior, only documenting it.

[M-A Ruel, 2014-01-14 17:41:20]

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
is a real bug, looking into it.

[Nico, 2014-01-14 19:52:11]

So it sounds like this CL changes things from "2 platforms keep attributes, 1
doesn't" to "1 platform keeps attributes, 2 do". It's not obvious to me why
that's better . The motivation seems to be some swarming thing – is that not a
problem on OS X?

[M-A Ruel, 2014-01-14 22:40:38]

On 2014/01/14 19:52:11, Nico wrote:
> So it sounds like this CL changes things from "2 platforms keep attributes, 1
> doesn't" to "1 platform keeps attributes, 2 do". It's not obvious to me why
> that's better . The motivation seems to be some swarming thing – is that
not a
> problem on OS X?

On anything-but-Windows, deleting a file require write access on the directory.
On Windows, deleting a file require not having the RO bit on the file.
CopyFile() affects the file but not the directory.

This has wide implications in the unit tests. CopyFile() is mostly (but not
exclusively) used in unit tests.

[Nico, 2014-01-15 05:38:20]

On 2014/01/14 22:40:38, M-A Ruel wrote:
> On 2014/01/14 19:52:11, Nico wrote:
> > So it sounds like this CL changes things from "2 platforms keep attributes,
1
> > doesn't" to "1 platform keeps attributes, 2 do". It's not obvious to me why
> > that's better . The motivation seems to be some swarming thing – is
that
> not a
> > problem on OS X?
> 
> On anything-but-Windows, deleting a file require write access on the
directory.
> On Windows, deleting a file require not having the RO bit on the file.
> CopyFile() affects the file but not the directory.
> 
> This has wide implications in the unit tests. CopyFile() is mostly (but not
> exclusively) used in unit tests.

^ lgtm with this in the CL description.

[M-A Ruel, 2014-01-15 15:30:21]

On 2014/01/15 05:38:20, Nico wrote:
> ^ lgtm with this in the CL description.

Done. I also committed a swarming_client DEPS roll at
https://codereview.chromium.org/135193003 which should fix the bug here with
android_dbg_triggered_tests. I ran a try job to see if it fixes this specific
error. I'll wait for this before committing.

[M-A Ruel, 2014-01-15 18:45:42]

On 2014/01/15 15:30:21, M-A Ruel wrote:
> On 2014/01/15 05:38:20, Nico wrote:
> > ^ lgtm with this in the CL description.
> 
> Done. I also committed a swarming_client DEPS roll at
> https://codereview.chromium.org/135193003 which should fix the bug here with
> android_dbg_triggered_tests. I ran a try job to see if it fixes this specific
> error. I'll wait for this before committing.

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
passed, media_unittests failure is unrelated. Ignore the 2 other jobs, I had
forgot to specify -r 244886 on them.

Committing.

[M-A Ruel, 2014-01-15 18:48:46]

Committed patchset #3 manually as r244947 (presubmit successful).