Bug 1423666 - Fail in symbolstore.py if dump_syms returns a non-zero exit code. r=ted
☠☠ backed out by 6fd88252e9c2 ☠ ☠
authorRyan Hunt <rhunt@eqrion.net>
Thu, 05 Apr 2018 07:47:56 -0500
changeset 413234 c06258f53c2c476ba35b51b7ab06b105f8d4102b
parent 413233 e73f2e16ec4195375a293f7361a6ff972ff89f21
child 413235 1a959252a8189ef91ed5e92d67439e6577471b99
push id33840
push userapavel@mozilla.com
push dateFri, 13 Apr 2018 21:56:54 +0000
treeherdermozilla-central@6547c27303bc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersted
bugs1423666
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1423666 - Fail in symbolstore.py if dump_syms returns a non-zero exit code. r=ted
toolkit/crashreporter/tools/symbolstore.py
toolkit/crashreporter/tools/unit-symbolstore.py
--- a/toolkit/crashreporter/tools/symbolstore.py
+++ b/toolkit/crashreporter/tools/symbolstore.py
@@ -519,21 +519,23 @@ class Dumper:
         return [self.dump_syms, file]
 
     def ProcessFileWork(self, file, arch_num, arch, vcs_root, dsymbundle=None):
         t_start = time.time()
         print("Processing file: %s" % file, file=sys.stderr)
 
         sourceFileStream = ''
         code_id, code_file = None, None
+        cmd = self.dump_syms_cmdline(file, arch, dsymbundle=dsymbundle)
+        print(' '.join(cmd), file=sys.stderr)
+
+        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE)
+
         try:
