1374500 - New sync engines for addresses and credit-cards

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

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

This bug is to implement a new sync engine for addresses and credit-cards. The implementation will have these engines pref'd off (ie, as well as there being a normal pref for whether the engine is enabled, there will be another pref for whether the engine is available at all.

Comment 1

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

Attachment: Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 2

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

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

Even though you both have contributed to this, it would be great if you can give this a look, treating it as a review (eg, all the nits etc that comes with a review). Once we've done that, we could maybe ask Ed to do a final review?

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/150658/#review155678

A number of nits, some of which will likely just be dropped. Biggest issue is probably the dependency between sync's xpcshell tests and these, and maybe it doesn't actually matter?

::: browser/extensions/formautofill/FormAutofillSync.jsm:103
(Diff revision 1)
> +  _subStorageName: null, // overridden below.
> +  _storage: null,
> +
> +  get storage() {
> +    if (!this._storage) {
> +      Async.promiseSpinningly(profileStorage.initialize());

Is there any chance the profileStorage won't be initialized here? If not, maybe we should just throw?

::: browser/extensions/formautofill/FormAutofillSync.jsm:184
(Diff revision 1)
> +    this._log.trace("Update record", record);
> +
> +    // XXX - until we get reconcilliation logic, this is dangerous - it
> +    // unconditionally updates, which may cause DATA LOSS
> +    this.storage.update(record.id, record.entry);
> +    this._log.debug("Updated local record", record);

This function probably doesn't need to log both before and after the storage call.

::: browser/extensions/formautofill/test/unit/test_sync.js:352
(Diff revision 1)
> +        "country": "AU",
> +        "tel": "123456",
> +      },
> +    ]);
> +
> +    // TODO: Check Sync fields, verify they're both SYNCING.

Should we, err, do this?

::: browser/extensions/formautofill/test/unit/test_sync.js:377
(Diff revision 1)
> +      entry: serverCopy,
> +    }), Date.now() / 1000));
> +
> +    engine.sync();
> +
> +    // TODO: check semantics - (ie, remote copy should have same ID, local

Same, although I believe we can't do it yet (even if we mocked the function that tells us "these records are different".

So, is this test worth having when it doesn't actually test anything yet? (The answer absolutely could be "yes because we don't want to forget")

::: browser/extensions/formautofill/test/unit/xpcshell.ini:40
(Diff revision 1)
>  [test_savedFieldNames.js]
>  [test_storage_tombstones.js]
>  [test_storage_syncfields.js]
>  [test_transformFields.js]
> +[test_sync.js]
> +head = head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js

So, unfortunately, the way this is written there's a caveat that if this file's test_sync.js is run before the sync xpcshell tests, it will fail (otherwise it will pass).

Is there a way to ensure that happens?

