Make generate_breakpad_symbols.py not silently ignore subprocess errors.

GetCommandOuput() used to pipe stderr to /dev/null, and it ignored
the command's return code. Use check_output() to check the return
code, and keep stderr attached to parent's stderr.

Also make breakpad_integration_test.py a bit simpler (this part is
supposed to be behavior-preserving.)

Bug: 813163
Change-Id: I8c55d3da9fff3b944111c3e868121ac34bf65c17
Reviewed-on: https://chromium-review.googlesource.com/1086981Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564531}

components/crash/content/tools/generate_breakpad_symbols.py

@@ -29,24 +29,6 @@ BINARY_INFO = collections.namedtuple('BINARY_INFO',
                                      ['platform', 'arch', 'hash', 'name'])
 
 
-def GetCommandOutput(command, env=None):
-  """Runs the command list, returning its output (stdout).
-
-  Args:
-    command: (list of strings) a command with arguments
-
-    env: (dict or None) optional environment for the command. If None,
-      inherits the existing environment, otherwise completely overrides it.
-
-  From chromium_utils.
-  """
-  devnull = open(os.devnull, 'w')
-  proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=devnull,
-                          bufsize=1, env=env)
-  output = proc.communicate()[0]
-  return output
-
-
 def GetDumpSymsBinary(build_dir=None):
   """Returns the path to the dump_syms binary."""
   DUMP_SYMS = 'dump_syms'
@@ -82,7 +64,7 @@ def GetSharedLibraryDependenciesLinux(binary):
   """Return absolute paths to all shared library dependecies of the binary.
 
   This implementation assumes that we're running on a Linux system."""
-  ldd = GetCommandOutput(['ldd', binary])
+  ldd = subprocess.check_output(['ldd', binary])
   lib_re = re.compile('\t.* => (.+) \(.*\)$')
   result = []
   for line in ldd.splitlines():
@@ -106,7 +88,7 @@ def GetDeveloperDirMac():
   if 'DEVELOPER_DIR' in os.environ:
     candidate_paths.append(os.environ['DEVELOPER_DIR'])
   candidate_paths.extend([
-    GetCommandOutput(['xcode-select', '-p']).strip(),
+    subprocess.check_output(['xcode-select', '-p']).strip(),
     # Most Mac 10.1[0-2] bots have at least one Xcode installed.
     '/Applications/Xcode.app',
     '/Applications/Xcode9.0.app',
@@ -130,7 +112,7 @@ def GetSharedLibraryDependenciesMac(binary, loader_path):
   developer_dir = GetDeveloperDirMac()
   if developer_dir:
     env['DEVELOPER_DIR'] = developer_dir
-  otool = GetCommandOutput(['otool', '-l', binary], env=env).splitlines()
+  otool = subprocess.check_output(['otool', '-l', binary], env=env).splitlines()
   rpaths = []
   dylib_id = None
   for idx, line in enumerate(otool):
@@ -141,7 +123,7 @@ def GetSharedLibraryDependenciesMac(binary, loader_path):
       m = re.match(' *name (.*) \(offset .*\)$', otool[idx+2])
       dylib_id = m.group(1)
 
-  otool = GetCommandOutput(['otool', '-L', binary], env=env).splitlines()
+  otool = subprocess.check_output(['otool', '-L', binary], env=env).splitlines()
   lib_re = re.compile('\t(.*) \(compatibility .*\)$')
   deps = []
   for line in otool:
@@ -224,7 +206,7 @@ def GenerateSymbols(options, binaries):
           break
 
         binary_info = GetBinaryInfoFromHeaderInfo(
-            GetCommandOutput([dump_syms, '-i', binary]).splitlines()[0])
+            subprocess.check_output([dump_syms, '-i', binary]).splitlines()[0])
         if not binary_info:
           should_dump_syms = False
           reason = "Could not obtain binary information."

content/shell/tools/breakpad_integration_test.py

@@ -22,10 +22,12 @@ import time
 
 
 CONCURRENT_TASKS=4
+BREAKPAD_TOOLS_DIR = os.path.join(
+  os.path.dirname(__file__), '..', '..', '..',
+  'components', 'crash', 'content', 'tools')
 
-def run_test(options, crash_dir, additional_arguments = []):
+def run_test(options, crash_dir, symbols_dir, additional_arguments = []):
   global failure
-  global breakpad_tools_dir
 
   print "# Run content_shell and make it crash."
   cmd = [options.binary,
@@ -61,7 +63,7 @@ def run_test(options, crash_dir, additional_arguments = []):
 
   if sys.platform not in ('darwin', 'win32'):
     minidump = os.path.join(crash_dir, 'minidump')
-    dmp_to_minidump = os.path.join(breakpad_tools_dir, 'dmp2minidump.py')
+    dmp_to_minidump = os.path.join(BREAKPAD_TOOLS_DIR, 'dmp2minidump.py')
     cmd = [dmp_to_minidump, dmp_file, minidump]
     if options.verbose:
       print ' '.join(cmd)
@@ -87,7 +89,6 @@ def run_test(options, crash_dir, additional_arguments = []):
     # stack = proc.communicate()[0]
   else:
     minidump_stackwalk = os.path.join(options.build_dir, 'minidump_stackwalk')
-    global symbols_dir
     cmd = [minidump_stackwalk, minidump, symbols_dir]
     if options.verbose:
       print ' '.join(cmd)
@@ -116,7 +117,6 @@ def run_test(options, crash_dir, additional_arguments = []):
 
 def main():
   global failure
-  global breakpad_tools_dir
 
   parser = optparse.OptionParser()
   parser.add_option('', '--build-dir', default='',
@@ -150,19 +150,15 @@ def main():
 
   # Create a temporary directory to store the crash dumps and symbols in.
   crash_dir = tempfile.mkdtemp()
+  symbols_dir = os.path.join(crash_dir, 'symbols')
 
   crash_service = None
 
   try:
     if sys.platform != 'win32':
       print "# Generate symbols."
-      global symbols_dir
-      breakpad_tools_dir = os.path.join(
-          os.path.dirname(__file__), '..', '..', '..',
-          'components', 'crash', 'content', 'tools')
       generate_symbols = os.path.join(
-          breakpad_tools_dir, 'generate_breakpad_symbols.py')
-      symbols_dir = os.path.join(crash_dir, 'symbols')
+          BREAKPAD_TOOLS_DIR, 'generate_breakpad_symbols.py')
       cmd = [generate_symbols,
              '--build-dir=%s' % options.build_dir,
              '--binary=%s' % options.binary,
@@ -175,11 +171,11 @@ def main():
       subprocess.check_call(cmd)
 
     print "# Running test without trap handler."
-    run_test(options, crash_dir);
+    run_test(options, crash_dir, symbols_dir)
     print "# Running test with trap handler."
-    run_test(options, crash_dir,
+    run_test(options, crash_dir, symbols_dir,
              additional_arguments =
-               ['--enable-features=WebAssemblyTrapHandler']);
+               ['--enable-features=WebAssemblyTrapHandler'])
 
   except:
     print "FAIL: %s" % failure