Wrap the autofill implementation in an Arc<Mutex<>> to help the sync …

…manager.

This patch works-around the lack of the functionlity described
in mozilla/uniffi-rs#417.

In short, it splits the store into 2 parts - the
uniffi-exposed "wrapper" and the actual implementation.
This "wrapper" wraps the implementation in an `Arc<Mutex>>`, so
that it can hand a clone of this `Arc<>` to the sync manager, so
that it can construct sync engines, causing us to end up with
multiple references to the `Store` being alive but without any
being actual `&` references.

(Thinking some more about it, I probably could have done this
slightly differently, and just wrapped the DB itself, but I
ended up somewhat automatically taking this route because this
is what the shape would naturally be if uniffi helped us.
I think passing the entire store around does make more sense
in the longer term anyway (eg, there might be other state beyond
the DB) and is what the other components do, but it does make
this patch bigger than it would otherwise be)

It also makes some changes in the sync manager, really just
enough to convince me that it will work - there will be plenty
of other places that will need similar treatment. I chose to
use the `SyncEngine` trait exclusively there, which I think makes
sense because ideally we'd use that trait *everywhere* to avoid the
Sync Manager knowing about the implementation (but it would still
require it to link to all those crates, so that's just a
code-simplicity issue)

components/autofill/Cargo.toml

@@ -11,6 +11,7 @@ license = "MPL-2.0"
 anyhow = "1.0"
 error-support = { path = "../support/error" }
 interrupt-support = { path = "../support/interrupt" }
+lazy_static = "1.4"
 log = "0.4"
 serde = "1"
 serde_derive = "1"
@@ -30,7 +31,6 @@ features = ["functions", "bundled", "serde_json", "unlock_notify"]
 
 [dev-dependencies]
 env_logger = { version = "0.7", default-features = false }
-lazy_static = "1.4"
 libsqlite3-sys = "0.20.1"
 
 [build-dependencies]

components/autofill/src/autofill.udl

@@ -67,6 +67,11 @@ enum Error {
    "SqlError", "IoError", "InterruptedError", "IllegalDatabasePath", "Utf8Error", "JsonError", "InvalidSyncPayload",
 };
 
