Closed Bug 1393904 Opened 2 years ago Closed 2 years ago

Ensure `insertTree` removes Sync tombstones for restored bookmarks

Categories

(Toolkit :: Places, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: lina, Assigned: lina)

References

(Depends on 1 open bug)

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.
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 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 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 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
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.