1363995 - Implement core reconciliation logic for formautofill engine

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

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

We are working on a sync engine for formautofill, which requires reconciliation/deduping logic that's not supported by Sync today.

This bug is to implement simple reconciliation logic which roughly matches the logic required by formautofill, and to tweak sync so that this new model is supported. The bug for the *actual* implementation of the reconciliation logic is bug 1361242 - this bug is to implement a very rough approximation and better define the semantics ready for 1361242.

Comment 1

[Julie McCracken (:julie)]

Logic is being worked on after good feedback from Mark.

Comment 2

[Lina Butler [:lina]]

I thought some more about merging two records with different versions, and came to the same conclusion as Mark: there's no general solution that can do this correctly. If we ever sync derived fields, or fields for which semantics change between versions, we might create a perfectly well-formed, yet nonsensical record. Once a record is upgraded, we can't make any truly "safe" modifications, because we don't know anything about the new fields.

But that's in the general case. For *specific* fields, we can make a better judgment about whether it's an irreversible change, and older clients can't modify the record, vs. "it's just an extra field that we should round-trip". That decision happens at the *field* level, not necessarily the record level. For example, if we add a derived field that depends on the address, changing the address on an older client wouldn't be safe, but changing the name is OK.

One idea is to make certain fields, or even the entire record, read-only on old clients. But that would make for a frustrating UX: I *see* my profile, why can't I change it? (Or, more confusingly, why can I only change *parts* of it?)

Another idea is transparently clone and hide a higher versioned record if you try to edit one, and have the second client that supports the higher version dedupe the clone when it syncs. We could also do this explicitly, via a prompt ("You can't edit this profile because it was saved by a newer version of Firefox. Would you like to make a copy that you can edit?"), but that's a confusing question that most folks won't have enough information to answer.

In the interest of getting this shipped quickly, here are some ideas:

* If remote record version is higher than local record version, and local record is not dirty, take remote.
* If you try to edit a higher-versioned record, fork, and keep only the fields we know about. This doesn't solve the problem of field semantics changing between versions. Let's agree (and document) to maybe not do that?
* If remote record version is higher than our local schema version, and local *is* dirty, fork, keeping only the fields we know about.
* If remote record version is lower than our local schema version, migrate remote and proceed with reconciliation.
* If local record version is still higher than migrated remote record (which is now at our local schema version), fork. We can't meaningfully merge these.
* We should never consider records higher than our local schema version for duping, because we don't know anything about their extra fields.

I'll keep thinking about this, but the basic idea is "fork unless we know what all the fields mean".

Comment 3

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

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #2)

We shouldn't overthink too much of this now. We can safely defer how to deal with earlier-schema records until the schema version actually changes, and we can even defer how to deal with newer-schema records for a short time (although we can't ignore it completely, as we need the basic framework for this in-place today)

> In the interest of getting this shipped quickly, here are some ideas:

Yeah - IMO, getting something vaguely sane shipped is what we should aim for, and anything that's not immediately obvious should be deferred.

> * If remote record version is higher than local record version, and local
> record is not dirty, take remote.

Agreed.

> * If you try to edit a higher-versioned record, fork, and keep only the
> fields we know about.

I'm not convinced that will always be the correct choice (but do agree that it's impossible to know in the general case.) It's just so hard to predict the future. I wonder if a schema version could be major.minor, where a major change does as you describe, while a minor change means something different. This means that when a schema change is needed, the decision can be made between how to bump the version based on how existing versions of the addon will treat the data?

> This doesn't solve the problem of field semantics
> changing between versions. Let's agree (and document) to maybe not do that?

Agreed.

> * If remote record version is higher than our local schema version, and
> local *is* dirty, fork, keeping only the fields we know about.

Maybe a major.minor schema version can help here too?

> * If remote record version is lower than our local schema version, migrate
> remote and proceed with reconciliation.

Agreed.

> * If local record version is still higher than migrated remote record (which
> is now at our local schema version), fork. We can't meaningfully merge these.

I don't understand this - I'd expect that the addon could always migrate from any version to the current version?

> * We should never consider records higher than our local schema version for
> duping, because we don't know anything about their extra fields.

Isn't that the same as the above "If remote record version is higher than our local schema version"? But maybe major.minor works here?

