Bug 1463956 - allow Sync to remove all address and credit-card records. r?MattN,kitcambridge draft
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 24 May 2018 12:36:37 +1000
changeset 799213 445eb368b120a370cef2c23cc64f744b5bd410ec
parent 799120 47e81ea1ef10189ef210867934bf36e14cf223dc
push id110970
push userbmo:markh@mozilla.com
push dateThu, 24 May 2018 07:56:00 +0000
reviewersMattN, kitcambridge
bugs1463956
milestone62.0a1
Bug 1463956 - allow Sync to remove all address and credit-card records. r?MattN,kitcambridge MozReview-Commit-ID: KSeJJfAGmRh
browser/components/payments/test/browser/browser_address_edit.js
browser/components/payments/test/browser/head.js
browser/extensions/formautofill/FormAutofillStorage.jsm
browser/extensions/formautofill/FormAutofillSync.jsm
browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_creditCardRecords.js
browser/extensions/formautofill/test/unit/test_savedFieldNames.js
browser/extensions/formautofill/test/unit/test_storage_remove.js
browser/extensions/formautofill/test/unit/test_sync.js
browser/extensions/formautofill/test/unit/xpcshell.ini
--- a/browser/components/payments/test/browser/browser_address_edit.js
+++ b/browser/components/payments/test/browser/browser_address_edit.js
@@ -369,17 +369,17 @@ add_task(async function test_edit_payer_
     info("clicking cancel");
     spawnPaymentDialogTask(frame, PTU.DialogContentTasks.manuallyClickCancel);
 
     await BrowserTestUtils.waitForCondition(() => win.closed, "dialog should be closed");
   });
 });
 
 add_task(async function test_private_persist_addresses() {
-  await formAutofillStorage.addresses._nukeAllRecords();
+  formAutofillStorage.addresses.removeAll();
   await setup();
 
   is((await formAutofillStorage.addresses.getAll()).length, 2,
      "Setup results in 2 stored addresses at start of test");
 
   await withNewTabInPrivateWindow({
     url: BLANK_PAGE_URL,
   }, async browser => {
--- a/browser/components/payments/test/browser/head.js
+++ b/browser/components/payments/test/browser/head.js
@@ -250,18 +250,18 @@ async function spawnInDialogForMerchantT
   });
 }
 
 async function setupFormAutofillStorage() {
   await formAutofillStorage.initialize();
 }
 
 function cleanupFormAutofillStorage() {
-  formAutofillStorage.addresses._nukeAllRecords();
-  formAutofillStorage.creditCards._nukeAllRecords();
+  formAutofillStorage.addresses.removeAll();
+  formAutofillStorage.creditCards.removeAll();
 }
 
 add_task(async function setup_head() {
   await setupFormAutofillStorage();
   registerCleanupFunction(function cleanup() {
     paymentSrv.cleanup();
     cleanupFormAutofillStorage();
   });
--- a/browser/extensions/formautofill/FormAutofillStorage.jsm
+++ b/browser/extensions/formautofill/FormAutofillStorage.jsm
@@ -365,16 +365,17 @@ class AutofillRecords {
     }
 
     this._data.push(recordToSave);
 
     this._store.saveSoon();
 
     Services.obs.notifyObservers({wrappedJSObject: {
       sourceSync,
+      guid: record.guid,
       collectionName: this._collectionName,
     }}, "formautofill-storage-changed", "add");
     return recordToSave.guid;
   }
 
   _generateGUID() {
     let guid;
     while (!guid || this._findByGUID(guid)) {
@@ -465,17 +466,20 @@ class AutofillRecords {
     if (!recordFound) {
       throw new Error("No matching record.");
     }
 
     recordFound.timesUsed++;
     recordFound.timeLastUsed = Date.now();
 
     this._store.saveSoon();
-    Services.obs.notifyObservers(null, "formautofill-storage-changed", "notifyUsed");
+    Services.obs.notifyObservers({wrappedJSObject: {
+      guid,
+      collectionName: this._collectionName,
+    }}, "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]
@@ -512,16 +516,17 @@ class AutofillRecords {
         // we can delete it.
         this._data.splice(index, 1);
       }
     }
 
     this._store.saveSoon();
     Services.obs.notifyObservers({wrappedJSObject: {
       sourceSync,
+      guid,
       collectionName: this._collectionName,
     }}, "formautofill-storage-changed", "remove");
   }
 
   /**
    * Returns the record with the specified GUID.
    *
    * @param   {string} guid
@@ -832,16 +837,18 @@ class AutofillRecords {
           keepSyncMetadata: false,
         });
       }
     }
 
     this._store.saveSoon();
     Services.obs.notifyObservers({wrappedJSObject: {
       sourceSync: true,
+      guid: remoteRecord.guid,
+      forkedGUID,
       collectionName: this._collectionName,
     }}, "formautofill-storage-changed", "reconcile");
 
     return {forkedGUID};
   }
 
   _removeSyncedRecord(guid) {
     let index = this._findIndexByGUID(guid, {includeDeleted: true});
@@ -1182,20 +1189,26 @@ class AutofillRecords {
       if (!record.deleted && this.mergeIfPossible(record.guid, targetRecord, strict)) {
         mergedGUIDs.push(record.guid);
       }
     }
     this.log.debug("Existing records matching and merging count is", mergedGUIDs.length);
     return mergedGUIDs;
   }
 
-  // A test-only helper.
-  _nukeAllRecords() {
+  /**
+   * Unconditionally remove all data and tombstones for this collection.
+   */
+  removeAll({sourceSync = false} = {}) {
     this._store.data[this._collectionName] = [];
-    // test-only, so there's no good reason to request a save!
+    this._store.saveSoon();
+    Services.obs.notifyObservers({wrappedJSObject: {
+      sourceSync,
+      collectionName: this._collectionName,
+    }}, "formautofill-storage-changed", "removeAll");
   }
 
   _stripComputedFields(record) {
     this.VALID_COMPUTED_FIELDS.forEach(field => delete record[field]);
   }
 
   // An interface to be inherited.
   _recordReadProcessor(record) {}
--- a/browser/extensions/formautofill/FormAutofillSync.jsm
+++ b/browser/extensions/formautofill/FormAutofillSync.jsm
@@ -327,16 +327,22 @@ FormAutofillEngine.prototype = {
   _deleteId(id) {
     this._noteDeletedId(id);
   },
 
   async _resetClient() {
     await formAutofillStorage.initialize();
     this._store.storage.resetSync();
   },
+
+  async _wipeClient() {
+    await formAutofillStorage.initialize();
+    this._store.storage.removeAll({sourceSync: true});
+  },
+
 };
 
 // The concrete engines
 
 function AddressesRecord(collection, id) {
   AutofillRecord.call(this, collection, id);
 }
 
--- a/browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js
+++ b/browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js
@@ -26,16 +26,24 @@ var ParentUtils = {
     let count = times;
 
     return new Promise(resolve => {
       Services.obs.addObserver(function observer(subject, obsTopic, data) {
         if (type && data != type || !!--count) {
           return;
         }
 
+        // every notification type should have the collection name.
+        let allowedNames = [ADDRESSES_COLLECTION_NAME, CREDITCARDS_COLLECTION_NAME];
+        assert.ok(allowedNames.includes(subject.wrappedJSObject.collectionName),
+                  "should include the collection name");
+        // every notification except removeAll should have a guid.
+        if (data != "removeAll") {
+          assert.ok(subject.wrappedJSObject.guid, "should have a guid");
+        }
         Services.obs.removeObserver(observer, obsTopic);
         resolve();
       }, topic);
     });
   },
 
   async _operateRecord(collectionName, type, msgData, contentMsg) {
     let times, topic;
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -477,17 +477,21 @@ add_task(async function test_notifyUsed(
   let guid = addresses[1].guid;
   let timeLastUsed = addresses[1].timeLastUsed;
   let timesUsed = addresses[1].timesUsed;
 
   profileStorage.addresses.pullSyncChanges(); // force sync metadata, which we check below.
   let changeCounter = getSyncChangeCounter(profileStorage.addresses, guid);
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
-                                          (subject, data) => data == "notifyUsed");
+    (subject, data) =>
+      data == "notifyUsed" &&
+      subject.wrappedJSObject.guid == guid &&
+      subject.wrappedJSObject.collectionName == COLLECTION_NAME
+  );
 
   profileStorage.addresses.notifyUsed(guid);
   await onChanged;
 
   let address = profileStorage.addresses.get(guid);
 
   Assert.equal(address.timesUsed, timesUsed + 1);
   Assert.notEqual(address.timeLastUsed, timeLastUsed);
@@ -506,16 +510,17 @@ add_task(async function test_remove() {
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[1].guid;
 
   let onChanged = TestUtils.topicObserved(
     "formautofill-storage-changed",
     (subject, data) =>
       data == "remove" &&
+      subject.wrappedJSObject.guid == guid &&
       subject.wrappedJSObject.collectionName == COLLECTION_NAME
   );
 
   Assert.equal(addresses.length, 2);
 
   profileStorage.addresses.remove(guid);
   await onChanged;
 
--- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
@@ -186,16 +186,17 @@ const MERGE_TESTCASES = [
 let prepareTestCreditCards = async function(path) {
   let profileStorage = new FormAutofillStorage(path);
   await profileStorage.initialize();
 
   let onChanged = TestUtils.topicObserved(
     "formautofill-storage-changed",
     (subject, data) =>
       data == "add" &&
+      subject.wrappedJSObject.guid &&
       subject.wrappedJSObject.collectionName == COLLECTION_NAME
   );
   Assert.ok(profileStorage.creditCards.add(TEST_CREDIT_CARD_1));
   await onChanged;
   Assert.ok(profileStorage.creditCards.add(TEST_CREDIT_CARD_2));
   await onChanged;
   await profileStorage._saveImmediately();
 };
@@ -364,16 +365,17 @@ add_task(async function test_update() {
   let creditCards = profileStorage.creditCards.getAll();
   let guid = creditCards[1].guid;
   let timeLastModified = creditCards[1].timeLastModified;
 
   let onChanged = TestUtils.topicObserved(
     "formautofill-storage-changed",
     (subject, data) =>
       data == "update" &&
+      subject.wrappedJSObject.guid == guid &&
       subject.wrappedJSObject.collectionName == COLLECTION_NAME
   );
 
   Assert.notEqual(creditCards[1]["cc-name"], undefined);
   profileStorage.creditCards.update(guid, TEST_CREDIT_CARD_3);
   await onChanged;
   await profileStorage._saveImmediately();
 
@@ -462,18 +464,23 @@ add_task(async function test_notifyUsed(
   let profileStorage = new FormAutofillStorage(path);
   await profileStorage.initialize();
 
   let creditCards = profileStorage.creditCards.getAll();
   let guid = creditCards[1].guid;
   let timeLastUsed = creditCards[1].timeLastUsed;
   let timesUsed = creditCards[1].timesUsed;
 
-  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
-                                          (subject, data) => data == "notifyUsed");
+  let onChanged = TestUtils.topicObserved(
+    "formautofill-storage-changed",
+    (subject, data) =>
+      data == "notifyUsed" &&
+      subject.wrappedJSObject.collectionName == COLLECTION_NAME &&
+      subject.wrappedJSObject.guid == guid
+  );
 
   profileStorage.creditCards.notifyUsed(guid);
   await onChanged;
   await profileStorage._saveImmediately();
 
   profileStorage = new FormAutofillStorage(path);
   await profileStorage.initialize();
 
@@ -495,16 +502,17 @@ add_task(async function test_remove() {
 
   let creditCards = profileStorage.creditCards.getAll();
   let guid = creditCards[1].guid;
 
   let onChanged = TestUtils.topicObserved(
     "formautofill-storage-changed",
     (subject, data) =>
       data == "remove" &&
+      subject.wrappedJSObject.guid == guid &&
       subject.wrappedJSObject.collectionName == COLLECTION_NAME
   );
 
   Assert.equal(creditCards.length, 2);
 
   profileStorage.creditCards.remove(guid);
   await onChanged;
   await profileStorage._saveImmediately();
--- a/browser/extensions/formautofill/test/unit/test_savedFieldNames.js
+++ b/browser/extensions/formautofill/test/unit/test_savedFieldNames.js
@@ -20,18 +20,18 @@ add_task(async function test_profileSave
 
 add_task(async function test_profileSavedFieldNames_observe() {
   let formAutofillParent = new FormAutofillParent();
   sinon.stub(formAutofillParent, "_updateSavedFieldNames");
 
   await formAutofillParent.init();
 
   // profile changed => Need to trigger updateValidFields
-  ["add", "update", "remove", "reconcile"].forEach(event => {
-    formAutofillParent.observe(null, "formautofill-storage-changed", "add");
+  ["add", "update", "remove", "reconcile", "removeAll"].forEach(event => {
+    formAutofillParent.observe(null, "formautofill-storage-changed", event);
     Assert.equal(formAutofillParent._updateSavedFieldNames.called, true);
   });
 
   // profile metadata updated => no need to trigger updateValidFields
   formAutofillParent._updateSavedFieldNames.reset();
   formAutofillParent.observe(null, "formautofill-storage-changed", "notifyUsed");
   Assert.equal(formAutofillParent._updateSavedFieldNames.called, false);
 });
new file mode 100644
--- /dev/null
+++ b/browser/extensions/formautofill/test/unit/test_storage_remove.js
@@ -0,0 +1,81 @@
+/**
+ * Tests removing all address/creditcard records.
+ */
+
+"use strict";
+
+const {FormAutofillStorage} = ChromeUtils.import("resource://formautofill/FormAutofillStorage.jsm", {});
+
+const TEST_STORE_FILE_NAME = "test-tombstones.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_CREDIT_CARD_1 = {
+  "cc-name": "John Doe",
+  "cc-number": "1234567812345678",
+  "cc-exp-month": 4,
+  "cc-exp-year": 2017,
+};
+
+const TEST_CREDIT_CARD_2 = {
+  "cc-name": "Timothy Berners-Lee",
+  "cc-number": "1111222233334444",
+  "cc-exp-month": 12,
+  "cc-exp-year": 2022,
+};
+
+// Like add_task, but actually adds 2 - one for addresses and one for cards.
+function add_storage_task(test_function) {
+  add_task(async function() {
+    let path = getTempFile(TEST_STORE_FILE_NAME).path;
+    let profileStorage = new FormAutofillStorage(path);
+    await profileStorage.initialize();
+    let address_records = [TEST_ADDRESS_1, TEST_ADDRESS_2];
+    let cc_records = [TEST_CREDIT_CARD_1, TEST_CREDIT_CARD_2];
+
+    for (let [storage, record] of [[profileStorage.addresses, address_records],
+                                   [profileStorage.creditCards, cc_records]]) {
+      await test_function(storage, record);
+    }
+  });
+}
+
+add_storage_task(async function test_remove_everything(storage, records) {
+  info("check simple tombstone semantics");
+
+  let guid = storage.add(records[0]);
+  Assert.equal(storage.getAll().length, 1);
+
+  storage.pullSyncChanges(); // force sync metadata, which triggers tombstone behaviour.
+
+  storage.remove(guid);
+
+  storage.add(records[1]);
+  // getAll() is still 1 as we deleted the first.
+  Assert.equal(storage.getAll().length, 1);
+
+  // check we have the tombstone.
+  Assert.equal(storage.getAll({includeDeleted: true}).length, 2);
+
+  storage.removeAll();
+
+  // should have deleted both the existing and deleted records.
+  Assert.equal(storage.getAll({includeDeleted: true}).length, 0);
+});
--- a/browser/extensions/formautofill/test/unit/test_sync.js
+++ b/browser/extensions/formautofill/test/unit/test_sync.js
@@ -702,8 +702,30 @@ add_task(async function test_reconcile_b
     let changeCounter = getSyncChangeCounter(profileStorage.addresses,
       forkedPayload.id);
     strictEqual(changeCounter, 0,
       "Forked record should be marked as syncing");
   } finally {
     await cleanup(server);
   }
 });
+
+add_task(async function test_wipe() {
+  let {profileStorage, server, engine} = await setup();
+  try {
+    let guid = profileStorage.addresses.add(TEST_PROFILE_1);
+
+    expectLocalProfiles(profileStorage, [{guid}]);
+
+    let promiseObserved = promiseOneObserver("formautofill-storage-changed");
+
+    await engine._wipeClient();
+
+    let {subject, data} = await promiseObserved;
+    Assert.equal(subject.wrappedJSObject.sourceSync, true, "it should be noted this came from sync");
+    Assert.equal(subject.wrappedJSObject.collectionName, "addresses", "got the correct collection");
+    Assert.equal(data, "removeAll", "a removeAll should be noted");
+
+    expectLocalProfiles(profileStorage, []);
+  } finally {
+    await cleanup(server);
+  }
+});
--- a/browser/extensions/formautofill/test/unit/xpcshell.ini
+++ b/browser/extensions/formautofill/test/unit/xpcshell.ini
@@ -44,12 +44,13 @@ support-files =
 [test_onFormSubmitted.js]
 [test_parseAddressFormat.js]
 [test_profileAutocompleteResult.js]
 [test_phoneNumber.js]
 [test_reconcile.js]
 [test_savedFieldNames.js]
 [test_toOneLineAddress.js]
 [test_storage_tombstones.js]
+[test_storage_remove.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