From c38ce09f506f146daf28304546a24628080ec2a0 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Wed, 17 Mar 2021 16:38:34 +1100 Subject: [PATCH] Wrap the autofill implementation in an Arc<> to help the sync manager. This patch works-around the lack of the functionlity described in https://github.com/mozilla/uniffi-rs/issues/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<>`, 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. The database itself continues to be wrapped in a `Mutex<>` 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) --- Cargo.lock | 1 + components/autofill/Cargo.toml | 2 +- components/autofill/src/autofill.udl | 7 + components/autofill/src/db/store.rs | 138 +++++++++++++++--- components/autofill/src/lib.rs | 3 + components/autofill/src/sync/address/mod.rs | 6 +- .../autofill/src/sync/credit_card/mod.rs | 8 +- components/autofill/src/sync/engine.rs | 67 +++++---- components/sync_manager/Cargo.toml | 1 + components/sync_manager/src/error.rs | 5 + components/sync_manager/src/manager.rs | 31 +++- 11 files changed, 208 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 56a8d9e6f0..27d4643707 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3191,6 +3191,7 @@ name = "sync_manager" version = "0.1.0" dependencies = [ "anyhow", + "autofill", "error-support", "ffi-support 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", "interrupt-support", diff --git a/components/autofill/Cargo.toml b/components/autofill/Cargo.toml index c23a2baef2..0c00009e21 100644 --- a/components/autofill/Cargo.toml +++ b/components/autofill/Cargo.toml @@ -12,6 +12,7 @@ anyhow = "1.0" error-support = { path = "../support/error" } interrupt-support = { path = "../support/interrupt" } jwcrypto = { path = "../support/jwcrypto" } +lazy_static = "1.4" log = "0.4" serde = "1" serde_derive = "1" @@ -31,7 +32,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] diff --git a/components/autofill/src/autofill.udl b/components/autofill/src/autofill.udl index 368a4b4203..b8e85f47d9 100644 --- a/components/autofill/src/autofill.udl +++ b/components/autofill/src/autofill.udl @@ -86,6 +86,11 @@ enum Error { "CryptoError", }; +// We don't really *need* this to be `Threadsafe` because we have a mutex to +// manage concurrent DB connections - but we might as well so our Mutexes +// do what we thing they are doing rather than being controlled by the hidden +// mutexes non-threadsafe uniffi gives us. +[Threadsafe] interface Store { [Throws=Error] constructor(string dbpath); @@ -125,4 +130,6 @@ interface Store { [Throws=Error] void touch_address(string guid); + + void register_with_sync_manager(); }; diff --git a/components/autofill/src/db/store.rs b/components/autofill/src/db/store.rs index 9c6feb2ec0..af52ff20e9 100644 --- a/components/autofill/src/db/store.rs +++ b/components/autofill/src/db/store.rs @@ -11,35 +11,143 @@ 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! { + // Mutex: just taken long enough to update the inner stuff - needed + // to wrap the RefCell as they aren't `Sync` + // RefCell: So we can replace what it holds. Normally you'd use `get_ref()` + // on the mutex and avoid the RefCell entirely, but that requires + // the mutex to be declared as `mut` which is apparently + // impossible in a `lazy_static` + // [Arc/Weak]: What the sync manager actually needs. + pub static ref STORE_FOR_MANAGER: Mutex>> = Mutex::new(RefCell::new(Weak::new())); +} + +// This is the type that uniffi exposes. It holds an `Arc<>` around the +// actual implementation, because we need to hand a clone of this `Arc<>` to +// the sync manager and to sync engines. 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 entirely (ie, `Store` and +// `StoreImpl` could be re-unified) +// Sadly, this is `pub` because our `autofill-utils` example uses it. pub struct Store { - db: Arc>, + store_impl: Arc, } -#[allow(dead_code)] impl Store { pub fn new(db_path: impl AsRef) -> Result { Ok(Self { - db: Arc::new(Mutex::new(AutofillDb::new(db_path)?)), + store_impl: Arc::new(StoreImpl::new(db_path)?), }) } - /// Creates a store backed by an in-memory database. - #[cfg(test)] - pub fn new_memory(db_path: &str) -> Result { + pub fn add_credit_card(&self, fields: UpdatableCreditCardFields) -> Result { + self.store_impl.add_credit_card(fields) + } + + pub fn get_credit_card(&self, guid: String) -> Result { + self.store_impl.get_credit_card(guid) + } + + pub fn get_all_credit_cards(&self) -> Result> { + self.store_impl.get_all_credit_cards() + } + + pub fn update_credit_card( + &self, + guid: String, + credit_card: UpdatableCreditCardFields, + ) -> Result<()> { + self.store_impl.update_credit_card(guid, credit_card) + } + + pub fn delete_credit_card(&self, guid: String) -> Result { + self.store_impl.delete_credit_card(guid) + } + + pub fn touch_credit_card(&self, guid: String) -> Result<()> { + self.store_impl.touch_credit_card(guid) + } + + pub fn add_address(&self, new_address: UpdatableAddressFields) -> Result
{ + self.store_impl.add_address(new_address) + } + + pub fn get_address(&self, guid: String) -> Result
{ + self.store_impl.get_address(guid) + } + + pub fn get_all_addresses(&self) -> Result> { + self.store_impl.get_all_addresses() + } + + pub fn update_address(&self, guid: String, address: UpdatableAddressFields) -> Result<()> { + self.store_impl.update_address(guid, address) + } + + pub fn delete_address(&self, guid: String) -> Result { + self.store_impl.delete_address(guid) + } + + pub fn touch_address(&self, guid: String) -> Result<()> { + self.store_impl.touch_address(guid) + } + + // This allows the embedding app to say "make this instance available to + // the sync manager". The implementation is more like "offer to sync mgr" + // (thereby avoiding us needing to link with the sync manager) but + // `register_with_sync_manager()` is logically what's happening so that's + // the name it gets. + pub fn register_with_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" (and hence why they + // are `pub` and not `pub(crate)`) - 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 - but then + // our example would link with places and logins etc. + pub fn create_credit_cards_sync_engine(&self) -> Box { + Box::new(crate::sync::credit_card::create_engine( + self.store_impl.clone(), + )) + } + + pub fn create_addresses_sync_engine(&self) -> Box { + Box::new(crate::sync::address::create_engine(self.store_impl.clone())) + } +} + +// This is the actual implementation. All code in this crate works with this. +// Sadly, it's forced to be `pub` because the SyncManager also uses it. +pub struct StoreImpl { + pub(crate) db: Mutex, +} + +impl StoreImpl { + pub fn new(db_path: impl AsRef) -> Result { Ok(Self { - db: Arc::new(Mutex::new(AutofillDb::new_memory(db_path)?)), + db: Mutex::new(AutofillDb::new(db_path)?), }) } + /// Creates a store backed by an in-memory database. #[cfg(test)] - pub fn db(&self) -> Arc> { - self.db.clone() + pub fn new_memory() -> Self { + Self { + db: Mutex::new(crate::db::test::new_mem_db()), + } } pub fn add_credit_card(&self, fields: UpdatableCreditCardFields) -> Result { @@ -108,14 +216,6 @@ impl Store { 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 { - Box::new(crate::sync::credit_card::create_engine(self.db.clone())) - } - - pub fn create_addresses_sync_engine(&self) -> Box { - Box::new(crate::sync::address::create_engine(self.db.clone())) - } } pub(crate) fn put_meta(conn: &Connection, key: &str, value: &dyn ToSql) -> Result<()> { diff --git a/components/autofill/src/lib.rs b/components/autofill/src/lib.rs index 738633c5fd..9e0cceda80 100644 --- a/components/autofill/src/lib.rs +++ b/components/autofill/src/lib.rs @@ -11,6 +11,9 @@ pub mod encryption; 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::*; diff --git a/components/autofill/src/sync/address/mod.rs b/components/autofill/src/sync/address/mod.rs index 47c2bd6fc2..3d0129ed76 100644 --- a/components/autofill/src/sync/address/mod.rs +++ b/components/autofill/src/sync/address/mod.rs @@ -17,18 +17,18 @@ use incoming::IncomingAddressesImpl; use outgoing::OutgoingAddressesImpl; use rusqlite::Transaction; use serde::{Deserialize, Serialize}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use sync_guid::Guid; use types::Timestamp; // The engine. -pub fn create_engine(db: Arc>) -> ConfigSyncEngine { +pub fn create_engine(store: Arc) -> ConfigSyncEngine { ConfigSyncEngine::new( EngineConfig { namespace: "addresses".to_string(), collection: "addresses", }, - db, + store, Box::new(AddressesEngineStorageImpl {}), ) } diff --git a/components/autofill/src/sync/credit_card/mod.rs b/components/autofill/src/sync/credit_card/mod.rs index 28c257cbf8..46f3ace653 100644 --- a/components/autofill/src/sync/credit_card/mod.rs +++ b/components/autofill/src/sync/credit_card/mod.rs @@ -19,20 +19,18 @@ use incoming::IncomingCreditCardsImpl; use outgoing::OutgoingCreditCardsImpl; use rusqlite::Transaction; use serde::{Deserialize, Serialize}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use sync_guid::Guid; use types::Timestamp; // The engine. -pub fn create_engine( - db: Arc>, -) -> ConfigSyncEngine { +pub fn create_engine(store: Arc) -> ConfigSyncEngine { ConfigSyncEngine::new( EngineConfig { namespace: "credit_cards".to_string(), collection: "creditcards", }, - db, + store, Box::new(CreditCardsEngineStorageImpl {}), ) } diff --git a/components/autofill/src/sync/engine.rs b/components/autofill/src/sync/engine.rs index 42d0d1666e..c6ec64f938 100644 --- a/components/autofill/src/sync/engine.rs +++ b/components/autofill/src/sync/engine.rs @@ -3,14 +3,14 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use super::{plan_incoming, ProcessIncomingRecordImpl, ProcessOutgoingRecordImpl, SyncRecord}; -use crate::db::AutofillDb; use crate::error::*; +use crate::StoreImpl as Store; use rusqlite::{ types::{FromSql, ToSql}, Connection, Transaction, }; use std::cell::RefCell; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use sync15::{ telemetry, CollSyncIds, CollectionRequest, EngineSyncAssociation, IncomingChangeset, OutgoingChangeset, ServerTimestamp, SyncEngine, @@ -45,7 +45,7 @@ pub trait SyncEngineStorageImpl { // A sync engine that gets functionality from an EngineConfig. pub struct ConfigSyncEngine { pub(crate) config: EngineConfig, - pub(crate) db: Arc>, + pub(crate) store: Arc, pub(crate) storage_impl: Box>, local_enc_key: RefCell>, } @@ -53,12 +53,12 @@ pub struct ConfigSyncEngine { impl ConfigSyncEngine { pub fn new( config: EngineConfig, - db: Arc>, + store: Arc, storage_impl: Box>, ) -> Self { Self { config, - db, + store, storage_impl, local_enc_key: RefCell::new(None), } @@ -96,7 +96,7 @@ impl SyncEngine for ConfigSyncEngine { assert_eq!(inbound.len(), 1, "we only request one item"); let inbound = inbound.into_iter().next().unwrap(); - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); crate::db::schema::create_empty_sync_temp_tables(&db.writer)?; let signal = db.begin_interrupt_scope(); @@ -151,7 +151,7 @@ impl SyncEngine for ConfigSyncEngine { new_timestamp: ServerTimestamp, records_synced: Vec, ) -> anyhow::Result<()> { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); self.put_meta( &db.writer, LAST_SYNC_META_KEY, @@ -170,7 +170,7 @@ impl SyncEngine for ConfigSyncEngine { &self, server_timestamp: ServerTimestamp, ) -> anyhow::Result> { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); let since = ServerTimestamp( self.get_meta::(&db.writer, LAST_SYNC_META_KEY)? .unwrap_or_default(), @@ -185,7 +185,7 @@ impl SyncEngine for ConfigSyncEngine { } fn get_sync_assoc(&self) -> anyhow::Result { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); let global = self.get_meta(&db.writer, GLOBAL_SYNCID_META_KEY)?; let coll = self.get_meta(&db.writer, COLLECTION_SYNCID_META_KEY)?; Ok(if let (Some(global), Some(coll)) = (global, coll) { @@ -196,7 +196,7 @@ impl SyncEngine for ConfigSyncEngine { } fn reset(&self, assoc: &EngineSyncAssociation) -> anyhow::Result<()> { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); let tx = db.unchecked_transaction()?; self.storage_impl.reset_storage(&tx)?; // Reset the last sync time, so that the next sync fetches fresh records @@ -233,13 +233,14 @@ mod tests { get_all, insert_tombstone_record, test_insert_mirror_record, }; use crate::db::models::credit_card::InternalCreditCard; - use crate::db::{schema::create_empty_sync_temp_tables, test::new_mem_db}; + use crate::db::schema::create_empty_sync_temp_tables; use crate::encryption::EncryptorDecryptor; use sql_support::ConnExt; // We use the credit-card engine here. - fn create_engine(db: AutofillDb) -> ConfigSyncEngine { - crate::sync::credit_card::create_engine(Arc::new(Mutex::new(db))) + fn create_engine() -> ConfigSyncEngine { + let store = crate::db::store::StoreImpl::new_memory(); + crate::sync::credit_card::create_engine(Arc::new(store)) } pub fn clear_cc_tables(conn: &Connection) -> rusqlite::Result<(), rusqlite::Error> { @@ -253,14 +254,14 @@ mod tests { #[test] fn test_credit_card_engine_sync_finished() -> Result<()> { - let db = new_mem_db(); - create_empty_sync_temp_tables(&db).expect("should create temp tables"); - - let credit_card_engine = create_engine(db); + let credit_card_engine = create_engine(); let test_key = crate::encryption::create_key().unwrap(); credit_card_engine .set_local_encryption_key(&test_key) .unwrap(); + { + create_empty_sync_temp_tables(&credit_card_engine.store.db.lock().unwrap())?; + } let last_sync = 24; let result = @@ -268,7 +269,7 @@ mod tests { assert!(result.is_ok()); // check that last sync metadata was set - let conn = &credit_card_engine.db.lock().unwrap().writer; + let conn = &credit_card_engine.store.db.lock().unwrap().writer; assert_eq!( credit_card_engine.get_meta::(conn, LAST_SYNC_META_KEY)?, @@ -280,8 +281,7 @@ mod tests { #[test] fn test_credit_card_engine_get_sync_assoc() -> Result<()> { - let db = new_mem_db(); - let credit_card_engine = create_engine(db); + let credit_card_engine = create_engine(); let result = credit_card_engine.get_sync_assoc(); assert!(result.is_ok()); @@ -297,7 +297,7 @@ mod tests { coll: coll_guid, }; { - let conn = &credit_card_engine.db.lock().unwrap().writer; + let conn = &credit_card_engine.store.db.lock().unwrap().writer; credit_card_engine.put_meta(conn, GLOBAL_SYNCID_META_KEY, &ids.global)?; credit_card_engine.put_meta(conn, COLLECTION_SYNCID_META_KEY, &ids.coll)?; } @@ -312,11 +312,9 @@ mod tests { #[test] fn test_engine_sync_reset() -> Result<()> { - let db = new_mem_db(); + let engine = create_engine(); let encdec = EncryptorDecryptor::new_test_key(); - let tx = db.writer.unchecked_transaction()?; - // create a normal record, a mirror record and a tombstone. let cc = InternalCreditCard { guid: Guid::random(), cc_name: "Ms Jane Doe".to_string(), @@ -327,12 +325,17 @@ mod tests { cc_type: "visa".to_string(), ..Default::default() }; - add_internal_credit_card(&tx, &cc)?; - test_insert_mirror_record(&tx, cc.clone().into_payload(&encdec).expect("is json")); - insert_tombstone_record(&tx, Guid::random().to_string())?; - tx.commit()?; - let engine = create_engine(db); + { + // temp scope for the mutex lock. + let db = &engine.store.db.lock().unwrap(); + let tx = db.writer.unchecked_transaction()?; + // create a normal record, a mirror record and a tombstone. + add_internal_credit_card(&tx, &cc)?; + test_insert_mirror_record(&tx, cc.clone().into_payload(&encdec).expect("is json")); + insert_tombstone_record(&tx, Guid::random().to_string())?; + tx.commit()?; + } // create sync metadata let global_guid = Guid::new("AAAA"); @@ -342,7 +345,7 @@ mod tests { coll: coll_guid.clone(), }; { - let conn = &engine.db.lock().unwrap().writer; + let conn = &engine.store.db.lock().unwrap().writer; engine.put_meta(conn, GLOBAL_SYNCID_META_KEY, &ids.global)?; engine.put_meta(conn, COLLECTION_SYNCID_META_KEY, &ids.coll)?; } @@ -353,7 +356,7 @@ mod tests { .expect("should work"); { - let conn = &engine.db.lock().unwrap().writer; + let conn = &engine.store.db.lock().unwrap().writer; // check that the mirror and tombstone tables have no records assert!(get_all(conn, "credit_cards_mirror".to_string())?.is_empty()); @@ -391,7 +394,7 @@ mod tests { .reset(&EngineSyncAssociation::Connected(ids)) .expect("should work"); - let conn = &engine.db.lock().unwrap().writer; + let conn = &engine.store.db.lock().unwrap().writer; // check that the meta records were set let retrieved_global_sync_id = engine.get_meta::(conn, GLOBAL_SYNCID_META_KEY)?; assert_eq!( diff --git a/components/sync_manager/Cargo.toml b/components/sync_manager/Cargo.toml index 13e56bfdf4..4965878e0e 100644 --- a/components/sync_manager/Cargo.toml +++ b/components/sync_manager/Cargo.toml @@ -7,6 +7,7 @@ license = "MPL-2.0" exclude = ["/android", "/ios"] [dependencies] +autofill = { path = "../autofill" } sync15 = { path = "../sync15" } places = { path = "../places" } logins = { path = "../logins" } diff --git a/components/sync_manager/src/error.rs b/components/sync_manager/src/error.rs index 2e6f9a9b8f..ad1dae790c 100644 --- a/components/sync_manager/src/error.rs +++ b/components/sync_manager/src/error.rs @@ -29,6 +29,10 @@ pub enum ErrorKind { LoginsError(#[from] logins::Error), #[error("Places error: {0}")] PlacesError(#[from] places::Error), + // We should probably upgrade this crate to anyhow, which would mean this + // gets replaced with AutofillError or similar. + #[error("External error: {0}")] + AnyhowError(#[from] anyhow::Error), } error_support::define_error! { @@ -41,5 +45,6 @@ error_support::define_error! { (JsonError, serde_json::Error), (LoginsError, logins::Error), (PlacesError, places::Error), + (AnyhowError, anyhow::Error), } } diff --git a/components/sync_manager/src/manager.rs b/components/sync_manager/src/manager.rs index 03326cccdc..e9daec0885 100644 --- a/components/sync_manager/src/manager.rs +++ b/components/sync_manager/src/manager.rs @@ -15,7 +15,7 @@ use std::time::SystemTime; use sync15::{ self, clients::{self, Command, CommandProcessor, CommandStatus, Settings}, - MemoryCachedState, + EngineSyncAssociation, MemoryCachedState, SyncEngine, }; use tabs::TabsStore; @@ -64,6 +64,23 @@ impl SyncManager { self.tabs = Arc::downgrade(&tabs); } + pub fn autofill_engine(engine: &str) -> Result>> { + let cell = autofill::STORE_FOR_MANAGER.lock().unwrap(); + // The cell holds a `Weak` - borrow it (which is safe as we have the + // mutex) and upgrade it to a real Arc. + let r = cell.borrow(); + match r.upgrade() { + None => Ok(None), + Some(arc) => match engine { + "addresses" => Ok(Some(Box::new(autofill::sync::address::create_engine(arc)))), + "creditcards" => Ok(Some(Box::new(autofill::sync::credit_card::create_engine( + arc, + )))), + _ => Err(ErrorKind::UnknownEngine(engine.into()).into()), + }, + } + } + pub fn wipe(&mut self, engine: &str) -> Result<()> { match engine { "logins" => { @@ -95,6 +112,12 @@ impl SyncManager { Err(ErrorKind::ConnectionClosed(engine.into()).into()) } } + "addresses" | "creditcards" => { + if let Some(engine) = Self::autofill_engine(engine)? { + engine.wipe()?; + } + Ok(()) + } _ => Err(ErrorKind::UnknownEngine(engine.into()).into()), } } @@ -142,6 +165,12 @@ impl SyncManager { Err(ErrorKind::ConnectionClosed(engine.into()).into()) } } + "addresses" | "creditcards" => { + if let Some(engine) = Self::autofill_engine(engine)? { + engine.reset(&EngineSyncAssociation::Disconnected)?; + } + Ok(()) + } _ => Err(ErrorKind::UnknownEngine(engine.into()).into()), } }