If the major.minor version idea isn't immediately shot down, I'd suggest we only implement the "minor version changed" logic now, which probably has much easier semantics to rationalize about, then work with the autofill team to better clarify these semantics (and ideally, hand this over to them completely - ultimately they need to own the final logic). We could probably do this without even defining the current version as "1.0".

Comment 4

[Lina Butler [:lina]]

Attachment: Bug 1363995 - Implement autofill record reconciliation.

This patch ignores cross-version migrations for now, and adds basic
three-way merging and forking that should handle most common
scenarios. We can figure out how to handle these later, once we have
some concrete changes we'd like to make.

I suspect modifying a record from a future version of the add-on
won't be safe in the general case. We might need to clone and hide
the original record so that it can be edited, then synced, migrated,
and deduped on a device that understands the new fields. But it's
hard to know without a concrete example, and easy to
overengineer.

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

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

Let's chat more about this patch in our meeting today.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review156970

::: browser/extensions/formautofill/FormAutofillSync.jsm:26
(Diff revision 1)
> -  return null;
> -}
> -
>  // A helper to sanitize address and creditcard records suitable for logging.
>  function sanitizeStorageObject(ob) {
> +  if (!ob) {

Let's lift these changes into bug 1374500.

::: browser/extensions/formautofill/FormAutofillSync.jsm:116
(Diff revision 1)
>        this._doUpdateRecord(remoteRecord);
>        return;
>      }
>  
>      // No matching local record. Try to dedupe a NEW local record.
> -    let localDupeID = /* this.storage. */findDuplicateGUID(remoteRecord.entry);
> +    let localDupeID = this.storage.findDuplicateGUID(remoteRecord.entry);

`findDuplicateGUID` can probably be factored out into a separate patch, too. The implementation doesn't depend on reconciliation.

::: browser/extensions/formautofill/FormAutofillSync.jsm:127
(Diff revision 1)
>        this._doUpdateRecord(remoteRecord);
>        return;
>      }
>  
>      // We didn't find a dupe, either, so must be a new record (or possibly
> -    // a non-deleted version of an item we have a tombstone for, which create()
> +    // a non-deleted version of an item we have a tombstone for, which add()

Bug 1374500.

