Bug 2018675 - Do not use a callback interface for MetricsHandler

components/nimbus/src/cirrus.udl

@@ -11,7 +11,8 @@ enum NimbusError {
     "ParseIntError", "TransformParameterError", "CirrusError", "UniFFICallbackError"
 };
 
-callback interface MetricsHandler {
+[Trait, WithForeign]
+interface MetricsHandler {
     void record_enrollment_statuses_v2(sequence<EnrollmentStatusExtraDef> enrollment_status_extras, string? nimbus_user_id);
 };
 
@@ -36,4 +37,4 @@ interface CirrusClient {
     /// Sets the experiments list in the CirrusClient's internal state.
     [Throws=NimbusError]
     void set_experiments(string experiments);
-};
+};

components/nimbus/src/metrics.rs

@@ -10,6 +10,7 @@ use crate::{EnrolledFeature, EnrollmentStatus};
 #[cfg(feature = "stateful")]
 use crate::enrollment::PreviousGeckoPrefState;
 
+#[uniffi::trait_interface]
 pub trait MetricsHandler: Send + Sync {
     #[cfg(feature = "stateful")]
     fn record_enrollment_statuses(&self, enrollment_status_extras: Vec<EnrollmentStatusExtraDef>);

components/nimbus/src/nimbus.udl

@@ -83,7 +83,8 @@ enum EnrollmentChangeEventType {
     "UnenrollFailed",
 };
 
-callback interface MetricsHandler {
+[Trait, WithForeign]
+interface MetricsHandler {
     void record_enrollment_statuses(sequence<EnrollmentStatusExtraDef> enrollment_status_extras);
     /// Feature activation is the pre-cursor to feature exposure: it is defined as the first time
     /// the feature configuration is asked for.

components/nimbus/src/stateful/nimbus_client.rs

@@ -47,7 +47,7 @@ use crate::stateful::targeting::{RecordedContext, validate_event_queries};
 use crate::stateful::updating::{read_and_remove_pending_experiments, write_pending_experiments};
 use crate::strings::fmt_with_map;
 #[cfg(test)]
-use crate::tests::helpers::{TestGeckoPrefHandler, TestMetrics, TestRecordedContext};
+use crate::tests::helpers::{TestGeckoPrefHandler, TestRecordedContext};
 use crate::{
     AvailableExperiment, AvailableRandomizationUnits, EnrolledExperiment, EnrollmentStatus,
 };
@@ -95,7 +95,7 @@ pub struct NimbusClient {
     event_store: Arc<Mutex<EventStore>>,
     recorded_context: Option<Arc<dyn RecordedContext>>,
     pub(crate) gecko_prefs: Option<Arc<GeckoPrefStore>>,
-    metrics_handler: Arc<Box<dyn MetricsHandler>>,
+    metrics_handler: Arc<dyn MetricsHandler>,
 }
 
 impl NimbusClient {
@@ -107,7 +107,7 @@ impl NimbusClient {
         recorded_context: Option<Arc<dyn RecordedContext>>,
         coenrolling_feature_ids: Vec<String>,
         db_path: P,
-        metrics_handler: Box<dyn MetricsHandler>,
+        metrics_handler: Arc<dyn MetricsHandler>,
         gecko_pref_handler: Option<Box<dyn GeckoPrefHandler>>,
         remote_settings_info: Option<NimbusServerSettings>,
     ) -> Result<Self> {
@@ -146,7 +146,7 @@ impl NimbusClient {
             event_store: Arc::default(),
             recorded_context,
             gecko_prefs: prefs,
-            metrics_handler: Arc::new(metrics_handler),
+            metrics_handler,
         })
     }
 
@@ -887,15 +887,6 @@ impl NimbusClient {
             }))
     }
 
