Fix the KeyError 'VmPeak' bug in browser.memory_stats

1. Filters out zombile processes in GetChildPids.

2. There is still a chance to see the KeyError even with the above fix.
   Therefore, Add zombie-test in GetMemoryStats().

3. Change the inheritance relations among PlatformBackends to share
   common code.

   +-- PlatformBackend
       +-- PosixPlatformBackend
       |   +-- MacPlatformBackend
       |   +-- LinuxPlatformBackend
       |       +-- CrosPlatformBackend
       +-- WindowsPlatformBackend

BUG=2413588
TEST=Run the unittest and manually test modified code on linux, CrOS, and Mac

Merge branch 'master' of https://chromium.googlesource.com/chromium/src

SVN changes up to revision 198643

Review URL: https://chromiumcodereview.appspot.com/15809002

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

5 files changed, 140 insertions, 126 deletions

diff --git a/tools/telemetry/telemetry/core/platform/cros_platform_backend.py b/tools/telemetry/telemetry/core/platform/cros_platform_backend.py
index bc0c7d3..fe5af28 100644
--- a/tools/telemetry/telemetry/core/platform/cros_platform_backend.py
+++ b/tools/telemetry/telemetry/core/platform/cros_platform_backend.py
@@ -2,38 +2,15 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-try:
-  import resource  # pylint: disable=F0401
-except ImportError:
-  resource = None  # Not available on all platforms
+from telemetry.core.platform import linux_platform_backend
 
-from telemetry.core.platform import platform_backend
 
+class CrosPlatformBackend(linux_platform_backend.LinuxPlatformBackend):
 
-class CrosPlatformBackend(platform_backend.PlatformBackend):
   def __init__(self, cri):
     super(CrosPlatformBackend, self).__init__()
     self._cri = cri
 
-  def _GetFileContents(self, filename):
-    try:
-      return self._cri.RunCmdOnDevice(['cat', filename])[0]
-    except AssertionError:
-      return ''
-
-  def _GetProcFileDict(self, filename):
-    retval = {}
-    for line in self._GetFileContents(filename).splitlines():
-      line = line.strip()
-      key_val = line.split(':')
-      assert len(key_val) == 2
-      try:
-        retval[key_val[0]] = int(key_val[1].replace('kB', ''))
-      except Exception:
-        pass
-    return retval
-
-  # pylint: disable=W0613
   def StartRawDisplayFrameRateMeasurement(self):
     raise NotImplementedError()
 
@@ -49,32 +26,11 @@ class CrosPlatformBackend(platform_backend.PlatformBackend):
   def HasBeenThermallyThrottled(self):
     raise NotImplementedError()
 
-  def GetSystemCommitCharge(self):
-    meminfo = self._GetProcFileDict('/proc/meminfo')
-    return (meminfo['MemTotal'] - meminfo['MemFree'] - meminfo['Buffers'] -
-            meminfo['Cached'])
+  def _RunCommand(self, args):
+    return self._cri.RunCommandOnDevice(args)[0]
 
-  def GetMemoryStats(self, pid):
-    status = self._GetProcFileDict('/proc/%s/status' % pid)
-    stats = self._GetFileContents('/proc/%s/stat' % pid).split(' ')
-    if not status or not stats:
-      return {}
-    return {'VM': int(stats[22]),
-            'VMPeak': status['VmPeak'] * 1024,
-            'WorkingSetSize': int(stats[23]) * resource.getpagesize(),
-            'WorkingSetSizePeak': status['VmHWM'] * 1024}
-
-  def GetChildPids(self, pid):
-    """Retunds a list of child pids of |pid|."""
-    child_pids = []
-    pid_ppid_list, _ = self._cri.RunCmdOnDevice(
-        ['ps', '-e', '-o', 'pid=', '-o', 'ppid='])
-    for pid_ppid in pid_ppid_list.splitlines():
-      curr_pid, curr_ppid = pid_ppid.split()
-      if int(curr_ppid) == pid:
-        child_pids.append(int(curr_pid))
-        child_pids.extend(self.GetChildPids(int(curr_pid)))
-    return child_pids
-
-  def GetCommandLine(self, pid):
-    return self._cri.RunCmdOnDevice(['ps', '-p', str(pid), '-o', 'command='])[0]
+  def _GetFileContents(self, filename):
+    try:
+      return self._cri.RunCmdOnDevice(['cat', filename])[0]
+    except AssertionError:
+      return ''
diff --git a/tools/telemetry/telemetry/core/platform/linux_platform_backend.py b/tools/telemetry/telemetry/core/platform/linux_platform_backend.py
index fc64dd8..5a5a954 100644
--- a/tools/telemetry/telemetry/core/platform/linux_platform_backend.py
+++ b/tools/telemetry/telemetry/core/platform/linux_platform_backend.py
@@ -6,26 +6,12 @@ try:
   import resource  # pylint: disable=F0401
 except ImportError:
   resource = None  # Not available on all platforms
