Closed Bug 1447832 Opened 3 years ago Closed 3 years ago

bookmark tombstones may be removed before they are synced

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

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?
Flags: needinfo?(kit)
Duplicate of this bug: 1440518
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!
Flags: needinfo?(kit)
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 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.
Attachment #8961247 - Flags: review?(kit) → review+
Assignee: nobody → markh
Status: NEW → ASSIGNED
Pushed by mhammond@skippinet.com.au:
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
Duplicate of this bug: 1442610
https://hg.mozilla.org/mozilla-central/rev/481b43bb28ef
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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
Attachment #8961247 - Flags: approval-mozilla-beta?
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+
Attachment #8961247 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.