diff --git a/components/places/src/ffi.rs b/components/places/src/ffi.rs index a116719f9..3783c352e 100644 --- a/components/places/src/ffi.rs +++ b/components/places/src/ffi.rs @@ -22,7 +22,7 @@ use crate::ConnectionType; use crate::VisitObservation; use crate::VisitTransition; use crate::{PlacesApi, PlacesDb}; -use error_support::{handle_error, report_error}; +use error_support::handle_error; use interrupt_support::{register_interrupt, SqlInterruptHandle}; use parking_lot::Mutex; use std::sync::{Arc, Weak}; @@ -114,51 +114,11 @@ lazy_static::lazy_static! { static ref SYNC_CONNECTIONS: Mutex>> = Mutex::new(Vec::new()); } -fn check_connection_count(conn_type: ConnectionType, conn: &Arc) { - match conn_type { - ConnectionType::ReadWrite => { - check_connection_count_inner( - &mut READ_WRITE_CONNECTIONS.lock(), - conn, - "MultiplePlacesReadWriteConnections", - ); - } - ConnectionType::Sync => { - check_connection_count_inner( - &mut SYNC_CONNECTIONS.lock(), - conn, - "MultiplePlacesSyncConnections", - ); - } - ConnectionType::ReadOnly => {} - }; -} - -fn check_connection_count_inner( - connections: &mut Vec>, - new_connection: &Arc, - error_str: &'static str, -) { - let mut i = 0; - while i < connections.len() { - if Weak::strong_count(&connections[i]) == 0 { - connections.swap_remove(i); - } else { - i += 1; - } - } - connections.push(Arc::downgrade(new_connection)); - if connections.len() > 1 { - error_support::report_error!(error_str, "{} connections", connections.len()); - } -} - impl PlacesApi { #[handle_error] fn new_connection(&self, conn_type: ConnectionType) -> ApiResult> { let db = self.open_connection(conn_type)?; let connection = Arc::new(PlacesConnection::new(db)); - check_connection_count(conn_type, &connection); register_interrupt(Arc::::downgrade(&connection)); Ok(connection) } @@ -448,18 +408,7 @@ impl PlacesConnection { // further syncing of older data #[handle_error] fn delete_everything_history(&self) -> ApiResult<()> { - // Do some extra work to track down #4856 - let conn = self.db.lock(); - let result = history::delete_everything(&conn); - if let Err(e) = &result { - if matches!( - e, - crate::error::Error::SqlError(rusqlite::Error::QueryReturnedNoRows) - ) { - report_error!("SqlErrorQueryReturnedNoRows", "{}", e); - } - } - result + history::delete_everything(&self.db.lock()) } // XXX - This just calls wipe_local under the hood... diff --git a/components/places/src/storage/history.rs b/components/places/src/storage/history.rs index a089eeb76..a4e551974 100644 --- a/components/places/src/storage/history.rs +++ b/components/places/src/storage/history.rs @@ -19,7 +19,6 @@ use crate::storage::{ }; use crate::types::{SyncStatus, VisitTransition, VisitTransitionSet}; use actions::*; -use error_support::breadcrumb; use rusqlite::types::ToSql; use rusqlite::Result as RusqliteResult; use rusqlite::Row; @@ -42,12 +41,10 @@ static DELETION_HIGH_WATER_MARK_META_KEY: &str = "history_deleted_hwm"; /// Returns the RowId of a new visit in moz_historyvisits, or None if no new visit was added. pub fn apply_observation(db: &PlacesDb, visit_ob: VisitObservation) -> Result> { - breadcrumb!("apply_observation: begin_transaction"); let tx = db.begin_transaction()?; let result = apply_observation_direct(db, visit_ob)?; delete_pending_temp_tables(db)?; tx.commit()?; - breadcrumb!("apply_observation: commit"); Ok(result) } @@ -360,8 +357,6 @@ fn insert_tombstone_for_page(db: &PlacesDb, guid: &SyncGuid) -> Result<()> { /// Deletes a page. Note that this throws a constraint violation if the page is /// bookmarked, or has a keyword or tags. fn delete_page(db: &PlacesDb, page_id: RowId) -> Result<()> { - // breadcrumb to track down #4856 - breadcrumb!("delete places page: {}", page_id); db.execute_cached( "DELETE FROM moz_places WHERE id = :page_id", @@ -373,21 +368,17 @@ fn delete_page(db: &PlacesDb, page_id: RowId) -> Result<()> { /// Deletes all visits for a page given its GUID, creating tombstones if /// necessary. pub fn delete_visits_for(db: &PlacesDb, guid: &SyncGuid) -> Result<()> { - breadcrumb!("delete_visits_for: begin_transaction"); let tx = db.begin_transaction()?; let result = delete_visits_for_in_tx(db, guid); tx.commit()?; - breadcrumb!("delete_visits_for: commit"); result } /// Delete all visits in a date range. pub fn delete_visits_between(db: &PlacesDb, start: Timestamp, end: Timestamp) -> Result<()> { - breadcrumb!("delete_visits_between: begin_transaction"); let tx = db.begin_transaction()?; delete_visits_between_in_tx(db, start, end)?; tx.commit()?; - breadcrumb!("delete_visits_between: commit"); Ok(()) } @@ -400,11 +391,9 @@ pub fn delete_place_visit_at_time_by_href( place: &str, visit: Timestamp, ) -> Result<()> { - breadcrumb!("delete_place_visit_at_time_by_href: begin_transaction"); let tx = db.begin_transaction()?; delete_place_visit_at_time_in_tx(db, place, visit)?; tx.commit()?; - breadcrumb!("delete_place_visit_at_time_by_href: commit"); Ok(()) } @@ -414,7 +403,6 @@ pub fn prune_destructively(db: &PlacesDb) -> Result<()> { } pub fn prune_older_visits(db: &PlacesDb) -> Result<()> { - breadcrumb!("prune_older_visits: begin_transaction"); let tx = db.begin_transaction()?; // Prune 6 items at a time, which matches desktops "small limit" value let limit: usize = 6; @@ -424,7 +412,6 @@ pub fn prune_older_visits(db: &PlacesDb) -> Result<()> { db_actions_from_visits_to_delete(find_visits_to_prune(db, limit, Timestamp::now())?), ); tx.commit()?; - breadcrumb!("prune_older_visits: commit"); result } @@ -504,11 +491,9 @@ fn find_exotic_visits_to_prune( } pub fn wipe_local(db: &PlacesDb) -> Result<()> { - breadcrumb!("apply_observation: begin_transaction"); let tx = db.begin_transaction()?; wipe_local_in_tx(db)?; tx.commit()?; - breadcrumb!("apply_observation: commit"); // Note: SQLite cannot VACUUM within a transaction. db.execute_batch("VACUUM")?; Ok(()) @@ -516,8 +501,6 @@ pub fn wipe_local(db: &PlacesDb) -> Result<()> { fn wipe_local_in_tx(db: &PlacesDb) -> Result<()> { use crate::frecency::DEFAULT_FRECENCY_SETTINGS; - // breadcrumb to track down #4856 - breadcrumb!("places: wipe_local_in_tx"); db.execute_all(&[ "DELETE FROM moz_places WHERE foreign_count == 0", "DELETE FROM moz_places_metadata", @@ -555,8 +538,6 @@ fn wipe_local_in_tx(db: &PlacesDb) -> Result<()> { #[allow(unreachable_code)] pub fn delete_everything(db: &PlacesDb) -> Result<()> { - // breadcrumb to track down #4856 - breadcrumb!("places history delete_everything: begin transaction"); let tx = db.begin_transaction()?; // Remote visits could have a higher date than `now` if our clock is weird. @@ -580,8 +561,6 @@ pub fn delete_everything(db: &PlacesDb) -> Result<()> { reset_in_tx(db, &EngineSyncAssociation::Disconnected)?; tx.commit()?; - // breadcrumb to track down #4856 - breadcrumb!("places history delete_everything: commit"); // Note: SQLite cannot VACUUM within a transaction. db.execute_batch("VACUUM")?; @@ -711,8 +690,6 @@ impl PageToClean { /// are no more foreign keys such as bookmarks) or updating /// their frecency. fn cleanup_pages(db: &PlacesDb, pages: &[PageToClean]) -> Result<()> { - // breadcrumb to track down #4856 - breadcrumb!("places cleanup_pages()"); // desktop does this frecency work using a function in a single sql // statement - we should see if we can do that too. let frec_ids = pages @@ -1032,8 +1009,6 @@ pub mod history_sync { } pub fn apply_synced_deletion(db: &PlacesDb, guid: &SyncGuid) -> Result<()> { - // breadcrumb to track down #4856 - breadcrumb!("places apply_synced_deletion: {}", guid); db.execute_cached( "DELETE FROM moz_places WHERE guid = :guid", &[(":guid", guid)],