Make sure tests delete temp files on Windows

base/test/launcher/test_launcher.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/test/launcher/test_launcher.h"
 
 #include <memory>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
@@ skipping 39 matching lines @@
 #include <fcntl.h>
 
 #include "base/files/file_descriptor_watcher_posix.h"
 #endif
 
 #if defined(OS_MACOSX)
 #include "base/mac/scoped_nsautorelease_pool.h"
 #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
 
 namespace base {
 
 // See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/nkdTP7sstSc/uT3FaE_sgkAJ .
 using ::operator<<;
 
 // The environment variable name for the total number of test shards.
 const char kTestTotalShards[] = "GTEST_TOTAL_SHARDS";
@@ skipping 93 matching lines @@
   fprintf(stdout, "\nCaught signal. Killing spawned test processes...\n");
   fflush(stdout);
 
   KillSpawnedTestProcesses();
 
   // The signal would normally kill the process, so exit now.
   _exit(1);
 }
 #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.
 bool TakeInt32FromEnvironment(const char* const var, int32_t* result) {
   std::unique_ptr<Environment> env(Environment::Create());
   std::string str_val;
 
   if (!env->GetVar(var, &str_val))
     return true;
 
@@ skipping 122 matching lines @@
 
   {
     // Note how we grab the lock before the process possibly gets created.
     // This ensures that when the lock is held, ALL the processes are registered
     // in the set.
     AutoLock lock(g_live_processes_lock.Get());
 
     process = LaunchProcess(command_line, new_options);
     if (!process.IsValid())
       return -1;
 

[grt (UTC plus 2), 2017/02/01 13:04:44]

since the problem is that new_options.stdout_handle is inherited into every
subproc, another approach is to revoke that handle's ability to be inherited
here just after the correct subprocess was launched. this is a bit hacky, but
something like:

    if (new_options.stdout_handle) {
      // Prevent leakage of this handle into other subprocs.
      ::SetHandleInformation(new_options.stdout_handle, HANDLE_FLAG_INHERIT,
                             0);
    }

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

It doesn't seem to work. I added the above code to
LaunchChildTestProcessWithOptions(). I'm still getting "Failed to delete"
warning from DoLaunchChildTestProcess(). Some temp files still stay in temp
folder when test is finished. Also test process no longer prints information
about failed test (for example when one of EXPECT_ gtest expectations fails).
Parent process just prints names of failed tests in the summary.

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

Did you try it without your other changes? I just tried locally and it seemed to
work as desired.

[Tomasz Moniuszko, 2017/02/03 10:08:48]

Yes, I did it on clean master. I used gpu_unittests for testing (I noticed that
these tests leave temp files almost every run).

Maybe I put the above code in the wrong place. Your fix is indeed much easier
than the solution I uploaded. I will close this review if your fix solves the
issue.

[grt (UTC plus 2), 2017/02/03 12:45:18]

This almost fixes it: https://codereview.chromium.org/2670213004/. See the CL
description for a taste of how tricky this is. I'll send up another CL to fix
base::OpenFile. Cheers.

     // TODO(rvargas) crbug.com/417532: Don't store process handles.
     g_live_processes.Get().insert(std::make_pair(process.Handle(),
                                                  command_line));
   }
 
   if (!launched_callback.is_null())
     launched_callback.Run(process.Handle(), process.Pid());
 
   int exit_code = 0;
   if (!process.WaitForExitWithTimeout(timeout, &exit_code)) {
@@ skipping 120 matching lines @@
   if (redirect_stdio) {
 #if defined(OS_WIN)
     FlushFileBuffers(handle.Get());
     handle.Close();
 #elif defined(OS_POSIX)
     output_file_fd.reset();
 #endif
   }
 
   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.
   task_runner->PostTask(
       FROM_HERE,
       Bind(&RunCallback, completed_callback, exit_code,
            TimeTicks::Now() - start_time, was_timeout, output_file_contents));
 }
 
 }  // namespace
@@ skipping 733 matching lines @@
   }
 
   std::string snippet(full_output.substr(run_pos));
   if (end_pos != std::string::npos)
     snippet = full_output.substr(run_pos, end_pos - run_pos);
 
   return snippet;
 }
 
 }  // namespace base