Bug 1363999 - Add sync metadata to formautofill records. r? draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 09 May 2017 15:27:09 +1000
changeset 600697 a975e078842992e6f7245f248eb0a5e01ca23c69
parent 600696 4eff4109a7c374512e08c570fd7730092e9f9000
child 600698 2c1a73cb5b6912dc5f342002113a9ebe1c4cf44b
push id65844
push userbmo:kit@mozilla.com
push dateTue, 27 Jun 2017 18:57:45 +0000
bugs1363999
milestone56.0a1
Bug 1363999 - Add sync metadata to formautofill records. r? This patch lands the metadata and related functions to the ProfileStorage necessary for Sync to function. * New "public" functions which are intended to be used only by Sync: ** pullSyncChanges: Gets metadata about what Sync needs to upload to the server. ** pushSyncChanges: Apply the changes made by Sync. ** resetSync: Reset all Sync metadata, used when Sync is disconnected from a device. ** changeGUID to change the GUID of an item that has no sync metadata. * New optional Sync metdata for a record. Once a record is marked as Syncing, it carries around a new _sync field. Currently this contains only a changeCounter. All records are marked as Syncing once pullSyncChanges is called - after this call, all local records will have Sync metadata. * Changes to tombstones semantics: If an item carries no Sync metadata (ie, hasn't previously been synced), then no tombstone will be left behind when it is deleted. * Many existing functions get an optional {includePrivateFields} option, which will include all fields with a leading underscore (_) - which obviously includes this new _sync field - to be returned in the record. * Many existing functions get an optional {sourceSync} option, which indicates Sync itself is making the change. This significantly changes the semantics of some operations (eg, counters will not be incremented, tombstones can be resurrected, etc) MozReview-Commit-ID: 3DjzNiA8buE
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_storage_syncfields.js
browser/extensions/formautofill/test/unit/test_storage_tombstones.js
browser/extensions/formautofill/test/unit/xpcshell.ini
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -37,16 +37,17 @@
  *       address-line3,
  *       country-name,
  *
  *       // metadata
  *       timeCreated,          // in ms
  *       timeLastUsed,         // in ms
  *       timeLastModified,     // in ms
  *       timesUsed
+ *       _sync: { ... sync metadata },
  *     }
  *   ],
  *   creditCards: [
  *     {
  *       guid,                 // 12 characters
  *       version,              // schema version in integer
  *
  *       // credit card fields
@@ -63,19 +64,34 @@
  *       cc-additional-name,
  *       cc-family-name,
  *
  *       // metadata
  *       timeCreated,          // in ms
  *       timeLastUsed,         // in ms
  *       timeLastModified,     // in ms
  *       timesUsed
+ *       _sync: { ... optional sync metadata },
  *     }
  *   ]
  * }
