Bug 1418885 - [Form Autofill] Ensure all computed fields are removed after normalizing and strip trailing newlines in "street-address". r=steveck draft
authorLuke Chang <lchang@mozilla.com>
Mon, 20 Nov 2017 13:59:21 +0800
changeset 700449 70f839ccfaf71e331292d0c663fb08db5c257b0e
parent 700338 dd08f8b19cc32da161811abb2f7093e0f5392e69
child 740870 aaca3d1482876edca22edfeebfeaa358ae7d1d98
push id89832
push userbmo:lchang@mozilla.com
push dateMon, 20 Nov 2017 06:08:26 +0000
reviewerssteveck
bugs1418885
milestone59.0a1
Bug 1418885 - [Form Autofill] Ensure all computed fields are removed after normalizing and strip trailing newlines in "street-address". r=steveck MozReview-Commit-ID: Llhi9AZ3T8H
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_creditCardRecords.js
browser/extensions/formautofill/test/unit/test_transformFields.js
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -308,17 +308,17 @@ 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);
+    let recordToSave = this._clone(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(recordToSave.guid, {
         includeDeleted: true,
       });
       if (index > -1) {
@@ -1318,51 +1318,46 @@ class Addresses extends AutofillRecords 
   _normalizeFields(address) {
     this._normalizeName(address);
     this._normalizeAddress(address);
     this._normalizeCountry(address);
     this._normalizeTel(address);
   }
 
   _normalizeName(address) {
-    if (!address.name) {
-      return;
-    }
-
-    let nameParts = FormAutofillNameUtils.splitName(address.name);
-    if (!address["given-name"] && nameParts.given) {
-      address["given-name"] = nameParts.given;
-    }
-    if (!address["additional-name"] && nameParts.middle) {
-      address["additional-name"] = nameParts.middle;
-    }
-    if (!address["family-name"] && nameParts.family) {
-      address["family-name"] = nameParts.family;
+    if (address.name) {
+      let nameParts = FormAutofillNameUtils.splitName(address.name);
+      if (!address["given-name"] && nameParts.given) {
+        address["given-name"] = nameParts.given;
+      }
+      if (!address["additional-name"] && nameParts.middle) {
+        address["additional-name"] = nameParts.middle;
+      }
+      if (!address["family-name"] && nameParts.family) {
+        address["family-name"] = nameParts.family;
+      }
     }
     delete address.name;
   }
 
   _normalizeAddress(address) {
-    if (STREET_ADDRESS_COMPONENTS.every(c => !address[c])) {
-      return;
-    }
+    if (STREET_ADDRESS_COMPONENTS.some(c => !!address[c])) {
+      // Treat "street-address" as "address-line1" if it contains only one line
+      // and "address-line1" is omitted.
+      if (!address["address-line1"] && address["street-address"] &&
+          !address["street-address"].includes("\n")) {
+        address["address-line1"] = address["street-address"];
+        delete address["street-address"];
+      }
 
-    // Treat "street-address" as "address-line1" if it contains only one line
-    // and "address-line1" is omitted.
-    if (!address["address-line1"] && address["street-address"] &&
-        !address["street-address"].includes("\n")) {
-      address["address-line1"] = address["street-address"];
-      delete address["street-address"];
+      // Concatenate "address-line*" if "street-address" is omitted.
+      if (!address["street-address"]) {
+        address["street-address"] = STREET_ADDRESS_COMPONENTS.map(c => address[c]).join("\n").replace(/\n+$/, "");
+      }
     }
-
-    // Concatenate "address-line*" if "street-address" is omitted.
-    if (!address["street-address"]) {
-      address["street-address"] = STREET_ADDRESS_COMPONENTS.map(c => address[c]).join("\n");
-    }
-
     STREET_ADDRESS_COMPONENTS.forEach(c => delete address[c]);
   }
 
   _normalizeCountry(address) {
     let country;
 
     if (address.country) {
       country = address.country.toUpperCase();
@@ -1376,30 +1371,27 @@ class Addresses extends AutofillRecords 
     } else {
       delete address.country;
     }
 
     delete address["country-name"];
   }
 
   _normalizeTel(address) {
-    if (!address.tel && TEL_COMPONENTS.every(c => !address[c])) {
-      return;
-    }
+    if (address.tel || TEL_COMPONENTS.some(c => !!address[c])) {
+      FormAutofillUtils.compressTel(address);
 
-    FormAutofillUtils.compressTel(address);
+      let possibleRegion = address.country || FormAutofillUtils.DEFAULT_COUNTRY_CODE;
+      let tel = PhoneNumber.Parse(address.tel, possibleRegion);
 
-    let possibleRegion = address.country || FormAutofillUtils.DEFAULT_COUNTRY_CODE;
-    let tel = PhoneNumber.Parse(address.tel, possibleRegion);
-
-    if (tel && tel.internationalNumber) {
-      // Force to save numbers in E.164 format if parse success.
-      address.tel = tel.internationalNumber;
+      if (tel && tel.internationalNumber) {
+        // Force to save numbers in E.164 format if parse success.
+        address.tel = tel.internationalNumber;
+      }
     }
-
     TEL_COMPONENTS.forEach(c => delete address[c]);
   }
 
   /**
    * Merge new address into the specified address if mergeable.
    *
    * @param  {string} guid
    *         Indicates which address to merge.
@@ -1552,21 +1544,20 @@ class CreditCards extends AutofillRecord
     if (creditCard["cc-given-name"] || creditCard["cc-additional-name"] || creditCard["cc-family-name"]) {
       if (!creditCard["cc-name"]) {
         creditCard["cc-name"] = FormAutofillNameUtils.joinNameParts({
           given: creditCard["cc-given-name"],
           middle: creditCard["cc-additional-name"],
           family: creditCard["cc-family-name"],
         });
       }
-
-      delete creditCard["cc-given-name"];
-      delete creditCard["cc-additional-name"];
-      delete creditCard["cc-family-name"];
     }
+    delete creditCard["cc-given-name"];
+    delete creditCard["cc-additional-name"];
+    delete creditCard["cc-family-name"];
   }
 
   _normalizeCCNumber(creditCard) {
     if (creditCard["cc-number"]) {
       creditCard["cc-number"] = FormAutofillUtils.normalizeCCNumber(creditCard["cc-number"]);
       if (!creditCard["cc-number"]) {
         delete creditCard["cc-number"];
       }
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -35,20 +35,35 @@ 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_WITH_EMPTY_FIELD = {
-  "name": "Tim Berners",
+  name: "Tim Berners",
   "street-address": "",
 };
 
+const TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD = {
+  name: "",
+  "address-line1": "",
+  "address-line2": "",
+  "address-line3": "",
+  "country-name": "",
+  "tel-country-code": "",
+  "tel-national": "",
+  "tel-area-code": "",
+  "tel-local": "",
+  "tel-local-prefix": "",
+  "tel-local-suffix": "",
+  email: "timbl@w3.org",
+};
+
 const TEST_ADDRESS_WITH_INVALID_FIELD = {
   "street-address": "Another Address",
   invalidField: "INVALID",
 };
 
 const TEST_ADDRESS_EMPTY_AFTER_NORMALIZE = {
   country: "XXXXXX",
 };
@@ -332,16 +347,21 @@ add_task(async function test_add() {
   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);
 
+  // Empty computed fields shouldn't cause any problem.
+  profileStorage.addresses.add(TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD);
+  address = profileStorage.addresses._data[3];
+  do_check_eq(address.email, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD.email);
+
   Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./);
 
   Assert.throws(() => profileStorage.addresses.add({}),
     /Record contains no valid field\./);
 
   Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_EMPTY_AFTER_NORMALIZE),
     /Record contains no valid field\./);
@@ -393,16 +413,24 @@ add_task(async function test_update() {
   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);
 
+  // Empty computed fields shouldn't cause any problem.
+  profileStorage.addresses.update(profileStorage.addresses._data[0].guid, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD, false);
+  address = profileStorage.addresses._data[0];
+  do_check_eq(address.email, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD.email);
+  profileStorage.addresses.update(profileStorage.addresses._data[1].guid, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD, true);
+  address = profileStorage.addresses._data[1];
+  do_check_eq(address.email, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD.email);
+
   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\./
@@ -571,19 +599,25 @@ add_task(async function test_mergeToStor
                                                 [TEST_ADDRESS_1, TEST_ADDRESS_2]);
   // Merge an address to storage
   let anotherAddress = profileStorage.addresses._clone(TEST_ADDRESS_2);
   profileStorage.addresses.add(anotherAddress);
   anotherAddress.email = "timbl@w3.org";
   do_check_eq(profileStorage.addresses.mergeToStorage(anotherAddress).length, 2);
   do_check_eq(profileStorage.addresses.getAll()[1].email, anotherAddress.email);
   do_check_eq(profileStorage.addresses.getAll()[2].email, anotherAddress.email);
+
+  // Empty computed fields shouldn't cause any problem.
+  do_check_eq(profileStorage.addresses.mergeToStorage(TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD).length, 3);
 });
 
 add_task(async function test_mergeToStorage_strict() {
   let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
                                                 [TEST_ADDRESS_1, TEST_ADDRESS_2]);
   // Try to merge a subset with empty string
   let anotherAddress = profileStorage.addresses._clone(TEST_ADDRESS_1);
   anotherAddress.email = "";
   do_check_eq(profileStorage.addresses.mergeToStorage(anotherAddress, true).length, 0);
   do_check_eq(profileStorage.addresses.getAll()[0].email, TEST_ADDRESS_1.email);
+
+  // Empty computed fields shouldn't cause any problem.
+  do_check_eq(profileStorage.addresses.mergeToStorage(TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD, true).length, 1);
 });
--- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
@@ -34,16 +34,24 @@ const TEST_CREDIT_CARD_4 = {
 };
 
 const TEST_CREDIT_CARD_WITH_EMPTY_FIELD = {
   "cc-name": "",
   "cc-number": "1234123412341234",
   "cc-exp-month": 1,
 };
 
+const TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD = {
+  "cc-given-name": "",
+  "cc-additional-name": "",
+  "cc-family-name": "",
+  "cc-exp": "",
+  "cc-number": "1928374619283746",
+};
+
 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",
@@ -255,16 +263,22 @@ add_task(async function test_add() {
   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);
 
+  // Empty computed fields shouldn't cause any problem.
+  profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD);
+  creditCard = profileStorage.creditCards._data[3];
+  do_check_eq(creditCard["cc-number"],
+    profileStorage.creditCards._getMaskedCCNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
+
   Assert.throws(() => profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./);
 
   Assert.throws(() => profileStorage.creditCards.add({}),
     /Record contains no valid field\./);
 
   Assert.throws(() => profileStorage.creditCards.add(TEST_CREDIT_CARD_EMPTY_AFTER_NORMALIZE),
     /Record contains no valid field\./);
@@ -299,16 +313,26 @@ add_task(async function test_update() {
   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);
 
+  // Empty computed fields shouldn't cause any problem.
+  profileStorage.creditCards.update(profileStorage.creditCards._data[0].guid, TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD, false);
+  creditCard = profileStorage.creditCards._data[0];
+  do_check_eq(creditCard["cc-number"],
+    profileStorage.creditCards._getMaskedCCNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
+  profileStorage.creditCards.update(profileStorage.creditCards._data[1].guid, TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD, true);
+  creditCard = profileStorage.creditCards._data[1];
+  do_check_eq(creditCard["cc-number"],
+    profileStorage.creditCards._getMaskedCCNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
+
   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\./
@@ -487,9 +511,12 @@ add_task(async function test_mergeToStor
   // Merge a creditCard to storage
   let anotherCreditCard = profileStorage.creditCards._clone(TEST_CREDIT_CARD_3);
   anotherCreditCard["cc-name"] = "Foo Bar";
   do_check_eq(profileStorage.creditCards.mergeToStorage(anotherCreditCard).length, 2);
   do_check_eq(profileStorage.creditCards.getAll()[0]["cc-name"], "Foo Bar");
   do_check_eq(profileStorage.creditCards.getAll()[0]["cc-exp"], "2000-01");
   do_check_eq(profileStorage.creditCards.getAll()[1]["cc-name"], "Foo Bar");
   do_check_eq(profileStorage.creditCards.getAll()[1]["cc-exp"], "2000-01");
+
+  // Empty computed fields shouldn't cause any problem.
+  do_check_eq(profileStorage.creditCards.mergeToStorage(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD).length, 0);
 });
--- a/browser/extensions/formautofill/test/unit/test_transformFields.js
+++ b/browser/extensions/formautofill/test/unit/test_transformFields.js
@@ -268,16 +268,54 @@ const ADDRESS_NORMALIZE_TESTCASES = [
       "street-address": "street address\nstreet address line 2",
       "address-line2": "line2",
       "address-line3": "line3",
     },
     expectedResult: {
       "street-address": "street address\nstreet address line 2",
     },
   },
+  {
+    description: "Has only \"address-line1~2\"",
+    address: {
+      "address-line1": "line1",
+      "address-line2": "line2",
+    },
+    expectedResult: {
+      "street-address": "line1\nline2",
+    },
+  },
+  {
+    description: "Has only \"address-line1\"",
+    address: {
+      "address-line1": "line1",
+    },
+    expectedResult: {
+      "street-address": "line1",
+    },
+  },
+  {
+    description: "Has only \"address-line2~3\"",
+    address: {
+      "address-line2": "line2",
+      "address-line3": "line3",
+    },
+    expectedResult: {
+      "street-address": "\nline2\nline3",
+    },
+  },
+  {
+    description: "Has only \"address-line2\"",
+    address: {
+      "address-line2": "line2",
+    },
+    expectedResult: {
+      "street-address": "\nline2",
+    },
+  },
 
   // Country
   {
     description: "Has \"country\" in lowercase",
     address: {
       "country": "us",
     },
     expectedResult: {