-import subprocess
 
-from telemetry.core.platform import platform_backend
+from telemetry.core.platform import posix_platform_backend
 
 
-class LinuxPlatformBackend(platform_backend.PlatformBackend):
-  def _GetProcFileDict(self, filename):
-    retval = {}
-    with open(filename, 'r') as f:
-      for line in f.readlines():
-        line = line.strip()
-        key_val = line.split(':')
-        assert len(key_val) == 2
-        try:
-          retval[key_val[0]] = int(key_val[1].replace('kB', ''))
-        except Exception:
-          pass
-    return retval
+class LinuxPlatformBackend(posix_platform_backend.PosixPlatformBackend):
 
-  # pylint: disable=W0613
   def StartRawDisplayFrameRateMeasurement(self):
     raise NotImplementedError()
 
@@ -41,41 +27,37 @@ class LinuxPlatformBackend(platform_backend.PlatformBackend):
   def HasBeenThermallyThrottled(self):
     raise NotImplementedError()
 
+  def _GetProcFileDict(self, filename):
+    retval = {}
+    for line in self._GetFileContents(filename).splitlines():
+      key, value = line.split(':')
+      retval[key.strip()] = value.strip()
+    return retval
+
+  def _ConvertKbToByte(self, value):
+    return int(value.replace('kB','')) * 1024
+
   def GetSystemCommitCharge(self):
     meminfo = self._GetProcFileDict('/proc/meminfo')
-    return (meminfo['MemTotal'] - meminfo['MemFree'] - meminfo['Buffers'] -
-            meminfo['Cached'])
+    return (self._ConvertKbToByte(meminfo['MemTotal'])
+            - self._ConvertKbToByte(meminfo['MemFree'])
+            - self._ConvertKbToByte(meminfo['Buffers'])
+            - self._ConvertKbToByte(meminfo['Cached']))
 
   def GetMemoryStats(self, pid):
     status = self._GetProcFileDict('/proc/%s/status' % pid)
-    stats = open('/proc/%s/stat' % pid, 'r').read().split(' ')
+    stats = self._GetFileContents('/proc/%s/stat' % pid).split()
+
+    if not status or not stats or 'Z' in status['State']:
+      return {}
     return {'VM': int(stats[22]),
-            'VMPeak': status['VmPeak'] * 1024,
+            'VMPeak': self._ConvertKbToByte(status['VmPeak']),
             'WorkingSetSize': int(stats[23]) * resource.getpagesize(),
-            'WorkingSetSizePeak': status['VmHWM'] * 1024}
+            'WorkingSetSizePeak': self._ConvertKbToByte(status['VmHWM'])}
 
   def GetIOStats(self, pid):
     io = self._GetProcFileDict('/proc/%s/io' % pid)
