STR: * Have an *incorrect* FxA password remembered locally and synced (ie, so the wrong password is saved locally and remotely) * Disconnect from Sync. * Login to sync, note the incorrect password is filled. Enter the correct password. * Note you are prompted if you want to update the password - say yes. * Let sync complete. * Check what the local password is: Expected: * The local password is the same as you recently entered and re-saved. Actual: * The incorrect password is still saved. It also remains incorrect on the server. The problem is that because this is a reconnection to Sync, all engines are reset. This in turn means that we ignore the last-modified of local items and treat them as modified long ago. This means the incoming record is preferred and the incoming item with the incorrect password is applied, overwriting the just changed locally record. This seems a significant problem as it may cause people to forget their FxA password, forcing a password reset and possible data loss. Also confusingly, the log in this case writes: > 1484887350906 Sync.Engine.Passwords DEBUG First sync, uploading all items which is wrong and misleading - all records are *not* uploaded, but instead the exact opposite actually happens - all records are *downloaded*. Identical items are removed from the list to upload, while not identical items overwrite the local version with the local one. If a non-identical item also has local modifications it actually *is* uploaded - but after the local modifications are overwritten by the incoming items. IIUC, that log message is correct exactly once - the very first sync for an account when there are no server records. I don't quite understand why we ignore the local lastModified times in this case - IIUC, all local changes made to existing items between the disconnection and reconnection, for all collections, are lost. Using our local timestamp would mean that all items are *considered* for upload (and thus should still repair items missing remotely) but would have less risk of data loss when an item was changed locally between the disconnect and reconnect. I don't see how clock skew would be an issue here (well, why it would be more of an issue than it already is for normal syncs) Richard, what's the reason for the current behaviour, and why would it be bad to consider our locally recorded time instead of assuming remote records must be more recent?

> The problem is that because this is a reconnection to Sync, all engines are > reset. This in turn means that we ignore the last-modified of local items > and treat them as modified long ago. This means the incoming record is > preferred and the incoming item with the incorrect password is applied, > overwriting the just changed locally record. This is also the cause of password un-deletion and other data loss reported by users. If you've cleaned some stuff up locally and you're not connected to Sync, you'll lose all of your local changes. (Deletes reappearing are really hard to solve, but undoing password changes is just mean.) > IIUC, that log message is correct exactly once - the very first sync for an account when > there are no server records. Yup. It's a very old (7 years) and very badly worded log message. > Richard, what's the reason for the current behaviour, and why would it be > bad to consider our locally recorded time instead of assuming remote records > must be more recent? "Blame Labs code". Bug 550627. "Instead of client wins on conflicts, do server wins if a changed item is pretty old." - // first sync special case: upload all items - // NOTE: we use a backdoor (of sorts) to the tracker so it - // won't save to disk this list over and over + // Mark all items to be uploaded, but treat them as changed from long ago if (!this.lastSync) { this._log.debug("First sync, uploading all items"); - this._tracker.clearChangedIDs(); - [i for (i in this._store.getAllIDs())] - .forEach(function(id) this._tracker.changedIDs[id] = true, this); + for (let id in this._store.getAllIDs()) + this._tracker.addChangedID(id, 0); } Specifically this was intended to fix Bug 537653; dolske suggested this behavior in Bug 537653 Comment 2. The idea is that -- in theory -- you're more likely to connect a stale machine to a live account than a live machine to a stale account, so the account's data should win. That of course has unintended side effects: clients pass through this flow in more situations than simply connecting an old machine to your current FxA.

This is an idea I had to alleviate the passwords problem, which could be extended to other engines. Not sure if I'm going in the right direction tho, I'm wondering if adding a lastModified method in SyncEngine is not an heresy?

Adding a see-also to Bug 1332290, because that's another place where we re-sync and upload local records.

(In reply to Edouard Oger [:eoger] from comment #3) > This is an idea I had to alleviate the passwords problem, which could be > extended to other engines. Not sure if I'm going in the right direction tho, > I'm wondering if adding a lastModified method in SyncEngine is not an heresy? Can you elaborate? Do you mean a server timestamp or a local timestamp? Would the value be stored somewhere? When would you use it? What would it do?

Added what we talked about yesterday on IRC: pullAllChanges for password.js. I haven't found a way to test that with TPS sadly, but manual testing shows that it works properly. We should open new bugs to do the same thing for other engines if this sticks. Also changed 2 unrelated lines to make eslint pass without warnings on password.js.

Comment on attachment 8832980 [details] Bug 1332556 - Set correct passwords' timestamps on first Sync. https://reviewboard.mozilla.org/r/109216/#review112244 ::: services/sync/modules/engines/passwords.js:101 (Diff revision 2) > } > + > + return null; > }, > + > + pullAllChanges() { This will not skip the entries for which getSyncCredentialsHosts match. Given getAllIDs in the engine already does that and already includes the entire metainfo for the password, I think you should just re-implement the base class pullAllChanges() but replace the 0 with the info - ie, something like: let changeset = new Changeset(); for (let [id, info] in Object.values(this._store.getAllIDs())) { changeset.set(id, info.timePasswordChanged); } return changeset; or similar.

Comment on attachment 8832980 [details] Bug 1332556 - Set correct passwords' timestamps on first Sync. https://reviewboard.mozilla.org/r/109216/#review112244 > This will not skip the entries for which getSyncCredentialsHosts match. Given getAllIDs in the engine already does that and already includes the entire metainfo for the password, I think you should just re-implement the base class pullAllChanges() but replace the 0 with the info - ie, something like: > > let changeset = new Changeset(); > for (let [id, info] in Object.values(this._store.getAllIDs())) { > changeset.set(id, info.timePasswordChanged); > } > return changeset; > > or similar. I mistakenly thought that we did not want getSyncCredentialsHosts - which is why I made another version. Thanks!

Comment on attachment 8832980 [details] Bug 1332556 - Set correct passwords' timestamps on first Sync. https://reviewboard.mozilla.org/r/109216/#review112662 Awesome, thanks!

Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3c9874eb6cc3 Set correct passwords' timestamps on first Sync. r=markh

Status: FIXED & VERIFIED. Browser: Firefox Aurora 54 OS:Windows 10 X64 The issue is no longer in Firefox Aurora and the sync password remains the same and it cannot be reproducible. [bugday-20170419]