Closed
Bug 1393904
Opened 7 years ago
Closed 7 years ago
Ensure `insertTree` removes Sync tombstones for restored bookmarks
Categories
(Toolkit :: Places, enhancement)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(1 file)
`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) |
Assignee | ||
Comment 2•7 years ago
|
||
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?
Comment 3•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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+
Comment hidden (mozreview-request) |
Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/23342d07ad15 Ensure `insertTree` removes Sync tombstones for restored bookmarks. r=mak
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/23342d07ad15
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•