-    #[cfg(test)]
-    pub fn get_metrics_handler(&self) -> &&TestMetrics {
-        let metrics = &**self.metrics_handler;
-        // SAFETY: The cast to TestMetrics is safe because the Rust instance is guaranteed to be
-        // a TestMetrics instance. TestMetrics is the only Rust-implemented version of
-        // MetricsHandler, and, like this method, is only used in tests.
-        unsafe { std::mem::transmute::<&&dyn MetricsHandler, &&TestMetrics>(&metrics) }
-    }
-
     #[cfg(test)]
     pub fn get_recorded_context(&self) -> &&TestRecordedContext {
         self.recorded_context

components/nimbus/src/stateless/cirrus_client.rs

@@ -65,13 +65,13 @@ pub struct CirrusClient {
     app_context: AppContext,
     coenrolling_feature_ids: Vec<String>,
     state: Mutex<CirrusMutableState>,
-    metrics_handler: Arc<Box<dyn MetricsHandler>>,
+    metrics_handler: Arc<dyn MetricsHandler>,
 }
 
 impl CirrusClient {
     pub fn new(
         app_context: String,
-        metrics_handler: Box<dyn MetricsHandler>,
+        metrics_handler: Arc<dyn MetricsHandler>,
         coenrolling_feature_ids: Vec<String>,
     ) -> Result<Self> {
         let app_context: AppContext = match serde_json::from_str(&app_context) {
@@ -82,7 +82,7 @@ impl CirrusClient {
             app_context,
             coenrolling_feature_ids,
             state: Default::default(),
-            metrics_handler: Arc::new(metrics_handler),
+            metrics_handler,
         })
     }
 

components/nimbus/src/tests/helpers.rs

@@ -154,7 +154,9 @@ impl RecordedContext for TestRecordedContext {
     }
 }
 
-#[derive(Default)]
+/// A Rust implementation of the MetricsHandler trait
+/// Used to test recording of Glean metrics across the FFI within Rust
+#[derive(Clone, Default)]
 struct MetricState {
     enrollment_statuses: Vec<EnrollmentStatusExtraDef>,
     #[cfg(feature = "stateful")]
@@ -173,16 +175,15 @@ struct MetricState {
 /// Used to test recording of Glean metrics across the FFI within Rust
 ///
 /// *NOTE: Use this struct's `new` method when instantiating it to lock the Glean store*
-#[derive(Clone, Default)]
 pub struct TestMetrics {
-    state: Arc<Mutex<MetricState>>,
+    state: Mutex<MetricState>,
 }
 
 impl TestMetrics {
-    pub fn new() -> Self {
-        TestMetrics {
+    pub fn new() -> Arc<Self> {
+        Arc::new(TestMetrics {
             state: Default::default(),
-        }
+        })
     }
 
     pub fn get_enrollment_statuses(&self) -> Vec<EnrollmentStatusExtraDef> {
@@ -198,10 +199,7 @@ impl TestMetrics {
 #[cfg(feature = "stateful")]
 impl TestMetrics {
     pub fn clear(&self) {
-        let mut state = self.state.lock().unwrap();
-        state.activations.clear();
-        state.enrollment_statuses.clear();
-        state.malformeds.clear();
+        std::mem::take(&mut *self.state.lock().unwrap());
     }
 
     pub fn get_activations(&self) -> Vec<FeatureExposureExtraDef> {

components/nimbus/src/tests/stateful/client/test_null_client.rs

@@ -12,7 +12,6 @@ use crate::tests::helpers::TestMetrics;
 #[cfg(feature = "rkv-safe-mode")]
 #[test]
 fn test_null_client() -> Result<()> {
-    let metrics = TestMetrics::new();
     error_support::init_for_tests();
 
     let tmp_dir = tempfile::tempdir()?;
@@ -22,7 +21,7 @@ fn test_null_client() -> Result<()> {
         Default::default(),
         Default::default(),
         tmp_dir.path(),
-        Box::new(metrics),
+        TestMetrics::new(),
         None,
         None,
     )?;