Reland "Make generate_breakpad_symbols.py not silently ignore subprocess errors."

This reverts commit 5a797379.

Reason for revert: Relanding without making lld failures critical for now (see https://crbug.com/849904).

Original change's description:
> Revert "Make generate_breakpad_symbols.py not silently ignore subprocess errors."
>
> This reverts commit 924c8465.
>
> Reason for revert: This appears to break the Android WebView bots.
>
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20WebView%20N%20%28dbg%29
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20WebView%20O%20(dbg)
>
> Traceback (most recent call last):
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 327, in <module>
>     sys.exit(main())
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 316, in main
>     deps = GetSharedLibraryDependencies(options, queue.pop(0), loader_path)
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 151, in GetSharedLibraryDependencies
>     deps = GetSharedLibraryDependenciesLinux(binary)
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 67, in GetSharedLibraryDependenciesLinux
>     ldd = subprocess.check_output(['ldd', binary])
>   File "/b/s/w/ir/cipd_bin_packages/lib/python2.7/subprocess.py", line 219, in check_output
>     raise CalledProcessError(retcode, cmd, output=output)
>
> Original change's description:
> > 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/1086981
> > Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> > Commit-Queue: Nico Weber <thakis@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#564531}
>
> TBR=thakis@chromium.org,jochen@chromium.org
>
> Change-Id: Iee5a81e9b7e9db21f1dda9bdc272d3392438f481
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 813163
> Reviewed-on: https://chromium-review.googlesource.com/1087871
> Reviewed-by: Ted Choc <tedchoc@chromium.org>
> Commit-Queue: Ted Choc <tedchoc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564707}

TBR=thakis@chromium.org,tedchoc@chromium.org,jochen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 813163,850055
Change-Id: I3b241dffac522af75c46dc1a7554bd954b2a8f57
Reviewed-on: https://chromium-review.googlesource.com/1092671
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565638}

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,10 @@ 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])
+  # TODO(thakis): Figure out how to make this work for android
+  # (https://crbug.com/849904) and use check_output().
+  p = subprocess.Popen(['ldd', binary], stdout=subprocess.PIPE)
+  ldd = p.communicate()[0]
   lib_re = re.compile('\t.* => (.+) \(.*\)$')
   result = []
   for line in ldd.splitlines():
@@ -106,7 +91,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 +115,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 +126,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 +209,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)
@@ -83,7 +85,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)
@@ -112,7 +113,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='',
@@ -146,19 +146,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,
@@ -171,11 +167,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