Bug 2019049 - Use a [Trait, WithForeign] interface for GeckoPrefHandler (#7433)

Additionally I rooted out the unneeded transmutes: since we've got an
`Arc<>`, we can already clone it and `Arc<T>` will cast to
`Arc<dyn Interface>` for `T: Interface`.

Author: freshstrangemusic

components/nimbus/src/enrollment.rs

@@ -1361,7 +1361,7 @@ where
 
 pub(crate) fn sort_experiments_by_published_date(experiments: &[Experiment]) -> Vec<&Experiment> {
     let mut experiments: Vec<_> = experiments.iter().collect();
-    experiments.sort_by(|a, b| a.published_date.cmp(&b.published_date));
+    experiments.sort_by_key(|e| e.published_date);
     experiments
 }
 

components/nimbus/src/nimbus.udl

@@ -147,13 +147,14 @@ dictionary MalformedFeatureConfigExtraDef {
     string part;
 };
 
-callback interface GeckoPrefHandler {
+[Trait, WithForeign]
+interface GeckoPrefHandler {
     record<string, record<string, GeckoPrefState>> get_prefs_with_state();
 
     void set_gecko_prefs_state(sequence<GeckoPrefState> new_prefs_state);
 
     void set_gecko_prefs_original_values(sequence<OriginalGeckoPref> original_gecko_prefs);
-
+    
 };
 
 dictionary GeckoPref {

components/nimbus/src/stateful/gecko_prefs.rs

@@ -125,6 +125,7 @@ pub fn create_feature_prop_pref_map(
     )
 }
 
+#[uniffi::trait_interface]
 pub trait GeckoPrefHandler: Send + Sync {
     /// Used to obtain the prefs values from Gecko
     fn get_prefs_with_state(&self) -> MapOfFeatureIdToPropertyNameToGeckoPrefState;
@@ -161,12 +162,12 @@ impl GeckoPrefStoreState {
 
 pub struct GeckoPrefStore {
     // This is Arc<Box<_>> because of FFI
-    pub handler: Arc<Box<dyn GeckoPrefHandler>>,
+    pub handler: Arc<dyn GeckoPrefHandler>,
     pub state: Mutex<GeckoPrefStoreState>,
 }
 
 impl GeckoPrefStore {
-    pub fn new(handler: Arc<Box<dyn GeckoPrefHandler>>) -> Self {
+    pub fn new(handler: Arc<dyn GeckoPrefHandler>) -> Self {
         Self {
             handler,
             state: Mutex::new(GeckoPrefStoreState::default()),

components/nimbus/src/stateful/nimbus_client.rs

@@ -52,7 +52,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, TestRecordedContext};
+use crate::tests::helpers::TestRecordedContext;
 use crate::{
     AvailableExperiment, AvailableRandomizationUnits, EnrolledExperiment, EnrollmentStatus,
 };
@@ -113,7 +113,7 @@ impl NimbusClient {
         coenrolling_feature_ids: Vec<String>,
         db_path: P,
         metrics_handler: Arc<dyn MetricsHandler>,
-        gecko_pref_handler: Option<Box<dyn GeckoPrefHandler>>,
+        gecko_pref_handler: Option<Arc<dyn GeckoPrefHandler>>,
         remote_settings_info: Option<NimbusServerSettings>,
     ) -> Result<Self> {
         let settings_client = Mutex::new(create_client(remote_settings_info)?);
@@ -128,7 +128,7 @@ impl NimbusClient {
 
         let mut prefs = None;
         if let Some(handler) = gecko_pref_handler {
-            prefs = Some(Arc::new(GeckoPrefStore::new(Arc::new(handler))));
+            prefs = Some(Arc::new(GeckoPrefStore::new(handler)));
         }
 
         info!(
@@ -930,23 +930,6 @@ impl NimbusClient {
             .expect("failed to unwrap RecordedContext object")
     }
 
-    #[cfg(test)]
-    pub fn get_gecko_pref_store(&self) -> Arc<Box<TestGeckoPrefHandler>> {
-        self.gecko_prefs.clone()
-            .clone()
-            .map(|ref pref_store|
-                // SAFETY: The cast to TestGeckoPrefHandler is safe because the Rust instance is
-                // guaranteed to be a TestGeckoPrefHandler instance. TestGeckoPrefHandler is the only
-                // Rust-implemented version of GeckoPrefHandler, and, like this method,  is only
-                // used in tests.
-                unsafe {
-                    std::mem::transmute::<Arc<Box<dyn GeckoPrefHandler>>, Arc<Box<TestGeckoPrefHandler>>>(
-                        pref_store.clone().handler.clone(),
-                    )
-                })
-            .expect("failed to unwrap GeckoPrefHandler object")
-    }
-
     pub fn set_install_time(&mut self, then: DateTime<Utc>) {
         let mut state = self.mutable_state.lock().unwrap();
         state.install_date = Some(then);

components/nimbus/src/tests/helpers.rs

@@ -189,15 +189,15 @@ pub struct TestGeckoPrefHandler {
 
 #[cfg(feature = "stateful")]
 impl TestGeckoPrefHandler {
-    pub(crate) fn new(prefs: MapOfFeatureIdToPropertyNameToGeckoPrefState) -> Self {
-        Self {
+    pub(crate) fn new(prefs: MapOfFeatureIdToPropertyNameToGeckoPrefState) -> Arc<Self> {
+        Arc::new(Self {
             prefs,
             state: Mutex::new(TestGeckoPrefHandlerState {
                 prefs_set: None,
                 original_prefs_state: None,
                 set_gecko_prefs_state_call_count: 0,
             }),
-        }
+        })
     }
 }
 

components/nimbus/src/tests/stateful/test_gecko_prefs.rs

@@ -25,7 +25,6 @@ fn test_gecko_pref_store_map_gecko_prefs_to_enrollment_slugs_and_update_store()
         "test_prop",
         pref_state.clone(),
     )]));
-    let handler: Arc<Box<dyn GeckoPrefHandler>> = Arc::new(Box::new(handler));
     let store = GeckoPrefStore::new(handler.clone());
     store.initialize()?;
 
@@ -60,11 +59,6 @@ fn test_gecko_pref_store_map_gecko_prefs_to_enrollment_slugs_and_update_store()
         true,
     );
 
-    let handler = unsafe {
-        std::mem::transmute::<Arc<Box<dyn GeckoPrefHandler>>, Arc<Box<TestGeckoPrefHandler>>>(
-            handler,
-        )
-    };
     let handler_state = handler
         .state
         .lock()
@@ -91,7 +85,6 @@ fn test_gecko_pref_store_map_gecko_prefs_to_enrollment_slugs_and_update_store_ex
         "test_prop",
         pref_state.clone(),
     )]));
-    let handler: Arc<Box<dyn GeckoPrefHandler>> = Arc::new(Box::new(handler));
     let store = GeckoPrefStore::new(handler.clone());
     store.initialize()?;
 
@@ -154,11 +147,6 @@ fn test_gecko_pref_store_map_gecko_prefs_to_enrollment_slugs_and_update_store_ex
         true,
     );
 
-    let handler = unsafe {
-        std::mem::transmute::<Arc<Box<dyn GeckoPrefHandler>>, Arc<Box<TestGeckoPrefHandler>>>(
-            handler,
-        )
-    };
     let handler_state = handler
         .state
         .lock()
@@ -215,7 +203,6 @@ fn test_gecko_pref_store_pref_is_user_set() -> Result<()> {
         ("test_feature", "test_prop_1", pref_state_1.clone()),
         ("test_feature", "test_prop_2", pref_state_2.clone()),
     ]));
-    let handler: Arc<Box<dyn GeckoPrefHandler>> = Arc::new(Box::new(handler));
     let store = GeckoPrefStore::new(handler.clone());
     store.initialize()?;
 
@@ -380,17 +367,11 @@ fn test_set_gecko_prefs_original_values() {
         "test_prop",
         pref_state_1.clone(),
     )]));
