Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage. r=steveck
authorLuke Chang <lchang@mozilla.com>
Wed, 01 Nov 2017 19:03:50 +0800
changeset 389833 57072e6ba78b252aeb7fbba3d6b9f4dcd55a66bb
parent 389832 b23c3a2f8e9d752be559f568ec8168b1ecf5577c
child 389834 e1a4d0b5eb152db7d06ccd20f919879a440ab6db
push id54606
push userlchang@mozilla.com
push dateThu, 02 Nov 2017 11:51:18 +0000
treeherderautoland@57072e6ba78b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssteveck
bugs1413479
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage. r=steveck MozReview-Commit-ID: Iwp05R4GS6X
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_creditCardRecords.js
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -308,52 +308,47 @@ class AutofillRecords {
    * @param {boolean} [options.sourceSync = false]
    *        Did sync generate this addition?
    * @returns {string}
    *          The GUID of the newly added item..
    */
   add(record, {sourceSync = false} = {}) {
     this.log.debug("add:", record);
 
+    let recordToSave = this._cloneAndCleanUp(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, {
+      let index = this._findIndexByGUID(recordToSave.guid, {
         includeDeleted: true,
       });
       if (index > -1) {
         let existing = this.data[index];
         if (existing.deleted) {
           this.data.splice(index, 1);
         } else {
-          throw new Error(`Record ${record.guid} already exists`);
+          throw new Error(`Record ${recordToSave.guid} already exists`);
         }
       }
-      let recordToSave = this._clone(record);
-      return this._saveRecord(recordToSave, {sourceSync});
-    }
+    } else if (!recordToSave.deleted) {
+      this._normalizeRecord(recordToSave);
+
+      recordToSave.guid = this._generateGUID();
+      recordToSave.version = this.version;
 
-    if (record.deleted) {
-      return this._saveRecord(record);
+      // Metadata
+      let now = Date.now();
+      recordToSave.timeCreated = now;
+      recordToSave.timeLastModified = now;
+      recordToSave.timeLastUsed = 0;
+      recordToSave.timesUsed = 0;
     }
 
-    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);
+    return this._saveRecord(recordToSave, {sourceSync});
   }
 
   _saveRecord(record, {sourceSync = false} = {}) {
     if (!record.guid) {
       throw new Error("Record missing GUID");
     }
 
     let recordToSave;
@@ -423,17 +418,17 @@ class AutofillRecords {
       let oldValue = recordFound[field];
       let newValue = recordToUpdate[field];
 
       // Resume the old field value in the perserve case
       if (preserveOldProperties && newValue === undefined) {
         newValue = oldValue;
       }
 
-      if (!newValue) {
+      if (newValue === undefined || newValue === "") {
         delete recordFound[field];
       } else {
         recordFound[field] = newValue;
       }
 
       this._maybeStoreLastSyncedField(recordFound, field, oldValue);
     }
 
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -34,17 +34,17 @@ const TEST_ADDRESS_3 = {
 
 const TEST_ADDRESS_4 = {
   "given-name": "Timothy",
   "additional-name": "John",
   "family-name": "Berners-Lee",
   organization: "World Wide Web Consortium",
 };
 
-const TEST_ADDRESS_FOR_UPDATE = {
+const TEST_ADDRESS_WITH_EMPTY_FIELD = {
   "name": "Tim Berners",
   "street-address": "",
 };
 
 const TEST_ADDRESS_WITH_INVALID_FIELD = {
   "street-address": "Another Address",
   invalidField: "INVALID",
 };
@@ -294,16 +294,22 @@ add_task(async function test_add() {
 
   do_check_neq(addresses[0].guid, undefined);
   do_check_eq(addresses[0].version, 1);
   do_check_neq(addresses[0].timeCreated, undefined);
   do_check_eq(addresses[0].timeLastModified, addresses[0].timeCreated);
   do_check_eq(addresses[0].timeLastUsed, 0);
   do_check_eq(addresses[0].timesUsed, 0);
 
+  // Empty string should be deleted before saving.
+  profileStorage.addresses.add(TEST_ADDRESS_WITH_EMPTY_FIELD);
+  let address = profileStorage.addresses.data[2];
+  do_check_eq(address.name, TEST_ADDRESS_WITH_EMPTY_FIELD.name);
+  do_check_eq(address["street-address"], undefined);
+
   Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./);
 });
 
 add_task(async function test_update() {
   let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
                                                 [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
@@ -328,31 +334,37 @@ add_task(async function test_update() {
   let address = profileStorage.addresses.get(guid, {rawData: true});
 
   do_check_eq(address.country, undefined);
   do_check_neq(address.timeLastModified, timeLastModified);
   do_check_record_matches(address, TEST_ADDRESS_3);
   do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
 
   // Test preserveOldProperties parameter and field with empty string.
-  profileStorage.addresses.update(guid, TEST_ADDRESS_FOR_UPDATE, true);
+  profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_EMPTY_FIELD, true);
   await onChanged;
   await profileStorage._saveImmediately();
 
   profileStorage.addresses.pullSyncChanges(); // force sync metadata, which we check below.
 
   address = profileStorage.addresses.get(guid, {rawData: true});
 
   do_check_eq(address["given-name"], "Tim");
   do_check_eq(address["family-name"], "Berners");
   do_check_eq(address["street-address"], undefined);
   do_check_eq(address["postal-code"], "12345");
   do_check_neq(address.timeLastModified, timeLastModified);
   do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 2);
 
+  // Empty string should be deleted while updating.
+  profileStorage.addresses.update(profileStorage.addresses.data[0].guid, TEST_ADDRESS_WITH_EMPTY_FIELD);
+  address = profileStorage.addresses.data[0];
+  do_check_eq(address.name, TEST_ADDRESS_WITH_EMPTY_FIELD.name);
+  do_check_eq(address["street-address"], undefined);
+
   Assert.throws(
     () => profileStorage.addresses.update("INVALID_GUID", TEST_ADDRESS_3),
     /No matching record\./
   );
 
   Assert.throws(
     () => profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./
--- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
@@ -23,16 +23,22 @@ const TEST_CREDIT_CARD_2 = {
 };
 
 const TEST_CREDIT_CARD_3 = {
   "cc-number": "9999888877776666",
   "cc-exp-month": 1,
   "cc-exp-year": 2000,
 };
 
+const TEST_CREDIT_CARD_WITH_EMPTY_FIELD = {
+  "cc-name": "",
+  "cc-number": "1234123412341234",
+  "cc-exp-month": 1,
+};
+
 const TEST_CREDIT_CARD_WITH_2_DIGITS_YEAR = {
   "cc-number": "1234123412341234",
   "cc-exp-month": 1,
   "cc-exp-year": 12,
 };
 
 const TEST_CREDIT_CARD_WITH_INVALID_FIELD = {
   "cc-name": "John Doe",
@@ -165,16 +171,22 @@ add_task(async function test_add() {
 
   do_check_neq(creditCards[0].guid, undefined);
   do_check_eq(creditCards[0].version, 1);
   do_check_neq(creditCards[0].timeCreated, undefined);
   do_check_eq(creditCards[0].timeLastModified, creditCards[0].timeCreated);
   do_check_eq(creditCards[0].timeLastUsed, 0);
   do_check_eq(creditCards[0].timesUsed, 0);
 
+  // Empty string should be deleted before saving.
+  profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_EMPTY_FIELD);
+  let creditCard = profileStorage.creditCards.data[2];
+  do_check_eq(creditCard["cc-exp-month"], TEST_CREDIT_CARD_WITH_EMPTY_FIELD["cc-exp-month"]);
+  do_check_eq(creditCard["cc-name"], undefined);
+
   Assert.throws(() => profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./);
 });
 
 add_task(async function test_update() {
   let path = getTempFile(TEST_STORE_FILE_NAME).path;
   await prepareTestCreditCards(path);
 
@@ -197,16 +209,22 @@ add_task(async function test_update() {
   await profileStorage.initialize();
 
   let creditCard = profileStorage.creditCards.get(guid);
 
   do_check_eq(creditCard["cc-name"], undefined);
   do_check_neq(creditCard.timeLastModified, timeLastModified);
   do_check_credit_card_matches(creditCard, TEST_CREDIT_CARD_3);
 
+  // Empty string should be deleted while updating.
+  profileStorage.creditCards.update(profileStorage.creditCards.data[0].guid, TEST_CREDIT_CARD_WITH_EMPTY_FIELD);
+  creditCard = profileStorage.creditCards.data[0];
+  do_check_eq(creditCard["cc-exp-month"], TEST_CREDIT_CARD_WITH_EMPTY_FIELD["cc-exp-month"]);
+  do_check_eq(creditCard["cc-name"], undefined);
+
   Assert.throws(
     () => profileStorage.creditCards.update("INVALID_GUID", TEST_CREDIT_CARD_3),
     /No matching record\./
   );
 
   Assert.throws(
     () => profileStorage.creditCards.update(guid, TEST_CREDIT_CARD_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./