1447832 - bookmark tombstones may be removed before they are synced

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

This takes a few goes to reproduce, but, with the old engine:
* ensure a first sync has run.
* in the bookmarks sidebar, create a new folder and leave the "new folder" dialog open.
* sync should start immediately, so within a second or 2, press "Esc" to cancel the new folder creation. This actually deletes the bookmark.
* check the bookmark validator - if the bug was triggered it will report the record for the now deleted folder exists on the server but not on the client.

What happens is:
* pullChanges sees the new folder.
* as sync is running the bookmark is deleted and a tombstone is created.
* pushChanges removes the tombstone as since bug 1405563 tombstones are deleted for all guids in the changeset, not just those that were marked as tombstones in the changeset.

Now that bug 1393904 is fixed, we might be able to revert to only removing tombstones that were initially tombstones in the changeset. Kit, WDYT?

Comment 2

[Lina Butler [:lina]]

Whoops, yes, this was an unintended consequence of bug 1393904. Your fix to check the changeset, and only delete entries that were initially tombstones (or that were modified during the sync, but for which we uploaded tombstones and updated the changeset, as in https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/services/sync/modules/engines/bookmarks.js#403-408) makes sense to me!

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1447832 - ensure a bookmark that is deleted during a sync gets a tombstone on the server.

Review commit: https://reviewboard.mozilla.org/r/230018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230018/

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8961247 [details]
Bug 1447832 - ensure a bookmark that is deleted during a sync gets a tombstone on the server.

https://reviewboard.mozilla.org/r/230018/#review235710

::: toolkit/components/places/PlacesSyncUtils.jsm:915
(Diff revision 1)
> -              WHERE guid = :guid`,
> +                WHERE guid = :guid`,
> -              updateParams);
> +                updateParams);
> -
> -            // Unconditionally delete tombstones, in case the GUID exists in
> -            // `moz_bookmarks` and `moz_bookmarks_deleted` (bug 1405563).
> -            let tombstoneGuidsToRemove = updateParams.map(({ guid }) => guid);
> +              // and if there are *both* bookmarks and tombstones for these
> +              // items, we nuke the tombstones.
> +              // This should be unlikely, but bad if it happens.
> +              let dupedGuids = updateParams.map(({ guid }) => guid);

It seemed relatively easy to have the best of both worlds here (and otherwise test_pullChanges_tombstones fails)

I'm not sure if the SQL is OK though - it could probably also be done with a WITH statement, but I've no idea how either will perform in practice.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8961247 [details]
Bug 1447832 - ensure a bookmark that is deleted during a sync gets a tombstone on the server.

https://reviewboard.mozilla.org/r/230018/#review235714

Thanks, Mark!

::: toolkit/components/places/PlacesSyncUtils.jsm:915
(Diff revision 1)
> -              WHERE guid = :guid`,
> +                WHERE guid = :guid`,
> -              updateParams);
> +                updateParams);
> -
> -            // Unconditionally delete tombstones, in case the GUID exists in
> -            // `moz_bookmarks` and `moz_bookmarks_deleted` (bug 1405563).
> -            let tombstoneGuidsToRemove = updateParams.map(({ guid }) => guid);
> +              // and if there are *both* bookmarks and tombstones for these
> +              // items, we nuke the tombstones.
> +              // This should be unlikely, but bad if it happens.
> +              let dupedGuids = updateParams.map(({ guid }) => guid);

Awesome! The SQL looks pretty good to me, a search by primary key for the first `IN`, and an index on `moz_bookmarks` that it can use for the second.

::: toolkit/components/places/PlacesSyncUtils.jsm:2554
(Diff revision 1)
> + * Removes tombstones for successfully synced items where the specified GUID
> + * exists in *both* the bookmarks and tombstones tables.
> + *
> + * @return {Promise}
> + */
> +var removeDuplicateTombstones = function(db, guids) {

Hmm, just from the name, "duplicate tombstones" makes me think this might be cleaning up multiple tombstones for the same GUID.

What about something like `removeTombstonesForExistingItems` or `removeUndeletedTombstones`? That's quite a mouthful, so, if you'd like to keep it as-is, that's fine.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8961247 [details]
Bug 1447832 - ensure a bookmark that is deleted during a sync gets a tombstone on the server.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230018/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/481b43bb28ef
ensure a bookmark that is deleted during a sync gets a tombstone on the server. r=kitcambridge

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/481b43bb28ef

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8961247 [details]
Bug 1447832 - ensure a bookmark that is deleted during a sync gets a tombstone on the server.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1405563
[User impact if declined]: Deleted bookmarks may reappear when synced to another device.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Limited to Syncing of bookmarks, problem is well understood and covered by tests.
[String changes made/needed]: None

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8961247 [details]
Bug 1447832 - ensure a bookmark that is deleted during a sync gets a tombstone on the server.

fix a bug with bookmark sync, beta60+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a641151295d6