Make ProcessWatcher use kqueues on Mac.

* Port ProcessWatcher::EnsureProcessTerminated() to kqueue() APIs on OS X.
* Make ProcessWatcher::EnsureProcessGetsReaped() Linux-only, since it's only used there.
* Add a unit test.

BUG=12731
TEST=Open Chrome/Mac, open and close a few tabs.  Processes shouldn't stay around.

Review URL: http://codereview.chromium.org/496007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34547 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 237 insertions, 9 deletions

diff --git a/base/base.gypi b/base/base.gypi
index 77a919e..0c076d4 100644
--- a/base/base.gypi
+++ b/base/base.gypi
@@ -287,7 +287,6 @@
                 'message_pump_glib.cc',
                 'nss_init.cc',
                 'nss_init.h',
-                'time_posix.cc',
               ],
           },],
           [ 'OS != "linux"', {
diff --git a/base/file_util.h b/base/file_util.h
index 9eb1729..543a86c 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -371,6 +371,20 @@ class ScopedFILEClose {
 
 typedef scoped_ptr_malloc<FILE, ScopedFILEClose> ScopedFILE;
 
+#if defined(OS_POSIX)
+// A class to handle auto-closing of FDs.
+class ScopedFDClose {
+ public:
+  inline void operator()(int* x) const {
+    if (x) {
+      close(*x);
+    }
+  }
+};
+
+typedef scoped_ptr_malloc<int, ScopedFDClose> ScopedFD;
+#endif  // OS_POSIX
+
 // A class for enumerating the files in a provided path. The order of the
 // results is not guaranteed.
 //
diff --git a/base/time.h b/base/time.h
index 33ab227..538659d 100644
--- a/base/time.h
+++ b/base/time.h
@@ -62,6 +62,10 @@ class TimeDelta {
     return delta_;
   }
 
+#if defined(OS_POSIX)
+  struct timespec ToTimeSpec() const;
+#endif
+
   // Returns the time delta in some unit. The F versions return a floating
   // point value, the "regular" versions return a rounded-down value.
   //
diff --git a/base/time_posix.cc b/base/time_posix.cc
index c64f8e7..af7ee25 100644
--- a/base/time_posix.cc
+++ b/base/time_posix.cc
@@ -14,6 +14,7 @@
 
 namespace base {
 
+#if !defined(OS_MACOSX)
 // The Time routines in this file use standard POSIX routines, or almost-
 // standard routines in the case of timegm.  We need to use a Mach-specific
 // function for TimeTicks::Now() on Mac OS X.
@@ -174,4 +175,19 @@ TimeTicks TimeTicks::HighResNow() {
   return Now();
 }
 
+#endif  // !OS_MACOSX
+
+struct timespec TimeDelta::ToTimeSpec() const {
+  int64 microseconds = InMicroseconds();
+  time_t seconds = 0;
+  if (microseconds >= Time::kMicrosecondsPerSecond) {
+    seconds = InSeconds();
+    microseconds -= seconds * Time::kMicrosecondsPerSecond;
+  }
+  struct timespec result =
+      {seconds,
+       microseconds * Time::kNanosecondsPerMicrosecond};
+  return result;
+}
+
 }  // namespace base
diff --git a/base/time_unittest.cc b/base/time_unittest.cc
index 81daab0..76b357a 100644
--- a/base/time_unittest.cc
+++ b/base/time_unittest.cc
@@ -142,6 +142,27 @@ TEST(TimeDelta, FromAndIn) {
   EXPECT_EQ(13, TimeDelta::FromMicroseconds(13).InMicroseconds());
 }
 
+#if defined(OS_POSIX)
+TEST(TimeDelta, TimeSpecConversion) {
+  struct timespec result = TimeDelta::FromSeconds(0).ToTimeSpec();
+  EXPECT_EQ(result.tv_sec, 0);
+  EXPECT_EQ(result.tv_nsec, 0);
+
+  result = TimeDelta::FromSeconds(1).ToTimeSpec();
+  EXPECT_EQ(result.tv_sec, 1);
+  EXPECT_EQ(result.tv_nsec, 0);
+
+  result = TimeDelta::FromMicroseconds(1).ToTimeSpec();
+  EXPECT_EQ(result.tv_sec, 0);
+  EXPECT_EQ(result.tv_nsec, 1000);
+
+  result = TimeDelta::FromMicroseconds(
+      Time::kMicrosecondsPerSecond + 1).ToTimeSpec();
+  EXPECT_EQ(result.tv_sec, 1);
+  EXPECT_EQ(result.tv_nsec, 1000);
+}
+#endif  // OS_POSIX
+
 // Our internal time format is serialized in things like databases, so it's
 // important that it's consistent across all our platforms.  We use the 1601
 // Windows epoch as the internal format across all platforms.
diff --git a/chrome/browser/child_process_launcher.cc b/chrome/browser/child_process_launcher.cc
index 956f836..1ab66c6 100644
--- a/chrome/browser/child_process_launcher.cc
+++ b/chrome/browser/child_process_launcher.cc
@@ -228,11 +228,11 @@ class ChildProcessLauncher::Context
       // through the zygote process.
       Singleton<ZygoteHost>()->EnsureProcessTerminated(handle);
     } else
-#endif  // defined(OS_LINUX)
+#endif  // OS_LINUX
     {
       ProcessWatcher::EnsureProcessTerminated(handle);
     }
-#endif
+#endif  // OS_POSIX
     process.Close();
   }
 
diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp
index d0dd9f3..8e909cf 100755
--- a/chrome/chrome.gyp
+++ b/chrome/chrome.gyp
@@ -559,6 +559,7 @@
         'common/pref_member.h',
         'common/pref_service.cc',
         'common/pref_service.h',
+        'common/process_watcher_mac.cc',
         'common/process_watcher_posix.cc',
         'common/process_watcher_win.cc',
         'common/process_watcher.h',
@@ -649,6 +650,11 @@
             '../build/linux/system.gyp:selinux',
           ],
         }],
