Improve temp dir handling for child procs on Windows.

Tests may leak files/directories in temp due to ScopedTempDir lifetime
issues (see linked bug) or as a result of crashes. This CL augments the
TestLauncher to handle such leaks a bit better. When launching a child
proc on Windows, the TestLauncher now directs that child to its own
dedicated temporary directory. Upon child exit, the launcher checks for
leaked files/directories in that dir and deletes the dir. On test
iteration completion, the launcher will emit a list of the tests that
produced leaks when run with --test-launcher-print-temp-leaks that looks
a lot like this:

ERROR: 2 files and/or directories were left behind in the temporary directory by one or more of these tests: PaymentManagerTest.ClearPaymentInstruments:PermissionControllerImplTest.ResettingOverridesForwardsReset:PermissionControllerImplTest.SettingOverridesForwardsUpdates:PermissionControllerImplTest.RequestPermissionsDelegatesIffMissingOverrides:PermissionControllerImplTest.GetPermissionStatusDelegatesIffNoOverrides:PermissionControllerImplTest.GetPermissionStatusForFrameDelegatesIffNoOverrides:PermissionControllerImplTest.NotifyChangedSubscriptionsCallsOnChangeOnly:PermissionControllerImplTest.PermissionsCannotBeOverriddenIfNotOverridable:PermissionControllerImplTest.GrantPermissionsReturnsStatusesBeingSetIfOverridable:PictureInPictureServiceImplTest.EnterPictureInPicture

Curious developers can binary search in that short list of tests to find
the one(s) that are leaking and clean them up.

This is a reland of https://crrev.com/728475 with macOS support
disabled and a new unit test.

BUG=546640

Change-Id: I93bef02a6282476b968ebd3938bc4d07357b88d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1991565
Auto-Submit: Greg Thompson <grt@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729785}

base/test/launcher/test_launcher.cc

@@ -15,6 +15,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/environment.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
@@ -465,10 +466,36 @@ struct ChildProcessResults {
   int exit_code;
 };
 