-            cmd = self.dump_syms_cmdline(file, arch, dsymbundle=dsymbundle)
-            print(' '.join(cmd), file=sys.stderr)
-            proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                                    stderr=open(os.devnull, 'wb'))
             module_line = proc.stdout.next()
             if module_line.startswith("MODULE"):
                 # MODULE os cpu guid debug_file
                 (guid, debug_file) = (module_line.split())[3:5]
                 # strip off .pdb extensions, and append .sym
                 sym_file = re.sub("\.pdb$", "", debug_file) + ".sym"
                 # we do want forward slashes here
                 rel_path = os.path.join(debug_file,
@@ -580,32 +582,38 @@ class Dumper:
                         bits = line.rstrip().split(None, 3)
                         if len(bits) == 4:
                             code_id, code_file = bits[2:]
                         f.write(line)
                     else:
                         # pass through all other lines unchanged
                         f.write(line)
                 f.close()
-                proc.wait()
                 # we output relative paths so callers can get a list of what
                 # was generated
                 print(rel_path)
                 if self.srcsrv and vcs_root:
                     # add source server indexing to the pdb file
                     self.SourceServerIndexing(debug_file, guid, sourceFileStream, vcs_root)
                 # only copy debug the first time if we have multiple architectures
                 if self.copy_debug and arch_num == 0:
                     self.CopyDebug(file, debug_file, guid,
                                    code_file, code_id)
         except StopIteration:
             pass
         except Exception as e:
             print("Unexpected error: %s" % str(e), file=sys.stderr)
             raise
+        finally:
+            proc.wait()
+
+        if proc.returncode != 0:
+            # unit tests pass an iterator instead of a file so we can't use proc.stderr.read()
+            print("Unexpected error: %s" % ''.join(list(proc.stderr)), file=sys.stderr)
+            raise subprocess.CalledProcessError(proc.returncode, cmd, None)
 
         if dsymbundle:
             shutil.rmtree(dsymbundle)
 
         elapsed = time.time() - t_start
         print('Finished processing %s in %.2fs' % (file, elapsed),
               file=sys.stderr)
 
--- a/toolkit/crashreporter/tools/unit-symbolstore.py
+++ b/toolkit/crashreporter/tools/unit-symbolstore.py
@@ -116,16 +116,17 @@ class TestCopyDebug(HelperMixin, unittes
             stdout_ = stdout_iter.next()
             # Eager evaluation for communicate(), below.
             stdout_ = list(stdout_)
             # stdout is really an iterator, so back to iterators we go.
             m.stdout = iter(stdout_)
             m.wait.return_value = 0
             # communicate returns the full text of stdout and stderr.
             m.communicate.return_value = ('\n'.join(stdout_), '')
+            m.returncode = 0
             return m
         self.mock_popen.side_effect = next_popen
         shutil.rmtree = patch("shutil.rmtree").start()
 
     def tearDown(self):
         HelperMixin.tearDown(self)
         patch.stopall()
         shutil.rmtree(self.symbol_dir)
@@ -296,16 +297,17 @@ if target_platform() == 'WINNT':
             os.environ["PDBSTR_PATH"] = "pdbstr"
             # mock calls to `dump_syms`, `hg parent` and
             # `hg showconfig paths.default`
             mock_Popen.return_value.stdout = iter([
                     "MODULE os x86 %s %s" % ("X" * 33, test_files[0]),
                     "FILE 0 %s" % sourcefile,
                     "PUBLIC xyz 123"
                     ])
+            mock_Popen.return_value.returncode = 0
             mock_communicate = mock_Popen.return_value.communicate
             mock_communicate.side_effect = [("abcd1234", ""),
                                             ("http://example.com/repo", ""),
                                             ]
             # And mock the call to pdbstr to capture the srcsrv stream data.
             global srcsrv_stream
             srcsrv_stream = None
             def mock_pdbstr(args, cwd="", **kwargs):
@@ -436,27 +438,53 @@ class TestFileMapping(HelperMixin, unitt
                 [
                     'FILE %d %s\n' % (i,s) for i, s in enumerate(files)
                 ] +
                 [
                     'PUBLIC xyz 123\n'
                 ]
             )
         mock_Popen.return_value.stdout = mk_output(dumped_files)
+        mock_Popen.return_value.returncode = 0
 
         d = symbolstore.Dumper('dump_syms', self.symboldir,
                                file_mapping=file_mapping)
         f = os.path.join(self.objdir, 'somefile')
         open(f, 'wb').write('blah')
         d.Process(f)
         expected_output = ''.join(mk_output(expected_files))
         symbol_file = os.path.join(self.symboldir,
                                    file_id[1], file_id[0], file_id[1] + '.sym')
         self.assertEqual(open(symbol_file, 'r').read(), expected_output)
 
+class TestReturnCode(HelperMixin, unittest.TestCase):
+    def setUp(self):
+        HelperMixin.setUp(self)
+        self.objdir = os.path.join(self.test_dir, 'obj')
+        os.mkdir(self.objdir)
+        self.symboldir = os.path.join(self.test_dir, 'symbols')
+        os.mkdir(self.symboldir)
+
+    @patch("subprocess.Popen")
+    def testReturnCode(self, mock_Popen):
+        # mock the dump_syms output
+        dump_syms_output = iter([''])
+        dump_syms_err = iter(['error'])
+        mock_Popen.return_value.returncode = 1
+        mock_Popen.return_value.stdout = dump_syms_output
+        mock_Popen.return_value.stderr = dump_syms_err
+
+        d = symbolstore.Dumper('dump_syms', self.symboldir)
+        f = os.path.join(self.objdir, 'somefile')
+
+        with self.assertRaises(subprocess.CalledProcessError) as e:
+            d.Process(f)
+            self.assertEqual(e.returncode, 1)
+            self.assertEqual(e.stderr, dump_syms_err)
+
 class TestFunctional(HelperMixin, unittest.TestCase):
     '''Functional tests of symbolstore.py, calling it with a real
     dump_syms binary and passing in a real binary to dump symbols from.
 
     Since the rest of the tests in this file mock almost everything and
     don't use the actual process pool like buildsymbols does, this tests
     that the way symbolstore.py gets called in buildsymbols works.
     '''