Bug 2050036 - Add Store::shutdown() to close the autofill DB connection

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 [Full Changelog](In progress)
 
+## ✨ What's Changed ✨
+
+### Autofill
+
+- Add `Store::shutdown()`, which closes the database connection early so it happens before Firefox Desktop's late-write shutdown barrier rather than during GC. Operations after shutdown return `DatabaseClosed`. ([Bug 2050036](https://bugzilla.mozilla.org/show_bug.cgi?id=2050036))
+
 # v154.0 (_2026-07-20_)
 
 ## ✨ What's Changed ✨

components/autofill/Cargo.toml

@@ -14,6 +14,7 @@ error-support = { path = "../support/error" }
 interrupt-support = { path = "../support/interrupt" }
 jwcrypto = { path = "../support/jwcrypto" }
 lazy_static = "1.4"
+parking_lot = ">=0.11,<=0.12"
 rusqlite = { version = "0.37.0", features = ["functions", "bundled", "serde_json", "unlock_notify"] }
 serde = "1"
 serde_derive = "1"

components/autofill/src/autofill.udl

@@ -208,6 +208,8 @@ interface Store {
     [Throws=AutofillApiError]
     void run_maintenance();
 
+    void shutdown();
+
     [Self=ByArc]
     void register_with_sync_manager();
 };

components/autofill/src/db/mod.rs

@@ -11,6 +11,7 @@ pub mod store;
 
 use crate::error::*;
 
+use error_support::error;
 use interrupt_support::{SqlInterruptHandle, SqlInterruptScope};
 use rusqlite::{Connection, OpenFlags};
 use sql_support::open_database;
@@ -61,6 +62,13 @@ impl AutofillDb {
     pub fn begin_interrupt_scope(&self) -> Result<SqlInterruptScope> {
         Ok(self.interrupt_handle.begin_interrupt_scope()?)
     }
+
+    pub fn close(self) {
+        if let Err((_, err)) = self.writer.close() {
+            // Log the error, but continue with shutdown.
+            error!("Failed to close the connection: {:?}", err);
+        }
+    }
 }
 
 impl Deref for AutofillDb {

components/autofill/src/db/store.rs

@@ -10,13 +10,14 @@ use crate::db::{
 };
 use crate::error::*;
 use error_support::handle_error;
+use parking_lot::{MappedMutexGuard, Mutex, MutexGuard};
 use rusqlite::{
     types::{FromSql, ToSql},
     Connection,
 };
 use sql_support::{self, run_maintenance, ConnExt};
 use std::path::Path;
-use std::sync::{Arc, Mutex, Weak};
+use std::sync::{Arc, Weak};
 use sync15::engine::{SyncEngine, SyncEngineId};
 use sync_guid::Guid;
 
@@ -25,7 +26,8 @@ lazy_static::lazy_static! {
     // Mutex: just taken long enough to update the contents - needed to wrap
     //        the Weak as it isn't `Sync`
     // [Arc/Weak]<Store>: What the sync manager actually needs.
-    static ref STORE_FOR_MANAGER: Mutex<Weak<Store>> = Mutex::new(Weak::new());
+    static ref STORE_FOR_MANAGER: std::sync::Mutex<Weak<Store>> =
+        std::sync::Mutex::new(Weak::new());
 }
 
 /// Called by the sync manager to get a sync engine via the store previously
@@ -48,49 +50,53 @@ pub fn get_registered_sync_engine(engine_id: &SyncEngineId) -> Option<Box<dyn Sy
 
 // This is the type that uniffi exposes.
 pub struct Store {
-    pub(crate) db: Mutex<AutofillDb>,
+    pub(crate) db: Mutex<Option<AutofillDb>>,
 }
 
 impl Store {
     #[handle_error(Error)]
     pub fn new(db_path: impl AsRef<Path>) -> ApiResult<Self> {
         Ok(Self {
-            db: Mutex::new(AutofillDb::new(db_path)?),
+            db: Mutex::new(Some(AutofillDb::new(db_path)?)),
         })
     }
 
     /// Creates a store backed by an in-memory database with its own memory API (required for unit tests).
     #[cfg(test)]
     pub fn new_memory() -> Self {
         Self {
-            db: Mutex::new(crate::db::test::new_mem_db()),
+            db: Mutex::new(Some(crate::db::test::new_mem_db())),
         }
     }
 
     /// Creates a store backed by an in-memory database that shares its memory API (required for autofill sync tests).
     #[handle_error(Error)]
     pub fn new_shared_memory(db_name: &str) -> ApiResult<Self> {
         Ok(Self {
-            db: Mutex::new(AutofillDb::new_memory(db_name)?),
+            db: Mutex::new(Some(AutofillDb::new_memory(db_name)?)),
         })
     }
 
+    pub(crate) fn lock_db(&self) -> Result<MappedMutexGuard<'_, AutofillDb>> {
+        MutexGuard::try_map(self.db.lock(), |db| db.as_mut()).map_err(|_| Error::DatabaseClosed)
+    }
+
     #[handle_error(Error)]
     pub fn add_credit_card(&self, fields: UpdatableCreditCardFields) -> ApiResult<CreditCard> {
-        let credit_card = credit_cards::add_credit_card(&self.db.lock().unwrap().writer, fields)?;
+        let credit_card = credit_cards::add_credit_card(&self.lock_db()?.writer, fields)?;
         Ok(credit_card.into())
     }
 
     #[handle_error(Error)]
     pub fn get_credit_card(&self, guid: String) -> ApiResult<CreditCard> {
         let credit_card =
-            credit_cards::get_credit_card(&self.db.lock().unwrap().writer, &Guid::new(&guid))?;
+            credit_cards::get_credit_card(&self.lock_db()?.writer, &Guid::new(&guid))?;
         Ok(credit_card.into())
     }
 
     #[handle_error(Error)]
     pub fn get_all_credit_cards(&self) -> ApiResult<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.lock_db()?.writer)?
             .into_iter()
             .map(|x| x.into())
             .collect();