+// Returns the path to a temporary directory within |task_temp_dir| for the
+// child process of index |child_index|, or an empty FilePath if per-child temp
+// dirs are not supported.
+FilePath CreateChildTempDirIfSupported(const FilePath& task_temp_dir,
+                                       int child_index) {
+  if (!TestLauncher::SupportsPerChildTempDirs())
+    return FilePath();
+  FilePath child_temp = task_temp_dir.AppendASCII(NumberToString(child_index));
+  CHECK(CreateDirectoryAndGetError(child_temp, nullptr));
+  return child_temp;
+}
+
+// Adds the platform-specific variable setting |temp_dir| as a process's
+// temporary directory to |environment|.
+void SetTemporaryDirectory(const FilePath& temp_dir,
+                           EnvironmentMap* environment) {
+#if defined(OS_WIN)
+  environment->emplace(L"TMP", temp_dir.value());
+#elif defined(OS_MACOSX)
+  environment->emplace("MAC_CHROMIUM_TMPDIR", temp_dir.value());
+#elif defined(OS_POSIX) || defined(OS_FUCHSIA)
+  environment->emplace("TMPDIR", temp_dir.value());
+#endif
+}
+
 // This launches the child test process, waits for it to complete,
 // and returns child process results.
 ChildProcessResults DoLaunchChildTestProcess(
     const CommandLine& command_line,
+    const FilePath& process_temp_dir,
     TimeDelta timeout,
     const TestLauncher::LaunchOptions& test_launch_options,
     bool redirect_stdio,
@@ -486,6 +513,10 @@ ChildProcessResults DoLaunchChildTestProcess(
   }
 
   LaunchOptions options;
+
+  // Tell the child process to use its designated temporary directory.
+  if (!process_temp_dir.empty())
+    SetTemporaryDirectory(process_temp_dir, &options.environment);
 #if defined(OS_WIN)
 
   options.inherit_mode = test_launch_options.inherit_mode;
@@ -581,25 +612,26 @@ class TestRunner {
  private:
   // Called to check if the next batch has to run on the same
   // sequence task runner and using the same temporary directory.
-  bool ShouldReuseStateFromLastBatch(
+  static bool ShouldReuseStateFromLastBatch(
       const std::vector<std::string>& test_names) {
     return test_names.size() == 1u &&
            test_names.front().find(kPreTestPrefix) != std::string::npos;
   }
 
-  // Launch next child process on |task_runner|,
-  // and clear |temp_dir| from previous process.
-  void LaunchNextTask(scoped_refptr<TaskRunner> task_runner, FilePath temp_dir);
+  // Launches the next child process on |task_runner| and clears
+  // |last_task_temp_dir| from the previous task.
+  void LaunchNextTask(scoped_refptr<TaskRunner> task_runner,
+                      const FilePath& last_task_temp_dir);
 
-  // Forward |temp_dir| and Launch next task on main thread.
+  // Forwards |last_task_temp_dir| and launches the next task on main thread.
   // The method is called on |task_runner|.
   void ClearAndLaunchNext(scoped_refptr<TaskRunner> main_thread_runner,
                           scoped_refptr<TaskRunner> task_runner,
-                          const FilePath& temp_dir) {
+                          const FilePath& last_task_temp_dir) {
     main_thread_runner->PostTask(
         FROM_HERE,
         BindOnce(&TestRunner::LaunchNextTask, weak_ptr_factory_.GetWeakPtr(),
-                 task_runner, temp_dir));
+                 task_runner, last_task_temp_dir));
   }
 
   ThreadChecker thread_checker_;
@@ -639,12 +671,13 @@ void TestRunner::Run(const std::vector<std::string>& test_names) {
 }
 
 void TestRunner::LaunchNextTask(scoped_refptr<TaskRunner> task_runner,
-                                FilePath temp_dir) {
+                                const FilePath& last_task_temp_dir) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // delete previous temporary directory
-  if (!temp_dir.empty() && !DeleteFileRecursively(temp_dir)) {
+  if (!last_task_temp_dir.empty() &&
+      !DeleteFileRecursively(last_task_temp_dir)) {
     // This needs to be non-fatal at least for Windows.
-    LOG(WARNING) << "Failed to delete " << temp_dir.AsUTF8Unsafe();
+    LOG(WARNING) << "Failed to delete " << last_task_temp_dir.AsUTF8Unsafe();
   }
 
   // No more tests to run, finish sequence.
@@ -656,10 +689,17 @@ void TestRunner::LaunchNextTask(scoped_refptr<TaskRunner> task_runner,
     return;
   }
 
-  CreateNewTempDirectory(FilePath::StringType(), &temp_dir);
+  // Create a temporary directory for this task. This directory will hold the
+  // flags and results files for the child processes as well as their User Data
+  // dir, where appropriate. For platforms that support per-child temp dirs,
+  // this directory will also contain one subdirectory per child for that
+  // child's process-wide temp dir.
+  base::FilePath task_temp_dir;
+  CHECK(CreateNewTempDirectory(FilePath::StringType(), &task_temp_dir));
   bool post_to_current_runner = true;
   size_t batch_size = (batch_size_ == 0) ? tests_to_run_.size() : batch_size_;
 
+  int child_index = 0;
   while (post_to_current_runner && !tests_to_run_.empty()) {
     batch_size = std::min(batch_size, tests_to_run_.size());
     std::vector<std::string> batch(tests_to_run_.rbegin(),
@@ -668,13 +708,29 @@ void TestRunner::LaunchNextTask(scoped_refptr<TaskRunner> task_runner,
     task_runner->PostTask(
         FROM_HERE,
         BindOnce(&TestLauncher::LaunchChildGTestProcess, Unretained(launcher_),
-                 ThreadTaskRunnerHandle::Get(), batch, temp_dir));
+                 ThreadTaskRunnerHandle::Get(), batch, task_temp_dir,
+                 CreateChildTempDirIfSupported(task_temp_dir, child_index++)));
     post_to_current_runner = ShouldReuseStateFromLastBatch(batch);
   }
   task_runner->PostTask(
       FROM_HERE,
       BindOnce(&TestRunner::ClearAndLaunchNext, Unretained(this),
-               ThreadTaskRunnerHandle::Get(), task_runner, temp_dir));
+               ThreadTaskRunnerHandle::Get(), task_runner, task_temp_dir));
+}
+
+// Returns the number of files and directories in |dir|, or 0 if |dir| is empty.
+int CountItemsInDirectory(const FilePath& dir) {
+  if (dir.empty())
+    return 0;
+  int items = 0;
+  FileEnumerator file_enumerator(
+      dir, /*recursive=*/false,
+      FileEnumerator::FILES | FileEnumerator::DIRECTORIES);
+  for (FilePath name = file_enumerator.Next(); !name.empty();
+       name = file_enumerator.Next()) {
+    ++items;
+  }
+  return items;
 }
 
 }  // namespace
