Closed Bug 1512867 Opened Last year Closed Last year

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Blocks: 1512868
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28ee88825b3d
Always update or upload synced bookmarks that only exist on one side. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/28ee88825b3d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
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
Attachment #9030157 - Flags: approval-mozilla-beta?
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.
Attachment #9030157 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.