-    return {'ReadOperationCount': io['syscr'],
-            'WriteOperationCount': io['syscw'],
-            'ReadTransferCount': io['rchar'],
-            'WriteTransferCount': io['wchar']}
-
-  def GetChildPids(self, pid):
-    """Retunds a list of child pids of |pid|."""
-    child_pids = []
-    pid_ppid_state_list = subprocess.Popen(
-        ['ps', '-e', '-o', 'pid,ppid,state'],
-        stdout=subprocess.PIPE).communicate()[0]
-    for pid_ppid_state in pid_ppid_state_list.splitlines()[1:]:  # Skip header
-      curr_pid, curr_ppid, state = pid_ppid_state.split()
-      if 'Z' in state:
-        continue  # Ignore zombie processes
-      if int(curr_ppid) == pid:
-        child_pids.append(int(curr_pid))
-        child_pids.extend(self.GetChildPids(int(curr_pid)))
-    return child_pids
-
-  def GetCommandLine(self, pid):
-    with open('/proc/%s/cmdline' % pid, 'r') as f:
-      return f.read()
+    return {'ReadOperationCount': int(io['syscr']),
+            'WriteOperationCount': int(io['syscw']),
+            'ReadTransferCount': int(io['rchar']),
+            'WriteTransferCount': int(io['wchar'])}
diff --git a/tools/telemetry/telemetry/core/platform/mac_platform_backend.py b/tools/telemetry/telemetry/core/platform/mac_platform_backend.py
index 845383c..9b582b3 100644
--- a/tools/telemetry/telemetry/core/platform/mac_platform_backend.py
+++ b/tools/telemetry/telemetry/core/platform/mac_platform_backend.py
@@ -6,13 +6,12 @@ try:
   import resource  # pylint: disable=F0401
 except ImportError:
   resource = None  # Not available on all platforms
-import subprocess
 
-from telemetry.core.platform import platform_backend
+from telemetry.core.platform import posix_platform_backend
 
 
-class MacPlatformBackend(platform_backend.PlatformBackend):
-  # pylint: disable=W0613
+class MacPlatformBackend(posix_platform_backend.PosixPlatformBackend):
+
   def StartRawDisplayFrameRateMeasurement(self):
     raise NotImplementedError()
 
@@ -29,8 +28,7 @@ class MacPlatformBackend(platform_backend.PlatformBackend):
     raise NotImplementedError()
 
   def GetSystemCommitCharge(self):
-    vm_stat = subprocess.Popen(['vm_stat'],
-                               stdout=subprocess.PIPE).communicate()[0]
+    vm_stat = self._RunCommand(['vm_stat'])
     for stat in vm_stat.splitlines():
       key, value = stat.split(':')
       if key == 'Pages active':
@@ -39,27 +37,9 @@ class MacPlatformBackend(platform_backend.PlatformBackend):
     return 0
 
   def GetMemoryStats(self, pid):
-    pid_rss_vsz_list = subprocess.Popen(['ps', '-e', '-o', 'pid=,rss=,vsz='],
-                                        stdout=subprocess.PIPE).communicate()[0]
-    for pid_rss_vsz in pid_rss_vsz_list.splitlines():
-      curr_pid, rss, vsz = pid_rss_vsz.split()
-      if int(curr_pid) == pid:
-        return {'VM': 1024 * int(vsz),
-                'WorkingSetSize': 1024 * int(rss)}
+    rss_vsz = self._GetPsOutput(['rss', 'vsz'], pid)
+    if rss_vsz:
+      rss, vsz = rss_vsz[0].split()
+      return {'VM': 1024 * int(vsz),
+              'WorkingSetSize': 1024 * int(rss)}
     return {}
