Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage. r=steveck draft
authorLuke Chang <lchang@mozilla.com>
Wed, 01 Nov 2017 19:03:50 +0800
changeset 692039 665c5bf261531af15f28bb5a5584de4ee9cb5cdb
parent 691808 a0334f789772302ba5cfb6fd61290408842c7432
child 738639 0e8de6a2f214d1405b8f9996e127eee3f1ee12b0
push id87372
push userbmo:lchang@mozilla.com
push dateThu, 02 Nov 2017 11:49:38 +0000
reviewerssteveck
bugs1413479
milestone58.0a1
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\./