@@ -99,7 +105,7 @@ impl Store {
 
     #[handle_error(Error)]
     pub fn count_all_credit_cards(&self) -> ApiResult<i64> {
-        let count = credit_cards::count_all_credit_cards(&self.db.lock().unwrap().writer)?;
+        let count = credit_cards::count_all_credit_cards(&self.lock_db()?.writer)?;
         Ok(count)
     }
 
@@ -109,36 +115,32 @@ impl Store {
         guid: String,
         credit_card: UpdatableCreditCardFields,
     ) -> ApiResult<()> {
-        credit_cards::update_credit_card(
-            &self.db.lock().unwrap().writer,
-            &Guid::new(&guid),
-            &credit_card,
-        )
+        credit_cards::update_credit_card(&self.lock_db()?.writer, &Guid::new(&guid), &credit_card)
     }
 
     #[handle_error(Error)]
     pub fn delete_credit_card(&self, guid: String) -> ApiResult<bool> {
-        credit_cards::delete_credit_card(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        credit_cards::delete_credit_card(&self.lock_db()?.writer, &Guid::new(&guid))
     }
 
     #[handle_error(Error)]
     pub fn touch_credit_card(&self, guid: String) -> ApiResult<()> {
-        credit_cards::touch(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        credit_cards::touch(&self.lock_db()?.writer, &Guid::new(&guid))
     }
 
     #[handle_error(Error)]
     pub fn add_address(&self, new_address: UpdatableAddressFields) -> ApiResult<Address> {
-        Ok(addresses::add_address(&self.db.lock().unwrap().writer, new_address)?.into())
+        Ok(addresses::add_address(&self.lock_db()?.writer, new_address)?.into())
     }
 
     #[handle_error(Error)]
     pub fn get_address(&self, guid: String) -> ApiResult<Address> {
-        Ok(addresses::get_address(&self.db.lock().unwrap().writer, &Guid::new(&guid))?.into())
+        Ok(addresses::get_address(&self.lock_db()?.writer, &Guid::new(&guid))?.into())
     }
 
     #[handle_error(Error)]
     pub fn get_all_addresses(&self) -> ApiResult<Vec<Address>> {
-        let addresses = addresses::get_all_addresses(&self.db.lock().unwrap().writer)?
+        let addresses = addresses::get_all_addresses(&self.lock_db()?.writer)?
             .into_iter()
             .map(|x| x.into())
             .collect();
@@ -147,38 +149,38 @@ impl Store {
 
     #[handle_error(Error)]
     pub fn count_all_addresses(&self) -> ApiResult<i64> {
-        let count = addresses::count_all_addresses(&self.db.lock().unwrap().writer)?;
+        let count = addresses::count_all_addresses(&self.lock_db()?.writer)?;
         Ok(count)
     }
 
     #[handle_error(Error)]
     pub fn update_address(&self, guid: String, address: UpdatableAddressFields) -> ApiResult<()> {
-        addresses::update_address(&self.db.lock().unwrap().writer, &Guid::new(&guid), &address)
+        addresses::update_address(&self.lock_db()?.writer, &Guid::new(&guid), &address)
     }
 
     #[handle_error(Error)]
     pub fn delete_address(&self, guid: String) -> ApiResult<bool> {
-        addresses::delete_address(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        addresses::delete_address(&self.lock_db()?.writer, &Guid::new(&guid))
     }
 
     #[handle_error(Error)]
     pub fn touch_address(&self, guid: String) -> ApiResult<()> {
-        addresses::touch(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        addresses::touch(&self.lock_db()?.writer, &Guid::new(&guid))
     }
 
     #[handle_error(Error)]
     pub fn add_passport(&self, fields: UpdatablePassportFields) -> ApiResult<Passport> {
-        Ok(passports::add_passport(&self.db.lock().unwrap().writer, fields)?.into())
+        Ok(passports::add_passport(&self.lock_db()?.writer, fields)?.into())
     }
 
     #[handle_error(Error)]
     pub fn get_passport(&self, guid: String) -> ApiResult<Passport> {
-        Ok(passports::get_passport(&self.db.lock().unwrap().writer, &Guid::new(&guid))?.into())
+        Ok(passports::get_passport(&self.lock_db()?.writer, &Guid::new(&guid))?.into())
     }
 
     #[handle_error(Error)]
     pub fn get_all_passports(&self) -> ApiResult<Vec<Passport>> {
-        let passports = passports::get_all_passports(&self.db.lock().unwrap().writer)?
+        let passports = passports::get_all_passports(&self.lock_db()?.writer)?
             .into_iter()
             .map(|x| x.into())
             .collect();
@@ -187,7 +189,7 @@ impl Store {
 
     #[handle_error(Error)]
     pub fn count_all_passports(&self) -> ApiResult<i64> {
-        passports::count_all_passports(&self.db.lock().unwrap().writer)
+        passports::count_all_passports(&self.lock_db()?.writer)
     }
 
     #[handle_error(Error)]
@@ -196,28 +198,24 @@ impl Store {
         guid: String,
         passport: UpdatablePassportFields,
     ) -> ApiResult<()> {
-        passports::update_passport(
-            &self.db.lock().unwrap().writer,
-            &Guid::new(&guid),
-            &passport,
-        )
+        passports::update_passport(&self.lock_db()?.writer, &Guid::new(&guid), &passport)
     }
 
     #[handle_error(Error)]
     pub fn delete_passport(&self, guid: String) -> ApiResult<bool> {
-        passports::delete_passport(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        passports::delete_passport(&self.lock_db()?.writer, &Guid::new(&guid))
     }
 
     #[handle_error(Error)]
     pub fn touch_passport(&self, guid: String) -> ApiResult<()> {
-        passports::touch(&self.db.lock().unwrap().writer, &Guid::new(&guid))
+        passports::touch(&self.lock_db()?.writer, &Guid::new(&guid))
     }
 
     #[handle_error(Error)]
     pub fn scrub_encrypted_data(self: Arc<Self>) -> ApiResult<()> {
         // scrub the data on disk
         // Currently only credit cards have encrypted data
-        credit_cards::scrub_encrypted_credit_card_data(&self.db.lock().unwrap().writer)?;
+        credit_cards::scrub_encrypted_credit_card_data(&self.lock_db()?.writer)?;
         // Force the sync engine to refetch data (only need to do this for the credit cards, since the
         // addresses engine doesn't store encrypted data).
         crate::sync::credit_card::create_engine(self).reset_local_sync_data()?;
@@ -229,10 +227,10 @@ impl Store {
         self: Arc<Self>,
         local_encryption_key: String,
     ) -> ApiResult<CreditCardsDeletionMetrics> {
-        let db = &self.db.lock().unwrap().writer;
+        let db = self.lock_db()?;
         let deletion_stats =
             credit_cards::scrub_undecryptable_credit_card_data_for_remote_replacement(
-                db,
+                &db.writer,
                 local_encryption_key,
             )?;
 
@@ -241,17 +239,23 @@ impl Store {
         // record that exists on the sync server to overwrite the local record and restore
         // the scrubbed credit card number.
         crate::sync::credit_card::create_engine(self.clone())
-            .reset_local_sync_data_for_verification(db)?;
+            .reset_local_sync_data_for_verification(&db.writer)?;
         Ok(deletion_stats)
     }
 
     #[handle_error(Error)]
     pub fn run_maintenance(&self) -> ApiResult<()> {
-        let conn = self.db.lock().unwrap();
+        let conn = self.lock_db()?;
         run_maintenance(&conn)?;
         Ok(())
     }
 
+    pub fn shutdown(&self) {
+        if let Some(db) = self.db.lock().take() {
+            db.close();
+        }
+    }
+
     // 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
@@ -364,6 +368,22 @@ mod tests {
         assert!(STORE_FOR_MANAGER.lock().unwrap().upgrade().is_none());
     }
 
+    #[test]
+    fn test_shutdown_closes_the_store() {
+        let store = Store::new_shared_memory("shutdown-test").expect("create store");
+        // Operations succeed before shutdown.
+        assert_eq!(store.count_all_passports().expect("count"), 0);
+
+        store.shutdown();
+
+        // After shutdown, operations return an error rather than panicking or
+        // operating on a half-closed store.
+        assert!(store.count_all_passports().is_err());
+
+        // shutdown is idempotent.
+        store.shutdown();
+    }
+
     #[test]
     fn test_scrub_undecryptable_credit_card_data_for_remote_replacement() {
         ensure_initialized();

components/autofill/src/error.rs

@@ -65,6 +65,9 @@ pub enum Error {
 
     #[error("No record with guid exists: {0}")]
     NoSuchRecord(String),
+
+    #[error("The store is closed")]
+    DatabaseClosed,
 }
 
 // Define how our internal errors are handled and converted to external errors
@@ -126,6 +129,13 @@ impl GetErrorHandling for Error {
                 ErrorHandling::convert(AutofillApiError::NoSuchRecord { guid: guid.clone() })
                     .log_warning()
             }
+
+            Self::DatabaseClosed => {
+                ErrorHandling::convert(AutofillApiError::UnexpectedAutofillApiError {
+                    reason: "The store is closed".to_string(),
+                })
+                .report_error("autofill-database-closed")
+            }
         }
     }
 }