If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ensure `insertTree` removes Sync tombstones for restored bookmarks

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Places
RESOLVED FIXED
25 days ago
10 days ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

`insert` currently removes tombstones when given an explicit GUID (https://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/toolkit/components/places/Bookmarks.jsm#1371-1376), but it looks like we need to give `insertTree` the same treatment.

Otherwise, Sync will upload an incomplete tree after restoring bookmarks: `eraseEverything` will write tombstones for all synced bookmarks, but `insertTree` won't remove them, so the GUIDs will exist in both, and `PlacesSyncUtils.bookmarks.pullChanges` will think those bookmarks are still deleted.
Comment hidden (mozreview-request)
Mark, I changed `addRowToChangeRecords` to throw if it sees a duplicate GUID in `moz_bookmarks` and `moz_bookmarks_deleted`. This might cause an uptick in failed syncs, including items with NULL GUIDs (bug 1380606)...but ignoring that case doesn't seem right, either. If an item doesn't have a GUID, or we have an entry *and* a tombstone for it, we're certain to corrupt the tree on the server, so we may as well bail. WDYT?
Blocks: 1305563
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #2)
> If an item doesn't have a
> GUID

Can we silently ignore this?

> or we have an entry *and* a tombstone for it, we're certain to corrupt
> the tree on the server, so we may as well bail. WDYT?

Yeah, that sounds like the only sane thing to do, and hopefully telemetry would tell us it is rare.

Comment 4

22 days ago
mozreview-review
Comment on attachment 8901298 [details]
Bug 1393904 - Ensure `insertTree` removes Sync tombstones for restored bookmarks.

https://reviewboard.mozilla.org/r/172744/#review178532

::: toolkit/components/places/Bookmarks.jsm:1442
(Diff revision 1)
>  
> +      // Remove stale tombstones for new items.
> +      await db.executeCached(
> +        `DELETE FROM moz_bookmarks_deleted WHERE guid = :guid`,
> +        items.map(item => ({ guid: item.guid }))
> +      );

Could we do this as a single query using IN? I know, it's not great, and we may hit again the limit if the number of bookmarks is extreem, but running another query for each guid is expensive. 
Damn, we really need bug 483318 :(

alternatively, we could add your deletion to moz_bookmarks_foreign_count_afterinsert_trigger
Then any insert will ensure to delete the guid from the deleted folder.
It's still one query per insert, but at least there isn't our querying overhead.
Attachment #8901298 - Flags: review?(mak77)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

22 days ago
mozreview-review-reply
Comment on attachment 8901298 [details]
Bug 1393904 - Ensure `insertTree` removes Sync tombstones for restored bookmarks.

https://reviewboard.mozilla.org/r/172744/#review178532

> Could we do this as a single query using IN? I know, it's not great, and we may hit again the limit if the number of bookmarks is extreem, but running another query for each guid is expensive. 
> Damn, we really need bug 483318 :(
> 
> alternatively, we could add your deletion to moz_bookmarks_foreign_count_afterinsert_trigger
> Then any insert will ensure to delete the guid from the deleted folder.
> It's still one query per insert, but at least there isn't our querying overhead.

Bug 483318 would be lovely, yes. Oh, well. :-/ Switched to using the `chunkArray` trick that we've used elsewhere. I opted not to make it part of the trigger because, depending on how bug 1343103 works out, we may want to give tombstones a `syncStatus`...so we'll need to check the source, which the trigger doesn't know about.

Comment 7

20 days ago
mozreview-review
Comment on attachment 8901298 [details]
Bug 1393904 - Ensure `insertTree` removes Sync tombstones for restored bookmarks.

https://reviewboard.mozilla.org/r/172744/#review179458
Attachment #8901298 - Flags: review?(mak77) → review+
Depends on: 1396251
Comment hidden (mozreview-request)

Comment 9

11 days ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23342d07ad15
Ensure `insertTree` removes Sync tombstones for restored bookmarks. r=mak
https://hg.mozilla.org/mozilla-central/rev/23342d07ad15
Status: ASSIGNED → RESOLVED
Last Resolved: 10 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.