Make sure tests delete temp files on Windows

base/test/launcher/test_launcher.cc

Index: base/test/launcher/test_launcher.cc
diff --git a/base/test/launcher/test_launcher.cc b/base/test/launcher/test_launcher.cc
index 2f22d32d50a4dba63b902aa4006e94a22db08071..b6c4d0c87807b7dcd16a86b38324e3b4a6eb5641 100644
--- a/base/test/launcher/test_launcher.cc
+++ b/base/test/launcher/test_launcher.cc
@@ -57,6 +57,7 @@
 #endif
 
 #if defined(OS_WIN)
+#include <limits>

[Paweł Hajdan Jr., 2017/02/01 11:10:48]

nit: Include system headers before chromium headers (except the first one of
course), i.e. right before <memory>. Probably no need to keep that
windows-specific.

 #include "base/win/windows_version.h"
 #endif
 
@@ -170,6 +171,53 @@ void OnShutdownPipeReadable() {
 }
 #endif  // defined(OS_POSIX)
 
+#if defined(OS_WIN)
+// Does the same as base::ReadFileToString() but uses FILE_SHARE_DELETE flag

[Paweł Hajdan Jr., 2017/02/01 11:10:48]

I'm worried about duplication here.

Actually from the bug, it seems a better solution would be the following:

"The launcher should use base::Launch's |handles_to_inherit| so rather than
|inherit_handles| so that each child only inherits its one output handle."

Let me know if you'd be interested in writing a patch to that effect.

[Tomasz Moniuszko, 2017/02/02 16:16:43]

I tried to add the handle to temp file to |handles_to_inherit| vector. With this
change temp files still persist. Additionally setting
options.inherit_handles=false seem to solve the temp files problem (although I
haven't tested it intensively) but prevents parent process from printing details
about test failures - just like in case of clearing HANDLE_FLAG_INHERIT for
handle (see my other comment).

Also, I wonder how this issue is related to child processes. My changes in this
CL don't change the way of passing handle to child process. It changes the way
how parent process opens the output file for reading after child process is
finished. ReadFileToString() opens file without FILE_SHARE_DELETE flag. It
closes the handle before calling DeleteFile() so everything should be just fine.
For some reason it's not. Maybe it's somehow related to system caching if
FILE_ATTRIBUTE_TEMPORARY is used:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).as...
Adding FILE_SHARE_DELETE flag solves this problem.

[grt (UTC plus 2), 2017/02/03 09:53:23]

I wrote that in the bug before realizing that specifying handles to inherit
isn't compatible with the current approach for specifying std handles.

+// when opening file.
+bool ReadFileToStringShareDelete(const FilePath& path, std::string* contents) {
+  if (contents)
+    contents->clear();
+  if (path.ReferencesParent())
+    return false;
+
+  win::ScopedHandle handle(
+      CreateFile(path.value().c_str(), GENERIC_READ,
+                 FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
+                 OPEN_EXISTING, FILE_ATTRIBUTE_TEMPORARY, NULL));
+  if (!handle.IsValid())
+    return false;
+
+  const size_t max_size = std::numeric_limits<size_t>::max();
+  const size_t kBufferSize = 1 << 16;
+  std::unique_ptr<char[]> buf(new char[kBufferSize]);
+  DWORD len;
+  size_t size = 0;
+  bool read_status = true;
+
+  // Many files supplied in |path| have incorrect size (proc files etc).
+  // Hence, the file is read sequentially as opposed to a one-shot read.
+  do {
+    read_status =
+        !!::ReadFile(handle.Get(), buf.get(), kBufferSize, &len, NULL);
+    if (!read_status || len <= 0)
+      break;
+    if (contents) {
+      contents->append(buf.get(),
+                       std::min(static_cast<size_t>(len), max_size - size));
+    }
+
+    if ((max_size - size) < len) {
+      read_status = false;
+      break;
+    }
+
+    size += len;
+  } while (true);
+
+  return read_status;
+}
+#endif  // OS_WIN
+
 // Parses the environment variable var as an Int32.  If it is unset, returns
 // true.  If it is set, unsets it then converts it to Int32 before
 // returning it in |result|.  Returns true on success.
@@ -453,12 +501,13 @@ void DoLaunchChildTestProcess(
   }
 
   std::string output_file_contents;
+#if defined(OS_WIN)
+  CHECK(ReadFileToStringShareDelete(output_file, &output_file_contents));
+#else
   CHECK(ReadFileToString(output_file, &output_file_contents));
+#endif
 
-  if (!DeleteFile(output_file, false)) {
-    // This needs to be non-fatal at least for Windows.
-    LOG(WARNING) << "Failed to delete " << output_file.AsUTF8Unsafe();
-  }
+  CHECK(DeleteFile(output_file, false));
 
   // Run target callback on the thread it was originating from, not on
   // a worker pool thread.