@@ -868,10 +924,11 @@ bool TestLauncher::Run(CommandLine* command_line) {
 void TestLauncher::LaunchChildGTestProcess(
     scoped_refptr<TaskRunner> task_runner,
     const std::vector<std::string>& test_names,
-    const FilePath& temp_dir) {
+    const FilePath& task_temp_dir,
+    const FilePath& child_temp_dir) {
   FilePath result_file;
-  CommandLine cmd_line =
-      launcher_delegate_->GetCommandLine(test_names, temp_dir, &result_file);
+  CommandLine cmd_line = launcher_delegate_->GetCommandLine(
+      test_names, task_temp_dir, &result_file);
 
   // Record the exact command line used to launch the child.
   CommandLine new_command_line(
@@ -880,8 +937,9 @@ void TestLauncher::LaunchChildGTestProcess(
   options.flags = launcher_delegate_->GetLaunchOptions();
 
   ChildProcessResults process_results = DoLaunchChildTestProcess(
-      new_command_line, launcher_delegate_->GetTimeout() * test_names.size(),
-      options, redirect_stdio_, launcher_delegate_);
+      new_command_line, child_temp_dir,
+      launcher_delegate_->GetTimeout() * test_names.size(), options,
+      redirect_stdio_, launcher_delegate_);
 
   // Invoke ProcessTestResults on the original thread, not
   // on a worker pool thread.
@@ -890,7 +948,8 @@ void TestLauncher::LaunchChildGTestProcess(
       BindOnce(&TestLauncher::ProcessTestResults, Unretained(this), test_names,
                result_file, process_results.output_file_contents,
                process_results.elapsed_time, process_results.exit_code,
-               process_results.was_timeout));
+               process_results.was_timeout,
+               CountItemsInDirectory(child_temp_dir)));
 }
 
 // Determines which result status will be assigned for missing test results.
