commit 9c002b5a70618ce822428e4b2d2e81a338f607f6
parent c4888cef7e32ab0438960020070050592d4bbaeb
Author: Alex Franchuk <afranchuk@mozilla.com>
Date: Mon, 15 Dec 2025 18:57:37 +0000
Bug 1989439 p2 - Exclusively acquire the Glean store in the crashping crate r=gsvelto
A Glean store cannot be accessed by more than one application at a time.
Differential Revision: https://phabricator.services.mozilla.com/D274951
Diffstat:
3 files changed, 344 insertions(+), 5 deletions(-)
diff --git a/toolkit/crashreporter/crashping/src/lib.rs b/toolkit/crashreporter/crashping/src/lib.rs
@@ -15,6 +15,8 @@ mod glean_metrics {
include!(env!("GLEAN_METRICS_FILE"));
}
mod annotations;
+mod single_instance;
+
use annotations::ANNOTATIONS;
const TELEMETRY_SERVER: &str = "https://incoming.telemetry.mozilla.org";
@@ -42,6 +44,18 @@ pub struct InitGlean {
pub clear_uploader_for_tests: bool,
}
+/// A handle taking ownership of the Glean store.
+pub struct GleanHandle {
+ single_instance: single_instance::SingleInstance,
+}
+
+impl GleanHandle {
+ /// Own the Glean store for the lifetime of the application.
+ pub fn application_lifetime(self) {
+ self.single_instance.retain_until_application_exit();
+ }
+}
+
impl InitGlean {
/// The data_dir should be a dedicated directory for use by Glean.
pub fn new(data_dir: PathBuf, app_id: &str, client_info_metrics: ClientInfoMetrics) -> Self {
@@ -56,15 +70,36 @@ impl InitGlean {
}
}
- pub fn initialize(self) {
+ pub fn initialize(self) -> std::io::Result<GleanHandle> {
self.init_with(glean::initialize)
}
+ /// Initialize using glean::test_reset_glean.
+ ///
+ /// This will not do any process locking of the Glean store.
pub fn test_reset_glean(self, clear_stores: bool) {
- self.init_with(move |c, m| glean::test_reset_glean(c, m, clear_stores))
+ self.init_with_no_lock(move |c, m| glean::test_reset_glean(c, m, clear_stores))
+ }
+
+ /// Initialize with the given function.
+ ///
+ /// This will take exclusive ownership of the Glean store for the current process (potentially
+ /// blocking). The returned GleanHandle tracks ownership.
+ pub fn init_with<F: FnOnce(Configuration, ClientInfoMetrics)>(
+ self,
+ f: F,
+ ) -> std::io::Result<GleanHandle> {
+ std::fs::create_dir_all(&self.configuration.data_path)?;
+ let handle = GleanHandle {
+ single_instance: single_instance::SingleInstance::acquire(
+ &self.configuration.data_path.join("crashping.pid"),
+ )?,
+ };
+ self.init_with_no_lock(f);
+ Ok(handle)
}
- pub fn init_with<F: FnOnce(Configuration, ClientInfoMetrics)>(self, f: F) {
+ pub fn init_with_no_lock<F: FnOnce(Configuration, ClientInfoMetrics)>(mut self, f: F) {
// Clear the uploader for tests, if configured.
// No need to check `cfg!(test)`, since we don't set an uploader in unit tests (and if we
// did, it would be test-specific).
@@ -74,9 +109,8 @@ impl InitGlean {
self.configuration.uploader = None;
self.configuration.server_endpoint = None;
}
-
init();
- f(self.configuration, self.client_info_metrics)
+ f(self.configuration, self.client_info_metrics);
}
}
diff --git a/toolkit/crashreporter/crashping/src/single_instance.rs b/toolkit/crashreporter/crashping/src/single_instance.rs
@@ -0,0 +1,151 @@
+/* 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/. */
+
+//! The Glean DB can only be accessed by one process at a time, so this module offers a simple way
+//! to negotiate this (allowing other processes to wait).
+//!
+//! TODO: Use std file locking when the MSRV is >=1.89.
+
+use std::{
+ fs::{File, OpenOptions},
+ io::{Result, Write},
+ path::Path,
+ process,
+};
+
+pub struct SingleInstance {
+ lockfile: File,
+}
+
+impl SingleInstance {
+ pub fn acquire(path: &Path) -> Result<Self> {
+ let mut lockfile = OpenOptions::new()
+ .read(true)
+ .write(true)
+ .create(true)
+ .open(path)?;
+
+ lock(&lockfile)?;
+ write!(&mut lockfile, "{}", process::id())?;
+ Ok(SingleInstance { lockfile })
+ }
+
+ pub fn retain_until_application_exit(self) {
+ std::mem::forget(self);
+ }
+}
+
+impl Drop for SingleInstance {
+ fn drop(&mut self) {
+ // Don't attempt to delete the file, as there are races that can't be avoided if we do.
+ let _ = self.lockfile.set_len(0);
+ if let Err(e) = unlock(&self.lockfile) {
+ log::error!("failed to unlock lockfile: {e}");
+ }
+ }
+}
+
+#[cfg(unix)]
+mod unix {
+ use std::ffi::c_int;
+ use std::fs::File;
+ use std::io::{Error, Result};
+ use std::os::fd::AsRawFd;
+
+ const LOCK_EX: c_int = 2;
+ const LOCK_UN: c_int = 8;
+
+ extern "C" {
+ fn flock(fd: c_int, operation: c_int) -> c_int;
+ }
+
+ pub fn lock(file: &File) -> Result<()> {
+ let result = unsafe { flock(file.as_raw_fd(), LOCK_EX) };
+ (result == 0).then_some(()).ok_or_else(Error::last_os_error)
+ }
+
+ pub fn unlock(file: &File) -> Result<()> {
+ let result = unsafe { flock(file.as_raw_fd(), LOCK_UN) };
+ (result == 0).then_some(()).ok_or_else(Error::last_os_error)
+ }
+}
+
+#[cfg(unix)]
+use unix::{lock, unlock};
+
+#[cfg(windows)]
+mod windows {
+ use std::ffi::{c_int, c_void};
+ use std::fs::File;
+ use std::io::{Error, Result};
+ use std::os::windows::io::AsRawHandle;
+
+ type HANDLE = *mut c_void;
+ type DWORD = u32;
+ type BOOL = c_int;
+
+ const LOCKFILE_EXCLUSIVE_LOCK: DWORD = 2;
+
+ #[repr(C)]
+ struct OVERLAPPED {
+ internal: usize,
+ internalhigh: usize,
+ // Omit the union of the offset fields with the `PVOID Pointer` since we don't need it.
+ offset: DWORD,
+ offsethigh: DWORD,
+ hevent: HANDLE,
+ }
+
+ impl Default for OVERLAPPED {
+ fn default() -> Self {
+ // # Safety
+ // A zeroed OVERLAPPED is defined as valid.
+ unsafe { std::mem::zeroed() }
+ }
+ }
+
+ extern "system" {
+ fn LockFileEx(
+ hfile: HANDLE,
+ dwflags: DWORD,
+ dwreserved: DWORD,
+ nnumberofbytestolocklow: DWORD,
+ nnumberofbytestolockhigh: DWORD,
+ lpoverlapped: *mut OVERLAPPED,
+ ) -> BOOL;
+
+ fn UnlockFileEx(
+ hfile: HANDLE,
+ dwreserved: DWORD,
+ nnumberofbytestounlocklow: DWORD,
+ nnumberofbytestounlockhigh: DWORD,
+ lpoverlapped: *mut OVERLAPPED,
+ ) -> BOOL;
+ }
+
+ pub fn lock(file: &File) -> Result<()> {
+ let mut overlapped = OVERLAPPED::default();
+ let result = unsafe {
+ LockFileEx(
+ file.as_raw_handle(),
+ LOCKFILE_EXCLUSIVE_LOCK,
+ 0,
+ u32::MAX,
+ u32::MAX,
+ &mut overlapped,
+ )
+ };
+ (result != 0).then_some(()).ok_or_else(Error::last_os_error)
+ }
+
+ pub fn unlock(file: &File) -> Result<()> {
+ let mut overlapped = OVERLAPPED::default();
+ let result =
+ unsafe { UnlockFileEx(file.as_raw_handle(), 0, u32::MAX, u32::MAX, &mut overlapped) };
+ (result != 0).then_some(()).ok_or_else(Error::last_os_error)
+ }
+}
+
+#[cfg(windows)]
+use windows::{lock, unlock};
diff --git a/toolkit/crashreporter/crashping/tests/single_instance.rs b/toolkit/crashreporter/crashping/tests/single_instance.rs
@@ -0,0 +1,154 @@
+use crashping::{ClientInfoMetrics, InitGlean};
+use std::process::{Child, Command};
+use std::sync::{
+ atomic::{AtomicBool, Ordering::Relaxed},
+ Arc, Condvar, Mutex,
+};
+use std::time::Duration;
+
+#[test]
+#[ignore]
+fn do_glean_init() {
+ let data_dir = std::env::temp_dir()
+ .join("crashping_tests")
+ .join("do_glean_init");
+ let _glean_handle = InitGlean::new(
+ data_dir,
+ "my_app",
+ ClientInfoMetrics {
+ app_build: "build".into(),
+ app_display_version: "version".into(),
+ channel: None,
+ locale: None,
+ },
+ )
+ .initialize()
+ .unwrap();
+
+ eprintln!("got handle");
+
+ // Wait and hold the glean handle indefinitely. The caller will kill us.
+ std::thread::park();
+}
+
+struct GleanInitChild {
+ child: Child,
+ has_handle: Arc<AtomicBool>,
+}
+
+#[derive(Default)]
+struct Signal {
+ condvar: Condvar,
+ gate: Mutex<()>,
+}
+
+impl Signal {
+ fn wait(&self, guard: std::sync::MutexGuard<'_, ()>) {
+ let (_guard, result) = self
+ .condvar
+ .wait_timeout(guard, Duration::from_millis(250))
+ .unwrap();
+ if result.timed_out() {
+ panic!("failed to see signal change before timeout");
+ }
+ }
+
+ fn notify(&self) {
+ self.condvar.notify_all();
+ }
+
+ fn lock(&self) -> std::sync::MutexGuard<'_, ()> {
+ self.gate.lock().unwrap()
+ }
+}
+
+impl GleanInitChild {
+ fn new(has_handle_changed: Arc<Signal>) -> Self {
+ let me = std::env::current_exe().unwrap();
+ let mut child = Command::new(me)
+ .args(["--ignored", "--exact", "--nocapture", "do_glean_init"])
+ .stdout(std::process::Stdio::null())
+ .stderr(std::process::Stdio::piped())
+ .spawn()
+ .expect("failed to execute test child");
+ let mut stderr = std::io::BufReader::new(child.stderr.take().unwrap());
+ let has_handle = Arc::new(AtomicBool::new(false));
+ let has_handle_t = has_handle.clone();
+ std::thread::spawn(move || {
+ use std::io::BufRead;
+ let mut line = String::new();
+ // Catch this sort of error with the top-level timeout.
+ let Ok(_) = stderr.read_line(&mut line) else {
+ return;
+ };
+ if line == "got handle\n" {
+ let _guard = has_handle_changed.lock();
+ has_handle_t.store(true, Relaxed);
+ has_handle_changed.notify();
+ }
+ });
+
+ GleanInitChild { child, has_handle }
+ }
+
+ fn has_handle(&mut self) -> bool {
+ self.has_handle.load(Relaxed)
+ }
+
+ fn kill(&mut self) {
+ self.child.kill().unwrap();
+ self.has_handle.store(false, Relaxed);
+ }
+}
+
+impl Drop for GleanInitChild {
+ fn drop(&mut self) {
+ self.kill();
+ }
+}
+
+#[test]
+fn exclusive_access() {
+ let has_handle_changed = Arc::new(Signal::default());
+
+ let guard = has_handle_changed.lock();
+
+ // Spawn two children
+ let mut child_a = GleanInitChild::new(has_handle_changed.clone());
+ let mut child_b = GleanInitChild::new(has_handle_changed.clone());
+
+ has_handle_changed.wait(guard);
+
+ // Give a little time for the other child to potentially get a handle too (if there's a bug).
+ std::thread::sleep(Duration::from_millis(100));
+
+ assert_ne!(
+ child_a.has_handle(),
+ child_b.has_handle(),
+ "exactly one of the children should have the handle"
+ );
+ let was_child_a = child_a.has_handle();
+
+ let guard = has_handle_changed.lock();
+ // Kill the child that has the glean store.
+ if was_child_a {
+ child_a.kill();
+ } else {
+ child_b.kill();
+ }
+ assert_eq!(child_a.has_handle() || child_b.has_handle(), false);
+ has_handle_changed.wait(guard);
+
+ // Ensure the other child acquired the handle.
+ if was_child_a {
+ assert!(
+ child_b.has_handle() && !child_a.has_handle(),
+ "child b should have the handle"
+ );
+ } else {
+ assert!(
+ child_a.has_handle() && !child_b.has_handle(),
+ "child a should ahve the handle"
+ );
+ }
+}
