Refactored webext-storage database close function

components/webext-storage/src/db.rs

@@ -11,7 +11,8 @@ use rusqlite::Connection;
 use rusqlite::OpenFlags;
 use sql_support::open_database::open_database_with_flags;
 use sql_support::ConnExt;
-use std::ops::{Deref, DerefMut};
+use std::ops::Deref;
+// use std::ops::{Deref, DerefMut};
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 use url::Url;
@@ -23,10 +24,16 @@ use url::Url;
 ///
 /// We only support a single writer connection - so that's the only thing we
 /// store. It's still a bit overkill, but there's only so many yaks in a day.
+pub enum WebExtStorageDb {
+    Open(Connection),
+    Closed,
+}
+
 pub struct StorageDb {
-    writer: Connection,
+    pub writer: WebExtStorageDb,
     interrupt_handle: Arc<SqlInterruptHandle>,
 }
+
 impl StorageDb {
     /// Create a new, or fetch an already open, StorageDb backed by a file on disk.
     pub fn new(db_path: impl AsRef<Path>) -> Result<Self> {
@@ -54,7 +61,7 @@ impl StorageDb {
         let conn = open_database_with_flags(db_path, flags, &schema::WebExtMigrationLogin)?;
         Ok(Self {
             interrupt_handle: Arc::new(SqlInterruptHandle::new(&conn)),
-            writer: conn,
+            writer: WebExtStorageDb::Open(conn),
         })
     }
 
@@ -73,31 +80,28 @@ impl StorageDb {
     /// underlying connection so the caller can retry but (a) that's very tricky
     /// in an Arc<Mutex<>> world and (b) we never actually took advantage of
     /// that retry capability.
-    pub fn close(self) -> Result<()> {
-        self.writer.close().map_err(|(writer, err)| {
-            // In rusqlite 0.28.0 and earlier, if we just let `writer` drop,
-            // the close would panic on failure.
-            // Later rusqlite versions will not panic, but this behavior doesn't
-            // hurt there.
-            std::mem::forget(writer);
-            err.into()
-        })
+    pub fn close(&mut self) -> Result<()> {
+        let conn = match std::mem::replace(&mut self.writer, WebExtStorageDb::Closed) {
+            WebExtStorageDb::Open(conn) => conn,
+            WebExtStorageDb::Closed => return Ok(()),
+        };
+        conn.close().map_err(|(_,y)| Error::SqlError(y))
     }
 }
 
-impl Deref for StorageDb {
-    type Target = Connection;
+// impl Deref for StorageDb {
+//     type Target = Connection;
 
-    fn deref(&self) -> &Self::Target {
-        &self.writer
-    }
-}
+//     fn deref(&self) -> &Self::Target {
+//         &self.writer
+//     }
+// }
 
-impl DerefMut for StorageDb {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.writer
-    }
-}
+// impl DerefMut for StorageDb {
+//     fn deref_mut(&mut self) -> &mut Self::Target {
+//         &mut self.writer
+//     }
+// }
 
 // We almost exclusively use this ThreadSafeStorageDb
 pub struct ThreadSafeStorageDb {

components/webext-storage/src/store.rs

@@ -3,7 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use crate::api::{self, StorageChanges};
-use crate::db::{StorageDb, ThreadSafeStorageDb};
+use crate::db::{StorageDb, ThreadSafeStorageDb, WebExtStorageDb};
 use crate::error::*;
 use crate::migration::{migrate, MigrationInfo};
 use crate::sync;
@@ -157,7 +157,7 @@ impl WebExtStorageStore {
             }
         };
         // consume the mutex and get back the inner.
-        let db = shared.into_inner();
+        let mut db = shared.into_inner();
         db.close()
     }
 
@@ -177,12 +177,17 @@ impl WebExtStorageStore {
     ///
     /// Note that `filename` isn't normalized or canonicalized.
     pub fn migrate(&self, filename: impl AsRef<Path>) -> Result<()> {
-        let db = self.db.lock();
-        let tx = db.unchecked_transaction()?;
+        let db = &self.db.lock().writer;
+        let conn = match db {
+            WebExtStorageDb::Open(y) => y,
+            WebExtStorageDb::Closed => return Err(Error::DatabaseConnectionClosed),
+        };
+
+        let tx = conn.unchecked_transaction()?;
         let result = migrate(&tx, filename.as_ref())?;
         tx.commit()?;
         // Failing to store this information should not cause migration failure.
-        if let Err(e) = result.store(&db) {
+        if let Err(e) = result.store(&conn) {
             debug_assert!(false, "Migration error: {:?}", e);
             log::warn!("Failed to record migration telmetry: {}", e);
         }