commit b9def205964f2869b1e2b2587386b0b80bb4d615
parent 4d3ded9dbfe94e473a9869299a7e724f09dcfa81
Author: Alex Franchuk <afranchuk@mozilla.com>
Date: Tue, 28 Oct 2025 17:21:47 +0000
Bug 1992420 - Do not retry the crash reporter necko backend upon critical failure r=gsvelto
We allow two attempts for the background task to succeed. If it fails
twice (not necessarily in a row), we simply use the curl backend
instead. Some failure modes of the background task are easily observable
(crashes produce signals or specific return values, and the launcher returns
255 on fatal error), however there are other failure modes, like
misconfigured network settings, for which we cannot account. This is why
we use a simpler means of fallback.
This also adds a mocking interface for statics, so that e.g. interior
mutability won't affect concurrently-run tests, and uses it to test the
behavior.
Differential Revision: https://phabricator.services.mozilla.com/D267469
Diffstat:
5 files changed, 167 insertions(+), 15 deletions(-)
diff --git a/toolkit/crashreporter/client/app/src/net/http.rs b/toolkit/crashreporter/client/app/src/net/http.rs
@@ -16,6 +16,7 @@ use crate::std::{
mem::ManuallyDrop,
path::{Path, PathBuf},
process::Child,
+ sync::atomic::{AtomicUsize, Ordering::Relaxed},
};
use anyhow::Context;
use once_cell::sync::Lazy;
@@ -53,6 +54,55 @@ pub fn user_agent() -> &'static str {
&*USER_AGENT
}
+pub(crate) struct BackgroundTaskAttempts {
+ remaining: AtomicUsize,
+}
+
+impl BackgroundTaskAttempts {
+ pub const fn new(count: usize) -> Self {
+ BackgroundTaskAttempts {
+ remaining: AtomicUsize::new(count),
+ }
+ }
+
+ /// Returns whether the background task should be attempted again.
+ pub fn should_attempt(&self) -> bool {
+ self.remaining.load(Relaxed) != 0
+ }
+
+ /// Record a failed background task attempt.
+ pub fn failed(&self) {
+ self.remaining.fetch_sub(1, Relaxed);
+ }
+
+ /// Remove all background task attempts.
+ pub fn drain(&self) {
+ self.remaining.store(0, Relaxed);
+ }
+}
+
+std::mock::mocked_static! {
+ /// How many times the background task may fail before discontinuing use.
+ ///
+ /// The background task may repeatedly fail for many reasons, for example due to a crash or
+ /// misconfigured network settings.
+ static BACKGROUND_TASK_ATTEMPTS: BackgroundTaskAttempts = BackgroundTaskAttempts::new(2);
+
+ impl BACKGROUND_TASK_ATTEMPTS {
+ fn should_attempt(self) -> bool {
+ Self.try_get(|v| v.should_attempt()).unwrap_or(true)
+ }
+
+ fn failed(self) {
+ Self.try_get(|v| v.failed());
+ }
+
+ fn drain(self) {
+ Self.try_get(|v| v.drain());
+ }
+ }
+}
+
// TODO set reasonable connect timeout and low speed limit?
/*
@@ -154,14 +204,19 @@ impl<'a> RequestBuilder<'a> {
// First we try to invoke a firefox background task to send the request. This is
// preferrable because it will respect the user's network settings, however it is less
// reliable if the cause of our crash is a bug in early firefox startup or network code.
- let background_task_err = match self.send_with_background_task(url) {
- Ok(r) => return Ok(r),
- Err(e) => e,
- };
-
- log::info!(
- "failed to invoke background task ({background_task_err}), falling back to curl backend"
- );
+ if BACKGROUND_TASK_ATTEMPTS.should_attempt() {
+ match self.send_with_background_task(url) {
+ Ok(r) => return Ok(r),
+ Err(e) => {
+ log::info!(
+ "failed to invoke background task ({e}), falling back to curl backend"
+ );
+ // A failure to spawn the background task more than likely indicates it will
+ // never work in the future.
+ BACKGROUND_TASK_ATTEMPTS.drain();
+ }
+ };
+ }
self.send_with_curl(url)
}
@@ -461,12 +516,20 @@ impl Request<'_> {
let output = child
.wait_with_output()
.context("failed to wait on background task process")?;
- anyhow::ensure!(
- output.status.success(),
- "process failed (exit status {}) with stderr: {}",
- output.status,
- String::from_utf8_lossy(&output.stderr)
- );
+
+ if !output.status.success() {
+ BACKGROUND_TASK_ATTEMPTS.failed();
+ if !BACKGROUND_TASK_ATTEMPTS.should_attempt() {
+ log::info!("the background task process has exceeded the acceptable number of failures and will no longer be used");
+ }
+
+ anyhow::bail!(
+ "process failed (exit status {}) with stderr: {}",
+ output.status,
+ String::from_utf8_lossy(&output.stderr)
+ );
+ }
+
file.rewind().context("failed to rewind response file")?;
let mut ret = Vec::new();
file.read_to_end(&mut ret)
diff --git a/toolkit/crashreporter/client/app/src/std/mock.rs b/toolkit/crashreporter/client/app/src/std/mock.rs
@@ -268,3 +268,15 @@ pub trait MockUnwrap {
pub fn unwrap<T: MockUnwrap>(value: T) -> T::Inner {
value.unwrap()
}
+
+/// A static which can be replaced with mocked values when mocking is enabled.
+///
+/// This is especially useful to avoid statics with interior mutations affecting tests.
+macro_rules! mocked_static {
+ ( $(#[$m:meta])* $vis:vis static $name:ident: $T:ty = $init:expr ; $($item:item)*) => {
+ mock_key! { $(#[$m])* #[allow(non_camel_case_types)] pub(crate) struct $name => std::sync::Arc<$T> }
+ $($item)*
+ };
+}
+
+pub(crate) use mocked_static;
diff --git a/toolkit/crashreporter/client/app/src/std/mock_stub.rs b/toolkit/crashreporter/client/app/src/std/mock_stub.rs
@@ -5,6 +5,17 @@
//! Stubs used when mocking isn't enabled.
+/// A static which can be replaced with mocked values when mocking is enabled.
+///
+/// This is especially useful to avoid statics with interior mutations affecting tests.
+macro_rules! mocked_static {
+ ( $(#[$m:meta])* $vis:vis static $name:ident: $T:ty = $init:expr ; $($item:item)* ) => {
+ $(#[$m])* $vis static $name: $T = $init;
+ }
+}
+
+pub(crate) use mocked_static;
+
/// Create a mock hook with the given name. When mocking isn't enabled, the given value will be
/// used instead. Panics if the hook isn't set.
#[inline(always)]
diff --git a/toolkit/crashreporter/client/app/src/std/process.rs b/toolkit/crashreporter/client/app/src/std/process.rs
@@ -214,7 +214,9 @@ impl std::io::Write for ChildStdin {
#[cfg(unix)]
pub fn exit_status(status: i32) -> ExitStatus {
use std::os::unix::process::ExitStatusExt;
- ExitStatus::from_raw(status)
+ // ExitStatus::from_raw actually takes a *wait status*, which stores the exit status in the
+ // second byte on BSDs and Linux.
+ ExitStatus::from_raw(status << 8)
}
#[cfg(windows)]
diff --git a/toolkit/crashreporter/client/app/src/test.rs b/toolkit/crashreporter/client/app/src/test.rs
@@ -1480,8 +1480,13 @@ fn background_task_curl_fallback() {
let mock_ran_bgtask = ran_bgtask.clone();
let ran_curl = Counter::new();
let mock_ran_curl = ran_curl.clone();
+ let background_task_attempts = Arc::new(net::http::BackgroundTaskAttempts::new(2));
test.mock
.set(
+ net::http::BACKGROUND_TASK_ATTEMPTS,
+ background_task_attempts.clone(),
+ )
+ .set(
Command::mock("work_dir/firefox"),
Box::new(move |cmd| {
if cmd.spawning {
@@ -1599,6 +1604,65 @@ fn background_task_curl_fallback() {
ran_bgtask.assert_one();
ran_curl.assert_one();
+ // Verify that background tasks are still enabled.
+ assert!(background_task_attempts.should_attempt());
+
+ test.assert_files()
+ .saved_settings(Settings::default())
+ .submitted();
+}
+
+#[test]
+fn background_task_disables() {
+ let mut test = GuiTest::new();
+ let ran_bgtask = Counter::new();
+ let mock_ran_bgtask = ran_bgtask.clone();
+ let ran_curl = Counter::new();
+ let mock_ran_curl = ran_curl.clone();
+ let background_task_attempts = Arc::new(net::http::BackgroundTaskAttempts::new(1));
+ test.mock
+ .set(
+ net::http::BACKGROUND_TASK_ATTEMPTS,
+ background_task_attempts.clone(),
+ )
+ .set(
+ Command::mock("work_dir/firefox"),
+ Box::new(move |cmd| {
+ if cmd.spawning {
+ return Ok(crate::std::process::success_output());
+ }
+ mock_ran_bgtask.inc();
+
+ Ok(crate::std::process::Output {
+ status: crate::std::process::exit_status(255),
+ stdout: vec![],
+ stderr: vec![],
+ })
+ }),
+ )
+ .set(
+ Command::mock("curl"),
+ Box::new(move |cmd| {
+ if cmd.spawning {
+ return Ok(crate::std::process::success_output());
+ }
+
+ let mut output = crate::std::process::success_output();
+ output.stdout = format!("CrashID={MOCK_REMOTE_CRASH_ID}").into();
+ mock_ran_curl.inc();
+ Ok(output)
+ }),
+ );
+ test.run(|interact| {
+ interact.element("quit", |_style, b: &model::Button| b.click.fire(&()));
+ });
+
+ ran_bgtask.assert_one();
+ ran_curl.assert_one();
+
+ // Verify that background tasks are now disabled.
+ assert_eq!(false, background_task_attempts.should_attempt());
+
test.assert_files()
.saved_settings(Settings::default())
.submitted();