-
-  def GetChildPids(self, pid):
-    """Retunds a list of child pids of |pid|."""
-    child_pids = []
-    pid_ppid_list = subprocess.Popen(['ps', '-e', '-o', 'pid=,ppid='],
-                                     stdout=subprocess.PIPE).communicate()[0]
-    for pid_ppid in pid_ppid_list.splitlines():
-      curr_pid, curr_ppid = pid_ppid.split()
-      if int(curr_ppid) == pid:
-        child_pids.append(int(curr_pid))
-        child_pids.extend(self.GetChildPids(int(curr_pid)))
-    return child_pids
-
-  def GetCommandLine(self, pid):
-    return subprocess.Popen(['ps', '-p', str(pid), '-o', 'command='],
-                            stdout=subprocess.PIPE).communicate()[0]
diff --git a/tools/telemetry/telemetry/core/platform/posix_platform_backend.py b/tools/telemetry/telemetry/core/platform/posix_platform_backend.py
new file mode 100644
index 0000000..5073bb1
--- /dev/null
+++ b/tools/telemetry/telemetry/core/platform/posix_platform_backend.py
@@ -0,0 +1,62 @@
+# Copyright 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.
+
+import subprocess
+
+from collections import defaultdict
+
+from telemetry.core.platform import platform_backend
+
+
+class PosixPlatformBackend(platform_backend.PlatformBackend):
+
+  # This is an abstract class. It is OK to have abstract methods.
+  # pylint: disable=W0223
+
+  def _RunCommand(self, args):
+    return subprocess.Popen(args, stdout=subprocess.PIPE).communicate()[0]
+
+  def _GetFileContents(self, path):
+    with open(path, 'r') as f:
+      return f.read()
+
+  def _GetPsOutput(self, columns, pid=None):
+    """Returns output of the 'ps' command as a list of lines.
+    Subclass should override this function.
+
+    Args:
+      columns: A list of require columns, e.g., ['pid', 'pss'].
+      pid: If nont None, returns only the information of the process
+         with the pid.
+    """
+    args = ['ps']
+    args.extend(['-p', str(pid)] if pid != None else ['-e'])
+    for c in columns:
+      args.extend(['-o', c + '='])
+    return self._RunCommand(args).splitlines()
+
+  def GetChildPids(self, pid):
+    """Returns a list of child pids of |pid|."""
+    pid_ppid_state_list = self._GetPsOutput(['pid', 'ppid', 'state'])
+
+    child_dict = defaultdict(list)
+    for pid_ppid_state in pid_ppid_state_list:
+      curr_pid, curr_ppid, state = pid_ppid_state.split()
+      if 'Z' in state:
+        continue  # Ignore zombie processes
+      child_dict[int(curr_ppid)].append(int(curr_pid))
+    queue = [pid]
+    child_ids = []
+    while queue:
+      parent = queue.pop()
+      if parent in child_dict:
+        children = child_dict[parent]
+        queue.extend(children)
+        child_ids.extend(children)
+    return child_ids
+
+  def GetCommandLine(self, pid):
+    command = self._GetPsOutput('command', pid)
+    return command[0] if command else None
+
diff --git a/tools/telemetry/telemetry/core/platform/posix_platform_backend_unittest.py b/tools/telemetry/telemetry/core/platform/posix_platform_backend_unittest.py
new file mode 100644
index 0000000..05ccae4
--- /dev/null
+++ b/tools/telemetry/telemetry/core/platform/posix_platform_backend_unittest.py
@@ -0,0 +1,34 @@
+# Copyright 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.
+import unittest
+
+from telemetry.core.platform import posix_platform_backend
+
+
+class PosixPlatformBackendTest(unittest.TestCase):
+
+  def _GetChildPids(self, mock_ps_output, pid):
+    class TestBackend(posix_platform_backend.PosixPlatformBackend):
+
+      # pylint: disable=W0223
+
+      def _GetPsOutput(self, columns, pid=None):
+        return mock_ps_output
+
+    return TestBackend().GetChildPids(pid)
+
+  def testGetChildPidsWithGrandChildren(self):
+    lines = ['1 0 S', '2 1 R', '3 2 S', '4 1 R', '5 4 R']
+    result = self._GetChildPids(lines, 1)
+    self.assertEquals(set(result), set([2, 3, 4, 5]))
+
+  def testGetChildPidsWithNoSuchPid(self):
+    lines = ['1 0 S', '2 1 R', '3 2 S', '4 1 R', '5 4 R']
+    result = self._GetChildPids(lines, 6)
+    self.assertEquals(set(result), set())
+
+  def testGetChildPidsWithZombieChildren(self):
+    lines = ['1 0 S', '2 1 R', '3 2 S', '4 1 R', '5 4 Z']
+    result = self._GetChildPids(lines, 1)
+    self.assertEquals(set(result), set([2, 3, 4]))