From 56d643b6f7f1ec8857ba75dad74340e4b35651c8 Mon Sep 17 00:00:00 2001
From: Alexis Beingessner <a.beingessner@gmail.com>
Date: Wed, 8 Dec 2021 21:48:00 +0000
Subject: [PATCH] Bug 1741205 - add more robust version checking to
 mozcrash.py. r=KrisWright

I forgot that mozboot only pulls in updates when you run `./mach bootstrap`,
so some people got the new mozcrash.py locally without actually having the
new rust-minidump-based version. So now we first run the stackwalk binary
with -V to check what version it is.

The rest of the details can be found in the comments I added.

Also updates rust-minidump to 0.9.6 get some CLI parsing fixes
and better --help documentation (socorro staging is already updated to
this version).

Differential Revision: https://phabricator.services.mozilla.com/D133251
---
 taskcluster/ci/fetch/toolchains.yml           |  2 +-
 testing/mozbase/mozcrash/mozcrash/mozcrash.py | 51 ++++++++++++++++++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/taskcluster/ci/fetch/toolchains.yml b/taskcluster/ci/fetch/toolchains.yml
index 45fd2172f47d..2dadb8daf7f6 100644
--- a/taskcluster/ci/fetch/toolchains.yml
+++ b/taskcluster/ci/fetch/toolchains.yml
@@ -425,7 +425,7 @@ rust-minidump:
     fetch:
         type: git
         repo: https://github.com/luser/rust-minidump/
-        revision: 0c90e02544797317503d1c4cff8daab0cabdea86
+        revision: 664dcd1d8ba8c227220f8b83927095880bd68a5f
 
 fix-stacks:
     description: fix-stacks source code
diff --git a/testing/mozbase/mozcrash/mozcrash/mozcrash.py b/testing/mozbase/mozcrash/mozcrash/mozcrash.py
index 346d469bb631..9d09d0b93027 100644
--- a/testing/mozbase/mozcrash/mozcrash/mozcrash.py
+++ b/testing/mozbase/mozcrash/mozcrash/mozcrash.py
@@ -337,12 +337,61 @@ class CrashInfo(object):
             and os.path.exists(self.stackwalk_binary)
             and os.access(self.stackwalk_binary, os.X_OK)
         ):
+            # If minidump_stackwalk -V fails, then we're using the old breakpad version,
+            # which is implicitly "human" output and doesn't support the --human flag.
+            #
+            # Otherwise we're using rust-minidump's minidump_stackwalk. Before 0.9.6
+            # --human had to be passed explicitly to get human output, but now it's
+            # the default (to behave more similarly to breakpad). But since we've
+            # already filtered out breakpad as an option, we can explicitly pass
+            # --human.
+            #
+            # In the future we would also like to use rust-minidump's --cyborg
+            # (introduced in 0.9.5), which outputs both human-readable *and*
+            # machine-readable (JSON) output, so we can output something nice to the
+            # CLI, but more reliably parse all the details we care about.
+            # The machine-readable output is also exactly what socorro (crash-stats)
+            # consumes, so in theory using it will make the two more compatible.
+            stackwalk_version_check = subprocess.Popen(
+                [self.stackwalk_binary, "-V"],
+                stdout=subprocess.DEVNULL,
+                stderr=subprocess.DEVNULL,
+            )
+            stackwalk_version_check.wait()
+
+            # Right now we don't have any rust-minidump-version-specific handling,
+            # so we don't need to bother parsing the output at all. But when we
+            # want to start using newer features (like --cyborg) we should parse
+            # the version string, so here's some notes on that:
+            #
+            # Example outputs:
+            # minidump_stackwalk 0.9.2
+            # minidump-stackwalk 0.9.5
+            #
+            # Either `_` or `-` may be used in the name, so be careful of that
+            # (newer versions should use `-`). Otherwise the actual version number
+            # is the usual `<major>.<minor>.<patch>` that can be parsed with
+            # `distutils.version.LooseVersion`.
+            rust_minidump = stackwalk_version_check.returncode == 0
+
+            # Now build up the actual command
+            command = [self.stackwalk_binary]
 
-            command = [self.stackwalk_binary, "--human", path, self.symbols_path]
             # Fallback to the symbols server for unknown symbols on automation
             # (mostly for system libraries).
             if "MOZ_AUTOMATION" in os.environ:
                 command.append("--symbols-url=https://symbols.mozilla.org/")
+
+            # Specify the kind of output
+            if rust_minidump:
+                command.append("--human")
+
+            # The minidump path and symbols_path values are positional and come last
+            # (in practice the CLI parsers are more permissive, but best not to
+            # unecessarily play with fire).
+            command.append(path)
+            command.append(self.symbols_path)
+
             self.logger.info(u"Copy/paste: {}".format(" ".join(command)))
             # run minidump_stackwalk
             p = subprocess.Popen(