Ensure places/origins are removed when a bookmark with no visits etc is removed.

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"] }

components/places/sql/create_shared_triggers.sql

@@ -4,21 +4,6 @@
 
 -- This file defines triggers shared between the main and Sync connections.
 
-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.
-    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
@@ -261,6 +246,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

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],
             }]
         );
@@ -827,6 +827,7 @@ mod tests {
             [],
         )
         .expect("should insert");
+        crate::storage::delete_pending_temp_tables(&conn).expect("should work");
 
         let _ = search_frecent(
             &conn,

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(())
     }

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,
@@ -757,6 +829,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]

components/places/src/storage/bookmarks.rs

@@ -1235,6 +1235,13 @@ mod tests {
         assert_eq!(get_pos(&conn, &guid3), 1);
         assert!(global_change_tracker.changed());
 
+        assert_eq!(
+            conn.query_one::<i64>(
+                "SELECT COUNT(*) FROM moz_origins WHERE host='www.example2.com';"
+            )?,
+            0
+        );
+
         Ok(())
     }