1752839 - Improve logins sync

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P2)

Description

[Sergey Galich [:serg]]

:markh brought this up in our conversation about logins sync issue where I had 2 synced clients with different login count.

sync only "reconciles" all items when signing in to sync - in the general case, it only updates items it believes has changed. Unfortunately, logins still uses the "old" way of "tracking" items - it uses observers and listens for login manager notifications at https://searchfox.org/mozilla-central/rev/78963fe42f8d5f582f84da84a5e78377b6c1fc32/services/sync/modules/engines/passwords.js#408

The "new" way of tracking things - done for bookmarks and some others - is to have this sync metadata be in the store itself - so the login manager could flip a flag to say "changed since last sync" - that way it's far less likely that sync will have incorrect info about changed items. We should consider doing that for logins.

Lets find time to review and modernize logins sync.

Comment 1

[Sergey Galich [:serg]]

Setting P2 for now to keep it visible.

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Thanks Sergey! The way we've modernized other engines is something like:

• Each login stores a "sync status" which is one of "new" (yet to be synced) or "normal" (syncing) and a "sync change counter" which is basically a flag to indicate if the item has changed (it's a counter so to handle the edge-case of the item changing during a sync - we record what the counter was at the start of a sync, and decrement at the end - if it's non-zero at that time, it changed during sync so is dirty). These are stored in the storage itself.

• In general, sync itself maintains "sync status", but the store itself helps manage the change counter - each thing that touched a login increments it.

• The sync modules then do not need to listen to the login manager observers (linked in comment 0), but instead it interacts with the store and these 2 new fields. "items to sync" are then something like "all items with status=="new" || counter > 0, etc.

Some extra work will be necessary to track "tombstones" correctly (a tombstone is a record on the server that indicates the item was deleted). Only items with a status=="normal" need a tombstone, but the fact the tombstone is needed (ie, in the time between the item being deleted and syncing) must be tracked somewhere. A tombstone really just needs to track the items GUID (and maybe the date of deletion, but maybe not)

Comment 3

[Neil Deakin]

Attachment: Bug 1752839, improve logins sync by not using the LegacyTracker, r=sgalich,dimi,joschmidt,markh,skhamis

Instead of recording a separate list of changes that could be incorrect, determine the list of modified logins when the sync occurs. Two flags are added to login info objects, the syncStatus that identifies that a login has been synced and a counter which is incremented during a sync and decremented by the same amount when the sync is complete. Deleted logins are flagged with a deleted marker rather than deleted directly.

This behaviour mirrors the form auto fill sync method.

Comment 4

[Neil Deakin]

Attachment: Bug 1752839, remove the nsILoginManagerStorage interface, r=sgalich

Removing this interface seemed simpler than adding a bunch of extra arguments/methods to both nsILoginManager and nsILoginManagerStorage

Comment 5

[Neil Deakin]

Attachment: Bug 1752839, add more password sync tests, r=dimi,sgalich

Fix the rounding of the default argument to insertRecord as it can cause intermittent errors

Depends on D170817

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e809ca5c010
remove the nsILoginManagerStorage interface, r=credential-management-reviewers,dimi,sgalich

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7e809ca5c010

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/788c9f71380f
improve logins sync by not using the LegacyTracker, r=sgalich,skhamis,sync-reviewers,dimi,joschmidt,markh
https://hg.mozilla.org/integration/autoland/rev/c7061d98b7b1
add more password sync tests, r=sgalich

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/788c9f71380f
https://hg.mozilla.org/mozilla-central/rev/c7061d98b7b1