+// We don't *need* this to be `Threadsafe` but because we manage Mutexes
+// internally for syncing, it logically *should* be Threadsafe. Later, if we
+// can work out how to have uniffi give us its internal `Arc<>`, thereby
+// allowing us to drop the workaround in store.rs, it would become necessary.
+[Threadsafe]
 interface Store {
     [Throws=Error]
     constructor(string dbpath);

components/autofill/src/db/store.rs

@@ -11,50 +11,163 @@ use rusqlite::{
     Connection,
 };
 use sql_support::{self, ConnExt};
+use std::cell::RefCell;
 use std::path::Path;
-use std::sync::{Arc, Mutex};
+use std::sync::{Arc, Mutex, Weak};
 use sync15_traits::SyncEngine;
 use sync_guid::Guid;
 
-#[allow(dead_code)]
+// Our "sync manager" will use whatever is stashed here.
+lazy_static::lazy_static! {
+    // Far out:
+    // Outer mutex: just taken long enough to update the inner stuff - needed
+    //              to wrap the RefCell as they aren't `Sync`
+    // RefCell: So I can replace what it holds. Normally you'd use `get_ref()`
+    //          on the mutex, but that requires the mutex to be declared as
+    //          `mut` which is apparently impossible.
+    // [Arc/Weak]<Mutex<StoreImpl>>: What the sync manager actually needs.
+    pub static ref STORE_FOR_MANAGER: Mutex<RefCell<Weak<Mutex<StoreImpl>>>> = Mutex::new(RefCell::new(Weak::new()));
+}
+
+// This is the type that uniffi exposes. It holds an `Arc<Mutex<>>` around the
+// actual implementation, because we need access to the `Arc<Mutex<>>` for
+// integration with the sync manager. Our UDL also defines us as being
+// "ThreadSafe" to we manage our own locking via the `Mutex` - but this means
+// in practice we are actually held via an `Arc<Arc<Mutex<>>>` - but one day
+// https://github.com/mozilla/uniffi-rs/issues/417 will give us access to the
+// `Arc<>` uniffi owns, which means we can drop this mess entirely (ie, `Store`)
+// can then just have a mutex around the DB.
 pub struct Store {
-    db: Arc<Mutex<AutofillDb>>,
+    store_impl: Arc<Mutex<StoreImpl>>,
 }
 
-#[allow(dead_code)]
 impl Store {
     pub fn new(db_path: impl AsRef<Path>) -> Result<Self> {
         Ok(Self {
-            db: Arc::new(Mutex::new(AutofillDb::new(db_path)?)),
+            store_impl: Arc::new(Mutex::new(StoreImpl::new(db_path)?)),
         })
     }
 
-    /// Creates a store backed by an in-memory database.
-    #[cfg(test)]
-    pub fn new_memory(db_path: &str) -> Result<Self> {
+    pub fn add_credit_card(&self, fields: UpdatableCreditCardFields) -> Result<CreditCard> {
+        self.store_impl.lock().unwrap().add_credit_card(fields)
+    }
+
+    pub fn get_credit_card(&self, guid: String) -> Result<CreditCard> {
+        self.store_impl.lock().unwrap().get_credit_card(guid)
+    }
+
+    pub fn get_all_credit_cards(&self) -> Result<Vec<CreditCard>> {
+        self.store_impl.lock().unwrap().get_all_credit_cards()
+    }
+
+    pub fn update_credit_card(
+        &self,
+        guid: String,
+        credit_card: UpdatableCreditCardFields,
+    ) -> Result<()> {
+        self.store_impl
+            .lock()
+            .unwrap()
+            .update_credit_card(guid, credit_card)
+    }
+
+    pub fn delete_credit_card(&self, guid: String) -> Result<bool> {
+        self.store_impl.lock().unwrap().delete_credit_card(guid)
+    }
+
+    pub fn touch_credit_card(&self, guid: String) -> Result<()> {
+        self.store_impl.lock().unwrap().touch_credit_card(guid)
+    }
+
+    pub fn add_address(&self, new_address: UpdatableAddressFields) -> Result<Address> {
+        self.store_impl.lock().unwrap().add_address(new_address)
+    }
+
+    pub fn get_address(&self, guid: String) -> Result<Address> {
+        self.store_impl.lock().unwrap().get_address(guid)
+    }
+
+    pub fn get_all_addresses(&self) -> Result<Vec<Address>> {
+        self.store_impl.lock().unwrap().get_all_addresses()
+    }
+
+    pub fn update_address(&self, guid: String, address: UpdatableAddressFields) -> Result<()> {
+        self.store_impl
+            .lock()
+            .unwrap()
+            .update_address(guid, address)
+    }
+
+    pub fn delete_address(&self, guid: String) -> Result<bool> {
+        self.store_impl.lock().unwrap().delete_address(guid)
+    }
+
+    pub fn touch_address(&self, guid: String) -> Result<()> {
+        self.store_impl.lock().unwrap().touch_address(guid)
+    }
+
+    // This allows the embedding app to say "make this instance available to
+    // the sync manager" - it's a strange name because it doesn't actually
+    // send it, it just sticks it in a global where the SyncManager may or
+    // may not grab it - there might not even be a sync manager! We do it this
+    // way to avoid forcing anything that wants this component to always link
+    // against the sync manager.
+    pub fn offer_to_sync_manager(&self) {
+        STORE_FOR_MANAGER
+            .lock()
+            .unwrap()
+            .replace(Arc::downgrade(&self.store_impl));
+    }
+
+    // These 2 are odd ones out - they don't just delegate but instead
+    // hand off the Arc.
+    // Currently the only consumer of this is our "example" - the sync manager
+    // duplicates it (because it doesn't have a reference to us, just to the
+    // store_impl)
+    // We could probably make the example work with the sync manager? If so,
+    // these can be killed.
+    pub fn create_credit_cards_sync_engine(&self) -> Box<dyn SyncEngine> {
+        Box::new(crate::sync::credit_card::create_engine(
+            self.store_impl.clone(),
+        ))
+    }
+
+    pub fn create_addresses_sync_engine(&self) -> Box<dyn SyncEngine> {
+        Box::new(crate::sync::address::create_engine(self.store_impl.clone()))
+    }
+}
+
+pub struct StoreImpl {
+    pub(crate) db: AutofillDb,
+}
+
+impl StoreImpl {
+    pub fn new(db_path: impl AsRef<Path>) -> Result<Self> {
         Ok(Self {
-            db: Arc::new(Mutex::new(AutofillDb::new_memory(db_path)?)),
+            db: AutofillDb::new(db_path)?,
         })
     }
 
+    /// Creates a store backed by an in-memory database.
     #[cfg(test)]
-    pub fn db(&self) -> Arc<Mutex<AutofillDb>> {
-        self.db.clone()
+    pub fn new_memory() -> Self {
+        Self {
+            db: crate::db::test::new_mem_db(),
+        }
     }
 
     pub fn add_credit_card(&self, fields: UpdatableCreditCardFields) -> Result<CreditCard> {
-        let credit_card = credit_cards::add_credit_card(&self.db.lock().unwrap().writer, fields)?;
+        let credit_card = credit_cards::add_credit_card(&self.db.writer, fields)?;
         Ok(credit_card.into())
     }
 
     pub fn get_credit_card(&self, guid: String) -> Result<CreditCard> {
-        let credit_card =
-            credit_cards::get_credit_card(&self.db.lock().unwrap().writer, &Guid::new(&guid))?;
+        let credit_card = credit_cards::get_credit_card(&self.db.writer, &Guid::new(&guid))?;
         Ok(credit_card.into())
     }
 
     pub fn get_all_credit_cards(&self) -> Result<Vec<CreditCard>> {
-        let credit_cards = credit_cards::get_all_credit_cards(&self.db.lock().unwrap().writer)?
+        let credit_cards = credit_cards::get_all_credit_cards(&self.db.writer)?
             .into_iter()
             .map(|x| x.into())
             .collect();
@@ -66,55 +179,43 @@ impl Store {
         guid: String,
         credit_card: UpdatableCreditCardFields,
     ) -> Result<()> {
-        credit_cards::update_credit_card(
-            &self.db.lock().unwrap().writer,
-            &Guid::new(&guid),
-            &credit_card,
-        )
+        credit_cards::update_credit_card(&self.db.writer, &Guid::new(&guid), &credit_card)
     }
 
     pub fn delete_credit_card(&self, guid: String) -> Result<bool> {
-        credit_cards::delete_credit_card(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        credit_cards::delete_credit_card(&self.db.writer, &Guid::new(&guid))
     }
 
     pub fn touch_credit_card(&self, guid: String) -> Result<()> {
-        credit_cards::touch(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        credit_cards::touch(&self.db.writer, &Guid::new(&guid))
     }
 
     pub fn add_address(&self, new_address: UpdatableAddressFields) -> Result<Address> {
-        Ok(addresses::add_address(&self.db.lock().unwrap().writer, new_address)?.into())
+        Ok(addresses::add_address(&self.db.writer, new_address)?.into())
     }
 
     pub fn get_address(&self, guid: String) -> Result<Address> {
-        Ok(addresses::get_address(&self.db.lock().unwrap().writer, &Guid::new(&guid))?.into())
+        Ok(addresses::get_address(&self.db.writer, &Guid::new(&guid))?.into())
     }
 
     pub fn get_all_addresses(&self) -> Result<Vec<Address>> {
-        let addresses = addresses::get_all_addresses(&self.db.lock().unwrap().writer)?
+        let addresses = addresses::get_all_addresses(&self.db.writer)?
             .into_iter()
             .map(|x| x.into())
             .collect();
         Ok(addresses)
     }
 
     pub fn update_address(&self, guid: String, address: UpdatableAddressFields) -> Result<()> {
-        addresses::update_address(&self.db.lock().unwrap().writer, &Guid::new(&guid), &address)
+        addresses::update_address(&self.db.writer, &Guid::new(&guid), &address)
     }
 
     pub fn delete_address(&self, guid: String) -> Result<bool> {
-        addresses::delete_address(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        addresses::delete_address(&self.db.writer, &Guid::new(&guid))
     }
 
     pub fn touch_address(&self, guid: String) -> Result<()> {
-        addresses::touch(&self.db.lock().unwrap().writer, &Guid::new(&guid))
-    }
-
-    pub fn create_credit_cards_sync_engine(&self) -> Box<dyn SyncEngine> {
-        Box::new(crate::sync::credit_card::create_engine(self.db.clone()))
-    }
-
-    pub fn create_addresses_sync_engine(&self) -> Box<dyn SyncEngine> {
-        Box::new(crate::sync::address::create_engine(self.db.clone()))
+        addresses::touch(&self.db.writer, &Guid::new(&guid))
     }
 }
 

components/autofill/src/lib.rs

@@ -10,6 +10,9 @@ pub mod db;
 pub mod error;
 pub mod sync;
 
+// Re-export stuff the sync manager needs.
+pub use crate::db::store::{StoreImpl, STORE_FOR_MANAGER};
+
 // Expose stuff needed by the uniffi generated code.
 use crate::db::models::address::*;
 use crate::db::models::credit_card::*;

components/autofill/src/sync/address/mod.rs

@@ -22,9 +22,9 @@ use sync_guid::Guid;
 use types::Timestamp;
 
 // The engine.
-pub fn create_engine(db: Arc<Mutex<crate::db::AutofillDb>>) -> ConfigSyncEngine<InternalAddress> {
+pub fn create_engine(store: Arc<Mutex<crate::StoreImpl>>) -> ConfigSyncEngine<InternalAddress> {
     ConfigSyncEngine {
-        db,
+        store,
         config: EngineConfig {
             namespace: "addresses".to_string(),
             collection: "addresses",

components/autofill/src/sync/credit_card/mod.rs

@@ -22,11 +22,9 @@ use sync_guid::Guid;
 use types::Timestamp;
 
 // The engine.
-pub fn create_engine(
-    db: Arc<Mutex<crate::db::AutofillDb>>,
-) -> ConfigSyncEngine<InternalCreditCard> {
+pub fn create_engine(store: Arc<Mutex<crate::StoreImpl>>) -> ConfigSyncEngine<InternalCreditCard> {
     ConfigSyncEngine {
-        db,
+        store,
         config: EngineConfig {
             namespace: "credit_cards".to_string(),
             collection: "creditcards",