From e925a832309b92b1738e8a932fa36a77e390ec6a Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Tue, 6 Jun 2023 15:46:37 -0400 Subject: [PATCH 1/4] Add tests which verifies some places behavior. When deleting a bookmark that has no visits, that origin will remain in moz_origins until a complete history wipe is done. This is the same behaviour as desktop, sadly considered a bug, and described in bug 1650511. --- components/places/Cargo.toml | 1 + .../places/sql/create_shared_triggers.sql | 9 +- components/places/src/api/matcher.rs | 1 + components/places/src/db/db.rs | 1 + components/places/src/db/schema.rs | 36 +++++ components/places/src/storage/bookmarks.rs | 10 ++ components/places/src/storage/mod.rs | 135 ++++++++++++++++++ 7 files changed, 192 insertions(+), 1 deletion(-) diff --git a/components/places/Cargo.toml b/components/places/Cargo.toml index 9ee26570db..9ba3254782 100644 --- a/components/places/Cargo.toml +++ b/components/places/Cargo.toml @@ -43,6 +43,7 @@ features = ["functions", "window", "bundled", "unlock_notify"] pretty_assertions = "0.6" tempfile = "3.1" env_logger = {version = "0.7", default-features = false} +sql-support = { path = "../support/sql" } [build-dependencies] uniffi = { version = "0.23", features = ["build"] } diff --git a/components/places/sql/create_shared_triggers.sql b/components/places/sql/create_shared_triggers.sql index 4002cceadc..df2a356657 100644 --- a/components/places/sql/create_shared_triggers.sql +++ b/components/places/sql/create_shared_triggers.sql @@ -4,13 +4,20 @@ -- This file defines triggers shared between the main and Sync connections. +-- markh doesn't understand why this trigger is needed. It seems to be doing roughly +-- the same work as moz_updateoriginsinsert_afterdelete_trigger and so we have 2 very +-- similar triggers for the same insert. A difference between them though is some +-- frecency stuff - and if you remove this filter, all tests pass except one obscure +-- one in matcher.rs, where a test site ends up with a frecency of zero rather than the +-- expected 1999. Both triggers exist in desktop too. +-- So if anyone can explain this, please update this comment :) CREATE TEMP TRIGGER moz_places_afterinsert_trigger AFTER INSERT ON moz_places FOR EACH ROW BEGIN INSERT OR IGNORE INTO moz_origins(prefix, host, rev_host, frecency) VALUES(get_prefix(NEW.url), get_host_and_port(NEW.url), reverse_host(get_host_and_port(NEW.url)), NEW.frecency); - -- This is temporary. + -- This is temporary (well, given that was written in 2018, as of 2023 I think we can declate it's not :) UPDATE moz_places SET origin_id = (SELECT id FROM moz_origins WHERE prefix = get_prefix(NEW.url) AND diff --git a/components/places/src/api/matcher.rs b/components/places/src/api/matcher.rs index bfd2eb4972..d2376b5ca4 100644 --- a/components/places/src/api/matcher.rs +++ b/components/places/src/api/matcher.rs @@ -827,6 +827,7 @@ mod tests { [], ) .expect("should insert"); + crate::storage::delete_pending_temp_tables(&conn).expect("should work"); let _ = search_frecent( &conn, diff --git a/components/places/src/db/db.rs b/components/places/src/db/db.rs index a33879863a..e574aed588 100644 --- a/components/places/src/db/db.rs +++ b/components/places/src/db/db.rs @@ -108,6 +108,7 @@ impl ConnectionInitializer for PlacesInitializer { "; conn.execute_batch(initial_pragmas)?; define_functions(conn, self.api_id)?; + sql_support::debug_tools::define_debug_functions(conn)?; conn.set_prepared_statement_cache_capacity(128); Ok(()) } diff --git a/components/places/src/db/schema.rs b/components/places/src/db/schema.rs index 28f19307f8..32a46b81a6 100644 --- a/components/places/src/db/schema.rs +++ b/components/places/src/db/schema.rs @@ -757,6 +757,42 @@ mod tests { .expect_err("changing the guid should fail"); } + #[test] + fn test_origin_triggers_simple_removal() { + let conn = PlacesDb::open_in_memory(ConnectionType::ReadWrite).expect("no memory db"); + let guid = SyncGuid::random(); + let url = String::from(Url::parse("http://example.com").expect("valid url")); + + conn.execute( + "INSERT INTO moz_places (guid, url, url_hash) VALUES (:guid, :url, hash(:url))", + rusqlite::named_params! { + ":guid": &guid, + ":url": &url, + }, + ) + .expect("should work"); + // origins are maintained via triggers, so make sure they are done. + crate::storage::delete_pending_temp_tables(&conn).expect("should work"); + + // We should have inserted the origin. + assert_eq!( + select_simple_int( + &conn, + "SELECT count(*) FROM moz_origins WHERE host = 'example.com'" + ), + 1 + ); + + // delete the place, ensure triggers have run, and check origin has gone. + conn.execute("DELETE FROM moz_places", []) + .expect("should work"); + crate::storage::delete_pending_temp_tables(&conn).expect("should work"); + assert_eq!( + select_simple_int(&conn, "SELECT count(*) FROM moz_origins"), + 0 + ); + } + const CREATE_V15_DB: &str = include_str!("../../sql/tests/create_v15_db.sql"); #[test] diff --git a/components/places/src/storage/bookmarks.rs b/components/places/src/storage/bookmarks.rs index d9ead6ffb0..27ed7c031b 100644 --- a/components/places/src/storage/bookmarks.rs +++ b/components/places/src/storage/bookmarks.rs @@ -1235,6 +1235,16 @@ mod tests { assert_eq!(get_pos(&conn, &guid3), 1); assert!(global_change_tracker.changed()); + // Should be no moz_origin entries left for the deleted bookmarks. + // For better or worse, origins are only cleaned up after history is cleared. + crate::storage::history::delete_everything(&conn)?; + assert_eq!( + conn.query_one::( + "SELECT COUNT(*) FROM moz_origins WHERE host='www.example2.com';" + )?, + 0 + ); + Ok(()) } diff --git a/components/places/src/storage/mod.rs b/components/places/src/storage/mod.rs index 8bf0844d3b..ac4c5d48dc 100644 --- a/components/places/src/storage/mod.rs +++ b/components/places/src/storage/mod.rs @@ -393,6 +393,12 @@ pub fn delete_pending_temp_tables(conn: &PlacesDb) -> Result<()> { mod tests { use super::*; use crate::api::places_api::test::new_mem_connection; + use crate::observation::VisitObservation; + use bookmarks::{ + delete_bookmark, insert_bookmark, BookmarkPosition, BookmarkRootGuid, InsertableBookmark, + InsertableItem, + }; + use history::apply_observation; #[test] fn test_meta() { @@ -415,4 +421,133 @@ mod tests { .is_none()); delete_meta(&conn, "foo").expect("delete non-existing should work"); } + + // Here we try and test that we replicate desktop behaviour, which isn't that obvious. + // * create a bookmark + // * remove the bookmark - this doesn't remove the place or origin - probably because in + // real browsers there will be visits for the URL existing, but this still smells like + // a bug - see https://bugzilla.mozilla.org/show_bug.cgi?id=1650511#c34 + // * Arrange for history for that item to be removed, via various means + // At this point the origin and place should be removed. The only code (in desktop and here) which + // removes places with a foreign_count of zero is that history removal! + + #[test] + fn test_removal_delete_visits_between() { + do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { + Ok(history::delete_visits_between( + conn, + Timestamp::EARLIEST, + Timestamp::now(), + )?) + }) + } + + #[test] + fn test_removal_delete_visits_for() { + do_test_removal_places_and_origins(|conn: &PlacesDb, guid: &SyncGuid| { + Ok(history::delete_visits_for(conn, guid)?) + }) + } + + #[test] + fn test_removal_prune() { + do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { + Ok(history::prune_older_visits(conn)?) + }) + } + + #[test] + fn test_removal_visit_at_time() { + do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { + let url = Url::parse("http://example.com/foo").unwrap(); + let visit = Timestamp::from(727_747_200_001); + Ok(history::delete_place_visit_at_time(conn, &url, visit)?) + }) + } + + #[test] + fn test_removal_everything() { + do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { + Ok(history::delete_everything(conn)?) + }) + } + + // The core test - takes a function which deletes history. + fn do_test_removal_places_and_origins(removal_fn: F) + where + F: FnOnce(&PlacesDb, &SyncGuid) -> Result<()>, + { + let conn = new_mem_connection(); + let url = Url::parse("http://example.com/foo").unwrap(); + let bm = InsertableItem::Bookmark { + b: InsertableBookmark { + parent_guid: BookmarkRootGuid::Unfiled.into(), + position: BookmarkPosition::Append, + date_added: None, + last_modified: None, + guid: None, + url: url.clone(), + title: Some("the title".into()), + }, + }; + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_bookmarks;") + .unwrap(), + 5 + ); // our 5 roots. + let bookmark_guid = insert_bookmark(&conn, bm).unwrap(); + let place_guid = fetch_page_info(&conn, &url) + .expect("should work") + .expect("must exist") + .page + .guid; + // the place should exist with a foreign_count of 1. + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_bookmarks;") + .unwrap(), + 6 + ); // our 5 roots + new bookmark + assert_eq!( + conn.query_one::( + "SELECT foreign_count FROM moz_places WHERE url = \"http://example.com/foo\";" + ) + .unwrap(), + 1 + ); + // visit the bookmark. + assert!(apply_observation( + &conn, + VisitObservation::new(url) + .with_at(Timestamp::from(727_747_200_001)) + .with_visit_type(VisitTransition::Link) + ) + .unwrap() + .is_some()); + + delete_bookmark(&conn, &bookmark_guid).unwrap(); + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_bookmarks;") + .unwrap(), + 5 + ); // our 5 roots + // the place should have no foreign references, but still exists. + assert_eq!( + conn.query_one::( + "SELECT foreign_count FROM moz_places WHERE url = \"http://example.com/foo\";" + ) + .unwrap(), + 0 + ); + removal_fn(&conn, &place_guid).expect("removal function should work"); + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_places;") + .unwrap(), + 0 + ); + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_origins;") + .unwrap(), + 0 + ); + } } From 8551c84601b699f5556f0c8a406041b63c4300cc Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Wed, 7 Jun 2023 10:41:21 -0400 Subject: [PATCH 2/4] New trigger so that places/origins are deleted when bookmarks are in some cases --- .../places/sql/create_shared_triggers.sql | 14 ++++ components/places/src/db/schema.rs | 74 ++++++++++++++++++- components/places/src/storage/mod.rs | 56 ++++++++++++++ 3 files changed, 143 insertions(+), 1 deletion(-) diff --git a/components/places/sql/create_shared_triggers.sql b/components/places/sql/create_shared_triggers.sql index df2a356657..3c2e868443 100644 --- a/components/places/sql/create_shared_triggers.sql +++ b/components/places/sql/create_shared_triggers.sql @@ -268,6 +268,20 @@ BEGIN WHERE id = OLD.placeId; END; +-- Similar to cleanup_pages, if the origin/place remains with no foreign references +-- and no visits it should be deleted. +-- This approach may not be suitable for desktop but seems to be for us - see +-- https://bugzilla.mozilla.org/show_bug.cgi?id=1650511#c41 for more discussion. +CREATE TEMP TRIGGER moz_cleanup_origin_bookmark_deleted_trigger +AFTER DELETE ON moz_bookmarks +BEGIN + DELETE FROM moz_places + WHERE id = OLD.fk + AND foreign_count = 0 + AND last_visit_date_local = 0 + AND last_visit_date_remote = 0; +END; + -- These triggers adjust the foreign count for tagged URLs, and bump the -- tag's last modified time when a URL is tagged or untagged. These are -- split out from the main connection's tag triggers because we also want diff --git a/components/places/src/db/schema.rs b/components/places/src/db/schema.rs index 32a46b81a6..4654c2875e 100644 --- a/components/places/src/db/schema.rs +++ b/components/places/src/db/schema.rs @@ -503,6 +503,20 @@ mod tests { assert_eq!(get_foreign_count(&conn, &guid1), 0); assert_eq!(get_foreign_count(&conn, &guid2), 0); + // record visits for both URLs, otherwise the place itself will be removed with the bookmark + conn.execute_cached( + "INSERT INTO moz_historyvisits (place_id, visit_date, visit_type, is_local) + VALUES (:place, 10000000, 1, 0);", + &[(":place", &place_id1)], + ) + .expect("should work"); + conn.execute_cached( + "INSERT INTO moz_historyvisits (place_id, visit_date, visit_type, is_local) + VALUES (:place, 10000000, 1, 1);", + &[(":place", &place_id2)], + ) + .expect("should work"); + // create a bookmark pointing at it. conn.execute_cached( "INSERT INTO moz_bookmarks @@ -642,7 +656,65 @@ mod tests { 0 ); - // Place should remain. + // Place should also be gone as bookmark url had no visits. + assert_eq!( + select_simple_int( + &conn, + "SELECT COUNT(*) from moz_places WHERE guid = 'place_guid__';" + ), + 0 + ); + } + + #[test] + fn test_bookmark_auto_deletes_place_remains() { + let conn = PlacesDb::open_in_memory(ConnectionType::ReadWrite).expect("no memory db"); + + conn.execute_all(&[ + // A folder to hold a bookmark. + "INSERT INTO moz_bookmarks + (type, parent, position, dateAdded, lastModified, guid) + VALUES + (3, 1, 0, 1, 1, 'folder_guid_')", + // A place for the bookmark. + "INSERT INTO moz_places + (guid, url, url_hash, foreign_count) -- here we pretend it has a foreign count. + VALUES ('place_guid__', 'http://example.com/', hash('http://example.com/'), 1)", + // The bookmark. + "INSERT INTO moz_bookmarks + (fk, type, parent, position, dateAdded, lastModified, guid) + VALUES + --fk, type + (last_insert_rowid(), 1, + -- parent + (SELECT id FROM moz_bookmarks WHERE guid = 'folder_guid_'), + -- positon, dateAdded, lastModified, guid + 0, 1, 1, 'bookmarkguid')", + ]) + .expect("inserts should work"); + + // Delete the folder - the bookmark should cascade delete. + conn.execute("DELETE FROM moz_bookmarks WHERE guid = 'folder_guid_';", []) + .expect("should work"); + + // folder should be gone. + assert_eq!( + select_simple_int( + &conn, + "SELECT count(*) FROM moz_bookmarks WHERE guid = 'folder_guid_'" + ), + 0 + ); + // bookmark should be gone. + assert_eq!( + select_simple_int( + &conn, + "SELECT count(*) FROM moz_bookmarks WHERE guid = 'bookmarkguid';" + ), + 0 + ); + + // Place should remain as we pretended it has a foreign reference. assert_eq!( select_simple_int( &conn, diff --git a/components/places/src/storage/mod.rs b/components/places/src/storage/mod.rs index ac4c5d48dc..80136610ea 100644 --- a/components/places/src/storage/mod.rs +++ b/components/places/src/storage/mod.rs @@ -550,4 +550,60 @@ mod tests { 0 ); } + + // Similar to the above, but if the bookmark has no visits the place/origin should die + // without requiring history removal + #[test] + fn test_visitless_removal_places_and_origins() { + let conn = new_mem_connection(); + let url = Url::parse("http://example.com/foo").unwrap(); + let bm = InsertableItem::Bookmark { + b: InsertableBookmark { + parent_guid: BookmarkRootGuid::Unfiled.into(), + position: BookmarkPosition::Append, + date_added: None, + last_modified: None, + guid: None, + url: url.clone(), + title: Some("the title".into()), + }, + }; + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_bookmarks;") + .unwrap(), + 5 + ); // our 5 roots. + let bookmark_guid = insert_bookmark(&conn, bm).unwrap(); + // the place should exist with a foreign_count of 1. + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_bookmarks;") + .unwrap(), + 6 + ); // our 5 roots + new bookmark + assert_eq!( + conn.query_one::( + "SELECT foreign_count FROM moz_places WHERE url = \"http://example.com/foo\";" + ) + .unwrap(), + 1 + ); + // Delete it. + delete_bookmark(&conn, &bookmark_guid).unwrap(); + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_bookmarks;") + .unwrap(), + 5 + ); // our 5 roots + // should be gone from places and origins. + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_places;") + .unwrap(), + 0 + ); + assert_eq!( + conn.query_one::("SELECT COUNT(*) FROM moz_origins;") + .unwrap(), + 0 + ); + } } From 5004879e405d5974b221dc3a322e9873741a27b2 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Fri, 9 Jun 2023 14:54:15 -0400 Subject: [PATCH 3/4] clippy/fmt --- components/places/src/storage/mod.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/components/places/src/storage/mod.rs b/components/places/src/storage/mod.rs index 80136610ea..007c6405fb 100644 --- a/components/places/src/storage/mod.rs +++ b/components/places/src/storage/mod.rs @@ -434,25 +434,21 @@ mod tests { #[test] fn test_removal_delete_visits_between() { do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { - Ok(history::delete_visits_between( - conn, - Timestamp::EARLIEST, - Timestamp::now(), - )?) + history::delete_visits_between(conn, Timestamp::EARLIEST, Timestamp::now()) }) } #[test] fn test_removal_delete_visits_for() { do_test_removal_places_and_origins(|conn: &PlacesDb, guid: &SyncGuid| { - Ok(history::delete_visits_for(conn, guid)?) + history::delete_visits_for(conn, guid) }) } #[test] fn test_removal_prune() { do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { - Ok(history::prune_older_visits(conn)?) + history::prune_older_visits(conn) }) } @@ -461,14 +457,14 @@ mod tests { do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { let url = Url::parse("http://example.com/foo").unwrap(); let visit = Timestamp::from(727_747_200_001); - Ok(history::delete_place_visit_at_time(conn, &url, visit)?) + history::delete_place_visit_at_time(conn, &url, visit) }) } #[test] fn test_removal_everything() { do_test_removal_places_and_origins(|conn: &PlacesDb, _guid: &SyncGuid| { - Ok(history::delete_everything(conn)?) + history::delete_everything(conn) }) } @@ -564,7 +560,7 @@ mod tests { date_added: None, last_modified: None, guid: None, - url: url.clone(), + url, title: Some("the title".into()), }, }; @@ -594,7 +590,7 @@ mod tests { .unwrap(), 5 ); // our 5 roots - // should be gone from places and origins. + // should be gone from places and origins. assert_eq!( conn.query_one::("SELECT COUNT(*) FROM moz_places;") .unwrap(), From 482f1f9cb2334281a93fd0fdc63b8c9d2de0c180 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Thu, 15 Jun 2023 10:05:45 -0400 Subject: [PATCH 4/4] Lina's comments --- .../places/sql/create_shared_triggers.sql | 22 ------------------- components/places/src/api/matcher.rs | 2 +- components/places/src/storage/bookmarks.rs | 3 --- components/places/src/storage/mod.rs | 6 ++--- 4 files changed, 4 insertions(+), 29 deletions(-) diff --git a/components/places/sql/create_shared_triggers.sql b/components/places/sql/create_shared_triggers.sql index 3c2e868443..df649d2553 100644 --- a/components/places/sql/create_shared_triggers.sql +++ b/components/places/sql/create_shared_triggers.sql @@ -4,28 +4,6 @@ -- This file defines triggers shared between the main and Sync connections. --- markh doesn't understand why this trigger is needed. It seems to be doing roughly --- the same work as moz_updateoriginsinsert_afterdelete_trigger and so we have 2 very --- similar triggers for the same insert. A difference between them though is some --- frecency stuff - and if you remove this filter, all tests pass except one obscure --- one in matcher.rs, where a test site ends up with a frecency of zero rather than the --- expected 1999. Both triggers exist in desktop too. --- So if anyone can explain this, please update this comment :) -CREATE TEMP TRIGGER moz_places_afterinsert_trigger -AFTER INSERT ON moz_places FOR EACH ROW -BEGIN - INSERT OR IGNORE INTO moz_origins(prefix, host, rev_host, frecency) - VALUES(get_prefix(NEW.url), get_host_and_port(NEW.url), reverse_host(get_host_and_port(NEW.url)), NEW.frecency); - - -- This is temporary (well, given that was written in 2018, as of 2023 I think we can declate it's not :) - UPDATE moz_places SET - origin_id = (SELECT id FROM moz_origins - WHERE prefix = get_prefix(NEW.url) AND - host = get_host_and_port(NEW.url) AND - rev_host = reverse_host(get_host_and_port(NEW.url))) - WHERE id = NEW.id; -END; - -- Note that while we create tombstones manually, we rely on this trigger to -- delete any which might exist when a new record is written to moz_places. CREATE TEMP TRIGGER moz_places_afterinsert_trigger_tombstone diff --git a/components/places/src/api/matcher.rs b/components/places/src/api/matcher.rs index d2376b5ca4..4d0654d9f2 100644 --- a/components/places/src/api/matcher.rs +++ b/components/places/src/api/matcher.rs @@ -745,7 +745,7 @@ mod tests { url: Url::parse("http://example.com/").unwrap(), title: "example.com/".into(), icon_url: None, - frecency: 1999, + frecency: 2000, reasons: vec![MatchReason::Origin], }] ); diff --git a/components/places/src/storage/bookmarks.rs b/components/places/src/storage/bookmarks.rs index 27ed7c031b..c67908bb2c 100644 --- a/components/places/src/storage/bookmarks.rs +++ b/components/places/src/storage/bookmarks.rs @@ -1235,9 +1235,6 @@ mod tests { assert_eq!(get_pos(&conn, &guid3), 1); assert!(global_change_tracker.changed()); - // Should be no moz_origin entries left for the deleted bookmarks. - // For better or worse, origins are only cleaned up after history is cleared. - crate::storage::history::delete_everything(&conn)?; assert_eq!( conn.query_one::( "SELECT COUNT(*) FROM moz_origins WHERE host='www.example2.com';" diff --git a/components/places/src/storage/mod.rs b/components/places/src/storage/mod.rs index 007c6405fb..63fbde7ebf 100644 --- a/components/places/src/storage/mod.rs +++ b/components/places/src/storage/mod.rs @@ -382,9 +382,9 @@ pub(crate) fn delete_meta(db: &PlacesDb, key: &str) -> Result<()> { /// Delete all items in the temp tables we use for staging changes. pub fn delete_pending_temp_tables(conn: &PlacesDb) -> Result<()> { conn.execute_batch( - "DELETE FROM moz_updateoriginsupdate_temp; - DELETE FROM moz_updateoriginsdelete_temp; - DELETE FROM moz_updateoriginsinsert_temp;", + "DELETE FROM moz_updateoriginsinsert_temp; + DELETE FROM moz_updateoriginsupdate_temp; + DELETE FROM moz_updateoriginsdelete_temp;", )?; Ok(()) }