Stop WebSocker server processes certainly

chrome/test/base/ui_test_utils.cc

 // Copyright (c) 2011 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 "chrome/test/base/ui_test_utils.h"
 
+#if defined(OS_WIN)
+#include <windows.h>
+#endif
+
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/json/json_reader.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
+#include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/automation/ui_controls.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/dom_operation_notification_details.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_test_util.h"
 #include "chrome/browser/tab_contents/thumbnail_generator.h"
@@ skipping 26 matching lines @@
 #include "ui/gfx/size.h"
 
 #if defined(TOOLKIT_VIEWS)
 #include "ui/views/focus/accelerator_handler.h"
 #endif
 
 #if defined(USE_AURA)
 #include "ui/aura/desktop.h"
 #endif
 
+static const int kDefaultWsPort = 8880;
+
 namespace ui_test_utils {
 
 namespace {
 
 class DOMOperationObserver : public content::NotificationObserver {
  public:
   explicit DOMOperationObserver(RenderViewHost* render_view_host)
       : did_respond_(false) {
     registrar_.Add(this, chrome::NOTIFICATION_DOM_OPERATION_RESPONSE,
                    content::Source<RenderViewHost>(render_view_host));
@@ skipping 645 matching lines @@
     std::string newpath(oldpath);
     newpath.append(":");
     newpath.append(dir.value());
     setenv(kPythonPath, newpath.c_str(), 1);
   }
 #endif
 }
 
 }  // anonymous namespace
 
-TestWebSocketServer::TestWebSocketServer() : started_(false) {
+#if defined(OS_POSIX)
+class OrphanedWebSocketServerFilter : public base::ProcessFilter {
+ public:
+  OrphanedWebSocketServerFilter(std::string script_name, std::string port)

[Yuta Kitamura, 2011/11/23 06:06:09]

Arguments should be typed as const std::string&, because std::string argument
yields a copy.

[Takashi Toyoshima, 2011/11/23 08:53:32]

Done.

+      : script_name_(script_name),
+        port_(port) {}
+
+  virtual bool Includes(const base::ProcessEntry& entry) const {
+    // The parent process of orphaned WebSocket servers must be root process.
+    if (entry.parent_pid() != 1)
+      return false;
+
+    // Python script name must be ended with specified script name.
+    const std::string &script_name = entry.cmd_line_args()[2];
+    if (script_name.find_last_of(script_name_) != script_name.size() - 1)
+      return false;
+
+    // Check if an argument sequence like "--port 8880" exist.

[Yuta Kitamura, 2011/11/23 06:06:09]

nit: exist -> exists.

[Takashi Toyoshima, 2011/11/23 08:53:32]

Done.

+    bool found = false;
+    for (std::vector<std::string>::const_iterator it =
+        entry.cmd_line_args().begin();
+        it != entry.cmd_line_args().end();
+        ++it) {
+      if (*it == "--port") {
+        ++it;
+        if (it == entry.cmd_line_args().end() && *it == port_) {
+          found = true;
+          break;
+        }
+      }
+    }
+    return found;
+  }
+
+ private:
+  std::string script_name_;
+  std::string port_;
+  DISALLOW_COPY_AND_ASSIGN(OrphanedWebSocketServerFilter);
+};
+#endif
+
+TestWebSocketServer::TestWebSocketServer()
+    : started_(false),
+      process_handle_(base::kNullProcessHandle) {

[Yuta Kitamura, 2011/11/23 06:06:09]

Don't you have to guard process_handle_ with #if defined(OS_POSIX)?

[Takashi Toyoshima, 2011/11/23 08:53:32]

Oh, right!
Nice catch.

 }
 
 bool TestWebSocketServer::Start(const FilePath& root_directory) {
   if (started_)
     return true;
   // Append CommandLine arguments after the server script, switches won't work.
   scoped_ptr<CommandLine> cmd_line(CreateWebSocketServerCommandLine());
   cmd_line->AppendArg("--server=start");
   cmd_line->AppendArg("--chromium");
   cmd_line->AppendArg("--register_cygwin");
   cmd_line->AppendArgNative(FILE_PATH_LITERAL("--root=") +
                             root_directory.value());
+  cmd_line->AppendArg("--port=" + base::IntToString(kDefaultWsPort));
   if (!temp_dir_.CreateUniqueTempDir()) {
     LOG(ERROR) << "Unable to create a temporary directory.";
     return false;
   }
   websocket_pid_file_ = temp_dir_.path().AppendASCII("websocket.pid");
   cmd_line->AppendArgNative(FILE_PATH_LITERAL("--pidfile=") +
                             websocket_pid_file_.value());
   SetPythonPath();
+
   base::LaunchOptions options;
+
+#if defined(OS_POSIX)
+  // Try to kill any orphaned WebSocket server processes that may be running.
+  OrphanedWebSocketServerFilter filter("standalone.py",
+                                       base::IntToString(kDefaultWsPort));
+  if (!base::KillProcesses("python", -1, &filter))

[Yuta Kitamura, 2011/11/23 06:06:09]

nit: Add curly braces if the content of a block spans two or more lines.

See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals

[Takashi Toyoshima, 2011/11/23 08:53:32]

Done.

+    LOG(WARNING) <<
+        "Failed to clean up older orphaned WebSocket server instance.";
+
+  options.new_process_group = true;
+#elif defined(OS_WIN)
+  job_handle_.Set(CreateJobObject(NULL, NULL));
+  if (!job_handle_.IsValid()) {
+    LOG(ERROR) << "Could not create JobObject.";
+    return false;
+  }
+
+  JOBOBJECT_EXTENDED_LIMIT_INFORMATION limit_info = {0};
+  limit_info.BasicLimitInformation.LimitFlags =
+      JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+  if (0 == SetInformationJobObject(job_handle_.Get(),

[Yuta Kitamura, 2011/11/23 06:06:09]

nit: Prefer (x == 0) over (0 == x). Maybe it's better to take the result of
SetInformationJobObject() to a named variable.

See http://dev.chromium.org/developers/coding-style#TOC-Code-formatting

[Takashi Toyoshima, 2011/11/23 08:53:32]

For now, other four place use this exactly same idiom.
I have another change to extract these common code into a function.
I'll fix it in the change.
You can follow the change in http://codereview.chromium.org/8667006/

+      JobObjectExtendedLimitInformation, &limit_info, sizeof(limit_info))) {
+    LOG(ERROR) << "Could not SetInformationJobObject.";
+    return false;
+  }
+
+  options.inherit_handles = true;
+  options.job_handle = job_handle_.Get();
+#endif
+
+  // Launch a new WebSocket server process.
   options.wait = true;
-  if (!base::LaunchProcess(*cmd_line.get(), options, NULL)) {
+  if (!base::LaunchProcess(*cmd_line.get(), options, &process_handle_)) {

[Yuta Kitamura, 2011/11/23 06:06:09]

Same here for #if defined(OS_POSIX) for process_handle_.

[Takashi Toyoshima, 2011/11/23 08:53:32]

Done.

     LOG(ERROR) << "Unable to launch websocket server.";
     return false;
   }
   started_ = true;
   return true;
 }
 
 CommandLine* TestWebSocketServer::CreatePythonCommandLine() {
   // Note: Python's first argument must be the script; do not append CommandLine
   // switches, as they would precede the script path and break this CommandLine.
@@ skipping 34 matching lines @@
     return;
   // Append CommandLine arguments after the server script, switches won't work.
   scoped_ptr<CommandLine> cmd_line(CreateWebSocketServerCommandLine());
   cmd_line->AppendArg("--server=stop");
   cmd_line->AppendArg("--chromium");
   cmd_line->AppendArgNative(FILE_PATH_LITERAL("--pidfile=") +
                             websocket_pid_file_.value());
   base::LaunchOptions options;
   options.wait = true;
   base::LaunchProcess(*cmd_line.get(), options, NULL);
+
+  // Just to make sure that the server process terminates certainly.
+  base::KillProcessGroup(process_handle_);

[Yuta Kitamura, 2011/11/23 06:06:09]

Ditto.

[Takashi Toyoshima, 2011/11/23 08:53:32]

Done.

 }
 
 TestNotificationObserver::TestNotificationObserver()
     : source_(content::NotificationService::AllSources()) {
 }
 
 TestNotificationObserver::~TestNotificationObserver() {}
 
 void TestNotificationObserver::Observe(
     int type,
@@ skipping 207 matching lines @@
   return taker.TakeRenderWidgetSnapshot(rwh, page_size, page_size, bitmap);
 }
 
 bool TakeEntirePageSnapshot(RenderViewHost* rvh, SkBitmap* bitmap) {
   DCHECK(bitmap);
   SnapshotTaker taker;
   return taker.TakeEntirePageSnapshot(rvh, bitmap);
 }
 
 }  // namespace ui_test_utils