Fixing it so that these dependencies can be required explicitly is nontrivial, unfortunately :(...

::: services/sync/modules/service.js:51
(Diff revision 1)
> -  Prefs: {module: "prefs.js", symbol: "PrefsEngine"},
> +    Prefs: {module: "prefs.js", symbol: "PrefsEngine"},
> -  Tab: {module: "tabs.js", symbol: "TabEngine"},
> +    Tab: {module: "tabs.js", symbol: "TabEngine"},
> -  ExtensionStorage: {module: "extension-storage.js", symbol: "ExtensionStorageEngine"},
> +    ExtensionStorage: {module: "extension-storage.js", symbol: "ExtensionStorageEngine"},
> +  }
> +
> +  if (Svc.Prefs.get("engine.addresses.available", false)) {

Should we check if formautofill itself is enabled too?

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

Looks good to me, modulo nits, but I also wrote a lot of it. (I don't think there was a way I could have done this from mozreview, is there?)

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/150658/#review156374

::: browser/extensions/formautofill/FormAutofillSync.jsm:67
(Diff revision 1)
> +  return result;
> +}
> +
> +// Three years in seconds
> +// XXX - stolen from forms engine, possibly should be different
> +const AUTOFILL_TTL = 3 * 365 * 24 * 60 * 60;

We probably shouldn't set a TTL at all. Form history is ephemeral, but addresses and payment data are pretty stable, and we don't expect users to have many of them.

::: browser/extensions/formautofill/FormAutofillSync.jsm:95
(Diff revision 1)
> +
> +function FormAutofillStore(name, engine) {
> +  Store.call(this, name, engine);
> +}
> +
> +FormAutofillStore.prototype = {

Nit: While we're here, we could spell this `class FormAutofillStore extends Store`, but I don't have a strong opinion about that.

::: browser/extensions/formautofill/FormAutofillSync.jsm:103
(Diff revision 1)
> +  _subStorageName: null, // overridden below.
> +  _storage: null,
> +
> +  get storage() {
> +    if (!this._storage) {
> +      Async.promiseSpinningly(profileStorage.initialize());

The storage methods call `ensureDataReady`, so in theory it's OK if we skip initialization here. OTOH, `ensureDataReady` will read the file synchronously on the main thread, which isn't ideal. This logic should be cleaner once Edouard's promise conversion patch lands.

::: browser/extensions/formautofill/FormAutofillSync.jsm:166
(Diff revision 1)
> +    let record = new AutofillRecord(collection, id);
> +    record.entry = this.storage.get(id, {
> +      noComputedFields: true,
> +    });
> +    if (!record.entry) {
> +      // We should consider getting a more authortative indication it's actually deleted.

This is probably OK. We could pass `includeDeleted: true`, but we shouldn't be asked to create a record for a profile we've never seen (no entry and no tombstone). If we are, I think uploading a tombstone is a safe fallback.

::: browser/extensions/formautofill/FormAutofillSync.jsm:173
(Diff revision 1)
> +      record.deleted = true;
> +    } else {
> +      // The GUID is already stored in record.id, so we nuke it from the entry
> +      // itself to save a tiny bit of space. The profileStorage clones profiles,
> +      // so nuking in-place is OK.
> +      delete record.entry.guid;

I wonder if we should consider removing some of the other internal fields, too. `timeLastUsed` and `timesUsed` are local-only, and we could argue that `timeLastModified` should be local, too. (We do want to sync `version` and `timeCreated`, though).

OTOH, this could be an issue if we decide later that we *do* want to sync these fields, and round-trip the record through an older client (`version` can help with this). So maybe the right approach is to sync everything except `guid`, and have the receiving client decide what to keep or filter out.

::: browser/extensions/formautofill/FormAutofillSync.jsm:181
(Diff revision 1)
> +  },
> +
> +  _doUpdateRecord(record) {
> +    this._log.trace("Update record", record);
> +
> +    // XXX - until we get reconcilliation logic, this is dangerous - it

Let's mention bug 1363995 here, even though we expect it to land soon after the engine.

::: browser/extensions/formautofill/FormAutofillSync.jsm:190
(Diff revision 1)
> +  },
> +
> +  // NOTE: Because we re-implement the incoming/reconcilliation logic we leave
> +  // the |create|, |remove| and |update| methods undefined - the base
> +  // implementation throws, which is what we want to happen so we can identify
> +  // any places they are "accidentally" called.

This is a good idea! :thumbsup:

::: browser/extensions/formautofill/FormAutofillSync.jsm:311
(Diff revision 1)
> +
> +  // XXX - do we need to look at, eg, the extensions.formautofill.addresses.enabled
> +  // preference and make sync a no-op when disabled? (But note that we don't
> +  // want to simply disable the engine in that case - that will explicitly
> +  // "decline" it, which probably isn't what the user expects if the feature
> +  // is enabled on a different device.)

Hmm, I think we'll still want to sync if autofill is enabled on another device.

::: browser/extensions/formautofill/test/unit/test_sync.js:17
(Diff revision 1)
> +Cu.import("resource://services-sync/service.js");
> +Cu.import("resource://services-sync/constants.js");
> +Cu.import("resource://testing-common/services/sync/utils.js");
> +Cu.import("resource://formautofill/ProfileStorage.jsm");
> +
> +let {sanitizeStorageObject, AutofillRecord, AddressesEngine} =

Nit: My impression is that we should avoid top-level `let` declarations, at least in JSMs (bug 1202902). This isn't a JSM, so I think it's OK, but still something I look for.

::: browser/extensions/formautofill/test/unit/test_sync.js:99
(Diff revision 1)
> +  generateNewKeys(Service.collectionKeys);
> +
> +  await SyncTestingInfrastructure(server);
> +
> +  let collection = server.user("foo").collection("addresses");
> +  Service.scheduler.syncThreshold = 10000000;

Can you help me understand why this needs to be set twice?

::: browser/extensions/formautofill/test/unit/test_sync.js:292
(Diff revision 1)
> +      },
> +      {
> +        guid: remoteGuid,
> +      },
> +    ]);
> +    // XXX - check tombstone on the server.

Let's make sure we do that. :-)

::: browser/extensions/formautofill/test/unit/test_sync.js:352
(Diff revision 1)
> +        "country": "AU",
> +        "tel": "123456",
> +      },
> +    ]);
> +
> +    // TODO: Check Sync fields, verify they're both SYNCING.

