Bug 1899371 - Clean up crash reporter code and fix logging env variables r=gsvelto

Differential Revision: https://phabricator.services.mozilla.com/D211890

toolkit/crashreporter/client/app/src/config.rs

@@ -9,6 +9,7 @@ use crate::std::ffi::{OsStr, OsString};
 use crate::std::path::{Path, PathBuf};
 use crate::{lang, logging::LogTarget, std};
 use anyhow::Context;
+use once_cell::sync::Lazy;
 
 /// The number of the most recent minidump files to retain when pruning.
 const MINIDUMP_PRUNE_SAVE_COUNT: usize = 10;
@@ -81,13 +82,6 @@ impl Config {
     /// Load a configuration from the application environment.
     #[cfg_attr(mock, allow(unused))]
     pub fn read_from_environment(&mut self) -> anyhow::Result<()> {
-        /// Most environment variables are prefixed with `MOZ_CRASHREPORTER_`.
-        macro_rules! ekey {
-            ( $name:literal ) => {
-                concat!("MOZ_CRASHREPORTER_", $name)
-            };
-        }
-
         self.auto_submit = env_bool(ekey!("AUTO_SUBMIT"));
         self.dump_all_threads = env_bool(ekey!("DUMP_ALL_THREADS"));
         self.delete_dump = !env_bool(ekey!("NO_DELETE_DUMP"));
@@ -492,18 +486,8 @@ impl Config {
 ///
 /// The returned path isn't guaranteed to exist.
 pub fn sibling_path<N: AsRef<OsStr>>(file: N) -> PathBuf {
-    // Expect shouldn't ever panic here because we need more than one argument to run
+    // Expect shouldn't panic as we don't invoke the program without a parent directory.
-    // the program in the first place (we've already previously iterated args).
+    let dir_path = self_path().parent().expect("program invoked based on PATH");
-    //
-    // We use argv[0] rather than `std::env::current_exe` because `current_exe` doesn't define
-    // how symlinks are treated, and we want to support running directly from the local build
-    // directory (which uses symlinks on linux and macos).
-    let dir_path = {
-        let mut path = self_path();
-        // Pop the executable off to get the parent directory.
-        path.pop();
-        path
-    };
 
     let mut path = dir_path.join(file.as_ref());
 
@@ -515,19 +499,28 @@ pub fn sibling_path<N: AsRef<OsStr>>(file: N) -> PathBuf {
         // The other applications we ship with Firefox are stored in the main bundle
         // (Firefox.app/Contents/MacOS/) so we we need to go back three directories
         // to reach them.
-        path = dir_path;
+
-        // 3 pops for `crashreporter.app/Contents/MacOS`.
+        // 3rd ancestor (the 0th element of ancestors has no paths removed) to remove
-        for _ in 0..3 {
+        // `crashreporter.app/Contents/MacOS`.
-            path.pop();
+        if let Some(ancestor) = dir_path.ancestors().nth(3) {
+            path = ancestor.join(file.as_ref());
         }
-        path.push(file.as_ref());
     }
 
     path
 }
 
-fn self_path() -> PathBuf {
+fn self_path() -> &'static Path {
+    static PATH: Lazy<PathBuf> = Lazy::new(|| {
+        // Expect shouldn't ever panic here because we need more than one argument to run
+        // the program in the first place (we've already previously iterated args).
+        //
+        // We use argv[0] rather than `std::env::current_exe` because `current_exe` doesn't define
+        // how symlinks are treated, and we want to support running directly from the local build
+        // directory (which uses symlinks on linux and macos).
         PathBuf::from(std::env::args_os().next().expect("failed to get argv[0]"))
+    });
+    &*PATH
 }
 
 fn env_bool<K: AsRef<OsStr>>(name: K) -> bool {

toolkit/crashreporter/client/app/src/lang/mod.rs

@@ -16,7 +16,7 @@ use std::collections::BTreeMap;
 pub fn load() -> anyhow::Result<LangStrings> {
     // TODO support langpacks, bug 1873210
     omnijar::read().unwrap_or_else(|e| {
-        log::warn!("failed to read localization data from the omnijar ({e}), falling back to bundled content");
+        log::warn!("failed to read localization data from the omnijar ({e:#}), falling back to bundled content");
         Default::default()
     }).load_strings()
 }

toolkit/crashreporter/client/app/src/logging.rs

@@ -19,8 +19,8 @@ pub fn init() -> LogTarget {
     env_logger::builder()
         .parse_env(
             env_logger::Env::new()
-                .filter("MOZ_CRASHEREPORTER")
+                .filter(ekey!("LOG"))
-                .write_style("MOZ_CRASHREPORTER_STYLE"),
+                .write_style(ekey!("LOG_STYLE")),
         )
         .target(env_logger::fmt::Target::Pipe(Box::new(
             log_target_inner.clone(),

toolkit/crashreporter/client/app/src/main.rs

@@ -36,10 +36,10 @@ use crate::std::sync::Arc;
 use anyhow::Context;
 use config::Config;
 
+// A few macros are defined here to allow use in all submodules via textual scope lookup.
+
 /// cc is short for Clone Capture, a shorthand way to clone a bunch of values before an expression
 /// (particularly useful for closures).
-///
-/// It is defined here to allow it to be used in all submodules (textual scope lookup).
 macro_rules! cc {
     ( ($($c:ident),*) $e:expr ) => {
         {
@@ -49,6 +49,15 @@ macro_rules! cc {
     }
 }
 
+/// Create a string literal to be used as an environment variable name.
+///
+/// This adds the application prefix `MOZ_CRASHREPORTER_`.
+macro_rules! ekey {
+    ( $name:literal ) => {
+        concat!("MOZ_CRASHREPORTER_", $name)
+    };
+}
+
 mod async_task;
 mod config;
 mod data;