-    let handler: Arc<Box<dyn GeckoPrefHandler>> = Arc::new(Box::new(handler));
     let store = Arc::new(GeckoPrefStore::new(handler.clone()));
     let _ = store.initialize();
 
     handler.set_gecko_prefs_original_values(original_gecko_prefs.clone());
-    let test_handler = unsafe {
-        std::mem::transmute::<Arc<Box<dyn GeckoPrefHandler>>, Arc<Box<TestGeckoPrefHandler>>>(
-            handler,
-        )
-    };
-    let test_handler_state = test_handler
+    let test_handler_state = handler
         .state
         .lock()
         .expect("Unable to lock transmuted handler state");

components/nimbus/src/tests/stateful/test_nimbus.rs

@@ -1796,7 +1796,7 @@ fn test_gecko_pref_enrollment() -> Result<()> {
         Default::default(),
         temp_dir.path(),
         TestMetrics::new(),
-        Some(Box::new(handler)),
+        Some(handler.clone()),
         None,
     )?;
     client.set_nimbus_id(&Uuid::from_str("00000000-0000-0000-0000-000000000004")?)?;
@@ -1820,11 +1820,7 @@ fn test_gecko_pref_enrollment() -> Result<()> {
     let active_experiments = client.get_active_experiments()?;
     assert_eq!(active_experiments.len(), 1);
 
-    let handler = client.get_gecko_pref_store();
-    let handler_state = handler
-        .state
-        .lock()
-        .expect("Unable to lock transmuted handler state");
+    let handler_state = handler.state.lock().expect("Unable to lock handler state");
     let prefs = handler_state.prefs_set.clone().unwrap();
 
     assert_eq!(1, prefs.len());
@@ -1872,7 +1868,7 @@ fn test_gecko_pref_unenrollment() -> Result<()> {
         Default::default(),
         temp_dir.path(),
         TestMetrics::new(),
-        Some(Box::new(handler)),
+        Some(handler.clone()),
         None,
     )?;
     client.set_nimbus_id(&Uuid::from_str("00000000-0000-0000-0000-000000000004")?)?;
@@ -1910,11 +1906,7 @@ fn test_gecko_pref_unenrollment() -> Result<()> {
     assert_eq!(active_experiments.len(), 2);
 
     {
-        let handler = client.get_gecko_pref_store();
-        let handler_state = handler
-            .state
-            .lock()
-            .expect("Unable to lock transmuted handler state");
+        let handler_state = handler.state.lock().expect("Unable to lock handler state");
         let prefs = handler_state.prefs_set.clone().unwrap();
 
         assert_eq!(1, prefs.len());
@@ -1959,11 +1951,7 @@ fn test_gecko_pref_unenrollment() -> Result<()> {
     assert_eq!(active_experiments.len(), 0);
 
     {
-        let handler = client.get_gecko_pref_store();
-        let handler_state = handler
-            .state
-            .lock()
-            .expect("Unable to lock transmuted handler state");
+        let handler_state = handler.state.lock().expect("Unable to lock handler state");
         let prefs = handler_state.prefs_set.clone().unwrap();
 
         assert_eq!(0, prefs.len());
@@ -2006,7 +1994,7 @@ fn test_gecko_pref_unenrollment_reverts() -> Result<()> {
         Default::default(),
         temp_dir.path(),
         TestMetrics::new(),
-        Some(Box::new(handler)),
+        Some(handler.clone()),
         None,
     )?;
     client.set_nimbus_id(&Uuid::from_str("00000000-0000-0000-0000-000000000004")?)?;
@@ -2060,11 +2048,7 @@ fn test_gecko_pref_unenrollment_reverts() -> Result<()> {
     assert_eq!(active_experiments.len(), 2);
 
     {
-        let handler = client.get_gecko_pref_store();
-        let handler_state = handler
-            .state
-            .lock()
-            .expect("Unable to lock transmuted handler state");
+        let handler_state = handler.state.lock().expect("Unable to lock handler state");
         let prefs = handler_state.prefs_set.clone().unwrap();
 
         assert_eq!(2, prefs.len());
@@ -2110,11 +2094,7 @@ fn test_gecko_pref_unenrollment_reverts() -> Result<()> {
     assert_eq!(active_experiments.len(), 0);
 
     {
-        let handler = client.get_gecko_pref_store();
-        let handler_state = handler
-            .state
-            .lock()
-            .expect("Unable to lock transmuted handler state");
+        let handler_state = handler.state.lock().expect("Unable to lock handler state");
 
         let original_prefs_stored = handler_state.original_prefs_state.clone().unwrap();
 
@@ -2151,7 +2131,7 @@ fn register_previous_gecko_pref_states() -> Result<()> {
         Default::default(),
         temp_dir.path(),
         metrics.clone(),
-        Some(Box::new(handler)),
+        Some(handler.clone()),
         None,
     )?;
     client.set_nimbus_id(&Uuid::from_str("00000000-0000-0000-0000-000000000004")?)?;
@@ -2245,8 +2225,7 @@ fn register_previous_gecko_pref_states() -> Result<()> {
         gecko_pref_state_3.clone(),
     ];
 
-    let call_count_before = client
-        .get_gecko_pref_store()
+    let call_count_before = handler
         .state
         .lock()
         .unwrap()
@@ -2258,8 +2237,7 @@ fn register_previous_gecko_pref_states() -> Result<()> {
     // Registration must not send pref values to Gecko.
     assert_eq!(
         call_count_before,
-        client
-            .get_gecko_pref_store()
+        handler
             .state
             .lock()
             .unwrap()
@@ -2350,7 +2328,7 @@ fn test_add_prev_gecko_pref_states_for_experiment() -> Result<()> {
         Default::default(),
         temp_dir.path(),
         metrics.clone(),
-        Some(Box::new(handler)),
+        Some(handler),
         None,
     )?;
     client.set_nimbus_id(&Uuid::from_str("00000000-0000-0000-0000-000000000004")?)?;