+        ['OS=="mac"', {
+          'sources!': [
+            'common/process_watcher_posix.cc',
+          ],
+        }],
         ['OS=="win"', {
           'include_dirs': [
             'third_party/wtl/include',
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 7b5a2f2..f103ff1 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -805,6 +805,7 @@
         'common/notification_service_unittest.cc',
         'common/pref_member_unittest.cc',
         'common/pref_service_unittest.cc',
+        'common/process_watcher_unittest.cc',
         'common/property_bag_unittest.cc',
         'common/resource_dispatcher_unittest.cc',
         'common/sandbox_mac_unittest.mm',
diff --git a/chrome/common/process_watcher.h b/chrome/common/process_watcher.h
index a1f909c..54b344b 100644
--- a/chrome/common/process_watcher.h
+++ b/chrome/common/process_watcher.h
@@ -20,14 +20,15 @@ class ProcessWatcher {
   // does not appear to have exited, then this function starts to become
   // aggressive about ensuring that the process terminates.
   //
-  // This method does not block the calling thread.
+  // On Linux this method does not block the calling thread.
+  // On OS X this method may block for up to 2 seconds.
   //
   // NOTE: The process handle must have been opened with the PROCESS_TERMINATE
   // and SYNCHRONIZE permissions.
   //
   static void EnsureProcessTerminated(base::ProcessHandle process_handle);
 
-#if defined(OS_POSIX)
+#if defined(OS_LINUX)
   // The nicer version of EnsureProcessTerminated() that is patient and will
   // wait for |process_handle| to finish and then reap it.
   static void EnsureProcessGetsReaped(base::ProcessHandle process_handle);
diff --git a/chrome/common/process_watcher_mac.cc b/chrome/common/process_watcher_mac.cc
new file mode 100644
index 0000000..2d5d185
--- /dev/null
+++ b/chrome/common/process_watcher_mac.cc
@@ -0,0 +1,118 @@
+// Copyright (c) 2009 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/common/process_watcher.h"
+
+#include <errno.h>
+#include <signal.h>
+#include <sys/event.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include "base/eintr_wrapper.h"
+#include "base/file_util.h"
+#include "base/time.h"
+
+namespace {
+
+// Reap |child| process.
+// This call blocks until completion.
+void BlockingReap(pid_t child) {
+  const pid_t result = HANDLE_EINTR(waitpid(child, NULL, 0));
+  if (result == -1) {
+    PLOG(ERROR) << "waitpid(" << child << ")";
+    NOTREACHED();
+  }
+}
+
+// Waits for |timeout| seconds for the given |child| to exit and reap it.
+// If the child doesn't exit within a couple of seconds, kill it.
+void WaitForChildToDie(pid_t child, unsigned timeout) {
+  int kq = HANDLE_EINTR(kqueue());
+  file_util::ScopedFD auto_close(&kq);
+  if (kq == -1) {
+    PLOG(ERROR) << "Failed to create kqueue";
+    return;
+  }
+
+  struct kevent event_to_add = {0};
+  EV_SET(&event_to_add, child, EVFILT_PROC, EV_ADD, NOTE_EXIT, 0, NULL);
+  // Register interest with kqueue.
+  int result = HANDLE_EINTR(kevent(kq, &event_to_add, 1, NULL, 0, NULL));
+  if (result == -1 && errno == ESRCH) {
+    // A "No Such Process" error is fine, the process may have died already
+    // and been reaped by someone else.
+    return;
+  }
+
+  if (result == -1) {
+    PLOG(ERROR) << "Failed to register event to listen for death of pid "
+                << child;
+    return;
+  }
+
+  struct kevent event = {0};
+
+  DCHECK(timeout != 0);
+
+  int num_processes_that_died = -1;
+  using base::Time;
+  using base::TimeDelta;
+  // We need to keep track of the elapsed time - if kevent() returns
+  // EINTR in the middle of blocking call we want to make up what's left
+  // of the timeout.
+  TimeDelta time_left = TimeDelta::FromSeconds(timeout);
+  Time wakeup = Time::Now() + time_left;
+  while(time_left.InMilliseconds() > 0) {
+    const struct timespec timeout = time_left.ToTimeSpec();
+    num_processes_that_died = kevent(kq, NULL, 0, &event, 1, &timeout);
+    if (num_processes_that_died >= 0)
+      break;
+    if (num_processes_that_died == -1 && errno == EINTR) {
+      time_left = wakeup - Time::Now();
+      continue;
+    }
+
+    // If we got here, kevent() must have returned -1.
+    PLOG(ERROR) << "kevent() failed";
+    break;
+  }
+
+  if (num_processes_that_died == -1) {
+    PLOG(ERROR) << "kevent failed";
+    return;
+  }
+  if (num_processes_that_died == 1) {
+    if (event.fflags & NOTE_EXIT &&
+        event.ident == static_cast<uintptr_t>(child)) {
+      // Process died, it's safe to make a blocking call here since the
+      // kqueue() notification occurs when the process is already zombified.
+      BlockingReap(child);
+      return;
+    } else {
+      PLOG(ERROR) << "kevent() returned unexpected result - ke.fflags ="
+                  << event.fflags
+                  << " ke.ident ="
+                  << event.ident
+                  << " while listening for pid="
+                  << child;
+    }
+  }
+
+  // If we got here the child is still alive so kill it...
+  if (kill(child, SIGKILL) == 0) {
+    // SIGKILL is uncatchable. Since the signal was delivered, we can
+    // just wait for the process to die now in a blocking manner.
+    BlockingReap(child);
+  } else {
+    PLOG(ERROR) << "While waiting for " << child << " to terminate we"
+               << " failed to deliver a SIGKILL signal";
+  }
+}
+
+}  // namespace
+
+void ProcessWatcher::EnsureProcessTerminated(base::ProcessHandle process) {
+  WaitForChildToDie(process, 2);
+}
diff --git a/chrome/common/process_watcher_posix.cc b/chrome/common/process_watcher_posix.cc
index afe6fe55..c0ea178 100644
--- a/chrome/common/process_watcher_posix.cc
+++ b/chrome/common/process_watcher_posix.cc
@@ -46,8 +46,8 @@ class BackgroundReaper : public PlatformThread::Delegate {
     if (timeout_ == 0) {
       pid_t r = HANDLE_EINTR(waitpid(child_, NULL, 0));
       if (r != child_) {
-        LOG(ERROR) << "While waiting for " << child_
-                   << " to terminate, we got the following result: " << r;
+        PLOG(ERROR) << "While waiting for " << child_
+                    << " to terminate, we got the following result: " << r;
       }
       return;
     }
@@ -83,7 +83,7 @@ class BackgroundReaper : public PlatformThread::Delegate {
 
 // static
 void ProcessWatcher::EnsureProcessTerminated(base::ProcessHandle process) {
-  // If the child is already dead, then there's nothing to do
+  // If the child is already dead, then there's nothing to do.
   if (IsChildDead(process))
     return;
 
@@ -94,7 +94,7 @@ void ProcessWatcher::EnsureProcessTerminated(base::ProcessHandle process) {
 
 // static
 void ProcessWatcher::EnsureProcessGetsReaped(base::ProcessHandle process) {
-  // If the child is already dead, then there's nothing to do
+  // If the child is already dead, then there's nothing to do.
   if (IsChildDead(process))
     return;
 
diff --git a/chrome/common/process_watcher_unittest.cc b/chrome/common/process_watcher_unittest.cc
new file mode 100644
index 0000000..505a468
--- /dev/null
+++ b/chrome/common/process_watcher_unittest.cc
@@ -0,0 +1,48 @@
+// Copyright (c) 2009 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/common/process_watcher.h"
+
+#if defined(OS_POSIX)
+#include <sys/wait.h>
+
+#include "base/eintr_wrapper.h"
+#include "base/multiprocess_test.h"
+#include "base/process_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class ProcessWatcherTest : public MultiProcessTest {
+};
+
+namespace {
+
+bool IsProcessDead(base::ProcessHandle child) {
+  // waitpid() will actually reap the process which is exactly NOT what we
+  // want to test for.  The good thing is that if it can't find the process
+  // we'll get a nice value for errno which we can test for.
+  const pid_t result = HANDLE_EINTR(waitpid(child, NULL, WNOHANG));
+  return result == -1 && errno == ECHILD;
+}
+
+}  // namespace
+
+TEST_F(ProcessWatcherTest, DelayedTermination) {
+  base::ProcessHandle child_process =
+      SpawnChild(L"process_watcher_test_never_die");
+  ProcessWatcher::EnsureProcessTerminated(child_process);
+  base::WaitForSingleProcess(child_process, 5000);
+
+  // Check that process was really killed.
+  EXPECT_TRUE(IsProcessDead(child_process));
+  base::CloseProcessHandle(child_process);
+}
+
+MULTIPROCESS_TEST_MAIN(process_watcher_test_never_die) {
+  while(1){
+    sleep(500);
+  }
+  return 0;
+}
+
+#endif  // OS_POSIX