+ *
+ * Sync Metadata:
+ *
+ * Records may also have a _sync field, which consists of:
+ * {
+ *   changeCounter, // integer - the number of changes made since a last sync.
+ * }
+ *
+ * Records with such a field have previously been synced. Records without such
+ * a field are yet to be synced, so are treated specially in some cases (eg,
+ * they don't need a tombstone, de-duping logic treats them as special etc).
+ * Records without the field are always considered "dirty" from Sync's POV
+ * (meaning they will be synced on the next sync), at which time they will gain
+ * this new field.
  */
 
 "use strict";
 
 // We expose a singleton from this module. Some tests may import the
 // constructor via a backstage pass.
 this.EXPORTED_SYMBOLS = ["profileStorage"];
 
@@ -183,61 +199,108 @@ class AutofillRecords {
     return this._schemaVersion;
   }
 
   /**
    * Adds a new record.
    *
    * @param {Object} record
    *        The new record for saving.
+   * @param {boolean} [options.sourceSync = false]
+   *        Did sync generate this addition?
    * @returns {string}
    *          The GUID of the newly added item..
    */
-  add(record) {
+  add(record, {sourceSync = false} = {}) {
     this.log.debug("add:", record);
+
+    if (sourceSync) {
+      // Remove tombstones for incoming items that were changed on another
+      // device. Local deletions always lose to avoid data loss.
+      let index = this._findIndexByGUID(record.guid, {
+        includeDeleted: true,
+      });
+      if (index > -1) {
+        let existing = this._store.data[this._collectionName][index];
+        if (existing.deleted) {
+          this._store.data[this._collectionName].splice(index, 1);
+        } else {
+          throw new Error(`Record ${record.guid} already exists`);
+        }
+      }
+      let recordToSave = Object.assign({
+        // `timeLastUsed` and `timesUsed` are always local.
+        timeLastUsed: 0,
+        timesUsed: 0,
+      }, record);
+      return this._saveRecord(recordToSave, {sourceSync});
+    }
+
+    if (record.deleted) {
+      return this._saveRecord(record, {sourceSync});
+    }
+
+    let recordToSave = this._clone(record);
+    this._normalizeRecord(recordToSave);
+
+    recordToSave.guid = this._generateGUID();
+    recordToSave.version = this.version;
+
+    // Metadata
+    let now = Date.now();
+    recordToSave.timeCreated = now;
+    recordToSave.timeLastModified = now;
+    recordToSave.timeLastUsed = 0;
+    recordToSave.timesUsed = 0;
+
+    return this._saveRecord(recordToSave);
+  }
+
+  _saveRecord(record, {sourceSync = false} = {}) {
+    if (!record.guid) {
+      throw new Error("Record missing GUID");
+    }
+
     let recordToSave;
     if (record.deleted) {
-      if (!record.guid) {
-        throw new Error("you must specify the GUID when creating a tombstone");
-      }
       if (this._findByGUID(record.guid, {includeDeleted: true})) {
         throw new Error("a record with this GUID already exists");
       }
       recordToSave = {
         guid: record.guid,
         timeLastModified: record.timeLastModified || Date.now(),
         deleted: true,
       };
     } else {
-      recordToSave = this._clone(record);
-      this._normalizeRecord(recordToSave);
-
-      let guid;
-      while (!guid || this._findByGUID(guid)) {
-        guid = gUUIDGenerator.generateUUID().toString()
-                             .replace(/[{}-]/g, "").substring(0, 12);
-      }
-      recordToSave.guid = guid;
-      recordToSave.version = this.version;
-
-      // Metadata
-      let now = Date.now();
-      recordToSave.timeCreated = now;
-      recordToSave.timeLastModified = now;
-      recordToSave.timeLastUsed = 0;
-      recordToSave.timesUsed = 0;
+      recordToSave = record;
     }
 
     this._store.data[this._collectionName].push(recordToSave);
+
+
+    if (sourceSync) {
+      let sync = this._getSyncMetaData(recordToSave, true);
+      sync.changeCounter = 0;
+    }
+
     this._store.saveSoon();
 
-    Services.obs.notifyObservers(null, "formautofill-storage-changed", "add");
+    Services.obs.notifyObservers({wrappedJSObject: {sourceSync}}, "formautofill-storage-changed", "add");
     return recordToSave.guid;
   }
 
+  _generateGUID() {
+    let guid;
+    while (!guid || this._findByGUID(guid)) {
+      guid = gUUIDGenerator.generateUUID().toString()
+                           .replace(/[{}-]/g, "").substring(0, 12);
+    }
+    return guid;
+  }
+
   /**
    * Update the specified record.
    *
    * @param  {string} guid
    *         Indicates which record to update.
    * @param  {Object} record
    *         The new record used to overwrite the old one.
    */
@@ -255,24 +318,28 @@ class AutofillRecords {
       if (recordToUpdate[field] !== undefined) {
         recordFound[field] = recordToUpdate[field];
       } else {
         delete recordFound[field];
       }
     }
 
     recordFound.timeLastModified = Date.now();
+    let syncMetadata = this._getSyncMetaData(recordFound);
+    if (syncMetadata) {
+      syncMetadata.changeCounter += 1;
+    }
 
     this._store.saveSoon();
 
     Services.obs.notifyObservers(null, "formautofill-storage-changed", "update");
   }
 
   /**
-   * Notifies the stroage of the use of the specified record, so we can update
+   * Notifies the storage of the use of the specified record, so we can update
    * the metadata accordingly.
    *
    * @param  {string} guid
    *         Indicates which record to be notified.
    */
   notifyUsed(guid) {
     this.log.debug("notifyUsed:", guid);
 
@@ -288,76 +355,100 @@ class AutofillRecords {
     Services.obs.notifyObservers(null, "formautofill-storage-changed", "notifyUsed");
   }
 
   /**
    * Removes the specified record. No error occurs if the record isn't found.
    *
    * @param  {string} guid
    *         Indicates which record to remove.
+   * @param  {boolean} [options.sourceSync = false]
+   *         Did Sync generate this removal?
    */
-  remove(guid) {
+  remove(guid, {sourceSync = false} = {}) {
     this.log.debug("remove:", guid);
 
-    let index = this._findIndexByGUID(guid);
-    if (index == -1) {
-      this.log.warn("attempting to remove non-existing entry", guid);
-      return;
+    if (sourceSync) {
+      this._removeSyncedRecord(guid);
+    } else {
+      let index = this._findIndexByGUID(guid, {includeDeleted: false});
+      if (index == -1) {
+        this.log.warn("attempting to remove non-existing entry", guid);
+        return;
+      }
+      let existing = this._store.data[this._collectionName][index];
+      if (existing.deleted) {
+        return; // already a tombstone - don't touch it.
+      }
+      let existingSync = this._getSyncMetaData(existing);
+      if (existingSync) {
+        // existing sync metadata means it has been synced. This means we must
+        // leave a tombstone behind.
+        this._store.data[this._collectionName][index] = {
+          guid,
+          timeLastModified: Date.now(),
+          deleted: true,
+          _sync: existingSync,
+        };
+        existingSync.changeCounter++;
+      } else {
+        // If there's no sync meta-data, this record has never been synced, so
+        // we can delete it.
+        this._store.data[this._collectionName].splice(index, 1);
+      }
     }
-    // replace the record with a tombstone.
-    this._store.data[this._collectionName][index] = {
-      guid,
-      timeLastModified: Date.now(),
-      deleted: true,
-    };
+
     this._store.saveSoon();
-
-    Services.obs.notifyObservers(null, "formautofill-storage-changed", "remove");
+    Services.obs.notifyObservers({wrappedJSObject: {sourceSync}}, "formautofill-storage-changed", "remove");
   }
 
   /**
    * Returns the record with the specified GUID.
    *
    * @param   {string} guid
    *          Indicates which record to retrieve.
+   * @param   {boolean} [options.includePrivateFields = false]
+   *          Should fields starting with underscore be included in the result?
    * @returns {Object}
    *          A clone of the record.
    */
-  get(guid) {
+  get(guid, {includePrivateFields = false} = {}) {
     this.log.debug("get:", guid);
 
     let recordFound = this._findByGUID(guid);
     if (!recordFound) {
       return null;
     }
 
     // The record is cloned to avoid accidental modifications from outside.
-    let clonedRecord = this._clone(recordFound);
+    let clonedRecord = this._clone(recordFound, {includePrivateFields});
     this._recordReadProcessor(clonedRecord);
     return clonedRecord;
   }
 
   /**
    * Returns all records.
    *
    * @param   {Object} config
    *          Specifies how data will be retrieved.
-   * @param   {boolean} [options.noComputedFields = false]
+   * @param   {boolean} [config.noComputedFields = false]
    *          Returns raw record without those computed fields.
-   * @param   {boolean} [options.includeDeleted = false]
+   * @param   {boolean} [config.includeDeleted = false]
    *          Also return any tombstone records.
+   * @param   {boolean} [config.includePrivateFields = false]
+   *          Should fields starting with underscore be included in the result?
    * @returns {Array.<Object>}
    *          An array containing clones of all records.
    */
-  getAll({noComputedFields = false, includeDeleted = false} = {}) {
-    this.log.debug("getAll", noComputedFields, includeDeleted);
+  getAll({noComputedFields = false, includeDeleted = false, includePrivateFields = false} = {}) {
+    this.log.debug("getAll", noComputedFields, includeDeleted, includePrivateFields);
 
     let records = this._store.data[this._collectionName].filter(r => !r.deleted || includeDeleted);
     // Records are cloned to avoid accidental modifications from outside.
-    let clonedRecords = records.map(this._clone);
+    let clonedRecords = records.map(r => this._clone(r, {includePrivateFields}));
     clonedRecords.forEach(record => this._recordReadProcessor(record, {noComputedFields}));
     return clonedRecords;
   }
 
   /**
    * Returns the filtered records based on input's information and searchString.
    *
    * @returns {Array.<Object>}
@@ -379,18 +470,201 @@ class AutofillRecords {
 
       return name && name.toLowerCase().startsWith(lcSearchString);
     });
 
     this.log.debug("getByFilter:", "Returning", result.length, "result(s)");
     return result;
   }
 
-  _clone(record) {
-    return Object.assign({}, record);
+  /**
+   * Functions intended to be used in the support of Sync.
+   */
+
+  _removeSyncedRecord(guid) {
+    let index = this._findIndexByGUID(guid, {includeDeleted: true});
+    if (index == -1) {
+      // Removing a record we don't know about. It may have been synced and
+      // removed by another device before we saw it. Store the tombstone in
+      // case the server is later wiped and we need to reupload everything.
+      let tombstone = {
+        guid,
+        timeLastModified: Date.now(),
+        deleted: true,
+      };
+      this._store.data[this._collectionName].push(tombstone);
+
+      let sync = this._getSyncMetaData(tombstone, true);
+      sync.changeCounter = 0;
+      return;
+    }
+
+    let existing = this._store.data[this._collectionName][index];
+    let sync = this._getSyncMetaData(existing, true);
+    if (sync.changeCounter > 0) {
+      // Deleting a record with unsynced local changes. To avoid potential
+      // data loss, we ignore the deletion in favor of the changed record.
+      this.log.info("Ignoring deletion for record with local changes",
+                    existing);
+      return;
+    }
+
+    if (existing.deleted) {
+      this.log.info("Ignoring deletion for tombstone", existing);
+      return;
+    }
+
+    // Removing a record that's not changed locally, and that's not already
+    // deleted. Replace the record with a synced tombstone.
+    this._store.data[this._collectionName][index] = {
+      guid,
+      timeLastModified: Date.now(),
+      deleted: true,
+      _sync: sync,
+    };
+  }
+
+  /**
+   * Provide an object that describes the changes to sync.
+   *
+   * This is called at the start of the sync process to determine what needs
+   * to be updated on the server. As the server is updated, sync will update
+   * entries in the returned object, and when sync is complete it will pass
+   * the object to pushSyncChanges, which will apply the changes to the store.
+   *
+   * @returns {object}
+   *          An object describing the changes to sync.
+   */
+  pullSyncChanges() {
+    let changes = {};
+
+    let profiles = this._store.data[this._collectionName];
+    for (let profile of profiles) {
+      let sync = this._getSyncMetaData(profile, true);
+      if (sync.changeCounter < 1) {
+        if (sync.changeCounter != 0) {
+          this.log.error("negative change counter", profile);
+        }
+        continue;
+      }
+      changes[profile.guid] = {
+        profile,
+        counter: sync.changeCounter,
+        modified: profile.timeLastModified,
+        synced: false,
+      };
+    }
+    this._store.saveSoon();
+
+    return changes;
+  }
+
+  /**
+   * Apply the metadata changes made by Sync.
+   *
+   * This is called with metadata about what was synced - see pullSyncChanges.
+   *
+   * @param {object} changes
+   *        The possibly modified object obtained via pullSyncChanges.
+   */
+  pushSyncChanges(changes) {
+    for (let [guid, {counter, synced}] of Object.entries(changes)) {
+      if (!synced) {
+        continue;
+      }
+      let recordFound = this._findByGUID(guid, {includeDeleted: true});
+      if (!recordFound) {
+        this.log.warn("No profile found to persist changes for guid " + guid);
+        continue;
+      }
+      let sync = this._getSyncMetaData(recordFound, true);
+      sync.changeCounter = Math.max(0, sync.changeCounter - counter);
+    }
+    this._store.saveSoon();
+  }
+
+  /**
+   * Reset all sync metadata for all items.
+   *
+   * This is called when Sync is disconnected from this device. All sync
+   * metadata for all items is removed.
+   */
+  resetSync() {
+    for (let record of this._store.data[this._collectionName]) {
+      delete record._sync;
+    }
+    // XXX - we should probably also delete all tombstones?
+    this.log.info("All sync metadata was reset");
+  }
+
+  /**
+   * Changes the GUID of an item. This should be called only by Sync. There
+   * must be an existing record with oldID and it must never have been synced
+   * or an error will be thrown. There must be no existing record with newID.
+   *
+   * No tombstone will be created for the old GUID - we check it hasn't
+   * been synced, so no tombstone is necessary.
+   *
+   * @param   {string} oldID
+   *          GUID of the existing item to change the GUID of.
+   * @param   {string} newID
+   *          The new GUID for the item.
+   */
+  changeGUID(oldID, newID) {
+    this.log.debug("changeGUID: ", oldID, newID);
+    if (oldID == newID) {
+      throw new Error("changeGUID: old and new IDs are the same");
+    }
+    if (this._findIndexByGUID(newID) >= 0) {
+      throw new Error("changeGUID: record with destination id exists already");
+    }
+
+    let index = this._findIndexByGUID(oldID);
+    let profile = this._store.data[this._collectionName][index];
+    if (!profile) {
+      throw new Error("changeGUID: no source record");
+    }
+    if (this._getSyncMetaData(profile)) {
+      throw new Error("changeGUID: existing record has already been synced");
+    }
+
+    profile.guid = newID;
+
+    this._store.saveSoon();
+  }
+
+  // Used to get, and optionally create, sync metadata. Brand new records will
+  // *not* have sync meta-data - it will be created when they are first
+  // synced.
+  _getSyncMetaData(record, forceCreate = false) {
+    if (!record._sync && forceCreate) {
+      // create default metadata and indicate we need to save.
+      record._sync = {
+        changeCounter: 1,
+      };
+      this._store.saveSoon();
+    }
+    return record._sync;
+  }
+
+  /**
+   * Internal helper functions.
+   */
+
+  _clone(record, {includePrivateFields = false} = {}) {
+    let result = Object.assign({}, record);
+    if (includePrivateFields) {
+      return result;
+    }
+    for (let key of Object.keys(result)) {
+      if (key.startsWith("_")) {
+        delete result[key];
+      }
+    }
+    return result;
   }
 
   _findByGUID(guid, {includeDeleted = false} = {}) {
     let found = this._findIndexByGUID(guid, {includeDeleted});
     return found < 0 ? undefined : this._store.data[this._collectionName][found];
   }
 
   _findIndexByGUID(guid, {includeDeleted = false} = {}) {
@@ -408,16 +682,22 @@ class AutofillRecords {
       }
       if (typeof record[key] !== "string" &&
           typeof record[key] !== "number") {
         throw new Error(`"${key}" contains invalid data type.`);
       }
     }
   }
 
+  // A test-only helper.
+  _nukeAllRecords() {
+    this._store.data[this._collectionName] = [];
+    // test-only, so there's no good reason to request a save!
+  }
+
   // An interface to be inherited.
   _recordReadProcessor(record, {noComputedFields = false} = {}) {}
 
   // An interface to be inherited.
   _recordWriteProcessor(record) {}
 
   // An interface to be inherited.
   mergeIfPossible(guid, record) {}
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -119,16 +119,20 @@ add_task(async function test_initialize(
   let data = profileStorage._store.data;
   Assert.deepEqual(data.addresses, []);
 
   await profileStorage._saveImmediately();
 
   profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
 
   Assert.deepEqual(profileStorage._store.data, data);
+  for (let {_sync} of profileStorage._store.data.addresses) {
+    Assert.ok(_sync);
+    Assert.equal(_sync.changeCounter, 1);
+  }
 });
 
 add_task(async function test_getAll() {
   let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
                                                 [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
 
@@ -145,16 +149,23 @@ add_task(async function test_getAll() {
   addresses = profileStorage.addresses.getAll({noComputedFields: true});
   do_check_eq(addresses[0].name, undefined);
   do_check_eq(addresses[0]["address-line1"], undefined);
   do_check_eq(addresses[0]["address-line2"], undefined);
 
   // Modifying output shouldn't affect the storage.
   addresses[0].organization = "test";
   do_check_record_matches(profileStorage.addresses.getAll()[0], TEST_ADDRESS_1);
+
+  // Test with includePrivateFields.
+  profileStorage.addresses.pullSyncChanges(); // force sync metadata, which is what we are checking.
+  addresses = profileStorage.addresses.getAll({includePrivateFields: true});
+  Assert.ok(addresses[0]._sync && addresses[1]._sync);
+  Assert.equal(addresses[0]._sync.changeCounter, 1);
+  Assert.equal(addresses[1]._sync.changeCounter, 1);
 });
 
 add_task(async function test_get() {
   let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
                                                 [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[0].guid;
@@ -162,16 +173,21 @@ add_task(async function test_get() {
   let address = profileStorage.addresses.get(guid);
   do_check_record_matches(address, TEST_ADDRESS_1);
 
   // Modifying output shouldn't affect the storage.
   address.organization = "test";
   do_check_record_matches(profileStorage.addresses.get(guid), TEST_ADDRESS_1);
 
   do_check_eq(profileStorage.addresses.get("INVALID_GUID"), null);
+
+  // includePrivateFields should include _sync, which should have a value of 1
+  profileStorage.addresses.pullSyncChanges(); // force sync metadata, which is what we are checking.
+  let {_sync} = profileStorage.addresses.get(guid, {includePrivateFields: true});
+  do_check_eq(_sync.changeCounter, 1);
 });
 
 add_task(async function test_getByFilter() {
   let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
                                                 [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let filter = {info: {fieldName: "street-address"}, searchString: "Some"};
   let addresses = profileStorage.addresses.getByFilter(filter);
@@ -237,21 +253,24 @@ add_task(async function test_update() {
                                           (subject, data) => data == "update");
 
   do_check_neq(addresses[1].country, undefined);
 
   profileStorage.addresses.update(guid, TEST_ADDRESS_3);
   await onChanged;
   await profileStorage._saveImmediately();
 
-  let address = profileStorage.addresses.get(guid);
+  profileStorage.addresses.pullSyncChanges(); // force sync metadata, which we check below.
+
+  let address = profileStorage.addresses.get(guid, {includePrivateFields: true});
 
   do_check_eq(address.country, undefined);
   do_check_neq(address.timeLastModified, timeLastModified);
   do_check_record_matches(address, TEST_ADDRESS_3);
+  do_check_eq(address._sync.changeCounter, 1);
 
   Assert.throws(
     () => profileStorage.addresses.update("INVALID_GUID", TEST_ADDRESS_3),
     /No matching record\./
   );
 
   Assert.throws(
     () => profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_INVALID_FIELD),
@@ -268,17 +287,16 @@ add_task(async function test_notifyUsed(
   let timeLastUsed = addresses[1].timeLastUsed;
   let timesUsed = addresses[1].timesUsed;
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "notifyUsed");
 
   profileStorage.addresses.notifyUsed(guid);
   await onChanged;
-  await profileStorage._saveImmediately();
 
   let address = profileStorage.addresses.get(guid);
 
   do_check_eq(address.timesUsed, timesUsed + 1);
   do_check_neq(address.timeLastUsed, timeLastUsed);
 
   Assert.throws(() => profileStorage.addresses.notifyUsed("INVALID_GUID"),
     /No matching record\./);
@@ -293,17 +311,16 @@ add_task(async function test_remove() {
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "remove");
 
   do_check_eq(addresses.length, 2);
 
   profileStorage.addresses.remove(guid);
   await onChanged;
-  await profileStorage._saveImmediately();
 
   addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 1);
 
   do_check_eq(profileStorage.addresses.get(guid), null);
 });
 
new file mode 100644
--- /dev/null
+++ b/browser/extensions/formautofill/test/unit/test_storage_syncfields.js
@@ -0,0 +1,431 @@
+/**
+ * Tests ProfileStorage objects support for sync related fields.
+ */
+
+"use strict";
+
+// The duplication of some of these fixtures between tests is unfortunate.
+const TEST_STORE_FILE_NAME = "test-profile.json";
+
+const TEST_ADDRESS_1 = {
+  "given-name": "Timothy",
+  "additional-name": "John",
+  "family-name": "Berners-Lee",
+  organization: "World Wide Web Consortium",
+  "street-address": "32 Vassar Street\nMIT Room 32-G524",
+  "address-level2": "Cambridge",
+  "address-level1": "MA",
+  "postal-code": "02139",
+  country: "US",
+  tel: "+1 617 253 5702",
+  email: "timbl@w3.org",
+};
+
+const TEST_ADDRESS_2 = {
+  "street-address": "Some Address",
+  country: "US",
+};
+
+const TEST_ADDRESS_3 = {
+  "street-address": "Other Address",
+  "postal-code": "12345",
+};
+
+let do_check_record_matches = (recordWithMeta, record) => {
+  for (let key in record) {
+    do_check_eq(recordWithMeta[key], record[key]);
+  }
+};
+
+// storage.get() doesn't support getting deleted items. However, this test
+// wants to do that, so rather than making .get() support that just for this
+// test, we use this helper.
+function findGUID(storage, guid, options) {
+  let all = storage.getAll(options);
+  let records = all.filter(r => r.guid == guid);
+  equal(records.length, 1);
+  return records[0];
+}
+
+add_task(async function test_changeCounter() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1]);
+
+  let address = profileStorage.addresses.getAll({includePrivateFields: true})[0];
+  // new records don't get the sync metadata.
+  ok(!address._sync);
+  // But we can force one.
+  profileStorage.addresses.pullSyncChanges();
+  address = profileStorage.addresses.getAll({includePrivateFields: true})[0];
+  equal(address._sync.changeCounter, 1);
+});
+
+add_task(async function test_changeCounter_update() {
+  // This test should probably also check .status, and need splitting so
+  // everything that changes the status is covered (this test uses .update,
+  // which no longer is special WRT sync.)
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+
+  let addresses = profileStorage.addresses.getAll({includePrivateFields: true});
+  let guid = addresses[1].guid;
+  let timeLastModified = addresses[1].timeLastModified;
+
+  // it doesn't have a change counter yet, so updates shouldn't create one.
+  do_check_neq(addresses[1].country, undefined);
+  profileStorage.addresses.update(guid, TEST_ADDRESS_3);
+
+  addresses = profileStorage.addresses.getAll({includePrivateFields: true});
+  ok(!addresses[0]._sync);
+
+  // force sync meta-data, after which updates should bump it.
+  profileStorage.addresses.pullSyncChanges();
+
+  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
+                                          (subject, data) => data == "update");
+
+
+  profileStorage.addresses.update(guid, TEST_ADDRESS_3);
+
+  await onChanged;
+
+  let address = profileStorage.addresses.get(guid, {includePrivateFields: true});
+
+  do_check_eq(address.country, undefined);
+  do_check_neq(address.timeLastModified, timeLastModified);
+  do_check_record_matches(address, TEST_ADDRESS_3);
+  do_check_eq(address._sync.changeCounter, 2, "update() updated the change counter");
+});
+
+add_task(async function test_pushChanges() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+
+  profileStorage.addresses.pullSyncChanges(); // force sync metadata for all items
+
+  let [, address] = profileStorage.addresses.getAll({includePrivateFields: true});
+  let guid = address.guid;
+
+  // Pretend we're doing a sync now, and an update occured mid-sync.
+  let changes = {
+    [guid]: {
+      profile: address,
+      counter: address._sync.changeCounter,
+      modified: address.timeLastModified,
+      synced: true,
+    },
+  };
+
+  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
+                                          (subject, data) => data == "update");
+  profileStorage.addresses.update(guid, TEST_ADDRESS_3);
+  await onChanged;
+
+  address = profileStorage.addresses.get(guid, {includePrivateFields: true});
+  do_check_eq(address._sync.changeCounter, 2);
+
+  profileStorage.addresses.pushSyncChanges(changes);
+  address = profileStorage.addresses.get(guid, {includePrivateFields: true});
+
+  // Counter should still be 1, since our sync didn't record the mid-sync change
+  do_check_eq(address._sync.changeCounter, 1, "Counter shouldn't be zero because it didn't record update");
+
+  // now, push a new set of changes, which should make the changeCounter 0
+  profileStorage.addresses.pushSyncChanges({
+    [guid]: {
+      profile: address,
+      counter: address._sync.changeCounter,
+      modified: address.timeLastModified,
+      synced: true,
+    },
+  });
+  address = profileStorage.addresses.get(guid, {includePrivateFields: true});
+
+  do_check_eq(address._sync.changeCounter, 0);
+});
+
+async function checkingSyncChange(action, callback) {
+  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
+                                          (subject, data) => data == action);
+  await callback();
+  let [subject] = await onChanged;
+  ok(subject.wrappedJSObject.sourceSync, "change notification should have source sync");
+}
+
+add_task(async function test_add_sourceSync() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, []);
+
+  // Hardcode a guid so that we don't need to generate a dynamic regex
+  let guid = "aaaaaaaaaaaa";
+  let testAddr = Object.assign({guid}, TEST_ADDRESS_1);
+
+  await checkingSyncChange("add", () =>
+    profileStorage.addresses.add(testAddr, {sourceSync: true}));
+
+  let added = profileStorage.addresses.get(guid, {includePrivateFields: true});
+  equal(added._sync.changeCounter, 0);
+
+  Assert.throws(() =>
+    profileStorage.addresses.add({guid, deleted: true}, {sourceSync: true}),
+    /Record aaaaaaaaaaaa already exists/
+  );
+});
+
+add_task(async function test_add_tombstone_sourceSync() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, []);
+
+  let guid = profileStorage.addresses._generateGUID();
+  let testAddr = {guid, deleted: true};
+  await checkingSyncChange("add", () =>
+    profileStorage.addresses.add(testAddr, {sourceSync: true}));
+
+  let added = findGUID(profileStorage.addresses, guid,
+    {includePrivateFields: true, includeDeleted: true});
+  ok(added);
+  equal(added._sync.changeCounter, 0);
+  ok(added.deleted);
+
+  // Adding same record again shouldn't throw (or change anything)
+  await checkingSyncChange("add", () =>
+    profileStorage.addresses.add(testAddr, {sourceSync: true}));
+
+  added = findGUID(profileStorage.addresses, guid,
+    {includePrivateFields: true, includeDeleted: true});
+  equal(added._sync.changeCounter, 0);
+  ok(added.deleted);
+});
+
+add_task(async function test_add_resurrects_tombstones() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, []);
+
+  let guid = profileStorage.addresses._generateGUID();
+
+  // Add a tombstone.
+  profileStorage.addresses.add({guid, deleted: true});
+
+  // You can't re-add an item with an explicit GUID.
+  let resurrected = Object.assign({}, TEST_ADDRESS_1, {guid});
+  Assert.throws(() => profileStorage.addresses.add(resurrected),
+                /"guid" is not a valid field/);
+
+  // But Sync can!
+  let guid3 = profileStorage.addresses.add(resurrected, {sourceSync: true});
+  equal(guid, guid3);
+
+  let got = profileStorage.addresses.get(guid);
+  equal(got["given-name"], TEST_ADDRESS_1["given-name"]);
+});
+
+add_task(async function test_remove_sourceSync_localChanges() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, [TEST_ADDRESS_1]);
+  profileStorage.addresses.pullSyncChanges(); // force sync metadata
+
+  let [{guid, _sync}] = profileStorage.addresses.getAll({includePrivateFields: true});
+
+  equal(_sync.changeCounter, 1);
+  // try and remove a record stored locally with local changes
+  await checkingSyncChange("remove", () =>
+    profileStorage.addresses.remove(guid, {sourceSync: true}));
+
+  let record = profileStorage.addresses.get(guid, {
+    includePrivateFields: true,
+  });
+  ok(record);
+  equal(record._sync.changeCounter, 1);
+});
+
+add_task(async function test_remove_sourceSync_unknown() {
+  // remove a record not stored locally
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, []);
+
+  let guid = profileStorage.addresses._generateGUID();
+  await checkingSyncChange("remove", () =>
+    profileStorage.addresses.remove(guid, {sourceSync: true}));
+
+  let tombstone = findGUID(profileStorage.addresses, guid, {
+    includePrivateFields: true,
+    includeDeleted: true,
+  });
+  ok(tombstone.deleted, 0);
+  equal(tombstone._sync.changeCounter, 0);
+});
+
+add_task(async function test_remove_sourceSync_unchanged() {
+  // Remove a local record without a change counter.
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, []);
+
+  let guid = profileStorage.addresses._generateGUID();
+  let addr = Object.assign({guid}, TEST_ADDRESS_1);
+  // add a record with sourceSync to guarantee changeCounter == 0
+  await checkingSyncChange("add", () =>
+    profileStorage.addresses.add(addr, {sourceSync: true}));
+
+  let added = profileStorage.addresses.get(guid, {includePrivateFields: true});
+  equal(added._sync.changeCounter, 0);
+
+  await checkingSyncChange("remove", () =>
+    profileStorage.addresses.remove(guid, {sourceSync: true}));
+
+  let tombstone = findGUID(profileStorage.addresses, guid, {
+    includePrivateFields: true,
+    includeDeleted: true,
+  });
+  ok(tombstone.deleted, 0);
+  equal(tombstone._sync.changeCounter, 0);
+});
+
+add_task(async function test_pullSyncChanges() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+
+  let startAddresses = profileStorage.addresses.getAll();
+  equal(startAddresses.length, 2);
+  // All should start without sync metadata
+  for (let addr of profileStorage.addresses._store.data.addresses) {
+    ok(!addr._sync);
+  }
+  profileStorage.addresses.pullSyncChanges(); // force sync metadata
+
+  let addedDirectGUID = profileStorage.addresses._generateGUID();
+  let testAddr = Object.assign({guid: addedDirectGUID},
+                               TEST_ADDRESS_1, TEST_ADDRESS_3);
+
+  await checkingSyncChange("add", () =>
+    profileStorage.addresses.add(testAddr, {sourceSync: true}));
+
+  let tombstoneGUID = profileStorage.addresses._generateGUID();
+  await checkingSyncChange("add", () =>
+    profileStorage.addresses.add(
+      {guid: tombstoneGUID, deleted: true},
+      {sourceSync: true}));
+
+  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
+                                          (subject, data) => data == "remove");
+
+  profileStorage.addresses.remove(startAddresses[0].guid);
+  await onChanged;
+
+  let addresses = profileStorage.addresses.getAll({
+    includeDeleted: true,
+    includePrivateFields: true,
+  });
+
+  // Should contain changes with a change counter
+  let changes = profileStorage.addresses.pullSyncChanges();
+  equal(Object.keys(changes).length, 2);
+
+  ok(changes[startAddresses[0].guid].profile.deleted);
+  equal(changes[startAddresses[0].guid].counter, 2);
+
+  ok(!changes[startAddresses[1].guid].profile.deleted);
+  equal(changes[startAddresses[1].guid].counter, 1);
+
+  ok(!changes[tombstoneGUID], "Missing because it's a tombstone from sourceSync");
+  ok(!changes[addedDirectGUID], "Missing because it was added with sourceSync");
+
+  for (let address of addresses) {
+    let change = changes[address.guid];
+    if (!change) {
+      continue;
+    }
+    equal(change.profile.guid, address.guid);
+    equal(change.counter, address._sync.changeCounter);
+    ok(!change.synced);
+  }
+});
+
+add_task(async function test_pullPushChanges() {
+  // round-trip changes between pull and push
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, []);
+  let psa = profileStorage.addresses;
+
+  let guid1 = psa.add(TEST_ADDRESS_1);
+  let guid2 = psa.add(TEST_ADDRESS_2);
+  let guid3 = psa.add(TEST_ADDRESS_3);
+
+  let changes = psa.pullSyncChanges();
+
+  equal(psa.get(guid1, {includePrivateFields: true})._sync.changeCounter, 1);
+  equal(psa.get(guid2, {includePrivateFields: true})._sync.changeCounter, 1);
+  equal(psa.get(guid3, {includePrivateFields: true})._sync.changeCounter, 1);
+
+  // between the pull and the push we change the second.
+  psa.update(guid2, Object.assign({}, TEST_ADDRESS_2, {country: "AU"}));
+  equal(psa.get(guid2, {includePrivateFields: true})._sync.changeCounter, 2);
+  // and update the changeset to indicated we did update the first 2, but failed
+  // to update the 3rd for some reason.
+  changes[guid1].synced = true;
+  changes[guid2].synced = true;
+
+  psa.pushSyncChanges(changes);
+
+  // first was synced correctly.
+  equal(psa.get(guid1, {includePrivateFields: true})._sync.changeCounter, 0);
+  // second was synced correctly, but it had a change while syncing.
+  equal(psa.get(guid2, {includePrivateFields: true})._sync.changeCounter, 1);
+  // 3rd wasn't marked as having synced.
+  equal(psa.get(guid3, {includePrivateFields: true})._sync.changeCounter, 1);
+});
+
+add_task(async function test_changeGUID() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, []);
+
+  let newguid = () => profileStorage.addresses._generateGUID();
+
+  let guid_synced = profileStorage.addresses.add(TEST_ADDRESS_1);
+
+  // pullSyncChanges so guid_synced is flagged as syncing.
+  profileStorage.addresses.pullSyncChanges();
+
+  // and 2 items that haven't been synced.
+  let guid_u1 = profileStorage.addresses.add(TEST_ADDRESS_2);
+  let guid_u2 = profileStorage.addresses.add(TEST_ADDRESS_3);
+
+  // Change a non-existing guid
+  Assert.throws(() => profileStorage.addresses.changeGUID(newguid(), newguid()),
+                /changeGUID: no source record/);
+  // Change to a guid that already exists.
+  Assert.throws(() => profileStorage.addresses.changeGUID(guid_u1, guid_u2),
+                /changeGUID: record with destination id exists already/);
+  // Try and change a guid that's already been synced.
+  Assert.throws(() => profileStorage.addresses.changeGUID(guid_synced, newguid()),
+                /changeGUID: existing record has already been synced/);
+
+  // Change an item to itself makes no sense.
+  Assert.throws(() => profileStorage.addresses.changeGUID(guid_u1, guid_u1),
+                /changeGUID: old and new IDs are the same/);
+
+  // and one that works.
+  equal(profileStorage.addresses.getAll({includeDeleted: true}).length, 3);
+  let targetguid = newguid();
+  profileStorage.addresses.changeGUID(guid_u1, targetguid);
+  equal(profileStorage.addresses.getAll({includeDeleted: true}).length, 3);
+
+  ok(profileStorage.addresses.get(guid_synced), "synced item still exists.");
+  ok(profileStorage.addresses.get(guid_u2), "guid we didn't touch still exists.");
+  ok(profileStorage.addresses.get(targetguid), "target guid exists.");
+  ok(!profileStorage.addresses.get(guid_u1), "old guid no longer exists.");
+});
+
+add_task(async function test_reset() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+
+  let addresses = profileStorage.addresses.getAll({includePrivateFields: true});
+  // All should start without sync metadata
+  for (let addr of addresses) {
+    ok(!addr._sync);
+  }
+  // pullSyncChanges should create the metadata.
+  profileStorage.addresses.pullSyncChanges();
+  addresses = profileStorage.addresses.getAll({includePrivateFields: true});
+  for (let addr of addresses) {
+    ok(addr._sync);
+  }
+  // and resetSync should wipe it.
+  profileStorage.addresses.resetSync();
+  addresses = profileStorage.addresses.getAll({includePrivateFields: true});
+  for (let addr of addresses) {
+    ok(!addr._sync);
+  }
+});
--- a/browser/extensions/formautofill/test/unit/test_storage_tombstones.js
+++ b/browser/extensions/formautofill/test/unit/test_storage_tombstones.js
@@ -59,16 +59,37 @@ add_storage_task(async function test_sim
 
   storage.remove(guid);
 
   // should be unable to get it normally.
   Assert.equal(storage.get(guid), null);
   // and getAll should also not return it.
   Assert.equal(storage.getAll().length, 0);
 
+  // but getAll allows us to access deleted items - but we didn't create
+  // a tombstone here, so even that will not get it.
+  let all = storage.getAll({includeDeleted: true});
+  Assert.equal(all.length, 0);
+});
+
+add_storage_task(async function test_simple_synctombstone(storage, record) {
+  do_print("check simple tombstone semantics for synced records");
+
+  let guid = storage.add(record);
+  do_check_eq(storage.getAll().length, 1);
+
+  storage.pullSyncChanges(); // force sync metadata, which triggers tombstone behaviour.
+
+  storage.remove(guid);
+
+  // should be unable to get it normally.
+  Assert.equal(storage.get(guid), null);
+  // and getAll should also not return it.
+  Assert.equal(storage.getAll().length, 0);
+
   // but getAll allows us to access deleted items.
   let all = storage.getAll({includeDeleted: true});
   Assert.equal(all.length, 1);
 
   do_check_tombstone_record(all[0]);
 });
 
 add_storage_task(async function test_add_tombstone(storage, record) {
--- a/browser/extensions/formautofill/test/unit/xpcshell.ini
+++ b/browser/extensions/formautofill/test/unit/xpcshell.ini
@@ -29,9 +29,10 @@ support-files =
 [test_isCJKName.js]
 [test_isFieldEligibleForAutofill.js]
 [test_markAsAutofillField.js]
 [test_nameUtils.js]
 [test_onFormSubmitted.js]
 [test_profileAutocompleteResult.js]
 [test_savedFieldNames.js]
 [test_storage_tombstones.js]
+[test_storage_syncfields.js]
 [test_transformFields.js]