Bug 1896097 - Fix location of the omnijar file(s) on macos r=gsvelto
authorAlex Franchuk <afranchuk@mozilla.com>
Mon, 13 May 2024 20:27:03 +0000 (14 months ago)
changeset 738546 080acbd234b3d95bed203c9e575f7841db819f01
parent 738545 6fc61adc95d0ba726d457eab0087dc49e6f392e9
child 738547 80d4f40d01124f8d105c4f96a504692d2afb449d
push id205607
push usergsvelto@mozilla.com
push dateMon, 13 May 2024 20:35:48 +0000 (14 months ago)
treeherderautoland@080acbd234b3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgsvelto
bugs1896097
milestone128.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 1896097 - Fix location of the omnijar file(s) on macos r=gsvelto The branding was also potentially not read correctly (from the browser omnijar) as a result of incorrect merging of changes from another commit, however the impact of this is far less as the branding is likely similar across locales. Differential Revision: https://phabricator.services.mozilla.com/D210046
toolkit/crashreporter/client/app/src/config.rs
toolkit/crashreporter/client/app/src/lang/omnijar.rs
toolkit/crashreporter/client/app/src/std/env.rs
--- a/toolkit/crashreporter/client/app/src/config.rs
+++ b/toolkit/crashreporter/client/app/src/config.rs
@@ -365,50 +365,26 @@ impl Config {
     /// program bundle. On other platforms this assumes siblings reside in the same directory as
     /// the crashreporter.
     ///
     /// The returned path isn't guaranteed to exist.
     // This method could be standalone rather than living in `Config`; it's here because it makes
     // sense that if it were to rely on anything, it would be the `Config` (and that may change in
     // the future).
     pub fn sibling_program_path<N: AsRef<OsStr>>(&self, program: N) -> PathBuf {
-        // 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).
-        let self_path = PathBuf::from(std::env::args_os().next().expect("failed to get argv[0]"));
+        let self_path = self_path();
         let exe_extension = self_path.extension().unwrap_or_default();
-
-        let mut program_path = self_path.clone();
-        // Pop the executable off to get the parent directory.
-        program_path.pop();
-        program_path.push(program.as_ref());
-        program_path.set_extension(exe_extension);
-
-        if !program_path.exists() && cfg!(all(not(mock), target_os = "macos")) {
-            // On macOS the crash reporter client is shipped as an application bundle contained
-            // within Firefox's main application bundle. So when it's invoked its current working
-            // directory looks like:
-            // Firefox.app/Contents/MacOS/crashreporter.app/Contents/MacOS/
-            // 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.
-
-            // 4 pops: 1 for the path that was just pushed, and 3 more for
-            // `crashreporter.app/Contents/MacOS`.
-            for _ in 0..4 {
-                program_path.pop();
-            }
-            program_path.push(program.as_ref());
-            program_path.set_extension(exe_extension);
+        if !exe_extension.is_empty() {
+            let mut p = program.as_ref().to_os_string();
+            p.push(".");
+            p.push(exe_extension);
+            sibling_path(p)
+        } else {
+            sibling_path(program)
         }
-
-        program_path
     }
 
     cfg_if::cfg_if! {
         if #[cfg(mock)] {
             fn get_data_dir(&self, vendor: &str, product: &str) -> anyhow::Result<PathBuf> {
                 let mut path = PathBuf::from("data_dir");
                 path.push(vendor);
                 path.push(product);
@@ -503,16 +479,62 @@ impl Config {
                 path.push(product);
                 path.push("Crash Reports");
                 Ok(path)
             }
         }
     }
 }
 
+/// Get the path of a file that is a sibling of the crashreporter.
+///
+/// On MacOS, this assumes that the crashreporter is its own application bundle within the main
+/// program bundle. On other platforms this assumes siblings reside in the same directory as
+/// the crashreporter.
+///
+/// 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
+    // 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).
+    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());
+
+    if !path.exists() && cfg!(all(not(mock), target_os = "macos")) {
+        // On macOS the crash reporter client is shipped as an application bundle contained
+        // within Firefox's main application bundle. So when it's invoked its current working
+        // directory looks like:
+        // Firefox.app/Contents/MacOS/crashreporter.app/Contents/MacOS/
+        // 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`.
+        for _ in 0..3 {
+            path.pop();
+        }
+        path.push(file.as_ref());
+    }
+
+    path
+}
+
+fn self_path() -> PathBuf {
+    PathBuf::from(std::env::args_os().next().expect("failed to get argv[0]"))
+}
+
 fn env_bool<K: AsRef<OsStr>>(name: K) -> bool {
     std::env::var(name).map(|s| !s.is_empty()).unwrap_or(false)
 }
 
 fn env_path<K: AsRef<OsStr>>(name: K) -> Option<PathBuf> {
     std::env::var_os(name).map(PathBuf::from)
 }
 
--- a/toolkit/crashreporter/client/app/src/lang/omnijar.rs
+++ b/toolkit/crashreporter/client/app/src/lang/omnijar.rs
@@ -1,29 +1,31 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use super::language_info::LanguageInfo;
+use crate::config::sibling_path;
 use crate::std::{
-    env::current_exe,
     fs::File,
     io::{BufRead, BufReader, Read},
     path::Path,
 };
 use anyhow::Context;
 use zip::read::ZipArchive;
 
 /// Read the appropriate localization fluent definitions from the omnijar files.
 ///
 /// Returns (locale name, fluent definitions).
 pub fn read() -> anyhow::Result<LanguageInfo> {
-    let mut path = current_exe().context("failed to get current executable")?;
-    path.pop();
-    path.push("omni.ja");
+    let mut path = sibling_path(if cfg!(target_os = "macos") {
+        "../Resources/omni.ja"
+    } else {
+        "omni.ja"
+    });
 
     let mut zip = read_omnijar_file(&path)?;
     let locales = {
         let buf = BufReader::new(
             zip.by_name("res/multilocale.txt")
                 .context("failed to read multilocale file in zip archive")?,
         );
         let line = buf
@@ -47,21 +49,26 @@ pub fn read() -> anyhow::Result<Language
     };
 
     // The brand ftl is in the browser omnijar.
     path.pop();
     path.push("browser");
     path.push("omni.ja");
 
     let ftl_branding = 'branding: {
-        for locale in &locales {
-            match read_branding(&locale, &mut zip) {
-                Ok(v) => break 'branding v,
-                Err(e) => log::warn!("failed to read branding from omnijar: {e:#}"),
+        match read_omnijar_file(&path) {
+            Ok(mut zip) => {
+                for locale in &locales {
+                    match read_branding(&locale, &mut zip) {
+                        Ok(v) => break 'branding v,
+                        Err(e) => log::warn!("failed to read branding from omnijar: {e:#}"),
+                    }
+                }
             }
+            Err(e) => log::warn!("failed to read browser omnijar: {e:#}"),
         }
         log::info!("using fallback branding info");
         LanguageInfo::default().ftl_branding
     };
 
     Ok(LanguageInfo {
         identifier: locale,
         ftl_definitions,
--- a/toolkit/crashreporter/client/app/src/std/env.rs
+++ b/toolkit/crashreporter/client/app/src/std/env.rs
@@ -35,11 +35,12 @@ pub fn var_os<K: AsRef<OsStr>>(_key: K) 
 }
 
 pub fn args_os() -> ArgsOs {
     MockCurrentExe.get(|r| ArgsOs {
         argv0: Some(r.clone().into()),
     })
 }
 
+#[allow(unused)]
 pub fn current_exe() -> std::io::Result<super::path::PathBuf> {
     Ok(MockCurrentExe.get(|r| r.clone().into()))
 }