Bug 2056071 - remove BridgedEngine, app-services side.

components/autofill/src/sync/engine.rs

@@ -105,10 +105,7 @@ impl<T: SyncRecord + std::fmt::Debug> SyncEngine for ConfigSyncEngine<T> {
         Ok(())
     }
 
-    fn prepare_for_sync(
-        &self,
-        _get_client_data: &dyn Fn() -> sync15::ClientData,
-    ) -> anyhow::Result<()> {
+    fn sync_started(&self) -> anyhow::Result<()> {
         let db = &self.store.db.lock().unwrap();
         let signal = db.begin_interrupt_scope()?;
         crate::db::schema::create_empty_sync_temp_tables(&db.writer)?;

components/logins/src/error.rs

@@ -196,8 +196,9 @@ impl GetErrorHandling for Error {
 }
 
 // The bridged sync engine (`sync::bridge`) deals in `anyhow::Result`, as that's
-// what the `sync15` BridgedEngine traits use. This lets UniFFI map those errors
-// onto our public error type when the bridge methods are exposed via the UDL.
+// what the `sync15` `SyncEngine`/`BridgedEngineWrapper` use. This lets UniFFI map
+// those errors onto our public error type when the bridge methods are exposed
+// via the UDL.
 impl From<anyhow::Error> for LoginsApiError {
     fn from(value: anyhow::Error) -> Self {
         LoginsApiError::UnexpectedLoginsApiError {

components/logins/src/logins.udl

@@ -324,16 +324,17 @@ interface LoginStore {
     void shutdown();
 };
 
-/// The Desktop-facing bridged sync engine. The canonical docs are in
-/// https://searchfox.org/mozilla-central/source/services/interfaces/mozIBridgedSyncEngine.idl
+/// The Desktop-facing bridged sync engine - a thin wrapper over the
+/// `sync15::engine::SyncEngine` implemented by this component (see
+/// `sync15::engine::BridgedEngineWrapper`).
 /// It's only actually used on Desktop, but it's fine to expose this everywhere.
 /// NOTE: all timestamps here are milliseconds.
 interface LoginsBridgedEngine {
     [Throws=LoginsApiError]
     i64 last_sync();
 
     [Throws=LoginsApiError]
-    void set_last_sync(i64 last_sync);
+    void reset_last_sync();
 
     [Throws=LoginsApiError]
     string? sync_id();
@@ -351,7 +352,7 @@ interface LoginsBridgedEngine {
     void store_incoming(sequence<string> incoming_envelopes_as_json);
 
     [Throws=LoginsApiError]
-    sequence<string> apply();
+    sequence<string> apply(i64 server_modified_millis);
 
     [Throws=LoginsApiError]
     void set_uploaded(i64 new_timestamp, sequence<string> uploaded_ids);

components/logins/src/sync/bridge.rs

@@ -6,8 +6,6 @@ use crate::sync::engine::LoginsSyncEngine;
 use crate::LoginStore;
 use anyhow::Result;
 use std::sync::Arc;
-use sync15::engine::BridgedEngineAdaptor;
-use sync15::ServerTimestamp;
 
 impl LoginStore {
     /// Returns a bridged sync engine for Desktop for this store.
@@ -18,55 +16,15 @@ impl LoginStore {
     /// `LoginsApiError` through `From<anyhow::Error>`.
     pub fn bridged_engine(self: Arc<Self>) -> Result<Arc<LoginsBridgedEngine>> {
         let engine = LoginsSyncEngine::new(self)?;
-        let bridged_engine = LoginsBridgedEngineAdaptor { engine };
-        Ok(Arc::new(LoginsBridgedEngine::new(Box::new(bridged_engine))))
-    }
-}
-
-/// `LoginsSyncEngine` only implements the internal `sync15::SyncEngine` trait,
-/// which is what the mobile (Android/iOS) sync manager drives. Desktop's Sync
-/// framework instead speaks the `mozIBridgedSyncEngine` interface, whose Rust
-/// shape is `sync15::BridgedEngine`. This adaptor wraps our `SyncEngine` and,
-/// via the blanket `impl<A: BridgedEngineAdaptor> BridgedEngine for A`, gives
-/// us a `BridgedEngine` for free. The adaptor exists only because these two
-/// sync-engine traits still live side by side; it can go away if they're ever
-/// unified.
-struct LoginsBridgedEngineAdaptor {
-    engine: LoginsSyncEngine,
-}
-
-/// see sync15/src/engine/bridged_engine.rs for required functions for the trait
-impl BridgedEngineAdaptor for LoginsBridgedEngineAdaptor {
-    fn last_sync(&self) -> Result<i64> {
-        // `get_last_sync` takes the `&LoginDb` to avoid deadlocking when called
-        // mid-sync (while the lock is already held). The bridge methods are
-        // always called outside a sync transaction, so we can lock here.
-        let db = self.engine.store.lock_db()?;
-        Ok(self
-            .engine
-            .get_last_sync(&db)?
-            .unwrap_or_default()
-            .as_millis())
-    }
-
-    fn set_last_sync(&self, last_sync_millis: i64) -> Result<()> {
-        let db = self.engine.store.lock_db()?;
-        self.engine
-            .set_last_sync(&db, ServerTimestamp::from_millis(last_sync_millis))?;
-        Ok(())
-    }
-
-    fn engine(&self) -> &dyn sync15::engine::SyncEngine {
-        &self.engine
+        Ok(Arc::new(LoginsBridgedEngine::new(Box::new(engine))))
     }
 }
 
 // The UniFFI-exposed `LoginsBridgedEngine` (a thin newtype around
 // `sync15::engine::BridgedEngineWrapper`) is generated by this macro, which
-// removes the facade + BSO marshalling boilerplate that used to live here.
-// logins' `set_uploaded` UDL row is `sequence<string>`, so the id element type
-// is `String`. See services/interfaces/mozIBridgedSyncEngine.idl for the contract.
-sync15::uniffi_bridged_engine!(LoginsBridgedEngine, String);
+// removes the facade + BSO marshalling boilerplate. The wrapper drives our
+// `LoginsSyncEngine`'s `SyncEngine` impl directly.
+sync15::uniffi_bridged_engine!(LoginsBridgedEngine);
 
 #[cfg(not(feature = "keydb"))]
 #[cfg(test)]
@@ -89,7 +47,7 @@ mod tests {
 
         // Fresh DB: never synced.
         assert_eq!(bridge.last_sync().unwrap(), 0);
-        bridge.set_last_sync(3).unwrap();
+        bridge.set_uploaded(3, vec![]).unwrap();
         assert_eq!(bridge.last_sync().unwrap(), 3);
 
         assert!(bridge.sync_id().unwrap().is_none());
@@ -98,14 +56,16 @@ mod tests {
         assert_eq!(bridge.sync_id().unwrap(), Some("some_guid".to_string()));
         // changing the sync ID should reset the timestamp
         assert_eq!(bridge.last_sync().unwrap(), 0);
-        bridge.set_last_sync(3).unwrap();
+        // Advance the engine-owned last_sync
+        bridge.set_uploaded(3, vec![]).unwrap();
 
         bridge.reset_sync_id().unwrap();
         // should now be a random guid.
         assert_ne!(bridge.sync_id().unwrap(), Some("some_guid".to_string()));
         // should have reset the last sync timestamp.
         assert_eq!(bridge.last_sync().unwrap(), 0);
-        bridge.set_last_sync(3).unwrap();
+        // Advance the engine-owned last_sync
+        bridge.set_uploaded(3, vec![]).unwrap();
 
         // `reset` clears the guid and the timestamp
         bridge.reset().unwrap();
@@ -162,7 +122,7 @@ mod tests {
 
         // Applying stores the remote record locally and returns the local-only
         // login for upload.
-        let outgoing = bridge.apply().expect("should apply");
+        let outgoing = bridge.apply(0).expect("should apply");
         let changes: HashMap<String, serde_json::Value> = outgoing
             .into_iter()
             .map(|s| {

components/logins/src/sync/engine.rs

@@ -450,7 +450,15 @@ impl SyncEngine for LoginsSyncEngine {
         telem: &mut telemetry::Engine,
     ) -> anyhow::Result<Vec<OutgoingBso>> {
         let inbound = self.staged.lock().unwrap().drain(..).collect();
-        Ok(self.do_apply_incoming(inbound, timestamp, telem)?)
+        let outgoing = self.do_apply_incoming(inbound, timestamp, telem)?;
+        // The engine owns its last-sync timestamp but during a sync, that
+        // value is known differently in desktop v mobile. Record a
+        // timestamp if we are given one.
+        if timestamp != ServerTimestamp(0) {
+            let db = self.store.lock_db()?;
+            self.set_last_sync(&db, timestamp)?;
+        }
+        Ok(outgoing)
     }
 
     fn set_uploaded(&self, new_timestamp: ServerTimestamp, ids: Vec<Guid>) -> anyhow::Result<()> {
@@ -460,6 +468,21 @@ impl SyncEngine for LoginsSyncEngine {
         )?)
     }
 
+    // For the Desktop bridge which makes the collection requests.
+    fn last_sync(&self) -> anyhow::Result<Option<ServerTimestamp>> {
+        let db = self.store.lock_db()?;
+        Ok(self.get_last_sync(&db)?)
+    }
+
+    // Force a full re-download next sync without a full reset. Desktop's bridged
+    // engine base calls this for every engine, so logins must implement it
+    // rather than fall back to the no-op default.
+    fn reset_last_sync(&self) -> anyhow::Result<()> {
+        let db = self.store.lock_db()?;
+        self.set_last_sync(&db, ServerTimestamp(0))?;
+        Ok(())
+    }
+
     fn get_collection_request(
         &self,
         server_timestamp: ServerTimestamp,

components/sync15/src/client/sync.rs

@@ -37,8 +37,9 @@ pub fn synchronize_with_clients_engine(
         }
     };
 
+    engine.sync_started()?;
     if let Some(clients) = clients {
-        engine.prepare_for_sync(&|| clients.get_client_data())?;
+        engine.set_clients(&|| clients.get_client_data())?;
     }
     interruptee.err_if_interrupted()?;
     // We assume an "engine" manages exactly one "collection" with the engine's name.

components/sync15/src/client_types.rs

@@ -9,8 +9,9 @@ use crate::DeviceType;
 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
 
-/// Argument to Store::prepare_for_sync. See comment there for more info. Only
-/// really intended to be used by tabs engine.
+/// Argument to `SyncEngine::set_clients` - a leaky abstraction of fxa/sync
+/// device ids. These are "short term" IDs in that they don't survive reauth
+/// etc, so used for "recent" things like open tabs.
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
 pub struct ClientData {
     pub local_client_id: String,