Yes. :-) I think we decided that `NEW` records won't have a `_sync` field, and `SYNCING` ones will, so the test can check for that.

::: browser/extensions/formautofill/test/unit/test_sync.js:377
(Diff revision 1)
> +      entry: serverCopy,
> +    }), Date.now() / 1000));
> +
> +    engine.sync();
> +
> +    // TODO: check semantics - (ie, remote copy should have same ID, local

Let's move this test to bug 1363995. The test will need some tweaking to actually make the records conflict, anyway.

::: services/sync/modules/engines.js:1452
(Diff revision 1)
>      // deleted flags.
>      if (item.deleted) {
>        // If the item doesn't exist locally, there is nothing for us to do. We
>        // can't check for duplicates because the incoming record has no data
>        // which can be used for duplicate detection.
> -      if (!existsLocally) {
> +      if (!existsLocally && !this.applyIncomingTombstones) {

Since we override reconciliation completely, I'm wondering if `applyIncomingTombstones` can be removed.

::: services/sync/modules/service.js:51
(Diff revision 1)
> -  Prefs: {module: "prefs.js", symbol: "PrefsEngine"},
> +    Prefs: {module: "prefs.js", symbol: "PrefsEngine"},
> -  Tab: {module: "tabs.js", symbol: "TabEngine"},
> +    Tab: {module: "tabs.js", symbol: "TabEngine"},
> -  ExtensionStorage: {module: "extension-storage.js", symbol: "ExtensionStorageEngine"},
> +    ExtensionStorage: {module: "extension-storage.js", symbol: "ExtensionStorageEngine"},
> +  }
> +
> +  if (Svc.Prefs.get("engine.addresses.available", false)) {

It probably wouldn't hurt, and then we can avoid loading an engine that doesn't do anything.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/150658/#review156374

> Nit: While we're here, we could spell this `class FormAutofillStore extends Store`, but I don't have a strong opinion about that.

Initially we were going to do this, but eslint got very mad since it doesn't really understand "EXPORTED_SYMBOLS" and how `this` works in jsms.

Comment 7

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

Thom, could you please incorporate your and Kit's comments into a new patch?

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/150658/#review156374

I can't seem to close/drop these issues, presumably because I'm not the author of this patch. Although, it looks like this might loose all the context when I submit the updated patch, so it might not matter.

I'm pretty sure I hit all of these, though.

> The storage methods call `ensureDataReady`, so in theory it's OK if we skip initialization here. OTOH, `ensureDataReady` will read the file synchronously on the main thread, which isn't ideal. This logic should be cleaner once Edouard's promise conversion patch lands.

Sounds like we should keep the spinning for now then, to avoid a risk of main thread IO.

> I wonder if we should consider removing some of the other internal fields, too. `timeLastUsed` and `timesUsed` are local-only, and we could argue that `timeLastModified` should be local, too. (We do want to sync `version` and `timeCreated`, though).
> 
> OTOH, this could be an issue if we decide later that we *do* want to sync these fields, and round-trip the record through an older client (`version` can help with this). So maybe the right approach is to sync everything except `guid`, and have the receiving client decide what to keep or filter out.

I think your 2nd point is why we were adding these. It's easy to keep them in, and avoids a case where we regret removing them later.

> Hmm, I think we'll still want to sync if autofill is enabled on another device.

After some discussion in IRC, this sounds like a way to get sync involved in an awkward privacy debate we don't want to be a part of. I think the right call here is to do what the comment suggests, so I've done that.

> Nit: My impression is that we should avoid top-level `let` declarations, at least in JSMs (bug 1202902). This isn't a JSM, so I think it's OK, but still something I look for.

I don't think it matters for test_foo files, but it does matter for head_foo, and you'll probably notice I've changed a few `const`s to `var`s because of this.

> Can you help me understand why this needs to be set twice?

It does not.

> Let's make sure we do that. :-)

Uh, really? This test currently fails and my reading of the code says that we actually would *not* expect a local or remote tombstone to be present.

> Let's move this test to bug 1363995. The test will need some tweaking to actually make the records conflict, anyway.

I've removed it from this patch and will let whoever ends up landing that patch add it (assuming you?)

> It probably wouldn't hurt, and then we can avoid loading an engine that doesn't do anything.

On further thought, we don't want to have things broken if/when the autofill prefs are changed at runtime, so I think we should always load these if they're enabled.

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

Let me know if I should have r?eoger instead. Wasn't sure.

Comment 11

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 12

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 13

[Thom Chiovoloni [:tcsc] (ex-moco)]

Sorry about that, removed (trivial) autofill sync engine and replaced the pref check with a call to a function in FormAutofillUtils.jsm. Both of these were at the request of MattN in IRC (for context, the relevant discussions were: https://gist.github.com/thomcc/dbd5b79195ae02580e1aebf7fc4cc19e)

(Two requests since for the first one I forgot to delete some dead code -- sorry!)

Comment 14

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

Marking the old patch as obsolete -- not sure if this means I didn't quite use the right incantation for mozreview.

Comment 15

[Lina Butler [:lina]]

Comment on attachment 8879360 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/150658/#review156374

> Uh, really? This test currently fails and my reading of the code says that we actually would *not* expect a local or remote tombstone to be present.

You're right, we changed this so we only dedupe unsynced. The test is fixed in bug 1363995, but we should probably move the fix into this patch.

> I've removed it from this patch and will let whoever ends up landing that patch add it (assuming you?)

Will do.

> On further thought, we don't want to have things broken if/when the autofill prefs are changed at runtime, so I think we should always load these if they're enabled.

Sounds good.

Comment 16

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

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/152288/#review158308

::: browser/extensions/formautofill/FormAutofillSync.jsm:100
(Diff revision 3)
> +  _subStorageName: null, // overridden below.
> +  _storage: null,
> +
> +  get storage() {
> +    if (!this._storage) {
> +      // Not strictly necessary since the methods call ensureDataReady, but we'd

I think the initialize call is strictly necessary - initialize sets up `this._store` and `ensureDataReady` references `this._store`. (ie, remove the comment)

::: browser/extensions/formautofill/FormAutofillSync.jsm:304
(Diff revision 3)
> +  // the priority for this engine is == addons, so will happen after bookmarks
> +  // prefs and tabs, but before forms, history, etc.
> +  syncPriority: 5,
> +
> +  _sync() {
> +    if (!FormAutofillUtils.isCollectionEnabled(this._store._subStorageName)) {

Should we just call http://searchfox.org/mozilla-central/source/browser/extensions/formautofill/FormAutofillPreferences.jsm#39 instead of adding the new function to FormAutofillUtils? That would mean moving this check into the concrete engine below, but that seems OK

::: browser/extensions/formautofill/FormAutofillSync.jsm:350
(Diff revision 3)
> +    this._store.storage.resetSync();
> +  },
> +};
> +
> +// The concrete engines
> +// XXX - these are (obviously) only partial...

I think this is largely complete now (ie, this XXX comment should be removed)

::: browser/extensions/formautofill/FormAutofillUtils.jsm:164
(Diff revision 3)
>      } while (parent);
>  
>      return [];
>    },
> +
> +  isCollectionEnabled(collectionName) {

see comment above - we can probably avoid this new function

::: services/sync/modules/engines.js
(Diff revision 3)
>                      remoteAge);
>  
>      // We handle deletions first so subsequent logic doesn't have to check
>      // deleted flags.
>      if (item.deleted) {
> -      // If the item doesn't exist locally, there is nothing for us to do. We

As discussed, this patch needs no changes to engines.js

Comment 17

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 18

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 19

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 20

[Lina Butler [:lina]]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/152288/#review157362

Ship it!

::: browser/extensions/formautofill/FormAutofillSync.jsm:49
(Diff revision 3)
> +    }
> +  }
> +  return null;
> +}
> +
> +// A helper to sanitize address and creditcard records suitable for logging.

Nit: Let's call these "storage records" or "entries", since we emphasize in `cleartextToString` that those are Sync records.

::: browser/extensions/formautofill/FormAutofillSync.jsm:51
(Diff revision 3)
> +  return null;
> +}
> +
> +// A helper to sanitize address and creditcard records suitable for logging.
> +function sanitizeStorageObject(ob) {
> +  const whitelist = ["timeCreated", "timeLastUsed", "timeLastModified"];

Probably everything in `INTERNAL_FIELDS`, at least for now, is safe to log as is.

::: browser/extensions/formautofill/FormAutofillSync.jsm:120
(Diff revision 3)
> +
> +  changeItemID(oldID, newID) {
> +    this.storage.changeGUID(oldID, newID);
> +  },
> +
> +  // Note: this function should return false in cases where we only have a

Nit: Let's change this to something like "Intentionally returns false". "Should" here reads like a TODO.

::: browser/extensions/formautofill/FormAutofillSync.jsm:154
(Diff revision 3)
> +
> +    // 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()
> +    // handles for us.)
> +    this._log.trace("Add record", remoteRecord);
> +    remoteRecord.entry.guid = remoteRecord.id;

Side note: I added `toEntry` and `fromEntry` methods to `AutofillRecord` in bug 1377246 (maybe `StorageRecord` is a better name than `Entry`), because there are other places we convert between the two. But I'm not sure if we want to keep that.

::: browser/extensions/formautofill/FormAutofillSync.jsm:182
(Diff revision 3)
> +  _doUpdateRecord(record) {
> +    this._log.trace("Updating record", record);
> +
> +    // TODO (bug 1363995) - until we get reconcilliation logic, this is
> +    // dangerous, it unconditionally updates, which may cause DATA LOSS.
> +    this.storage.update(record.id, record.entry);

I wonder if we should throw here for now, since `update` doesn't support `sourceSync` and does the wrong thing, anyway. On the other hand, if we coordinate landing this, bug 1377246, and bug 1363995, it doesn't matter much.

::: browser/extensions/formautofill/test/unit/test_sync.js:50
(Diff revision 3)
> +    includePrivateFields: true,
> +    includeDeleted: true,
> +  });
> +  expected.sort((a, b) => a.guid.localeCompare(b.guid));
> +  profiles.sort((a, b) => a.guid.localeCompare(b.guid));
> +  let doCheckObject = (a, b) => {



::: services/sync/modules/service.js:56
(Diff revision 3)
> +    result["Addresses"] = {
> +      module: "resource://formautofill/FormAutofillSync.jsm",
> +      symbol: "AddressesEngine",
> +    };
> +  }
> +  if (Svc.Prefs.get("engine.creditcards.available", false)) {

Comment 21

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 22

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

Thom, can you please upload a new version of the patch now that we've split some and landed it via bug 1380094?

Comment 23

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 24

[Thom Chiovoloni [:tcsc] (ex-moco)]

Done. Also rebased and fixed issues now that bug 1210296 and bug 1368008 have landed

Comment 25

[Lina Butler [:lina]]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

https://reviewboard.mozilla.org/r/152288/#review163164

::: browser/extensions/formautofill/FormAutofillSync.jsm:47
(Diff revision 8)
> +    if (same) {
> +      return profile.guid;
> +    }
> +  }
> +  dump(`
> +    FAILED TO FIND DUPE OF ${JSON.stringify(record)} IN ${JSON.stringify(profileStorage.addresses.getAll())}

Oops. :-) Though this is removed in bug 1377246, anyway.

Comment 26

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8880923 [details]
Bug 1374500 - Add a new sync engine for addresses and credit-cards.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9fcc95f6c42
Add a new sync engine for addresses and credit-cards. r=markh,kitcambridge

Comment 28

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

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