Skip to content

Bug 2039791 - Filter orphaned and non-folder-parent structure rows from fetch_remote_tree #7375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mhammond wants to merge 1 commit into
base: main
Choose a base branch
from
+101 −25
Open

Bug 2039791 - Filter orphaned and non-folder-parent structure rows from fetch_remote_tree #7375

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 101 additions & 25 deletions components/places/src/bookmark_sync/engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1641,11 +1641,88 @@ impl dogear::Store for Merger<'_> {
}
}

// An "endpoint" here means the guid/parent in a `moz_bookmarks_synced_structure` row.
// There are *possibly* schema changes here which would make some of these states unrepresentable.
// eg, no FK relationship between `moz_bookmarks_synced_structure` and `moz_bookmarks_synced(guid)`,
// and a trigger to reject non-parent items. However, other existing code should already be
// enforcing these invariants.
// Working out which shape(s) are occurring in the wild might help us narrow down where the
// defect is.
//
// So we check for and report structure rows with bad endpoints before skipping them.
let folder_kind = SyncedBookmarkKind::Folder as u8;
let root_guid_str = BookmarkRootGuid::Root.as_guid();
let (child_tombstoned, child_absent, parent_tombstoned, parent_absent, non_folder_parent):
(bool, bool, bool, bool, bool) = self.db.query_row(
&format!(
Copy link
Copy Markdown
Contributor

@skhamis skhamis May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the exists here we could potentially use MAX that way we don't need the 5 exists. This would reduce the amount of scans this query would be doing.

SELECT
  MAX(child.isDeleted IS 1),
  MAX(child.guid IS NULL),
  MAX(parent.isDeleted IS 1),
  MAX(parent.guid IS NULL),
  MAX(parent.isDeleted IS 0 AND parent.kind <> ?)
FROM moz_bookmarks_synced_structure s
LEFT JOIN moz_bookmarks_synced child  ON child.guid  = s.guid
LEFT JOIN moz_bookmarks_synced parent ON parent.guid = s.parentGuid
WHERE s.guid <> ?

Copy link
Copy Markdown
Contributor

@skhamis skhamis May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually while thinking about it more we could also just do:

SELECT
  SUM(child.isDeleted IS 1),
  SUM(child.guid IS NULL),
  SUM(parent.isDeleted IS 1),
  SUM(parent.guid IS NULL),
  SUM(parent.isDeleted IS 0 AND parent.kind <> ?)
FROM moz_bookmarks_synced_structure s
LEFT JOIN moz_bookmarks_synced child  ON child.guid  = s.guid
LEFT JOIN moz_bookmarks_synced parent ON parent.guid = s.parentGuid
WHERE s.guid <> ?

Then we can know exactly how many rows are tombstoned? Idk if we really care about that though in regards to what you're trying to actually do.

Copy link
Copy Markdown
Member Author

@mhammond mhammond May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's a good idea. I took the first one because we don't really need to know counts.

"SELECT
MAX(child.isDeleted IS 1),
MAX(child.guid IS NULL),
MAX(parent.isDeleted IS 1),
MAX(parent.guid IS NULL),
MAX(parent.isDeleted IS 0 AND parent.kind <> {folder_kind})
FROM moz_bookmarks_synced_structure s
LEFT JOIN moz_bookmarks_synced child ON child.guid = s.guid
LEFT JOIN moz_bookmarks_synced parent ON parent.guid = s.parentGuid
WHERE s.guid <> '{root_guid}'
",
root_guid = root_guid_str.as_str(),
folder_kind = folder_kind,
),
[],
|row| Ok((row.get(0)?, row.get(1)?, row.get(2)?, row.get(3)?, row.get(4)?)),
)?;
if child_tombstoned {
// The child GUID exists, but is a tombstone.
error_support::report_error!(
"places-bookmarks-structure-child-tombstoned",
"moz_bookmarks_synced_structure has rows whose child guid is tombstoned"
);
}
if child_absent {
// The child GUID does not exist
error_support::report_error!("places-bookmarks-structure-child-absent",
"moz_bookmarks_synced_structure has rows whose child guid is absent from moz_bookmarks_synced");
}
if parent_tombstoned {
// The parent GUID exists, but is a tombstone.
error_support::report_error!(
"places-bookmarks-structure-parent-tombstoned",
"moz_bookmarks_synced_structure has rows whose parent guid is tombstoned"
);
}
if parent_absent {
// The parent GUID does not exist.
// This should be "impossible" because our "ON DELETE CASCADE" on `moz_bookmarks_synced_structure`.
error_support::report_error!("places-bookmarks-structure-parent-absent",
"moz_bookmarks_synced_structure has rows whose parent guid is absent from moz_bookmarks_synced");
}
if non_folder_parent {
// Both sides are OK other than the fact the parent isn't actually a folder.
error_support::report_error!(
"places-bookmarks-structure-non-folder-parent",
"moz_bookmarks_synced_structure has rows whose parent is a non-folder item"
);
}

