Adds basic test case for migration

CHANGES_UNRELEASED.md

@@ -33,3 +33,4 @@ Use the template below to make assigning a version number during the release cut
 ### What's new
   - Exposed a function in Swift `migrateHistoryFromBrowserDb` to migrate history from `browser.db` to `places.db`, the function will migrate all the local visits in one go. ([#5077](https://github.com/mozilla/application-services/pull/5077)).
     - The migration might take some time if a user had a lot of history, so make sure it is **not** run on a thread that shouldn't wait.
+    - The migration runs on a writer connection. This means that other writes to the `places.db` will be delayed until the migration is done.

components/places/ios/Places/Places.swift

@@ -75,12 +75,6 @@ public class PlacesAPI {
             try self.api.placesBookmarksImportFromIos(dbPath: path)
         }
     }
-
-    open func migrateHistoryFromBrowserDb(path: String) throws -> HistoryMigrationResult {
-        try queue.sync {
-            try self.api.placesHistoryImportFromIos(dbPath: path)
-        }
-    }
 
     /**
      * Open a new reader connection.
@@ -939,4 +933,11 @@ public class PlacesWriteConnection: PlacesReadConnection {
             return try self.conn.applyObservation(visit: visitObservation)
         }
     }
+
+    open func migrateHistoryFromBrowserDb(path: String) throws -> HistoryMigrationResult {
+        return try queue.sync {
+            try self.checkApi()
+            return try self.conn.placesHistoryImportFromIos(dbPath: path)
+        }
+    }
 }

components/places/src/ffi.rs

@@ -234,11 +234,6 @@ impl PlacesApi {
         Ok(())
     }
 
-    fn places_history_import_from_ios(&self, db_path: String) -> Result<HistoryMigrationResult> {
-        let metrics = import_ios_history(self, &db_path)?;
-        Ok(metrics)
-    }
-
     fn bookmarks_reset(&self) -> Result<()> {
         self.reset_bookmarks()?;
         Ok(())
@@ -585,6 +580,10 @@ impl PlacesConnection {
     fn bookmarks_update(&self, item: BookmarkUpdateInfo) -> Result<()> {
         self.with_conn(|conn| bookmarks::update_bookmark_from_info(conn, item))
     }
+
+    fn places_history_import_from_ios(&self, db_path: String) -> Result<HistoryMigrationResult> {
+        self.with_conn(|conn| import_ios_history(conn, &db_path))
+    }
 }
 
 impl AsRef<SqlInterruptHandle> for PlacesConnection {

components/places/src/import/common.rs

@@ -25,16 +25,19 @@ lazy_static::lazy_static! {
 pub mod sql_fns {
     use crate::import::common::NOW;
     use crate::storage::URL_LENGTH_MAX;
-    use rusqlite::{functions::Context, types::ValueRef, Result};
+    use rusqlite::{
+        functions::Context,
+        types::{FromSql, ValueRef},
+        Result,
+    };
     use types::Timestamp;
     use url::Url;
 
-    #[inline(never)]
-    pub fn sanitize_timestamp(ctx: &Context<'_>) -> Result<Timestamp> {
+    fn sanitize_timestamp<T: FromSql + TryInto<Timestamp>>(ctx: &Context<'_>) -> Result<Timestamp> {
         let now = *NOW;
         let is_sane = |ts: Timestamp| -> bool { Timestamp::EARLIEST <= ts && ts <= now };
-        if let Ok(ts) = ctx.get::<i64>(0) {
-            let ts = Timestamp(u64::try_from(ts).unwrap_or(0));
+        if let Ok(ts) = ctx.get::<T>(0) {
+            let ts = ts.try_into().unwrap_or(now);
             if is_sane(ts) {
                 return Ok(ts);
             }
@@ -47,6 +50,19 @@ pub mod sql_fns {
         Ok(now)
     }
 
+    // Unfortunately dates for history visits in old iOS databases
+    // have a type of `REAL` in their schema. This means they are represented
+    // as a float value and have to be read as f64s
+    #[inline(never)]
+    pub fn sanitize_float_timestamp(ctx: &Context<'_>) -> Result<Timestamp> {
+        sanitize_timestamp::<f64>(ctx)
+    }
+
+    #[inline(never)]
+    pub fn sanitize_integer_timestamp(ctx: &Context<'_>) -> Result<Timestamp> {
+        sanitize_timestamp::<i64>(ctx)
+    }
+
     // Possibly better named as "normalize URL" - even in non-error cases, the
     // result string may not be the same href used passed as input.
     #[inline(never)]
@@ -131,10 +147,10 @@ impl Drop for ExecuteOnDrop<'_> {
     }
 }
 
-pub fn select_count(conn: &PlacesDb, stmt: &str) -> u32 {
+pub fn select_count(conn: &PlacesDb, stmt: &str) -> Result<u32> {
     let count: Result<Option<u32>> =
         conn.try_query_row(stmt, [], |row| Ok(row.get::<_, u32>(0)?), false);
-    count.unwrap().unwrap()
+    count.map(|op| op.unwrap_or(0))
 }
 
 #[derive(Serialize, PartialEq, Debug, Clone, Default)]
@@ -157,7 +173,7 @@ pub fn define_history_migration_functions(c: &Connection) -> Result<()> {
         "sanitize_timestamp",
         1,
         FunctionFlags::SQLITE_UTF8 | FunctionFlags::SQLITE_DETERMINISTIC,
-        crate::import::common::sql_fns::sanitize_timestamp,
+        crate::import::common::sql_fns::sanitize_integer_timestamp,
     )?;
     c.create_scalar_function(
         "hash",
@@ -177,5 +193,11 @@ pub fn define_history_migration_functions(c: &Connection) -> Result<()> {
         FunctionFlags::SQLITE_UTF8 | FunctionFlags::SQLITE_DETERMINISTIC,
         crate::import::common::sql_fns::sanitize_utf8,
     )?;
+    c.create_scalar_function(
+        "sanitize_float_timestamp",
+        1,
+        FunctionFlags::SQLITE_UTF8 | FunctionFlags::SQLITE_DETERMINISTIC,
+        crate::import::common::sql_fns::sanitize_float_timestamp,
+    )?;
     Ok(())
 }

components/places/src/import/fennec/bookmarks.rs

@@ -437,7 +437,7 @@ fn bookmark_data_from_fennec_pinned(
 }
 
 mod sql_fns {
-    use crate::import::common::sql_fns::{sanitize_timestamp, sanitize_utf8, validate_url};
+    use crate::import::common::sql_fns::{sanitize_integer_timestamp, sanitize_utf8, validate_url};
     use rusqlite::{
         functions::{Context, FunctionFlags},
         Connection, Result,
@@ -460,7 +460,7 @@ mod sql_fns {
             "sanitize_timestamp",
             1,
             FunctionFlags::SQLITE_UTF8 | FunctionFlags::SQLITE_DETERMINISTIC,
-            sanitize_timestamp,
+            sanitize_integer_timestamp,
         )?;
         c.create_scalar_function(
             "sanitize_utf8",

components/places/src/import/fennec/history.rs

@@ -49,7 +49,7 @@ fn do_import(places_api: &PlacesApi, android_db_file_url: Url) -> Result<History
     let tx = conn.begin_transaction()?;
 
     log::debug!("Counting Fennec history visits");
-    let num_total = select_count(&conn, &COUNT_FENNEC_HISTORY_VISITS);
+    let num_total = select_count(&conn, &COUNT_FENNEC_HISTORY_VISITS)?;
 
     log::debug!("Creating and populating staging table");
     conn.execute_batch(&CREATE_STAGING_TABLE)?;
@@ -74,7 +74,7 @@ fn do_import(places_api: &PlacesApi, android_db_file_url: Url) -> Result<History
     log::info!("Successfully imported history visits!");
 
     log::debug!("Counting Fenix history visits");
-    let num_succeeded = select_count(&conn, &COUNT_FENIX_HISTORY_VISITS);
+    let num_succeeded = select_count(&conn, &COUNT_FENIX_HISTORY_VISITS)?;
     let num_failed = num_total - num_succeeded;
 
     auto_detach.execute_now()?;

components/places/src/import/ios/bookmarks.rs

@@ -1,9 +1,6 @@
 /* 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/. */
-/* 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/. */
 
 use crate::api::places_api::PlacesApi;
 use crate::bookmark_sync::{
@@ -469,7 +466,7 @@ lazy_static::lazy_static! {
 }
 
 mod sql_fns {
-    use crate::import::common::sql_fns::{sanitize_timestamp, validate_url};
+    use crate::import::common::sql_fns::{sanitize_integer_timestamp, validate_url};
     use rusqlite::{functions::FunctionFlags, Connection, Result};
 
     pub(super) fn define_functions(c: &Connection) -> Result<()> {
@@ -483,7 +480,7 @@ mod sql_fns {
             "sanitize_timestamp",
             1,
             FunctionFlags::SQLITE_UTF8 | FunctionFlags::SQLITE_DETERMINISTIC,
-            sanitize_timestamp,
+            sanitize_integer_timestamp,
         )?;
         Ok(())
     }

components/places/src/import/ios/history.rs

@@ -4,12 +4,12 @@
 
 use std::time::Instant;
 
-use crate::api::places_api::PlacesApi;
 use crate::bookmark_sync::engine::update_frecencies;
 use crate::error::Result;
 use crate::import::common::{
     attached_database, define_history_migration_functions, select_count, HistoryMigrationResult,
 };
+use crate::PlacesDb;
 use url::Url;
 
 /// This import is used for iOS users migrating from `browser.db`-based
@@ -30,26 +30,29 @@ use url::Url;
 /// - Update frecency for new items.
 /// - Cleanup (detach iOS database, etc).
 pub fn import(
-    places_api: &PlacesApi,
+    conn: &PlacesDb,
     path: impl AsRef<std::path::Path>,
 ) -> Result<HistoryMigrationResult> {
     let url = crate::util::ensure_url_path(path)?;
-    do_import(places_api, url)
+    do_import(conn, url)
 }
 
-fn do_import(places_api: &PlacesApi, ios_db_file_url: Url) -> Result<HistoryMigrationResult> {
-    let conn_mutex = places_api.get_sync_connection()?;
-    let conn = conn_mutex.lock();
+fn do_import(conn: &PlacesDb, ios_db_file_url: Url) -> Result<HistoryMigrationResult> {
     let scope = conn.begin_interrupt_scope()?;
-    define_history_migration_functions(&conn)?;
-
+    define_history_migration_functions(conn)?;
+    // TODO: for some reason opening the db as read-only in **iOS** causes
+    // the migration to fail with an "attempting to write to a read-only database"
+    // when the migration is **not** writing to the BrowserDB database.
+    // this only happens in the simulator with artifacts built for iOS and not
+    // in unit tests.
+
+    // ios_db_file_url.query_pairs_mut().append_pair("mode", "ro");
     let import_start = Instant::now();
     log::debug!("Attaching database {}", ios_db_file_url);
-    let auto_detach = attached_database(&conn, &ios_db_file_url, "ios")?;
+    let auto_detach = attached_database(conn, &ios_db_file_url, "ios")?;
     let tx = conn.begin_transaction()?;
-    let num_total = select_count(&conn, &COUNT_IOS_HISTORY_VISITS);
+    let num_total = select_count(conn, &COUNT_IOS_HISTORY_VISITS)?;
     log::debug!("The number of visits is: {:?}", num_total);
-
     log::debug!("Creating and populating staging table");
     conn.execute_batch(&CREATE_STAGING_TABLE)?;
     conn.execute_batch(&FILL_STAGING)?;
@@ -67,13 +70,13 @@ fn do_import(places_api: &PlacesApi, ios_db_file_url: Url) -> Result<HistoryMigr
     // Note: update_frecencies manages its own transaction, which is fine,
     // since nothing that bad will happen if it is aborted.
     log::debug!("Updating frecencies");
-    update_frecencies(&conn, &scope)?;
+    update_frecencies(conn, &scope)?;
 
     log::info!("Successfully imported history visits!");
 
     log::debug!("Counting Places history visits");
-    let num_succeeded = select_count(&conn, &COUNT_PLACES_HISTORY_VISITS);
-    let num_failed = num_total - num_succeeded;
+    let num_succeeded = select_count(conn, &COUNT_PLACES_HISTORY_VISITS)?;
+    let num_failed = num_total.saturating_sub(num_succeeded);
 
     auto_detach.execute_now()?;
 
@@ -100,16 +103,18 @@ lazy_static::lazy_static! {
             id INTEGER PRIMARY KEY,
             url TEXT,
             url_hash INTEGER NOT NULL,
-            title TEXT
+            title TEXT,
+            is_deleted TINYINT NOT NULL
         ) WITHOUT ROWID;";
 
    static ref FILL_STAGING: &'static str = "
-    INSERT OR IGNORE INTO temp.iOSHistoryStaging(id, url, url_hash, title)
+    INSERT OR IGNORE INTO temp.iOSHistoryStaging(id, url, url_hash, title, is_deleted)
         SELECT
             h.id,
             validate_url(h.url),
             hash(validate_url(h.url)),
-            sanitize_utf8(h.title)
+            sanitize_utf8(h.title),
+            h.is_deleted
         FROM ios.history h
         WHERE url IS NOT NULL"
    ;
@@ -127,7 +132,8 @@ lazy_static::lazy_static! {
             t.title,
             -1,
             1
-        FROM temp.iOSHistoryStaging t"
+        FROM temp.iOSHistoryStaging t
+        WHERE t.is_deleted = 0"
    ;
 
    // Insert history visits
@@ -136,11 +142,12 @@ lazy_static::lazy_static! {
         SELECT
             NULL, -- iOS does not store enough information to rebuild redirect chains.
             (SELECT p.id FROM main.moz_places p WHERE p.url_hash = t.url_hash AND p.url = t.url),
-            sanitize_timestamp(v.date),
+            sanitize_float_timestamp(v.date),
             v.type, -- iOS stores visit types that map 1:1 to ours.
             v.is_local
         FROM ios.visits v
-        LEFT JOIN temp.iOSHistoryStaging t on v.siteID = t.id"
+        LEFT JOIN temp.iOSHistoryStaging t on v.siteID = t.id
+        WHERE t.is_deleted = 0"
    ;
 
 

components/places/src/places.udl

@@ -54,9 +54,6 @@ interface PlacesApi {
     [Throws=PlacesError]
     void places_bookmarks_import_from_ios(string db_path);
 
-    [Throws=PlacesError]
-    HistoryMigrationResult places_history_import_from_ios(string db_path);
-
     [Throws=PlacesError]
     void bookmarks_reset();
 };
@@ -190,6 +187,9 @@ interface PlacesConnection {
 
     [Throws=PlacesError]
     Guid bookmarks_insert(InsertableBookmarkItem bookmark);
+
+    [Throws=PlacesError]
+    HistoryMigrationResult places_history_import_from_ios(string db_path);
 };
 
 /**

components/places/src/storage/history.rs

@@ -899,7 +899,6 @@ pub mod history_sync {
 
         let visits = visits
             .iter()
-            .cloned()
             .filter(|v| Timestamp::from(v.date) > visit_ignored_mark)
             .collect::<Vec<_>>();
 

components/support/types/src/lib.rs

@@ -33,6 +33,11 @@ impl Timestamp {
         SystemTime::from(self).checked_sub(d).map(Timestamp::from)
     }
 
+    #[inline]
+    pub fn checked_add(self, d: Duration) -> Option<Timestamp> {
+        SystemTime::from(self).checked_add(d).map(Timestamp::from)
+    }
+
     pub fn as_millis(self) -> u64 {
         self.0
     }
@@ -79,6 +84,36 @@ impl From<u64> for Timestamp {
     }
 }
 
+impl TryFrom<i64> for Timestamp {
+    type Error = std::num::TryFromIntError;
+    #[inline]
+    fn try_from(value: i64) -> Result<Self, Self::Error> {
+        Ok(Timestamp(u64::try_from(value).unwrap_or(0)))
+    }
+}
+
+impl TryFrom<f64> for Timestamp {
+    type Error = std::num::TryFromIntError;
+    #[inline]
+    fn try_from(value: f64) -> Result<Self, Self::Error> {
+        // This is not perfect, floating numbers are complicated
+        // but we reject any zeros, negative timestamps, NaNs, infinite or subnormal values
+        // There is a dependency `conv` that can do the approximation
+        // and fail if it's not possible to approximate, however,
+        // adding a dependency is not worth the current use case, where this is only
+        // used in migrations and unless the data is **very** badly
+        // malformed, the floats should be roundable and convertable
+        // to i64s.
+        Ok(Timestamp({
+            if value.is_normal() && value > 0.0 {
+                value.round() as u64
+            } else {
+                0
+            }
+        }))
+    }
+}
+
 impl fmt::Display for Timestamp {
     #[inline]
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {