1405563 - Uptick of failures in Firefox 57: bookmarks fails with "Duplicate entry for <guid> in changeset"

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

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

And I think guid is most often null, but often what appears to be a valid guid.

Appears to start on Sept 9 and accounts for ~30% of the failures, putting it second to bug 1380606.

Probably not a huge deal as 57 has a higher success rate for that engine than 56 or 55, but still worth seeing if there's a sane and simple fix we can upload. I'm marking it as a regression anyway.

https://nbviewer.jupyter.org/gist/mhammond/649a6f6c5606cff0e0d9d8a4f0b9e9a1/Success-Failure-Error%20rates%20per%20engine%2C%20by%20version.ipynb

Comment 1

[Lina Butler [:lina]]

This is informative! I suspect this happens because `insertTree` didn't remove tombstones for already existing items (bug 1393904), so it's possible for the same GUID to appear in `moz_bookmarks` *and* `moz_bookmarks_deleted`. (One advantage of iOS storing tombstones and bookmarks in the same table is that collisions like this can't occur).

We always want the "real" bookmark to supersede the tombstone, and, separately, we want to drop GUIDs from `moz_bookmarks_deleted` that also exist in `moz_bookmarks`. The latter can be part of Places maintenance.

Comment 2

[Lina Butler [:lina]]

Attachment: Bug 1405563 - Ignore and clean up tombstones for undeleted synced bookmarks.

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

Comment 3

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

Speaking with Kit, we've decided that this is too risky for 57, especially given 57 has the highest success rate for this engine.

Comment 4

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

Comment on attachment 8915321 [details]
Bug 1405563 - Ignore and clean up tombstones for undeleted synced bookmarks.

https://reviewboard.mozilla.org/r/186518/#review191686

Looks great, thanks. I think you should get review from Mak too for the maint. task.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8915321 [details]
Bug 1405563 - Ignore and clean up tombstones for undeleted synced bookmarks.

Mak, how does this Places maintenance task look to you? See comment 1 for context. This is going to conflict with the task in bug 1380606; I'll rebase on top of whichever one lands first.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8915321 [details]
Bug 1405563 - Ignore and clean up tombstones for undeleted synced bookmarks.

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

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8915321 [details]
Bug 1405563 - Ignore and clean up tombstones for undeleted synced bookmarks.

https://reviewboard.mozilla.org/r/186518/#review192748

As you said, it will need some unbitrot with the other bug, but nothing too complex.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8915321 [details]
Bug 1405563 - Ignore and clean up tombstones for undeleted synced bookmarks.

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

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8915321 [details]
Bug 1405563 - Ignore and clean up tombstones for undeleted synced bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186518/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a97ced54c839
Ignore and clean up tombstones for undeleted synced bookmarks. r=mak,markh

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a97ced54c839

Comment 12

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

Telemetry indicates this is indeed fixed - nice work!