1512867 - Always update or upload synced bookmarks that only exist on one side

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Lina Butler (ex-Mozilla)]

https://searchfox.org/mozilla-central/rev/9abd7f512c9b29071776e7ba3f991d5b5fd59ff6/toolkit/components/places/SyncedBookmarksMirror.jsm#3128 is subtly wrong. For remote merge states where the item doesn't exist locally, we should always return true, even if the remote node indicates the item doesn't need to be merged. In theory, we could return true and always take the remote state, but that'll trigger a lot of unneeded writes for keywords and tags if they didn't actually change.

For symmetry, we should also do the same for local items: if we're taking the local state, and the item doesn't exist remotely, always upload it.

Both catch the case where the mirror's `needsMerge` flags are out of sync with Places; for example, the mirror says an item doesn't need to be merged, but isn't known locally, or Places has an item with `syncStatus = NORMAL` that doesn't exist at all in the mirror.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1512867 - Always update or upload synced bookmarks that only exist on one side. r?tcsc

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28ee88825b3d
Always update or upload synced bookmarks that only exist on one side. r=tcsc

Comment 3

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/28ee88825b3d

Comment 4

[Lina Butler (ex-Mozilla)]

Comment on attachment 9030157 [details]
Bug 1512867 - Always update or upload synced bookmarks that only exist on one side. r?tcsc

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1496878

User impact if declined: In cases where the bookmarks mirror is out of sync with the Places database, Sync might ignore new bookmarks that should be applied, and corrupt bookmark positions in local folders.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a small change to two functions, and has an xpcshell test. Additionally, new bookmark sync is pref'd off by default on Beta and Release.

String changes made/needed: None

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9030157 [details]
Bug 1512867 - Always update or upload synced bookmarks that only exist on one side. r?tcsc

[Triage Comment]
Fixes possible bookmarks syncing issues and includes a new automated test. Approved for 65.0b5.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/067a6752b180