// Only tell dogear about child/parent relationships where both endpoints are live,
// non-deleted items and the parent is a folder. Orphaned or mis-typed
// structure rows are silently skipped here; the items themselves are still
// inserted into the builder (or recorded as tombstones) by the pass above,
// so any stale structure rows simply produce no structural claim (so dogear
// might end up reparenting to 'unfiled' etc.)
let sql = format!(
"SELECT guid, parentGuid FROM moz_bookmarks_synced_structure
WHERE guid <> '{root_guid}'
ORDER BY parentGuid, position",
root_guid = BookmarkRootGuid::Root.as_guid().as_str()
"SELECT s.guid, s.parentGuid
FROM moz_bookmarks_synced_structure s
JOIN moz_bookmarks_synced child
ON child.guid = s.guid AND NOT child.isDeleted
JOIN moz_bookmarks_synced parent
ON parent.guid = s.parentGuid AND NOT parent.isDeleted
AND parent.kind = {folder_kind}
WHERE s.guid <> '{root_guid}'
ORDER BY s.parentGuid, s.position",
folder_kind = folder_kind,
root_guid = root_guid_str.as_str()
);
let mut stmt = self.db.prepare(&sql)?;
let mut results = stmt.query([])?;
Expand Down Expand Up @@ -2137,17 +2214,12 @@ mod tests {
))?;

let merger = Merger::new(&conn, &interrupt_scope, ServerTimestamp(0));
let result = merger.fetch_remote_tree();

assert!(
matches!(
result,
Err(Error::MergeError(ref e))
if matches!(e.kind(), dogear::ErrorKind::MissingParentForUnknownChild(..))
),
"expected MissingParentForUnknownChild, got {:?}",
result
);
// With bug 2039791, fetch_remote_tree filters out orphaned structure rows and
// succeeds. Without that patch, this returned MissingParentForUnknownChild.
let tree = merger.fetch_remote_tree()?;
// The orphaned guids are skipped and do not appear in the tree.
assert!(tree.node_for_guid(&"Cccccccccccc".into()).is_none());
assert!(tree.node_for_guid(&"Pppppppppppp".into()).is_none());
Ok(())
}

Expand All @@ -2174,16 +2246,20 @@ mod tests {
))?;

let merger = Merger::new(&conn, &interrupt_scope, ServerTimestamp(0));
let result = merger.fetch_remote_tree();

assert!(
matches!(
result,
Err(Error::MergeError(ref e))
if matches!(e.kind(), dogear::ErrorKind::InvalidParent(..))
),
"expected InvalidParent, got {:?}",
result
// With bug 2039791, fetch_remote_tree filters out structure rows whose parent
// is not a folder and succeeds. Without that patch this returned InvalidParent.
let tree = merger.fetch_remote_tree()?;
// Both items are still in the tree (inserted as live items by pass 2), but
// without the bad structural relationship: Cccccccccccc is not a child of
// Pppppppppppp. Both items are orphans reparented to unfiled.
let c_node = tree
.node_for_guid(&"Cccccccccccc".into())
.expect("Cccccccccccc should be in tree as orphan");
let c_parent_guid = c_node.parent().map(|p| p.item().guid.clone());
assert_ne!(
c_parent_guid.as_deref(),
Some("Pppppppppppp"),
"Cccccccccccc should not be parented to the non-folder Pppppppppppp"
);
Ok(())
}
Expand Down