Persist remote tabs to a sql database.

components/tabs/Cargo.toml

@@ -7,20 +7,26 @@ license = "MPL-2.0"
 exclude = ["/android", "/ios"]
 
 [dependencies]
-sync15 = { path = "../sync15" }
+anyhow = "1.0"
+error-support = { path = "../support/error" }
+interrupt-support = { path = "../support/interrupt" }
+lazy_static = "1.4"
+log = "0.4"
+rusqlite = { version = "0.24.2", features = ["bundled", "unlock_notify"] }
 serde = "1"
 serde_derive = "1"
 serde_json = "1"
-lazy_static = "1.4"
-log = "0.4"
-url = "2.2"
-error-support = { path = "../support/error" }
-interrupt-support = { path = "../support/interrupt" }
+sql-support = { path = "../support/sql" }
 sync-guid = { path = "../support/guid", features = ["random"] }
+sync15 = { path = "../sync15" }
 thiserror = "1.0"
-anyhow = "1.0"
 uniffi = "^0.17"
 uniffi_macros = "^0.17"
+url = "2.2"
+
+[dev-dependencies]
+tempfile = "3.1"
+env_logger = { version = "0.8.0", default-features = false, features = ["termcolor", "atty", "humantime"] }
 
 [build-dependencies]
 uniffi_build = { version = "^0.17", features = [ "builtin-bindgen" ]}

components/tabs/src/error.rs

