Refactored places to use the new error handling system

Replaced the error_support method with the new system that consists of:
  - `PlacesInternalError`: used internally in the crate.  This replaces
    the old `ErrorKind` enum and is the error for `Result<>`.
  - `PlacesError`: used for external errors that we return to UniFFI
    consumers.  This replaces the old `Error` enum and is the error for
    `ApiResult<>`
  - A `GetErrorHandling` impl to convert internal errors to external
    errors and report to sentry.  For now, I tried to make the sentry
    error reporting more-or-less match the old error reporting, which
    means reporting lots of internal errors.  The plan is going to be to
    check sentry and most likely turn some of those into logs rather
    than errors.

Reworked all top-level API methods to return `PlacesApiResults` and use
the `handle_error!` macro to do the error conversion/reporting.

Use `thiserror` to handle converting errors from other crates into
`PlacesInternalError`.

components/logins/src/error.rs

@@ -15,13 +15,6 @@ use sync15::Error as Sync15Error;
 //
 // Named `LoginsStorageError` for backwards compatibility reasons, although
 // this name shouldn't need to be used anywhere other than this file and the .udl
-//
-// Note that there is no `Into` between public and internal errors, but
-// instead the `ErrorHandling` mechanisms are used to explicitly convert
-// when necessary.
-//
-// XXX - not clear that these actually need to use `thiserror`? Certainly
-// not necessary to use `#[from]` though.
 #[derive(Debug, thiserror::Error)]
 pub enum LoginsStorageError {
     #[error("Invalid login: {0}")]

components/places/src/api/places_api.rs

@@ -10,6 +10,7 @@ use crate::storage::{
     self, bookmarks::bookmark_sync, delete_meta, get_meta, history::history_sync, put_meta,
 };
 use crate::util::normalize_path;
+use error_support::handle_error;
 use interrupt_support::register_interrupt;
 use lazy_static::lazy_static;
 use parking_lot::Mutex;
@@ -119,8 +120,10 @@ pub struct SyncState {
 /// For uniffi we need to expose our `Arc` returning constructor as a global function :(
 /// https://github.com/mozilla/uniffi-rs/pull/1063 would fix this, but got some pushback
 /// meaning we are forced into this unfortunate workaround.
-pub fn places_api_new(db_name: impl AsRef<Path>) -> Result<Arc<PlacesApi>> {
-    PlacesApi::new(db_name)
+pub fn places_api_new(db_name: impl AsRef<Path>) -> ApiResult<Arc<PlacesApi>> {
+    handle_error! {
+        PlacesApi::new(db_name)
+    }
 }
 
 /// The entry-point to the places API. This object gives access to database
@@ -210,7 +213,7 @@ impl PlacesApi {
                 // We only allow one of these.
                 let mut guard = self.write_connection.lock();
                 match mem::replace(&mut *guard, None) {
-                    None => Err(ErrorKind::ConnectionAlreadyOpen.into()),
+                    None => Err(PlacesInternalError::ConnectionAlreadyOpen),
                     Some(db) => Ok(db),
                 }
             }
@@ -253,7 +256,7 @@ impl PlacesApi {
     /// connection, you can re-fetch it using open_connection.
     pub fn close_connection(&self, connection: PlacesDb) -> Result<()> {
         if connection.api_id() != self.id {
-            return Err(ErrorKind::WrongApiForClose.into());
+            return Err(PlacesInternalError::WrongApiForClose);
         }
         if connection.conn_type() == ConnectionType::ReadWrite {
             // We only allow one of these.
@@ -441,22 +444,15 @@ impl PlacesApi {
         Ok(())
     }
 
-    pub fn wipe_history(&self) -> Result<()> {
-        // Take the lock to prevent syncing while we're doing this.
-        let _guard = self.sync_state.lock();
-        let conn = self.get_sync_connection()?;
-
-        storage::history::delete_everything(&conn.lock())?;
-        Ok(())
-    }
-
-    pub fn reset_history(&self) -> Result<()> {
-        // Take the lock to prevent syncing while we're doing this.
-        let _guard = self.sync_state.lock();
-        let conn = self.get_sync_connection()?;
+    pub fn reset_history(&self) -> ApiResult<()> {
+        handle_error! {
+            // Take the lock to prevent syncing while we're doing this.
+            let _guard = self.sync_state.lock();
+            let conn = self.get_sync_connection()?;
 
-        history_sync::reset(&conn.lock(), &EngineSyncAssociation::Disconnected)?;
-        Ok(())
+            history_sync::reset(&conn.lock(), &EngineSyncAssociation::Disconnected)?;
+            Ok(())
+        }
     }
 }
 
@@ -577,11 +573,10 @@ mod tests {
             .open_connection(ConnectionType::ReadWrite)
             .expect("should get writer 2");
 
-        // No PartialEq on ErrorKind, so we abuse match.
-        match api.close_connection(fake_writer).unwrap_err().kind() {
-            &ErrorKind::WrongApiForClose => {}
-            e => panic!("Expected error WrongApiForClose, got {:?}", e),
-        }
+        assert!(matches!(
+            api.close_connection(fake_writer).unwrap_err(),
+            PlacesInternalError::WrongApiForClose
+        ));
     }
 
     #[test]

components/places/src/bookmark_sync/engine.rs

@@ -1329,7 +1329,7 @@ impl<'a> Merger<'a> {
 
 impl<'a> dogear::Store for Merger<'a> {
     type Ok = ();
-    type Error = Error;
+    type Error = PlacesInternalError;
 
     /// Builds a fully rooted, consistent tree from all local items and
     /// tombstones.
@@ -1348,7 +1348,11 @@ impl<'a> dogear::Store for Merger<'a> {
                 let (item, _) = self.local_row_to_item(row)?;
                 Tree::with_root(item)
             }
-            None => return Err(ErrorKind::Corruption(Corruption::InvalidLocalRoots).into()),
+            None => {
+                return Err(PlacesInternalError::Corruption(
+                    Corruption::InvalidLocalRoots,
+                ))
+            }
         };
 
         // Add items and contents to the builder, keeping track of their
@@ -1436,7 +1440,9 @@ impl<'a> dogear::Store for Merger<'a> {
                 },
                 false,
             )?
-            .ok_or(ErrorKind::Corruption(Corruption::InvalidSyncedRoots))?;
+            .ok_or(PlacesInternalError::Corruption(
+                Corruption::InvalidSyncedRoots,
+            ))?;
         builder.reparent_orphans_to(&dogear::UNFILED_GUID);
 
         let sql = format!(

components/places/src/bookmark_sync/incoming.rs

@@ -52,9 +52,9 @@ impl<'a> IncomingApplicator<'a> {
                 Some("livemark") => self.store_incoming_livemark(timestamp, &value)?,
                 Some("separator") => self.store_incoming_sep(timestamp, &value)?,
                 _ => {
-                    return Err(
-                        ErrorKind::UnsupportedIncomingBookmarkType(value["type"].clone()).into(),
-                    )
+                    return Err(PlacesInternalError::UnsupportedIncomingBookmarkType(
+                        value["type"].clone(),
+                    ))
                 }
             };
         }
@@ -179,9 +179,9 @@ impl<'a> IncomingApplicator<'a> {
                         v.as_str().unwrap().into(),
                     ));
                 } else {
-                    return Err(
-                        ErrorKind::InvalidPlaceInfo(InvalidPlaceInfo::InvalidChildGuid).into(),
-                    );
+                    return Err(PlacesInternalError::InvalidPlaceInfo(
+                        InvalidPlaceInfo::InvalidChildGuid,
+                    ));
                 }
             }
             children
@@ -489,7 +489,9 @@ impl<'a> IncomingApplicator<'a> {
     fn maybe_store_url(&self, url: Option<Url>) -> Result<Url> {
         if let Some(url) = url {
             if url.as_str().len() > URL_LENGTH_MAX {
-                return Err(ErrorKind::InvalidPlaceInfo(InvalidPlaceInfo::UrlTooLong).into());
+                return Err(PlacesInternalError::InvalidPlaceInfo(
+                    InvalidPlaceInfo::UrlTooLong,
+                ));
             }
             self.db.execute_cached(
                 "INSERT OR IGNORE INTO moz_places(guid, url, url_hash, frecency)
@@ -502,7 +504,9 @@ impl<'a> IncomingApplicator<'a> {
             )?;
             Ok(url)
         } else {
-            Err(ErrorKind::InvalidPlaceInfo(InvalidPlaceInfo::NoUrl).into())
+            Err(PlacesInternalError::InvalidPlaceInfo(
+                InvalidPlaceInfo::NoUrl,
+            ))
         }
     }
 }
@@ -511,7 +515,9 @@ fn unpack_id(key: &str, data: &JsonValue) -> Result<BookmarkRecordId> {
     if let Some(id) = data[key].as_str() {
         Ok(BookmarkRecordId::from_payload_id(id.into()))
     } else {
-        Err(ErrorKind::InvalidPlaceInfo(InvalidPlaceInfo::InvalidGuid).into())
+        Err(PlacesInternalError::InvalidPlaceInfo(
+            InvalidPlaceInfo::InvalidGuid,
+        ))
     }
 }
 
@@ -922,12 +928,11 @@ mod tests {
         match applicator
             .apply_payload(payload, ServerTimestamp(0))
             .expect_err("Should not apply record with unknown type")
-            .kind()
         {
-            ErrorKind::UnsupportedIncomingBookmarkType(t) => {
+            PlacesInternalError::UnsupportedIncomingBookmarkType(t) => {
                 assert_eq!(t.as_str().unwrap(), "fancy")
             }
-            kind => panic!("Wrong error kind for unknown type: {:?}", kind),
+            error => panic!("Wrong error variant for unknown type: {:?}", error),
         }
     }
 }

components/places/src/bookmark_sync/mod.rs

@@ -35,7 +35,7 @@ impl SyncedBookmarkKind {
             3 => Ok(SyncedBookmarkKind::Folder),
             4 => Ok(SyncedBookmarkKind::Livemark),
             5 => Ok(SyncedBookmarkKind::Separator),
-            _ => Err(ErrorKind::UnsupportedSyncedBookmarkKind(v).into()),
+            _ => Err(PlacesInternalError::UnsupportedSyncedBookmarkKind(v)),
         }
     }
 }
@@ -97,7 +97,7 @@ impl SyncedBookmarkValidity {
             1 => Ok(SyncedBookmarkValidity::Valid),
             2 => Ok(SyncedBookmarkValidity::Reupload),
             3 => Ok(SyncedBookmarkValidity::Replace),
-            _ => Err(ErrorKind::UnsupportedSyncedBookmarkValidity(v).into()),
+            _ => Err(PlacesInternalError::UnsupportedSyncedBookmarkValidity(v)),
         }
     }
 }