Bug 2084607 - reinstate recursive_triggers in Places by ddurst · Pull Request #7685 · mozilla/application-services

ddurst · 2026-10-23T18:39:17Z

For bug 2084607, this adds an explicit PRAGMA recursive_triggers = ON. Accordingly, also it adds the following:

a clarifying comment about moz_bookmarks_parent_afterdelete_trigger in sql/create_shared_triggers.sql
a test for the foreign_count decrement/increment in src/db/schema.rs
a test for the delete triggering in src/storage/bookmarks.rs

This restores the behavior for cascading deletions that were present with the previous FK, which were assumed to still be in effect.

Two more changes will be needed, addressed separately (handling existing erroneous orphans, and recalculating the moz_places.foreign_count).

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
n/a Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

ddurst · 2026-10-26T15:15:13Z

There's a followup patch that should land with this, I'll update momentarily.

ddurst · 2026-10-26T16:32:23Z

Added the migration from 20 => 21 to clean up any existing orphans that should be deleted, and recalculates foreign_count on what's left.

bendk

This looks right to me, with the possible exception of the comment I left about SyncStatus::normal. Mark should also take a look though, since he knows this code better than me.

bendk · 2026-10-26T16:57:07Z

+
+                 DELETE FROM moz_places
+                 WHERE foreign_count = 0 AND last_visit_date_local = 0 AND last_visit_date_remote = 0;",
+                 normal = SyncStatus::Normal as u8,


Why only SyncStatus::Normal? I think this bug could also happen for SyncStatus::new

Could you have a new one that's an existing orphan due to its grandparent being deleted?

This is where we need Mark, but I thought SyncStatus::New just meant that it hasn't been synced yet.

Oh. If that's what it means, it's probably ok since it should've been deleted?

Couldn't it have hit the same bug you're fixing now? I didn't think it was sync only, just that we only observed it during sync.

I misunderstood the question: so yeah, this could affect non-Sync bookmark trees, and since only Sync (iiuc) complains about it, we -- and the user -- would never know about it.

Yeah, agree that second check should not check Normal. The tombstone check for Normal is correct though.

I'm confused: the second check is the tombstone check? I thought Ben is suggesting removing line 377, and Mark is saying leave line 411?

(And now these are in different places)

Ah nevermind. I think you meant the constraint in the insertion to moz_bookmarks_deleted. Leaving this open so you can double-check.

mhammond

this is looking good to me!

mhammond · 2026-10-29T01:08:43Z

        let mut child_guids_by_parent_guid: HashMap<SyncGuid, Vec<dogear::Guid>> = HashMap::new();
+        // Items whose parent is missing can't be placed in the tree, so leave
+        // them and their descendants out rather than failing the merge.
+        let orphan_guids = self.fetch_local_orphan_guids()?;


IIUC, this patch should also be arranging for this to never actually be hit, right? If so, I wonder if this is valuable, and whether just allowing the same error we see today to continue is fine? While it could be argued this is a kind of defense-in-depth, I'm fairly sure there are other places where we also assume sqlite is enforcing our model and will fail where it does not?

(IOW, I'm wondering if paying the cost of the orphan check is actually worthwhile, and whether we will be able to deduce it working from existing telemetry in the same we we found this?)

This is only there in case something slips through (that way the error reported shows us that it's happening).

Given that we should see the "nonexistent parent" errors and places-sync-bookmarks-orphaned errors go to zero. So we could just watch those and see if they tick up again.

I'd include this check, because it's a safety net -- but if it's a performance drag and we think we'd see the signs of a similar thing happening again, I'm fine losing it.

Maybe we could put this in run_maintenance, where performance doesn't matter that much.

mhammond · 2026-10-29T01:17:44Z

+
+                 DELETE FROM moz_places
+                 WHERE foreign_count = 0 AND last_visit_date_local = 0 AND last_visit_date_remote = 0;",
+                 normal = SyncStatus::Normal as u8,


Yeah, agree that second check should not check Normal. The tombstone check for Normal is correct though.

ddurst · 2026-11-02T21:10:21Z

Sorry, another migration 20 beat this last week, so I had to update this to account for that.

…an up after orphan bookmarks mitigation

bendk

This is good from my POV. I left a couple questions/suggestions, but however you want to deal with those is fine with me.

bendk · 2026-11-03T14:26:47Z

        let mut child_guids_by_parent_guid: HashMap<SyncGuid, Vec<dogear::Guid>> = HashMap::new();
+        // Items whose parent is missing can't be placed in the tree, so leave
+        // them and their descendants out rather than failing the merge.
+        let orphan_guids = self.fetch_local_orphan_guids()?;


Maybe we could put this in run_maintenance, where performance doesn't matter that much.

bendk · 2026-11-03T14:35:40Z

+                 WHERE foreign_count > 0
+                   AND id NOT IN (SELECT place_id FROM placeForeignCounts)
+                   AND last_visit_date_local = 0 AND last_visit_date_remote = 0
+                   AND NOT EXISTS (SELECT 1 FROM moz_places_metadata m WHERE m.place_id = moz_places.id)",


This is probably me not understanding places, but it seems like placesForeignCounts is all we need to know if a bookmark should be garbage collected or not.

What would it mean for id NOT IN (SELECT place_id FROM placeForeignCounts) to be true, but either of the following clauses to be false?

The way I understand it, placesForeignCounts is all of the references to a places id (from bookmarks, keywords, or tags) -- so a history entry that has none of those things should have a foreign_count of 0. But one of the reasons to recalculate foreign_count is because the REPLACE INTO moz_bookmarks_synced statement while recursive triggers were off could've contributed to the foreign_count drifting high because the decrements that should've happened didn't (because delete triggers weren't firing). So if something doesn't have any references (isn't in placeForeignCounts), it could still have a nonzero value for last_visit_date_local or last_visit_date_remote.

The metadata one is similar in that it's not a reference either, but technically could be created without an actual visit.

So these checks are here to ensure we don't delete something that legitimately could exist -- whereas anything that's in zeroedPlaces has an erroneous foreign_count and it's not about to be fixed by the recalculation that follows.

ddurst requested review from a team, bendk and mhammond and removed request for a team October 23, 2026 18:43

ddurst marked this pull request as ready for review October 26, 2026 16:33

bendk requested changes Oct 26, 2026

View reviewed changes

ddurst force-pushed the main branch from b5ec704 to 1db17da Compare October 26, 2026 17:36

mhammond reviewed Oct 29, 2026

View reviewed changes

Bug 2084607 - Reinstate Places pragma recursive_triggers to ON

0d7a4c2

ddurst force-pushed the main branch from 1db17da to 28ad2b9 Compare November 2, 2026 21:09

Bug 2084607 - Add migration and recalculation of foreign_count to cle…

2e3e659

…an up after orphan bookmarks mitigation

ddurst force-pushed the main branch from 28ad2b9 to 2e3e659 Compare November 2, 2026 21:38

ddurst requested review from bendk and mhammond November 2, 2026 21:40

bendk approved these changes Nov 3, 2026

View reviewed changes

Conversation

ddurst commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

Uh oh!

ddurst commented Oct 26, 2026

Uh oh!

ddurst commented Oct 26, 2026

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddurst commented Nov 2, 2026

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddurst Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ddurst commented Oct 23, 2026 •

edited

Loading

ddurst Nov 3, 2026 •

edited

Loading