@@ -12,12 +12,20 @@ pub enum ErrorKind {
 
     #[error("Error parsing URL: {0}")]
     UrlParseError(#[from] url::ParseError),
+
+    #[error("Error executing SQL: {0}")]
+    SqlError(#[from] rusqlite::Error),
+
+    #[error("Error opening database: {0}")]
+    OpenDatabaseError(#[from] sql_support::open_database::Error),
 }
 
 error_support::define_error! {
     ErrorKind {
         (SyncAdapterError, sync15::Error),
         (JsonError, serde_json::Error),
         (UrlParseError, url::ParseError),
+        (SqlError, rusqlite::Error),
+        (OpenDatabaseError, sql_support::open_database::Error),
     }
 }

components/tabs/src/lib.rs

@@ -7,6 +7,7 @@
 
 #[macro_use]
 pub mod error;
+mod schema;
 mod storage;
 mod sync;
 

components/tabs/src/schema.rs

@@ -0,0 +1,70 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// Tabs is a bit special - it's a trivial SQL schema and is only used as a persistent
+// cache, and the semantics of the "tabs" collection means there's no need for
+// syncChangeCounter/syncStatus nor a mirror etc.
+
+use rusqlite::{Connection, Transaction};
+use sql_support::open_database::{
+    ConnectionInitializer as MigrationLogic, Error as MigrationError, Result as MigrationResult,
+};
+
+// The payload is json and this module doesn't need to deserialize, so we just
+// store each "payload" as a row.
+// We delete the entire DB each sync and re-populate it with every record on
+// the server. When we read the DB, we also read every single record.
+// So we have no primary keys, no foreign keys, and really completely waste the
+// fact we are using sql.
+const CREATE_SCHEMA_SQL: &str = "
+    CREATE TABLE IF NOT EXISTS tabs (
+        payload TEXT NOT NULL
+    );
+";
+
+pub struct TabsMigrationLogin;
+
+impl MigrationLogic for TabsMigrationLogin {
+    const NAME: &'static str = "tabs storage db";
+    const END_VERSION: u32 = 1;
+
+    fn prepare(&self, conn: &Connection) -> MigrationResult<()> {
+        let initial_pragmas = "
+            -- We don't care about temp tables being persisted to disk.
+            PRAGMA temp_store = 2;
+            -- we unconditionally want write-ahead-logging mode.
+            PRAGMA journal_mode=WAL;
+            -- foreign keys seem worth enforcing (and again, we don't care in practice)
+            PRAGMA foreign_keys = ON;
+        ";
+        conn.execute_batch(initial_pragmas)?;
+        // This is where we'd define our sql functions if we had any!
+        conn.set_prepared_statement_cache_capacity(128);
+        Ok(())
+    }
+
+    fn init(&self, db: &Transaction<'_>) -> MigrationResult<()> {
+        log::debug!("Creating schema");
+        db.execute_batch(CREATE_SCHEMA_SQL)?;
+        Ok(())
+    }
+
+    fn upgrade_from(&self, _db: &Transaction<'_>, version: u32) -> MigrationResult<()> {
+        Err(MigrationError::IncompatibleVersion(version))
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::storage::TabsStorage;
+
+    #[test]
+    fn test_create_schema_twice() {
+        let mut db = TabsStorage::new_with_mem_path("test");
+        let conn = db.open_or_create().unwrap();
+        conn.execute_batch(CREATE_SCHEMA_SQL)
+            .expect("should allow running twice");
+    }
+}

components/tabs/src/storage.rs

@@ -7,42 +7,122 @@ const URI_LENGTH_MAX: usize = 65536;
 // https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/services/sync/modules/engines/tabs.js#8
 const TAB_ENTRIES_LIMIT: usize = 5;
 
+use crate::error::*;
 use crate::DeviceType;
+use rusqlite::{Connection, OpenFlags};
+use serde_derive::{Deserialize, Serialize};
+use sql_support::open_database::{self, open_database_with_flags};
+use sql_support::ConnExt;
 use std::cell::RefCell;
+use std::path::{Path, PathBuf};
 
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub struct RemoteTab {
     pub title: String,
     pub url_history: Vec<String>,
     pub icon: Option<String>,
     pub last_used: i64, // In ms.
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct ClientRemoteTabs {
     pub client_id: String, // Corresponds to the `clients` collection ID of the client.
     pub client_name: String,
+    #[serde(
+        default = "devicetype_default_deser",
+        skip_serializing_if = "devicetype_is_unknown"
+    )]
     pub device_type: DeviceType,
     pub remote_tabs: Vec<RemoteTab>,
 }
 
-pub struct TabsStorage {
-    local_tabs: RefCell<Option<Vec<RemoteTab>>>,
-    remote_tabs: RefCell<Option<Vec<ClientRemoteTabs>>>,
+fn devicetype_default_deser() -> DeviceType {
+    // replace with `DeviceType::default_deser` once #4861 lands.
+    DeviceType::Unknown
 }
 
-impl Default for TabsStorage {
-    fn default() -> Self {
-        Self::new()
-    }
+// Unlike most other uses-cases, here we do allow serializing ::Unknown, but skip it.
+fn devicetype_is_unknown(val: &DeviceType) -> bool {
+    matches!(val, DeviceType::Unknown)
+}
+
+// Tabs has unique requirements for storage:
+// * The "local_tabs" exist only so we can sync them out. There's no facility to
+//   query "local tabs", so there's no need to store these persistently - ie, they
+//   are write-only.
+// * The "remote_tabs" exist purely for incoming items via sync - there's no facility
+//   to set them locally - they are read-only.
+// Note that this means a database is only actually needed after Sync fetches remote tabs,
+// and because sync users are in the minority, the use of a database here is purely
+// optional and created on demand. The implication here is that asking for the "remote tabs"
+// when no database exists is considered a normal situation and just implies no remote tabs exist.
+// (Note however we don't attempt to remove the database when no remote tabs exist, so having
+// no remote tabs in an existing DB is also a normal situation)
+pub struct TabsStorage {
+    local_tabs: RefCell<Option<Vec<RemoteTab>>>,
+    db_path: PathBuf,
+    db_connection: Option<Connection>,
 }
 
 impl TabsStorage {
-    pub fn new() -> Self {
+    pub fn new(db_path: impl AsRef<Path>) -> Self {
         Self {
             local_tabs: RefCell::default(),
-            remote_tabs: RefCell::default(),
+            db_path: db_path.as_ref().to_path_buf(),
+            db_connection: None,
+        }
+    }
+
+    /// Arrange for a new memory-based TabsStorage. As per other DB semantics, creating
+    /// this isn't enough to actually create the db!
+    #[cfg(test)]
+    pub fn new_with_mem_path(db_path: &str) -> Self {
+        let name = PathBuf::from(format!("file:{}?mode=memory&cache=shared", db_path));
+        Self::new(name)
+    }
+
+    /// If a DB file exists, open and return it.
+    pub fn open_if_exists(&mut self) -> Result<Option<&Connection>> {
+        if let Some(ref existing) = self.db_connection {
+            return Ok(Some(existing));
         }
+        let flags = OpenFlags::SQLITE_OPEN_NO_MUTEX
+            | OpenFlags::SQLITE_OPEN_URI
+            | OpenFlags::SQLITE_OPEN_READ_WRITE;
+        match open_database_with_flags(
+            self.db_path.clone(),
+            flags,
+            &crate::schema::TabsMigrationLogin,
+        ) {
+            Ok(conn) => {
+                self.db_connection = Some(conn);
+                Ok(self.db_connection.as_ref())
+            }
+            Err(open_database::Error::SqlError(rusqlite::Error::SqliteFailure(code, _)))
+                if code.code == rusqlite::ErrorCode::CannotOpen =>
+            {
+                Ok(None)
+            }
+            Err(e) => Err(e.into()),
+        }
+    }
+
+    /// Open and return the DB, creating it if it exists.
+    pub fn open_or_create(&mut self) -> Result<&Connection> {
+        if let Some(ref existing) = self.db_connection {
+            return Ok(existing);
+        }
+        let flags = OpenFlags::SQLITE_OPEN_NO_MUTEX
+            | OpenFlags::SQLITE_OPEN_URI
+            | OpenFlags::SQLITE_OPEN_READ_WRITE
+            | OpenFlags::SQLITE_OPEN_CREATE;
+        let conn = open_database_with_flags(
+            self.db_path.clone(),
+            flags,
+            &crate::schema::TabsMigrationLogin,
+        )?;
+        self.db_connection = Some(conn);
+        Ok(self.db_connection.as_ref().unwrap())
     }
 
     pub fn update_local_state(&mut self, local_state: Vec<RemoteTab>) {
@@ -77,20 +157,65 @@ impl TabsStorage {
         None
     }
 
-    pub fn get_remote_tabs(&self) -> Option<Vec<ClientRemoteTabs>> {
-        self.remote_tabs.borrow().clone()
+    pub fn get_remote_tabs(&mut self) -> Option<Vec<ClientRemoteTabs>> {
+        match self.open_if_exists() {
+            Err(e) => {
+                log::error!("Failed to read remote tabs: {}", e);
+                None
+            }
+            Ok(None) => None,
+            Ok(Some(c)) => {
+                match c.query_rows_and_then_named_cached(
+                    "SELECT payload FROM tabs",
+                    &[],
+                    |row| -> Result<_> { Ok(serde_json::from_str(&row.get::<_, String>(0)?)?) },
+                ) {
+                    Ok(crts) => {
+                        // only return clients that actually have tabs.
+                        Some(
+                            crts.into_iter()
+                                .filter(|c: &ClientRemoteTabs| !c.remote_tabs.is_empty())
+                                .collect(),
+                        )
+                    }
+                    Err(e) => {
+                        log::error!("Failed to read database: {}", e);
+                        None
+                    }
+                }
+            }
+        }
     }
 
-    pub(crate) fn replace_remote_tabs(&self, new_remote_tabs: Vec<ClientRemoteTabs>) {
-        let mut remote_tabs = self.remote_tabs.borrow_mut();
-        remote_tabs.replace(new_remote_tabs);
+    pub(crate) fn replace_remote_tabs(
+        &mut self,
+        new_remote_tabs: Vec<ClientRemoteTabs>,
+    ) -> Result<()> {
+        let connection = self.open_or_create()?;
+        let tx = connection.unchecked_transaction()?;
+        // delete the world - we rebuild it from scratch every sync.
+        tx.execute_batch("DELETE FROM tabs")?;
+
+        for crt in new_remote_tabs {
+            tx.execute_named_cached(
+                "INSERT INTO tabs (payload) VALUES (:payload);",
+                rusqlite::named_params! {
+                    ":payload": serde_json::to_string(&crt).expect("tabs don't fail to serialize"),
+                },
+            )?;
+        }
+        tx.commit()?;
+        Ok(())
     }
 
-    pub fn wipe_remote_tabs(&self) {
-        self.remote_tabs.replace(None);
+    pub(crate) fn wipe_remote_tabs(&mut self) -> Result<()> {
+        if let Some(db) = self.open_if_exists()? {
+            db.execute_batch("DELETE FROM tabs")?;
+        }
+        Ok(())
     }
 
-    pub fn wipe_local_tabs(&self) {
+    pub(crate) fn wipe_local_tabs(&self) {
         self.local_tabs.replace(None);
     }
 }
@@ -115,13 +240,27 @@ mod tests {
         assert!(is_url_syncable("https://bobo.com"));
         assert!(is_url_syncable("ftp://bobo.com"));
         assert!(!is_url_syncable("about:blank"));
+        // XXX - this smells wrong - we should insist on a valid complete URL?
         assert!(is_url_syncable("aboutbobo.com"));
         assert!(!is_url_syncable("file:///Users/eoger/bobo"));
     }
 
+    #[test]
+    fn test_open_if_exists_no_file() {
+        let dir = tempfile::tempdir().unwrap();
+        let db_name = dir.path().join("test_open_for_read_no_file.db");
+        let mut storage = TabsStorage::new(db_name.clone());
+        assert!(storage.open_if_exists().unwrap().is_none());
+        storage.open_or_create().unwrap(); // will have created it.
+                                           // make a new storage, but leave the file alone.
+        let mut storage = TabsStorage::new(db_name);
+        // db file exists, so opening for read should open it.
+        assert!(storage.open_if_exists().unwrap().is_some());
+    }
+
     #[test]
     fn test_prepare_local_tabs_for_upload() {
-        let mut storage = TabsStorage::new();
+        let mut storage = TabsStorage::new_with_mem_path("test_prepare_local_tabs_for_upload");
         assert_eq!(storage.prepare_local_tabs_for_upload(), None);
         storage.update_local_state(vec![
             RemoteTab {