Only do aggressive metro viewer termination in tests

The previous patch https://codereview.chromium.org/584513004 ran on all
metro viewer shutdowns, which would lead to Alt-F4 not being processed
which means that the proper metro process shutdown wouldn't happen,
potentially leaving the "ghost" icon.

Instead, only do the aggressive termination from the process_host side
and only when running test binaries (where we're concerned about having
the viewer be able to run again immediately.)

R=ananta@chromium.org
BUG=411147,416356

Review URL: https://codereview.chromium.org/584213002

Cr-Commit-Position: refs/heads/master@{#295995}

ash/test/ash_test_base.cc

@@ -39,7 +39,6 @@
 
 #if defined(OS_WIN)
 #include "ash/test/test_metro_viewer_process_host.h"
-#include "base/test/test_process_killer_win.h"
 #include "base/win/metro.h"
 #include "base/win/windows_version.h"
 #include "ui/aura/remote_window_tree_host_win.h"
@@ -176,15 +175,10 @@ void AshTestBase::TearDown() {
 #if defined(OS_WIN)
   ui::test::SetUsePopupAsRootWindowForTest(false);
   // Kill the viewer process if we spun one up.
-  metro_viewer_host_.reset();
-
-  // Clean up any dangling viewer processes as the metro APIs sometimes leave
-  // zombies behind. A default browser process in metro will have the
-  // following command line arg so use that to avoid killing all processes named
-  // win8::test::kDefaultTestExePath.
-  const wchar_t kViewerProcessArgument[] = L"DefaultBrowserServer";
-  base::KillAllNamedProcessesWithArgument(win8::test::kDefaultTestExePath,
-                                          kViewerProcessArgument);
+  if (metro_viewer_host_) {
+    metro_viewer_host_->TerminateViewer();
+    metro_viewer_host_.reset();
+  }
 #endif
 
   event_generator_.reset();

ash/test/test_metro_viewer_process_host.cc

@@ -22,6 +22,22 @@ TestMetroViewerProcessHost::TestMetroViewerProcessHost(
 TestMetroViewerProcessHost::~TestMetroViewerProcessHost() {
 }
 
+void TestMetroViewerProcessHost::TerminateViewer() {
+  base::ProcessId viewer_process_id = GetViewerProcessId();
+  if (viewer_process_id != base::kNullProcessId) {
+    base::ProcessHandle viewer_process = NULL;
+    base::OpenProcessHandleWithAccess(
+        viewer_process_id,
+        PROCESS_QUERY_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE,
+        &viewer_process);
+    if (viewer_process) {
+      ::TerminateProcess(viewer_process, 0);
+      ::WaitForSingleObject(viewer_process, INFINITE);
+      ::CloseHandle(viewer_process);
+    }
+  }
+}
+
 void TestMetroViewerProcessHost::OnChannelError() {
   closed_unexpectedly_ = true;
   aura::RemoteWindowTreeHostWin::Instance()->Disconnected();

ash/test/test_metro_viewer_process_host.h

@@ -20,6 +20,10 @@ class TestMetroViewerProcessHost : public win8::MetroViewerProcessHost {
 
   bool closed_unexpectedly() { return closed_unexpectedly_; }
 
+  // Forcibly terminate the viewer. Used on completion of tests to ensure that
+  // it's gone (quickly) so that we can start the next test immediately.
+  void TerminateViewer();
+
  private:
   // win8::MetroViewerProcessHost implementation
   virtual void OnChannelError() OVERRIDE;

base/base.gyp

@@ -939,8 +939,6 @@
         'test/test_listener_ios.mm',
         'test/test_pending_task.cc',
         'test/test_pending_task.h',
-        'test/test_process_killer_win.cc',
-        'test/test_process_killer_win.h',
         'test/test_reg_util_win.cc',
         'test/test_reg_util_win.h',
         'test/test_shortcut_win.cc',

base/test/BUILD.gn

@@ -76,8 +76,6 @@ source_set("test_support") {
     "test_listener_ios.mm",
     "test_pending_task.cc",
     "test_pending_task.h",
-    "test_process_killer_win.cc",
-    "test_process_killer_win.h",
     "test_reg_util_win.cc",
     "test_reg_util_win.h",
     "test_shortcut_win.cc",

base/test/test_process_killer_win.cc

-// Copyright (c) 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/test_process_killer_win.h"
-
-#include <windows.h>
-#include <winternl.h>
-
-#include <algorithm>
-
-#include "base/logging.h"
-#include "base/process/kill.h"
-#include "base/process/process_iterator.h"
-#include "base/strings/string_util.h"
-#include "base/win/scoped_handle.h"
-
-namespace {
-
-typedef LONG WINAPI
-NtQueryInformationProcess(
-  IN HANDLE ProcessHandle,
-  IN PROCESSINFOCLASS ProcessInformationClass,
-  OUT PVOID ProcessInformation,
-  IN ULONG ProcessInformationLength,
-  OUT PULONG ReturnLength OPTIONAL
-);
-
-// Get the function pointer to NtQueryInformationProcess in NTDLL.DLL
-static bool GetQIP(NtQueryInformationProcess** qip_func_ptr) {
-  static NtQueryInformationProcess* qip_func =
-      reinterpret_cast<NtQueryInformationProcess*>(
-          GetProcAddress(GetModuleHandle(L"ntdll.dll"),
-          "NtQueryInformationProcess"));
-  DCHECK(qip_func) << "Could not get pointer to NtQueryInformationProcess.";
-  *qip_func_ptr = qip_func;
-  return qip_func != NULL;
-}
-
-// Get the command line of a process
-bool GetCommandLineForProcess(uint32 process_id, base::string16* cmd_line) {
-  DCHECK(process_id != 0);
-  DCHECK(cmd_line);
-
-  // Open the process
-  base::win::ScopedHandle process_handle(::OpenProcess(
-      PROCESS_QUERY_INFORMATION | PROCESS_VM_READ,
-      false,
-      process_id));
-  if (!process_handle) {
-    DLOG(ERROR) << "Failed to open process " << process_id << ", last error = "
-                << GetLastError();
-  }
-
-  // Obtain Process Environment Block
-  NtQueryInformationProcess* qip_func = NULL;
-  if (process_handle) {
-    GetQIP(&qip_func);
-  }
-
-  // Read the address of the process params from the peb.
-  DWORD process_params_address = 0;
-  if (qip_func) {
-    PROCESS_BASIC_INFORMATION info = { 0 };
-    // NtQueryInformationProcess returns an NTSTATUS for whom negative values
-    // are negative. Just check for that instead of pulling in DDK macros.
-    if ((qip_func(process_handle.Get(),
-                  ProcessBasicInformation,
-                  &info,
-                  sizeof(info),
-                  NULL)) < 0) {
-      DLOG(ERROR) << "Failed to invoke NtQueryProcessInformation, last error = "
-                  << GetLastError();
-    } else {
-      BYTE* peb = reinterpret_cast<BYTE*>(info.PebBaseAddress);
-
-      // The process command line parameters are (or were once) located at
-      // the base address of the PEB + 0x10 for 32 bit processes. 64 bit
-      // processes have a different PEB struct as per
-      // http://msdn.microsoft.com/en-us/library/aa813706(VS.85).aspx.
-      // TODO(robertshield): See about doing something about this.
-      SIZE_T bytes_read = 0;
-      if (!::ReadProcessMemory(process_handle.Get(),
-                               peb + 0x10,
-                               &process_params_address,
-                               sizeof(process_params_address),
-                               &bytes_read)) {
-        DLOG(ERROR) << "Failed to read process params address, last error = "
-                    << GetLastError();
-      }
-    }
-  }
-
-  // Copy all the process parameters into a buffer.
-  bool success = false;
-  base::string16 buffer;
-  if (process_params_address) {
-    SIZE_T bytes_read;
-    RTL_USER_PROCESS_PARAMETERS params = { 0 };
-    if (!::ReadProcessMemory(process_handle.Get(),
-                             reinterpret_cast<void*>(process_params_address),
-                             &params,
-                             sizeof(params),
-                             &bytes_read)) {
-      DLOG(ERROR) << "Failed to read RTL_USER_PROCESS_PARAMETERS, "
-                  << "last error = " << GetLastError();
-    } else {
-      // Read the command line parameter
-      const int max_cmd_line_len = std::min(
-          static_cast<int>(params.CommandLine.MaximumLength),
-          4096);
-      buffer.resize(max_cmd_line_len + 1);
-      if (!::ReadProcessMemory(process_handle.Get(),
-                               params.CommandLine.Buffer,
-                               &buffer[0],
-                               max_cmd_line_len,
-                               &bytes_read)) {
-        DLOG(ERROR) << "Failed to copy process command line, "
-                    << "last error = " << GetLastError();
-      } else {
-        *cmd_line = buffer;
-        success = true;
-      }
-    }
-  }
-
-  return success;
-}
-
-// Used to filter processes by process ID.
-class ArgumentFilter : public base::ProcessFilter {
- public:
-  explicit ArgumentFilter(const base::string16& argument)
-      : argument_to_find_(argument) {}
-
-  // Returns true to indicate set-inclusion and false otherwise.  This method
-  // should not have side-effects and should be idempotent.
-  virtual bool Includes(const base::ProcessEntry& entry) const {
-    bool found = false;
-    base::string16 command_line;
-    if (GetCommandLineForProcess(entry.pid(), &command_line)) {
-      base::string16::const_iterator it =
-          std::search(command_line.begin(),
-                      command_line.end(),
-                      argument_to_find_.begin(),
-                      argument_to_find_.end(),
-                      base::CaseInsensitiveCompareASCII<wchar_t>());
-      found = (it != command_line.end());
-    }
-    return found;
-  }
-
- protected:
-  base::string16 argument_to_find_;
-};
-
-}  // namespace
-
-namespace base {
-
-bool KillAllNamedProcessesWithArgument(const string16& process_name,
-                                       const string16& argument) {
-  ArgumentFilter argument_filter(argument);
-  return base::KillProcesses(process_name, 0, &argument_filter);
-}
-
-}  // namespace base

base/test/test_process_killer_win.h

-// Copyright (c) 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.
-
-#ifndef BASE_TEST_TEST_PROCESS_KILLER_WIN_H_
-#define BASE_TEST_TEST_PROCESS_KILLER_WIN_H_
-
-#include "base/strings/string16.h"
-
-namespace base {
-
-// Kills all running processes named |process_name| that have the string
-// |argument| on their command line.
-bool KillAllNamedProcessesWithArgument(const string16& process_name,
-                                       const string16& argument);
-
-}  // namespace base
-
-#endif  // BASE_TEST_TEST_PROCESS_KILLER_WIN_H_

win8/metro_driver/chrome_app_view_ash.cc

@@ -970,9 +970,6 @@ void ChromeAppViewAsh::OnMetroExit(MetroTerminateMethod method) {
 
     globals.app_exit->Exit();
   }
-
-  // Try really hard, see http://crbug.com/411147 for details.
-  ::TerminateProcess(::GetCurrentProcess(), 0);
 }
 
 void ChromeAppViewAsh::OnInputSourceChanged() {