From 264d1fe3a58a688b460cee662fb4deb727bea848 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 3 Sep 2026 17:14:33 -0700 Subject: [PATCH 1/5] fix: telemetry shutdown --- components/ads-client/src/client.rs | 2 +- components/ads-client/src/ffi.rs | 56 ++++++++++++++++++++++++---- components/ads-client/src/lib.rs | 57 +++++++++++++++++++++++++---- 3 files changed, 100 insertions(+), 15 deletions(-) diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 739b592204..eb7331909c 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -537,7 +537,7 @@ mod tests { } #[test] - fn test_shutdown_telemetry() { + fn test_shutdown_telemetry_basic() { viaduct_dev::init_backend_dev(); // test with client created from config diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 7f8726cd01..9bce97f60b 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -7,6 +7,8 @@ pub mod error; pub mod telemetry; use std::sync::Arc; +#[cfg(test)] +use std::sync::Weak; use crate::client::config::{AdsCacheConfig, AdsClientConfig}; use crate::client::{AdsClient, ContextIdProvider}; @@ -20,8 +22,8 @@ use crate::mars::ad_response::{ }; use crate::mars::Environment; use crate::mars::ReportReason; -use crate::AdsClientUrl; use crate::MozAdsClient; +use crate::{AdsClientUrl, ShutdownReferences}; use parking_lot::Mutex; use std::collections::HashMap; @@ -125,7 +127,12 @@ impl MozAdsClientBuilder { } pub fn build(&self) -> MozAdsClient { - let inner = self.0.lock(); + let mut inner = self.0.lock(); + let telemetry = inner + .telemetry + .take() + .map(MozAdsTelemetryWrapper::new) + .unwrap_or_else(MozAdsTelemetryWrapper::noop); let client_config = AdsClientConfig { cache_config: inner.cache_config.clone().map(Into::into), context_id_provider: inner @@ -134,15 +141,12 @@ impl MozAdsClientBuilder { .map(MozAdsContextIdProviderWrapper::new) .map(Into::into), environment: inner.environment.unwrap_or_default().into(), - telemetry: inner - .telemetry - .clone() - .map(MozAdsTelemetryWrapper::new) - .unwrap_or_else(MozAdsTelemetryWrapper::noop), + telemetry: telemetry.clone(), }; let client = AdsClient::new(client_config); MozAdsClient { inner: Mutex::new(client), + shutdown_references: ShutdownReferences { telemetry }, } } @@ -170,6 +174,13 @@ impl MozAdsClientBuilder { } } +impl MozAdsClientBuilder { + #[cfg(test)] + pub fn fetch_telemetry(&self) -> Option> { + self.0.lock().telemetry.as_ref().map(|t| Arc::downgrade(&t)) + } +} + #[derive(Clone, Copy, Debug, Default, uniffi::Enum, Eq, PartialEq)] pub enum MozAdsEnvironment { #[default] @@ -473,3 +484,34 @@ impl From<&MozAdsPlacementRequestWithCount> for AdPlacementRequest { } } } + +#[cfg(test)] +mod tests { + use crate::{ffi::telemetry::NoopMozAdsTelemetry, MozAdsClientBuilder}; + use std::sync::Arc; + + #[test] + fn test_telemetry_not_held_by_builder() { + // Related to Bug 2064543 + // The builder can hold a reference to the passed telemetry, meaning that if the builder still exists, `shutdown` doesn't drop all references. + + // Make a builder and pass in telemetry. + let builder = Arc::new(MozAdsClientBuilder::new()); + assert!(builder.fetch_telemetry().is_none()); + let builder = MozAdsClientBuilder::telemetry(builder, Box::new(NoopMozAdsTelemetry)); + let weak_telemetry = builder + .fetch_telemetry() + .expect("Telemetry should be set in builder after being passed"); + assert_eq!(weak_telemetry.strong_count(), 1); + + // Building the MozAdsClient should pass the telemetry, not clone it. + let built_client = builder.build(); + assert_eq!(weak_telemetry.strong_count(), 1); + + // Shutting down, even though the builder still exists, should successfully shutdown all telemetry references. + built_client.shutdown().unwrap(); + assert_eq!(weak_telemetry.strong_count(), 0); + builder.build(); + assert_eq!(weak_telemetry.strong_count(), 0); + } +} diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index c59fc6bd85..8a4cacc584 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -12,7 +12,6 @@ use parking_lot::Mutex; use url::Url as AdsClientUrl; use client::AdsClient; -use error_support::error; use http_cache::CachePolicy; use mars::ad_request::{AdPlacementRequest, AdRequestFlags}; mod client; @@ -23,7 +22,7 @@ pub mod telemetry; pub use ffi::*; -use crate::ffi::telemetry::MozAdsTelemetryWrapper; +use crate::{ffi::telemetry::MozAdsTelemetryWrapper, telemetry::Telemetry}; #[cfg(test)] mod test_utils; @@ -39,6 +38,7 @@ uniffi::custom_type!(AdsClientUrl, String, { #[derive(uniffi::Object)] pub struct MozAdsClient { inner: Mutex>, + shutdown_references: ShutdownReferences, } #[uniffi::export] @@ -54,13 +54,10 @@ impl MozAdsClient { // Allows the ads-client to unload some references and prepare for a safe shutdown. // Other methods should not be called after this one. + // Currently it is not possible to return an error, but it may yet be possible to do so, so we keep the Result. #[uniffi::method()] pub fn shutdown(&self) -> AdsClientApiResult<()> { - let mut inner = self.inner.lock(); - if let Err(err) = inner.shutdown_client() { - // Log the error, but continue with shutdown. - error!("Failed to shutdown the ads client: {:?}", err); - } + self.shutdown_references.shutdown(); Ok(()) } @@ -177,3 +174,49 @@ impl MozAdsClient { Ok(response.into_iter().map(|(k, v)| (k, v.into())).collect()) } } + +pub struct ShutdownReferences { + telemetry: MozAdsTelemetryWrapper, +} + +impl ShutdownReferences { + fn shutdown(&self) { + self.telemetry.shutdown(); + } +} + +#[cfg(test)] +mod tests { + use crate::MozAdsClientBuilder; + use std::{sync::mpsc, thread, time::Duration}; + + fn test_timeout(timeout: Duration, func: F) + where + F: FnOnce() + Send + 'static, + { + let (tx, rx) = mpsc::channel(); + let handle = thread::spawn(move || { + func(); + tx.send(()) + .expect("Internal test error: Could not send completion signal"); + }); + + match rx.recv_timeout(timeout) { + Ok(_) => handle.join().unwrap(), + Err(_) => panic!("Test exceeded timeout duration"), + } + } + #[test] + fn shutdown_does_not_require_ads_client_lock() { + test_timeout(Duration::from_secs(5), || { + let builder = MozAdsClientBuilder::new().build(); + let lock = builder.inner.lock(); + + // Holding a inner lock, we try to run shutdown. + builder.shutdown().unwrap(); + + // We explicitly drop the lock at the end. + drop(lock); + }); + } +} From 4158af6456ad5429bfc9383d1813c638a8585303 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 4 Sep 2026 09:35:58 -0700 Subject: [PATCH 2/5] fix: some cleanup and refactoring --- components/ads-client/src/client.rs | 83 +---------------- components/ads-client/src/ffi.rs | 2 +- components/ads-client/src/lib.rs | 49 +--------- components/ads-client/src/mars.rs | 1 + components/ads-client/src/shutdown.rs | 127 ++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 130 deletions(-) create mode 100644 components/ads-client/src/shutdown.rs diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index eb7331909c..2b88bf711a 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -98,18 +98,6 @@ where self.client.clear_cache() } - // Shutdown the db connection and drop references to telemetry callbacks. - // Should be used only when dropping the ads client, this may be extended to drop more things. - pub fn shutdown_client(&mut self) -> Result<(), rusqlite::Error> { - // Drop telemetry (within the telemetry wrapper) - self.telemetry.shutdown(); - - // Shutdown DB - self.client.shutdown_db()?; - - Ok(()) - } - pub fn get_context_id(&self) -> context_id::ApiResult { self.context_id_provider.context_id() } @@ -286,7 +274,7 @@ pub enum ClientOperationEvent { #[cfg(test)] mod tests { - use std::{assert_eq, assert_ne, sync::Arc}; + use std::assert_eq; use crate::{ ffi::telemetry::MozAdsTelemetryWrapper, @@ -535,73 +523,4 @@ mod tests { m1.assert(); m2.assert(); } - - #[test] - fn test_shutdown_telemetry_basic() { - viaduct_dev::init_backend_dev(); - - // test with client created from config - let noop_telemetry = MozAdsTelemetryWrapper::noop(); - let weak_reference = Arc::downgrade( - &noop_telemetry - .clone_inner_arc() - .expect("Inner telemetry should be Some before dropping"), - ); - let config = AdsClientConfig { - cache_config: None, - context_id_provider: None, - environment: Environment::Test, - telemetry: noop_telemetry, - }; - let mut client = AdsClient::new(config); - - // weak ref will show 0 strong references when the Arc is gone. - assert_ne!(weak_reference.strong_count(), 0); - client.shutdown_client().unwrap(); - assert_eq!(weak_reference.strong_count(), 0); - - // test also with internal function from_mars - let noop_telemetry = MozAdsTelemetryWrapper::noop(); - let weak_reference = Arc::downgrade( - &noop_telemetry - .clone_inner_arc() - .expect("Inner telemetry should be Some before dropping"), - ); - let cache = HttpCache::builder("test_shutdown_telemetry") - .build() - .unwrap(); - let mars_client = MARSClient::new(Environment::Test, Some(cache), noop_telemetry); - let mut client = new_with_mars_client(mars_client); - - // weak ref will show 0 strong references when the Arc is gone. - assert_ne!(weak_reference.strong_count(), 0); - client.shutdown_client().unwrap(); - assert_eq!(weak_reference.strong_count(), 0); - } - - #[test] - fn test_shutdown_is_idempotent() { - viaduct_dev::init_backend_dev(); - - let noop_telemetry = MozAdsTelemetryWrapper::noop(); - let weak_reference = Arc::downgrade( - &noop_telemetry - .clone_inner_arc() - .expect("Inner telemetry should be Some before dropping"), - ); - // A real cache so the second shutdown exercises the db close path. - let cache = HttpCache::builder("test_shutdown_is_idempotent") - .build() - .unwrap(); - let mars_client = MARSClient::new(Environment::Test, Some(cache), noop_telemetry); - let mut client = new_with_mars_client(mars_client); - - client.shutdown_client().unwrap(); - assert_eq!(weak_reference.strong_count(), 0); - - // Repeated shutdowns must not error or re-close an already closed connection. - client.shutdown_client().unwrap(); - client.shutdown_client().unwrap(); - assert_eq!(weak_reference.strong_count(), 0); - } } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 9bce97f60b..4cf6c03c09 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -146,7 +146,7 @@ impl MozAdsClientBuilder { let client = AdsClient::new(client_config); MozAdsClient { inner: Mutex::new(client), - shutdown_references: ShutdownReferences { telemetry }, + shutdown_references: ShutdownReferences::new(telemetry), } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 8a4cacc584..c8efa4e844 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -18,11 +18,12 @@ mod client; mod ffi; pub mod http_cache; mod mars; +pub mod shutdown; pub mod telemetry; pub use ffi::*; -use crate::{ffi::telemetry::MozAdsTelemetryWrapper, telemetry::Telemetry}; +use crate::{ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences}; #[cfg(test)] mod test_utils; @@ -174,49 +175,3 @@ impl MozAdsClient { Ok(response.into_iter().map(|(k, v)| (k, v.into())).collect()) } } - -pub struct ShutdownReferences { - telemetry: MozAdsTelemetryWrapper, -} - -impl ShutdownReferences { - fn shutdown(&self) { - self.telemetry.shutdown(); - } -} - -#[cfg(test)] -mod tests { - use crate::MozAdsClientBuilder; - use std::{sync::mpsc, thread, time::Duration}; - - fn test_timeout(timeout: Duration, func: F) - where - F: FnOnce() + Send + 'static, - { - let (tx, rx) = mpsc::channel(); - let handle = thread::spawn(move || { - func(); - tx.send(()) - .expect("Internal test error: Could not send completion signal"); - }); - - match rx.recv_timeout(timeout) { - Ok(_) => handle.join().unwrap(), - Err(_) => panic!("Test exceeded timeout duration"), - } - } - #[test] - fn shutdown_does_not_require_ads_client_lock() { - test_timeout(Duration::from_secs(5), || { - let builder = MozAdsClientBuilder::new().build(); - let lock = builder.inner.lock(); - - // Holding a inner lock, we try to run shutdown. - builder.shutdown().unwrap(); - - // We explicitly drop the lock at the end. - drop(lock); - }); - } -} diff --git a/components/ads-client/src/mars.rs b/components/ads-client/src/mars.rs index fa17857286..07a6fb37d6 100644 --- a/components/ads-client/src/mars.rs +++ b/components/ads-client/src/mars.rs @@ -57,6 +57,7 @@ where self.transport.clear_cache() } + #[allow(dead_code)] pub fn shutdown_db(&mut self) -> Result<(), rusqlite::Error> { self.transport.shutdown_db() } diff --git a/components/ads-client/src/shutdown.rs b/components/ads-client/src/shutdown.rs new file mode 100644 index 0000000000..80d788396c --- /dev/null +++ b/components/ads-client/src/shutdown.rs @@ -0,0 +1,127 @@ +use crate::{ffi::telemetry::MozAdsTelemetryWrapper, telemetry::Telemetry}; + +pub struct ShutdownReferences { + telemetry: MozAdsTelemetryWrapper, +} + +impl ShutdownReferences { + pub fn new(telemetry: MozAdsTelemetryWrapper) -> ShutdownReferences { + ShutdownReferences { telemetry } + } + + // Shutdown anything that needs to be shut down safely and drop references to telemetry callbacks. + // Should be called only when dropping the ads client. This may be extended to drop more things. + pub fn shutdown(&self) { + // Drop telemetry (within the telemetry wrapper) + self.telemetry.shutdown(); + + // TODO: It may be prudent to call the MARSClient `shutdown_db` function here as well. + // However, this requires a mutable lock to be held over the MARSClient (and/or AdsClient), + // which might get held elsewhere over a network request. We can consider re-adding this after + // a refactor or for the new stateful sqlite database. + } +} + +#[cfg(test)] +mod tests { + use crate::{ffi::telemetry::NoopMozAdsTelemetry, MozAdsCacheConfig, MozAdsClientBuilder}; + use std::{ + sync::{mpsc, Arc}, + thread, + time::Duration, + }; + + fn test_timeout(timeout: Duration, func: F) + where + F: FnOnce() + Send + 'static, + { + let (tx, rx) = mpsc::channel(); + let handle = thread::spawn(move || { + func(); + tx.send(()) + .expect("Internal test error: Could not send completion signal"); + }); + + match rx.recv_timeout(timeout) { + Ok(_) => handle.join().unwrap(), + Err(_) => panic!("Test exceeded timeout duration"), + } + } + + // Shutdown procedure must not require a lock to be held on the inner AdsClient. + // This is because sync functions like `request_tile_ads` require (at worst) to wait on a hanging non-cancellable network request to resolve, + // and they hold the lock for the entirety of that time. Shutdown should only require the minimal amount of waiting/locking possible. + #[test] + fn shutdown_does_not_require_ads_client_lock() { + test_timeout(Duration::from_secs(5), || { + let builder = MozAdsClientBuilder::new().build(); + let lock = builder.inner.lock(); + + // Holding a inner lock, we try to run shutdown. + builder.shutdown().unwrap(); + + // We explicitly drop the lock at the end. + drop(lock); + }); + } + + #[test] + fn test_shutdown_telemetry_basic() { + viaduct_dev::init_backend_dev(); + + // test with client created from config with no cache + let builder = Arc::new(MozAdsClientBuilder::new()).telemetry(Box::new(NoopMozAdsTelemetry)); + let weak_reference = builder + .fetch_telemetry() + .expect("Inner telemetry should be Some in builder"); + let client = builder.build(); + + // weak ref will show 0 strong references when the Arc is gone. + assert_ne!(weak_reference.strong_count(), 0); + client.shutdown().unwrap(); + assert_eq!(weak_reference.strong_count(), 0); + + // test also with http cache + let builder = Arc::new(MozAdsClientBuilder::new()) + .telemetry(Box::new(NoopMozAdsTelemetry)) + .cache_config(MozAdsCacheConfig { + db_path: "test_shutdown_is_idempotent".to_string(), + default_cache_ttl_seconds: None, + max_size_mib: None, + }); + let weak_reference = builder + .fetch_telemetry() + .expect("Inner telemetry should be Some in builder"); + let client = builder.build(); + + // weak ref will show 0 strong references when the Arc is gone. + assert_ne!(weak_reference.strong_count(), 0); + client.shutdown().unwrap(); + assert_eq!(weak_reference.strong_count(), 0); + } + + #[test] + fn test_shutdown_is_idempotent() { + viaduct_dev::init_backend_dev(); + + let builder = Arc::new(MozAdsClientBuilder::new()) + .telemetry(Box::new(NoopMozAdsTelemetry)) + .cache_config(MozAdsCacheConfig { + db_path: "test_shutdown_is_idempotent".to_string(), + default_cache_ttl_seconds: None, + max_size_mib: None, + }); + let weak_reference = builder + .fetch_telemetry() + .expect("Inner telemetry should be Some in builder"); + let client = builder.build(); + + client.shutdown().unwrap(); + assert_eq!(weak_reference.strong_count(), 0); + + // Repeated shutdowns must not error or re-close an already closed connection. + client.shutdown().unwrap(); + client.shutdown().unwrap(); + assert_eq!(weak_reference.strong_count(), 0); + } +} From 2f557a642cf56d195ebf8b1685e60e7480359a4a Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 4 Sep 2026 09:42:10 -0700 Subject: [PATCH 3/5] fix: clippy --- components/ads-client/src/ffi.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 4cf6c03c09..06582f97a2 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -177,7 +177,7 @@ impl MozAdsClientBuilder { impl MozAdsClientBuilder { #[cfg(test)] pub fn fetch_telemetry(&self) -> Option> { - self.0.lock().telemetry.as_ref().map(|t| Arc::downgrade(&t)) + self.0.lock().telemetry.as_ref().map(|t| Arc::downgrade(t)) } } From 0fa7bb7c9405a70cb4350fcef3c53591e2cfdaed Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 4 Sep 2026 09:45:03 -0700 Subject: [PATCH 4/5] fix: adds changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c901cd1344..5170a68ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ ### Ads-Client - Added `blocks: Vec` to `ffi::MozAdsRequestOptions`, `AdsClient::request*_ads`, `MARSClient::fetch_ads`, `mars::AdRequest`, and `mars::AdRequest::try_new`. This is serialized and passed to MARS so that it can remove blocks server-side. +- `shutdown` no longer requires a full `AdsClient` lock (at the cost of no longer shutting down the sqlite db), and telemetry is no longer cloned in the `MozAdsClientBuilder` functions. # v156.0 (_2026-08-27_) From c9c557cbab22de150a84a493b62433cc3f76113b Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 4 Sep 2026 09:58:47 -0700 Subject: [PATCH 5/5] fix: clippy --- components/ads-client/src/ffi.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 06582f97a2..d82898bf9c 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -177,7 +177,7 @@ impl MozAdsClientBuilder { impl MozAdsClientBuilder { #[cfg(test)] pub fn fetch_telemetry(&self) -> Option> { - self.0.lock().telemetry.as_ref().map(|t| Arc::downgrade(t)) + self.0.lock().telemetry.as_ref().map(Arc::downgrade) } }