Mac: stop zombie ps processes from being created.

This fixes two bugs:
- the output buffer for the ps command being run could be too small (fix: buffer
  enlargened); and
- if the output buffer was too small, waitpid() wouldn't be called (fix: always
  call waitpid(), obviously).

BUG=31378
TEST=On a big, long-running Chrome/Chromium (with many processes, e.g., renderers), check that there are no zombie ps processes (with PPID the browser process); looking at about:memory may help speed up the creation of such zombies. Also, run the new test, ProcessUtilTest.GetAppOutputRestrictedNoZombies.

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

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

4 files changed, 54 insertions, 13 deletions

diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index 16469db..21626732 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -580,31 +580,33 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
         close(pipe_fd[1]);
 
         char buffer[256];
-        std::string buf_output;
-        size_t output_left = max_output;
+        std::string output_buf;
+        size_t output_buf_left = max_output;
         ssize_t bytes_read = 1;  // A lie to properly handle |max_output == 0|
                                  // case in the logic below.
 
-        while (output_left > 0) {
+        while (output_buf_left > 0) {
           bytes_read = HANDLE_EINTR(read(pipe_fd[0], buffer,
-                                    std::min(output_left, sizeof(buffer))));
+                                    std::min(output_buf_left, sizeof(buffer))));
           if (bytes_read <= 0)
             break;
-          buf_output.append(buffer, bytes_read);
-          output_left -= static_cast<size_t>(bytes_read);
+          output_buf.append(buffer, bytes_read);
+          output_buf_left -= static_cast<size_t>(bytes_read);
         }
         close(pipe_fd[0]);
 
+        // Always wait for exit code (even if we know we'll declare success).
+        int exit_code = EXIT_FAILURE;
+        bool success = WaitForExitCode(pid, &exit_code);
+
         // If we stopped because we read as much as we wanted, we always declare
         // success (because the child may exit due to |SIGPIPE|).
-        if (output_left || bytes_read <= 0) {
-          int exit_code = EXIT_FAILURE;
-          bool success = WaitForExitCode(pid, &exit_code);
+        if (output_buf_left || bytes_read <= 0) {
           if (!success || exit_code != EXIT_SUCCESS)
             return false;
         }
 
-        output->swap(buf_output);
+        output->swap(output_buf);
         return true;
       }
   }
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 8bbd784..4b313a3 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -337,6 +337,28 @@ TEST_F(ProcessUtilTest, GetAppOutputRestricted) {
   EXPECT_STREQ("", output.c_str());
 }
 
+TEST_F(ProcessUtilTest, GetAppOutputRestrictedNoZombies) {
+  std::vector<std::string> argv;
+  argv.push_back("/bin/sh");  // argv[0]
+  argv.push_back("-c");       // argv[1]
+  argv.push_back("echo 123456789012345678901234567890");  // argv[2]
+
+  // Run |GetAppOutputRestricted()| 300 (> default per-user processes on Mac OS
+  // 10.5) times with an output buffer big enough to capture all output.
+  for (int i = 0; i < 300; i++) {
+    std::string output;
+    EXPECT_TRUE(GetAppOutputRestricted(CommandLine(argv), &output, 100));
+    EXPECT_STREQ("123456789012345678901234567890\n", output.c_str());
+  }
+
+  // Ditto, but with an output buffer too small to capture all output.
+  for (int i = 0; i < 300; i++) {
+    std::string output;
+    EXPECT_TRUE(GetAppOutputRestricted(CommandLine(argv), &output, 10));
+    EXPECT_STREQ("1234567890", output.c_str());
+  }
+}
+
 #if defined(OS_LINUX)
 TEST_F(ProcessUtilTest, GetParentProcessId) {
   base::ProcessId ppid = GetParentProcessId(GetCurrentProcId());
diff --git a/chrome/browser/process_info_snapshot.h b/chrome/browser/process_info_snapshot.h
index 88064c6..14e8ceb 100644
--- a/chrome/browser/process_info_snapshot.h
+++ b/chrome/browser/process_info_snapshot.h
@@ -31,9 +31,14 @@ class ProcessInfoSnapshot {
   ProcessInfoSnapshot();
   ~ProcessInfoSnapshot();
 
+  // Maximum size of lists of PIDs which this class will accept; used in
+  // |Sample()| below.
+  static const size_t kMaxPidListSize;
+
   // Capture a snapshot of process memory information (by running ps) for the
   // given list of PIDs. Call only from the file thread.
-  //   |pid_list| - list of |ProcessId|s on which to capture information.
+  //   |pid_list| - list of |ProcessId|s on which to capture information; must
+  //     have no more than |kMaxPidListSize| (above) entries,
   //   returns - |true| if okay, |false| on error.
   bool Sample(std::vector<base::ProcessId> pid_list);
 
diff --git a/chrome/browser/process_info_snapshot_mac.cc b/chrome/browser/process_info_snapshot_mac.cc
index 611f8f4..4e2608b 100644
--- a/chrome/browser/process_info_snapshot_mac.cc
+++ b/chrome/browser/process_info_snapshot_mac.cc
@@ -7,6 +7,7 @@
 #include <iostream>
 #include <sstream>
 
+#include "base/logging.h"
 #include "base/string_util.h"
 #include "base/thread.h"
 
@@ -21,6 +22,8 @@ ProcessInfoSnapshot::~ProcessInfoSnapshot() {
   Reset();
 }
 
+const size_t ProcessInfoSnapshot::kMaxPidListSize = 1000;
+
 // Capture the information by calling '/bin/ps'.
 // Note: we ignore the "tsiz" (text size) display option of ps because it's
 // always zero (tested on 10.5 and 10.6).
@@ -28,6 +31,15 @@ bool ProcessInfoSnapshot::Sample(std::vector<base::ProcessId> pid_list) {
   const char* kPsPathName = "/bin/ps";
   Reset();
 
+  // Nothing to do if no PIDs given.
+  if (pid_list.size() == 0)
+    return true;
+  if (pid_list.size() > kMaxPidListSize) {
+    // The spec says |pid_list| *must* not have more than this many entries.
+    NOTREACHED();
+    return false;
+  }
+
   std::vector<std::string> argv;
   argv.push_back(kPsPathName);
   // Get PID, PPID, (real) UID, effective UID, resident set size, virtual memory
@@ -43,8 +55,8 @@ bool ProcessInfoSnapshot::Sample(std::vector<base::ProcessId> pid_list) {
 
   std::string output;
   CommandLine command_line(argv);
-  if (!base::GetAppOutputRestricted(command_line,
-                                    &output, (pid_list.size() + 10) * 100)) {
+  // Limit output read to a megabyte for safety.
+  if (!base::GetAppOutputRestricted(command_line, &output, 1024 * 1024)) {
     LOG(ERROR) << "Failure running " << kPsPathName << " to acquire data.";
     return false;
   }