@@ -920,7 +979,8 @@ void TestLauncher::ProcessTestResults(
     const std::string& output,
     TimeDelta elapsed_time,
     int exit_code,
-    bool was_timeout) {
+    bool was_timeout,
+    int leaked_items) {
   std::vector<TestResult> test_results;
   bool crashed = false;
   bool have_test_results =
@@ -1000,6 +1060,9 @@ void TestLauncher::ProcessTestResults(
     i.output_snippet = GetTestOutputSnippet(i, output);
   }
 
+  if (leaked_items)
+    results_tracker_.AddLeakedItems(leaked_items, test_names);
+
   launcher_delegate_->ProcessTestResults(final_results, elapsed_time);
 
   for (const auto& result : final_results)

base/test/launcher/test_launcher.h

@@ -135,18 +135,32 @@ class TestLauncher {
 
   // Launches a child process (assumed to be gtest-based binary) which runs
   // tests indicated by |test_names|.
-  // |task_runner| is used to post results back to the launcher
-  // on the main thread. |temp_dir| is used for child process files,
-  // such as user data, result file, and flag_file.
+  // |task_runner| is used to post results back to the launcher on the main
+  // thread. |task_temp_dir| is used for child process files such as user data,
+  // result file, and flag_file. |child_temp_dir|, if not empty, specifies a
+  // directory (within task_temp_dir) that the child process will use as its
+  // process-wide temporary directory.
   // virtual to mock in testing.
   virtual void LaunchChildGTestProcess(
       scoped_refptr<TaskRunner> task_runner,
       const std::vector<std::string>& test_names,
-      const FilePath& temp_dir);
+      const FilePath& task_temp_dir,
+      const FilePath& child_temp_dir);
 
   // Called when a test has finished running.
   void OnTestFinished(const TestResult& result);
 
+  // Returns true if child test processes should have dedicated temporary
+  // directories.
+  static constexpr bool SupportsPerChildTempDirs() {
+#if defined(OS_WIN)
+    return true;
+#else
+    // TODO(https://crbug.com/1038857): Enable for macOS, Linux, and Fuchsia.
+    return false;
+#endif
+  }
+
  private:
   bool Init(CommandLine* command_line) WARN_UNUSED_RESULT;
 
@@ -206,12 +220,15 @@ class TestLauncher {
   // EXPECT/ASSERT/DCHECK statements. Test launcher parses that
   // file to get additional information about test run (status,
   // error-messages, stack-traces and file/line for failures).
+  // |leaked_items| is the number of files and/or directories remaining in the
+  // child process's temporary directory upon its termination.
   void ProcessTestResults(const std::vector<std::string>& test_names,
                           const FilePath& result_file,
                           const std::string& output,
                           TimeDelta elapsed_time,
                           int exit_code,
-                          bool was_timeout);
+                          bool was_timeout,
+                          int leaked_items);
 
   // Make sure we don't accidentally call the wrong methods e.g. on the worker
   // pool thread.  Should be the first member so that it's destroyed last: when

base/test/launcher/test_results_tracker.cc

@@ -22,6 +22,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/test/gtest_util.h"
 #include "base/test/launcher/test_launcher.h"
+#include "base/test/test_switches.h"
 #include "base/time/time.h"
 #include "base/values.h"
 
@@ -166,6 +167,9 @@ bool TestResultsTracker::Init(const CommandLine& command_line) {
     return false;
   }
 
+  print_temp_leaks_ =
+      command_line.HasSwitch(switches::kTestLauncherPrintTempLeaks);
+
   if (!command_line.HasSwitch(kGTestOutputFlag))
     return true;
 
@@ -270,6 +274,13 @@ void TestResultsTracker::AddTestResult(const TestResult& result) {
   aggregate_test_result.test_results.push_back(result);
 }
 
+void TestResultsTracker::AddLeakedItems(
+    int count,
+    const std::vector<std::string>& test_names) {
+  DCHECK(count);
+  per_iteration_data_.back().leaked_temp_items.emplace_back(count, test_names);
+}
+
 void TestResultsTracker::GeneratePlaceholderIteration() {
   CHECK(thread_checker_.CalledOnValidThread());
 
@@ -316,6 +327,13 @@ void TestResultsTracker::PrintSummaryOfCurrentIteration() const {
              "had unknown result");
   PrintTests(tests_by_status[TestResult::TEST_NOT_RUN].begin(),
              tests_by_status[TestResult::TEST_NOT_RUN].end(), "not run");
+
+  if (print_temp_leaks_) {
+    for (const auto& leaking_tests :
+         per_iteration_data_.back().leaked_temp_items) {
+      PrintLeaks(leaking_tests.first, leaking_tests.second);
+    }
+  }
 }
 
 void TestResultsTracker::PrintSummaryOfAllIterations() const {
@@ -574,6 +592,16 @@ void TestResultsTracker::PrintTests(InputIterator first,
   fflush(stdout);
 }
 
+void TestResultsTracker::PrintLeaks(
+    int count,
+    const std::vector<std::string>& test_names) const {
+  fprintf(stdout,
+          "ERROR: %d files and/or directories were left behind in the temporary"
+          " directory by one or more of these tests: %s\n",
+          count, JoinString(test_names, ":").c_str());
+  fflush(stdout);
+}
+
 TestResultsTracker::AggregateTestResult::AggregateTestResult() = default;
 
 TestResultsTracker::AggregateTestResult::AggregateTestResult(

base/test/launcher/test_results_tracker.h

@@ -61,6 +61,10 @@ class TestResultsTracker {
   // Adds |result| to the stored test results.
   void AddTestResult(const TestResult& result);
 
+  // Adds to the current iteration the fact that |count| items were leaked by
+  // one or more tests in |test_names| in its temporary directory.
+  void AddLeakedItems(int count, const std::vector<std::string>& test_names);
+
   // Even when no iterations have occurred, we still want to generate output
   // data with "NOTRUN" status for each test. This method generates a
   // placeholder iteration. The first iteration will overwrite the data in the
@@ -101,6 +105,7 @@ class TestResultsTracker {
   void PrintTests(InputIterator first,
                   InputIterator last,
                   const std::string& description) const;
+  void PrintLeaks(int count, const std::vector<std::string>& test_names) const;
 
   struct AggregateTestResult {
     AggregateTestResult();
@@ -118,6 +123,9 @@ class TestResultsTracker {
     // Aggregate test results grouped by full test name.
     typedef std::map<std::string, AggregateTestResult> ResultsMap;
     ResultsMap results;
+
+    // A sequence of tests that leaked files/dirs in their temp directory.
+    std::vector<std::pair<int, std::vector<std::string>>> leaked_temp_items;
   };
 
   struct CodeLocation {
@@ -130,6 +138,9 @@ class TestResultsTracker {
 
   ThreadChecker thread_checker_;
 
+  // Print tests that leak files and/or directories in their temp dir.
+  bool print_temp_leaks_ = false;
+
   // Set of global tags, i.e. strings indicating conditions that apply to
   // the entire test run.
   std::set<std::string> global_tags_;

base/test/launcher/unit_test_launcher.cc

@@ -104,7 +104,11 @@ void PrintUsage() {
           "    Sets the shard index to run to N (from 0 to TOTAL - 1).\n"
           "\n"
           "  --dont-use-job-objects\n"
-          "    Avoids using job objects in Windows.\n");
+          "    Avoids using job objects in Windows.\n"
+          "\n"
+          "  --test-launcher-print-temp-leaks\n"
+          "    Prints information about leaked files and/or directories in\n"
+          "    child process's temporary directories (Windows and macOS).\n");
   fflush(stdout);
 }
 

base/test/test_switches.cc

@@ -58,6 +58,11 @@ const char switches::kIsolatedScriptTestLauncherRetryLimit[] =
 const char switches::kTestLauncherSummaryOutput[] =
     "test-launcher-summary-output";
 
+// Causes the test launcher to print information about leaked files and/or
+// directories in child process's temporary directories.
+const char switches::kTestLauncherPrintTempLeaks[] =
+    "test-launcher-print-temp-leaks";
+
 // Flag controlling when test stdio is displayed as part of the launcher's
 // standard output.
 const char switches::kTestLauncherPrintTestStdio[] =

base/test/test_switches.h

@@ -25,6 +25,7 @@ extern const char kTestLauncherOutput[];
 extern const char kTestLauncherRetryLimit[];
 extern const char kIsolatedScriptTestLauncherRetryLimit[];
 extern const char kTestLauncherSummaryOutput[];
+extern const char kTestLauncherPrintTempLeaks[];
 extern const char kTestLauncherPrintTestStdio[];
 extern const char kTestLauncherPrintWritablePath[];
 extern const char kTestLauncherShardIndex[];

content/public/test/test_launcher.cc

@@ -116,7 +116,11 @@ void PrintUsage() {
           "    Sets the total number of shards to N.\n"
           "\n"
           "  --test-launcher-shard-index=N\n"
-          "    Sets the shard index to run to N (from 0 to TOTAL - 1).\n");
+          "    Sets the shard index to run to N (from 0 to TOTAL - 1).\n"
+          "\n"
+          "  --test-launcher-print-temp-leaks\n"
+          "    Prints information about leaked files and/or directories in\n"
+          "    child process's temporary directories (Windows and macOS).\n");
 }
 
 // Implementation of base::TestLauncherDelegate. This is also a test launcher,