::: browser/extensions/formautofill/ProfileStorage.jsm:80
(Diff revision 1)
>   *
>   * Sync Metadata:
>   *
>   * Records may also have a _sync field, which consists of:
>   * {
> - *   changeCounter, // integer - the number of changes made since a last sync.
> + *   changeCounter,    // integer - the number of changes made since the last

Bug 1374500.

::: browser/extensions/formautofill/ProfileStorage.jsm:327
(Diff revision 1)
>      }
>  
>      recordFound.timeLastModified = Date.now();
>      let syncMetadata = this._getSyncMetaData(recordFound);
>      if (syncMetadata) {
>        syncMetadata.changeCounter += 1;

Let's make sure that we don't mark the record as dirty if `timesUsed`, `timeLastUsed`, or any other field that's not synced changes. We should only bump `changeCounter` for changes that should trigger a sync.

::: browser/extensions/formautofill/ProfileStorage.jsm:539
(Diff revision 1)
> +    let sync = this._getSyncMetaData(localRecord, true);
> +
> +    let mergedRecord = {};
> +    for (let field of this.VALID_FIELDS) {
> +      let lastSyncedFieldsValue = (field in sync.lastSyncedFields ?
> +                                     sync.lastSyncedFields :

Nit: whitespace alignment.

::: browser/extensions/formautofill/ProfileStorage.jsm:554
(Diff revision 1)
> +      } else if (!isLocalSame && isRemoteSame) {
> +        // We don't need to bump the change counter when taking the local
> +        // change, because the counter must already be > 0 if we're attempting
> +        // a three-way merge.
> +        value = localRecord[field];
> +      } else if (localRecord[field] == remoteRecord[field]) {

Consider hashing the values in `lastSyncedFields`. Keeping the old value around might be a privacy concern, and we're not interested in the old value; just whether it's the same or not.

::: browser/extensions/formautofill/ProfileStorage.jsm:579
(Diff revision 1)
> +   * fields and Sync metadata.
> +   *
> +   * @param   {number} index
> +   * @param   {Object} remoteRecord
> +   * @param   ?boolean} [options.keepSyncMetadata = false]
> +   *          Should we copy Sync metadata? TODO: It might be better to make

Some more context about this option: we only pass `keepSyncMetadata: true` if we were able to merge successfully. We do this because we might still have local changes that need to be uploaded to the server. We also keep everything in `lastSyncedFields`, since we haven't uploaded the merged record yet. That prevents us from losing changes if the sync is interrupted after we merged, but before we could upload.

If we threw away `lastSyncedFields`, we'd throw away local changes made to the record on the next sync. We'd download the same record from the server, see that `isLocalSame && !isRemoteSame`, and overwrite the new local value with the old value from the server. (It's still technically possible to lose data if you interrupt the sync on one device, make the same change manually on another device, sync, revert the change, and then sync the original device again...but that's *extremely* unlikely to happen, whereas interrupted syncs are more common. I already expect that changes to the same record on multiple devices will be uncommon).

::: browser/extensions/formautofill/ProfileStorage.jsm:594
(Diff revision 1)
> +
> +    // Copy the GUID, version, and other internal fields. Using `_clone` and
> +    // copying fields isn't safe with cross-version upgrades. TODO: Consider
> +    // adding a `_cloneForSync` method to handle this.
> +    let newRecord = this._clone(remoteRecord);
> +    for (let field of this.INTERNAL_FIELDS) {

Actually, this isn't right. We sync all internal fields, too, but we should copy `timesUsed` and `timeLastUsed` from `localRecord`.

::: browser/extensions/formautofill/ProfileStorage.jsm:600
(Diff revision 1)
> +      if (localRecord[field] != null) {
> +        newRecord[field] = localRecord[field];
> +      }
> +    }
> +    if (keepSyncMetadata) {
> +      // XXX - we should probably copy this, right?

I don't think we need to. We never hand out public references to the `_sync` object, and we're replacing `localRecord` with `newRecord` in the array, so no need to clone.

::: browser/extensions/formautofill/ProfileStorage.jsm:616
(Diff revision 1)
> +
> +    if (timeLastModified > newRecord.timeLastModified) {
> +      newRecord.timeLastModified = timeLastModified;
> +      sync.changeCounter++;
> +    }
> +

See the comment in bug 1363999, comment 3 about whether or not we should call `_recordWriteProcessor` here.

Probably not: it's likely unsafe for cross-version reconciles, and we don't sync computed fields, anyway, but maybe we should.

::: browser/extensions/formautofill/ProfileStorage.jsm:641
(Diff revision 1)
> +    this._store.data[this._collectionName].push(forkedLocalRecord);
> +
> +    let sync = this._getSyncMetaData(forkedLocalRecord, true);
> +    sync.changeCounter++;
> +
> +    let clonedRecord = this._clone(forkedLocalRecord);

We only return the cloned record for testing and logging, but maybe we should just return the GUIDs instead, so that we can save on unnecessary work?

::: browser/extensions/formautofill/test/unit/test_reconcile.js:303
(Diff revision 1)
> +      // An incoming record has a different given-name than any of the above!
> +      "given-name": "Kip",
> +      "family-name": "Hammond",
> +    },
> +    merged: {
> +      // So we've forked the local record to a new GUID (and the next sync is

No need to wait for the next sync. We'll write it as part of the current sync, since we wait to pull changes until after we've reconciled.

::: browser/extensions/formautofill/test/unit/test_sync.js:43
(Diff revision 1)
>  const TEST_PROFILE_2 = {
>    "street-address": "Some Address",
>    country: "US",
>  };
>  
> +function objectMatches(object, fields) {

Bug 1374500.

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review157636

The patch looks good to me with the concern of schema version handling.

::: browser/extensions/formautofill/ProfileStorage.jsm:533
(Diff revision 1)
> +   *          The remote record.
> +   * @returns {Object|null}
> +   *          The merged record, or `null` if there are conflicts and the
> +   *          records can't be merged.
> +   */
> +  _mergeSyncedRecords(localRecord, remoteRecord) {

Since bug 1368008 will provide `_migrateRecord` function to migrate a record, are you going to use it in the functions, e.g. _mergeSyncedRecords, _replaceRecordAt?

Currently, we only have schema version 1, but I think we need to consider the future schema version handling.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

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

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review157636

> Since bug 1368008 will provide `_migrateRecord` function to migrate a record, are you going to use it in the functions, e.g. _mergeSyncedRecords, _replaceRecordAt?
> 
> Currently, we only have schema version 1, but I think we need to consider the future schema version handling.

We talked about this in today's meeting, and agreed to defer migration until after the MVP. Instead, we'll have `reconcile` throw if it sees an incoming record with a schema version that doesn't match ours.

In the general case, it's tricky to upgrade old records to a newer schema version, because that prevents old versions of the add-on from updating the record without data loss. However, we're not planning to add or deprecate new fields soon, or change the semantics of existing fields. We're also not going to sync computed fields; those will be recomputed as needed.

In the future, we can think about keying `lastSyncedFields` by version, though that might not be necessary because older clients can't update fields they don't know about. (We'd need to make sure that `update` doesn't throw away fields from newer versions that aren't in `VALID_FIELDS`, though).

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

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

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

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

Comment 12

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

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review159390

Nice work Kit!

::: browser/extensions/formautofill/ProfileStorage.jsm:83
(Diff revision 4)
>   *
>   * Records may also have a _sync field, which consists of:
>   * {
> - *   changeCounter, // integer - the number of changes made since a last sync.
> + *   changeCounter,    // integer - the number of changes made since the last
> + *                     // sync.
> + *   lastSyncedFields, // object - the original values for fields changed since

this comment should be changed to reflect it is the hashed values.

::: browser/extensions/formautofill/ProfileStorage.jsm:219
(Diff revision 4)
>     */
>    get version() {
>      return this._schemaVersion;
>    }
>  
> +  // Ensures that we don't try to apply synced records with newer schema

I think it would be nice if this comment reflects that different version numbers are a future TODO and this is a sanity checker to ensure we don't accidentally do it in the meantime.

::: browser/extensions/formautofill/ProfileStorage.jsm:515
(Diff revision 4)
>    /**
>     * Functions intended to be used in the support of Sync.
>     */
>  
> +  /**
> +   * Stores the last synced value for a field in a locally updated record. We

add "hashed" somewhere here :)

::: browser/extensions/formautofill/test/unit/test_sync.js:682
(Diff revision 4)
> +    ]);
> +
> +    ok(profileStorage.addresses.isSyncing(forkedPayload.id),
> +      "Forked record should be marked as syncing");
>    } finally {
>      await cleanup(server);

A test that a version=2 record throws seems like it might be useful?

Comment 13

[Lina Butler [:lina]]

Hi Sean! I see Luke is on PTO until 7/12, but I wanted to make sure this bug, bug 1377246, and bug 1363999 are still on your radar. There's no hurry to land them, but it would be great to start testing sync and see if there are any other changes we need to make in 56 outside the system add-on. Thanks!

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Sorry for the delay. I am working on the bug review these days since I have to read the related bug e.g. bug 1374500 to understand the full picture.

Comment 15

[Lina Butler [:lina]]

No problem, we're starting to split out some patches out for 56 (bug 1380094). Please take your time, and thank you for reviewing!

Comment 16

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

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

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review159390

> this comment should be changed to reflect it is the hashed values.

Done! I also changed the hash function to SHA-512, because I am a neophyte and I read it's theoretically more efficient on 64-bit (though I doubt we're hashing large enough values so frequently for that to matter).

> A test that a version=2 record throws seems like it might be useful?

That test lives in `test_reconcile` (`test_reconcile_unknown_version`), but I can move it here, or elsewhere if you'd prefer.

Comment 18

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review162758

::: browser/extensions/formautofill/ProfileStorage.jsm:577
(Diff revision 5)
> +   * @param   {string} lastSyncedValue
> +   *          The last synced field value.
> +   */
> +  _maybeStoreLastSyncedField(record, field, lastSyncedValue) {
> +    let isSyncing = "_sync" in record;
> +    if (!isSyncing) {

Could we just use `if ("_sync" in record) {` or `if (record._sync) {` here? Do you want to use `isSyncing` as a comment to explain its meaning?

::: browser/extensions/formautofill/ProfileStorage.jsm:668
(Diff revision 5)
> +   * Replaces a local record with a remote or merged record, copying internal
> +   * fields and Sync metadata.
> +   *
> +   * @param   {number} index
> +   * @param   {Object} remoteRecord
> +   * @param   ?boolean} [options.keepSyncMetadata = false]

nit: {boolean}

::: browser/extensions/formautofill/ProfileStorage.jsm:750
(Diff revision 5)
> +   *          A `{forkedGUID}` tuple. `forkedGUID` is `null` if the merge
> +   *          succeeded without conflicts, or a new GUID referencing the
> +   *          existing locally modified record if the conflicts could not be
> +   *          resolved.
> +   */
> +  reconcile(remoteRecord) {

Since FormAutofillStore.applyIncoming will take care any `remoteRecord.deleted` case, `reconcile` does not have to.
Do you think we should add a condition to return early for `remoteRecord.deleted` case?
Or add a description in JSDOC that this function only handles the non-deleted records.
This is like an improvement to prevent the incorrect usage of `reconcile`.

::: browser/extensions/formautofill/test/unit/test_reconcile.js:442
(Diff revision 5)
> +
> +add_task(async function test_reconcile_unknown_version() {
> +  let path = getTempFile(TEST_STORE_FILE_NAME).path;
> +
> +  let profileStorage = new ProfileStorage(path);
> +  await profileStorage.initialize();

Use the helper instead:
```
let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
```

::: browser/extensions/formautofill/test/unit/test_reconcile.js:459
(Diff revision 5)
> +
> +add_task(async function test_reconcile_idempotent() {
> +  let path = getTempFile(TEST_STORE_FILE_NAME).path;
> +
> +  let profileStorage = new ProfileStorage(path);
> +  await profileStorage.initialize();

As above.

::: browser/extensions/formautofill/test/unit/test_reconcile.js:519
(Diff revision 5)
> +
> +add_task(async function test_reconcile_three_way_merge() {
> +  let path = getTempFile(TEST_STORE_FILE_NAME).path;
> +
> +  let profileStorage = new ProfileStorage(path);
> +  await profileStorage.initialize();

As above.

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review162758

> Could we just use `if ("_sync" in record) {` or `if (record._sync) {` here? Do you want to use `isSyncing` as a comment to explain its meaning?

That was my intent. I inlined `if (!record._sync)` and added a comment instead.

> Since FormAutofillStore.applyIncoming will take care any `remoteRecord.deleted` case, `reconcile` does not have to.
> Do you think we should add a condition to return early for `remoteRecord.deleted` case?
> Or add a description in JSDOC that this function only handles the non-deleted records.
> This is like an improvement to prevent the incorrect usage of `reconcile`.

Agreed. I changed `reconcile` to throw, and clarified in the docs that `remoteRecord` must exist locally and can't be a tombstone.

Comment 20

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151928/diff/5-6/

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review163446

LGTM! Thank you for implementing this patch!

::: browser/extensions/formautofill/test/unit/xpcshell.ini:42
(Diff revision 6)
>  [test_phoneNumber.js]
>  [test_savedFieldNames.js]
>  [test_toOneLineAddress.js]
>  [test_storage_tombstones.js]
>  [test_storage_syncfields.js]
> +[test_reconcile.js]

Sort the test files by alphabet order.

Comment 22

[Sean Lee [:seanlee][:weilonge]]

BTW, I see there is an error when trying `test_sync.js` locally. It's about Telephone library loading, so please rebase it and make sure all the test can be pass on treeherder. Thanks.
 0:02.00 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "Failed to load module resource://formautofill/phonenumberutils/PhoneNumber.jsm." {file: "resource://gre/modules/XPCOMUtils.jsm" line: 279}]
XPCU_moduleLambda@resource://gre/modules/XPCOMUtils.jsm:279:9
get@resource://gre/modules/XPCOMUtils.jsm:198:21
_normalizeTel@resource://formautofill/ProfileStorage.jsm:1355:9
_normalizeFields@resource://formautofill/ProfileStorage.jsm:1273:5
_normalizeRecord@resource://formautofill/ProfileStorage.jsm:1130:5
add@resource://formautofill/ProfileStorage.jsm:307:5
test_outgoing@/Users/seanlee/Projects/mozilla/src/gecko/obj-x86_64-apple-darwin15.6.0/_tests/xpcshell/browser/extensions/formautofill/test/unit/test_sync.js:130:24
async*asyncFunction@resource://gre/modules/Task.jsm:241:18
Task_spawn@resource://gre/modules/Task.jsm:166:12
_run_next_test@/Users/seanlee/Projects/mozilla/src/gecko/testing/xpcshell/head.js:1545:9
run@/Users/seanlee/Projects/mozilla/src/gecko/testing/xpcshell/head.js:707:9
_do_main@/Users/seanlee/Projects/mozilla/src/gecko/testing/xpcshell/head.js:221:3
_execute_test@/Users/seanlee/Projects/mozilla/src/gecko/testing/xpcshell/head.js:550:5
@-e:1:1
"

Comment 23

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review163452

It looks good to me with some comments below. Sorry for the late review.

::: browser/extensions/formautofill/ProfileStorage.jsm:592
(Diff revision 6)
> +   *          The field name.
> +   * @param   {string} lastSyncedValue
> +   *          The last synced field value.
> +   */
> +  _maybeStoreLastSyncedField(record, field, lastSyncedValue) {
> +    let syncMetadata = record._sync;

Not sure if we should use `_getSyncMetaData` to align with others?

::: browser/extensions/formautofill/ProfileStorage.jsm:745
(Diff revision 6)
> +  _forkLocalRecord(localRecord) {
> +    let forkedLocalRecord = this._clone(localRecord);
> +    forkedLocalRecord.guid = this._generateGUID();
> +    this._store.data[this._collectionName].push(forkedLocalRecord);
> +
> +    let sync = this._getSyncMetaData(forkedLocalRecord, true);
> +    sync.changeCounter++;

Since `forkedLocalRecord` is cloned without `_sync` data, `_getSyncMetaData(..., true)` always sets `changeCounter` to be 1. Maybe we don't need this `changeCounter++`?

::: browser/extensions/formautofill/test/unit/test_reconcile.js:331
(Diff revision 6)
> +      "family-name": "Hammond",
> +      "country": "CA",
> +    },
> +  },
> +  {
> +    description: "Field deleted locally, changed remotely",

Isn't it "Field changed locally, deleted remotely"?

::: browser/extensions/formautofill/test/unit/test_reconcile.js:477
(Diff revision 6)
> +  };
> +
> +  {
> +    let {forkedGUID} = profileStorage.addresses.reconcile(remote);
> +    let updatedRecord = profileStorage.addresses.get(guid, {
> +      noComputedFields: true,

Needs to change to `rawData`. So do the followings.

Comment 24

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review156970

> Let's make sure that we don't mark the record as dirty if `timesUsed`, `timeLastUsed`, or any other field that's not synced changes. We should only bump `changeCounter` for changes that should trigger a sync.

I added an assertion for this to `test_notifyUsed`.

Comment 25

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review163452

> Isn't it "Field changed locally, deleted remotely"?

Yes. :-) Renamed, and added an actual test for "field deleted locally, changed remotely". Good catch!

Comment 26

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151928/diff/6-7/

Comment 27

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review163982

::: browser/extensions/formautofill/ProfileStorage.jsm:698
(Diff revision 7)
> +   */
> +  _replaceRecordAt(index, remoteRecord, {keepSyncMetadata = false} = {}) {
> +    let localRecord = this._store.data[this._collectionName][index];
> +
> +    let newRecord = this._clone(remoteRecord);
> +    this._store.data[this._collectionName][index] = newRecord;

Forgot to mention that you need to call

```js
this._stripComputedFields(newRecord);
this._computeFields(newRecord);
```

to make sure we have up-to-date computed fields for the new record. (`_stripComputedFields` isn't necessary if `newRecord` is supposed not to contain old computed fields.)

Comment 28

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

https://reviewboard.mozilla.org/r/151928/#review163982

> Forgot to mention that you need to call
> 
> ```js
> this._stripComputedFields(newRecord);
> this._computeFields(newRecord);
> ```
> 
> to make sure we have up-to-date computed fields for the new record. (`_stripComputedFields` isn't necessary if `newRecord` is supposed not to contain old computed fields.)

`newRecord` shouldn't have computed fields, since we don't sync them, but I'll make sure we call `_computeFields` here and everywhere else we insert synced records.

Comment 29

[Lina Butler [:lina]]

Comment on attachment 8880638 [details]
Bug 1363995 - Implement autofill record reconciliation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151928/diff/7-8/

Comment 30

[Luke Chang [:lchang] (inactive)]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #28)
> `newRecord` shouldn't have computed fields, since we don't sync them, but
> I'll make sure we call `_computeFields` here and everywhere else we insert
> synced records.

Thanks.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6a547a170d
Implement autofill record reconciliation. r=seanlee,lchang,markh

Comment 32

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

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