Bug 1008718 - sending to wrong email (list) if "name" is in address book twice and one of them is a list. r=jcranmer, a=standard8 for landing on CLOSED TREE
authorMagnus Melin <mkmelin+mozilla@iki.fi>
Wed, 27 Aug 2014 21:27:21 +0300
changeset 16520 148e7a1145c4b1c5e087074e57c032a8c2c8ac28
parent 16519 8f57b03404285702405cfb4516765e5ad83e4193
child 16521 350ababdb84f4bd71b56d2f7cbf43fa949a3d63e
push id1234
push usermbanner@mozilla.com
push dateMon, 13 Oct 2014 17:59:18 +0000
treeherdercomm-esr52@b5a0add9fb18 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcranmer, standard8
bugs1008718
Bug 1008718 - sending to wrong email (list) if "name" is in address book twice and one of them is a list. r=jcranmer, a=standard8 for landing on CLOSED TREE
mailnews/compose/src/nsMsgCompose.cpp
mailnews/compose/test/unit/data/listexpansion.mab
mailnews/compose/test/unit/test_expandMailingLists.js
mailnews/compose/test/unit/xpcshell.ini
--- a/mailnews/compose/src/nsMsgCompose.cpp
+++ b/mailnews/compose/src/nsMsgCompose.cpp
@@ -4601,44 +4601,52 @@ nsresult nsMsgCompose::BuildMailListArra
           }
         }
       }
     }
   }
   return rv;
 }
 
+/**
+ * Comparator for use with nsTArray::IndexOf to find a recipient.
+ * This comparator will check if an "address" is a mail list or not.
+ */
 struct nsMsgMailListComparator
 {
+  // A mail list will have one of the formats
+  //  1) "mName <mDescription>" when the list has a description
+  //  2) "mName <mName>" when the list lacks description
+  // A recipient is of the form "mName <mEmail>" - for equality the list
+  // name must be the same. The recipient "email" must match the list name for
+  // case 1, and the list description for case 2.
   bool Equals(const nsMsgMailList &mailList,
               const nsMsgRecipient &recipient) const {
-    if (mailList.mName.Equals(recipient.mName,
-        nsCaseInsensitiveStringComparator()))
-      return true;
-    return mailList.mDescription.Equals(
-      recipient.mEmail.IsEmpty() ? recipient.mName : recipient.mEmail,
-      nsCaseInsensitiveStringComparator());
+    if (!mailList.mName.Equals(recipient.mName,
+                               nsCaseInsensitiveStringComparator()))
+      return false;
+    return mailList.mDescription.IsEmpty() ?
+      mailList.mName.Equals(recipient.mEmail, nsCaseInsensitiveStringComparator()) :
+      mailList.mDescription.Equals(recipient.mEmail, nsCaseInsensitiveStringComparator());
   }
 };
 
 nsresult
 nsMsgCompose::LookupAddressBook(RecipientsArray &recipientsList)
 {
   nsresult rv = NS_OK;
 
   // First, build some arrays with the original recipients.
 
   nsAutoString originalRecipients[MAX_OF_RECIPIENT_ARRAY];
   m_compFields->GetTo(originalRecipients[0]);
   m_compFields->GetCc(originalRecipients[1]);
   m_compFields->GetBcc(originalRecipients[2]);
 
-  uint32_t i, j, k;
-
-  for (i = 0; i < MAX_OF_RECIPIENT_ARRAY; ++i)
+  for (uint32_t i = 0; i < MAX_OF_RECIPIENT_ARRAY; ++i)
   {
     if (originalRecipients[i].IsEmpty())
       continue;
 
     rv = m_compFields->SplitRecipientsEx(originalRecipients[i],
                                          recipientsList[i]);
     NS_ENSURE_SUCCESS(rv, rv);
   }
@@ -4652,17 +4660,17 @@ nsMsgCompose::LookupAddressBook(Recipien
   nsCOMArray<nsIAbDirectory> addrbookDirArray;
   rv = GetABDirectories(NS_LITERAL_CSTRING(kAllDirectoryRoot),
                         addrbookDirArray);
   if (NS_SUCCEEDED(rv))
   {
     nsString dirPath;
     uint32_t nbrAddressbook = addrbookDirArray.Count();
 
-    for (k = 0; k < nbrAddressbook && stillNeedToSearch; ++k)
+    for (uint32_t k = 0; k < nbrAddressbook && stillNeedToSearch; ++k)
     {
       // Avoid recursive mailing lists
       if (abDirectory && (addrbookDirArray[k] == abDirectory))
       {
         stillNeedToSearch = false;
         break;
       }
 
@@ -4679,20 +4687,20 @@ nsMsgCompose::LookupAddressBook(Recipien
       mailListArray.Clear();
 
       // Collect all mailing lists defined in this address book
       rv = BuildMailListArray(abDirectory, mailListArray);
       if (NS_FAILED(rv))
         return rv;
 
       stillNeedToSearch = false;
-      for (i = 0; i < MAX_OF_RECIPIENT_ARRAY; i ++)
+      for (uint32_t i = 0; i < MAX_OF_RECIPIENT_ARRAY; i ++)
       {
         // Note: We check this each time to allow for length changes.
-        for (j = 0; j < recipientsList[i].Length(); ++j)
+        for (uint32_t j = 0; j < recipientsList[i].Length(); ++j)
         {
           nsMsgRecipient &recipient = recipientsList[i][j];
           if (!recipient.mDirectory)
           {
             // First check if it's a mailing list
             size_t index = mailListArray.IndexOf(recipient, 0,
               nsMsgMailListComparator());
             if (index != mailListArray.NoIndex &&
@@ -4763,17 +4771,16 @@ nsMsgCompose::ExpandMailingLists()
           nsCOMPtr<nsIAbCard> existingCard(do_QueryElementAt(mailListAddresses,
                                            nbrAddresses - 1, &rv));
           NS_ENSURE_SUCCESS(rv, rv);
 
           nsMsgRecipient newRecipient;
           bool bIsMailList;
           rv = existingCard->GetIsMailList(&bIsMailList);
           NS_ENSURE_SUCCESS(rv, rv);
-
           NS_ASSERTION(!bIsMailList,
              "Bug 40301 means we don't support this feature.");
 
           rv = existingCard->GetDisplayName(newRecipient.mName);
           NS_ENSURE_SUCCESS(rv, rv);
           rv = existingCard->GetPrimaryEmail(newRecipient.mEmail);
           NS_ENSURE_SUCCESS(rv, rv);
 
new file mode 100644
--- /dev/null
+++ b/mailnews/compose/test/unit/data/listexpansion.mab
@@ -0,0 +1,75 @@
+// <!-- <mdb:mork:z v="1.4"/> -->
+< <(a=c)> // (f=iso-8859-1)
+  (B8=LastModifiedDate)(B9=RecordKey)(BA=AddrCharSet)(BB=LastRecordKey)
+  (BC=ns:addrbk:db:table:kind:pab)(BD=ListName)(BE=ListNickName)
+  (BF=ListDescription)(C0=ListTotalAddresses)(C1=LowercaseListName)
+  (C2=ns:addrbk:db:table:kind:deleted)(C3=_Yahoo)(C4=_MSN)
+  (C5=_GoogleTalk)(C6=_Skype)(C7=_IRC)(C8=_JabberId)
+  (C9=PreferDisplayName)(CA=PhotoURI)(CB=PhotoType)(CC=PhotoName)
+  (CD=DbRowID)(CE=_QQ)(CF=_ICQ)(80=ns:addrbk:db:row:scope:card:all)
+  (81=ns:addrbk:db:row:scope:list:all)
+  (82=ns:addrbk:db:row:scope:data:all)(83=FirstName)(84=LastName)
+  (85=PhoneticFirstName)(86=PhoneticLastName)(87=DisplayName)
+  (88=NickName)(89=PrimaryEmail)(8A=LowercasePrimaryEmail)
+  (8B=SecondEmail)(8C=LowercaseSecondEmail)(8D=PreferMailFormat)
+  (8E=PopularityIndex)(8F=WorkPhone)(90=HomePhone)(91=FaxNumber)
+  (92=PagerNumber)(93=CellularNumber)(94=WorkPhoneType)(95=HomePhoneType)
+  (96=FaxNumberType)(97=PagerNumberType)(98=CellularNumberType)
+  (99=HomeAddress)(9A=HomeAddress2)(9B=HomeCity)(9C=HomeState)
+  (9D=HomeZipCode)(9E=HomeCountry)(9F=WorkAddress)(A0=WorkAddress2)
+  (A1=WorkCity)(A2=WorkState)(A3=WorkZipCode)(A4=WorkCountry)
+  (A5=JobTitle)(A6=Department)(A7=Company)(A8=_AimScreenName)
+  (A9=AnniversaryYear)(AA=AnniversaryMonth)(AB=AnniversaryDay)
+  (AC=SpouseName)(AD=FamilyName)(AE=WebPage1)(AF=WebPage2)(B0=BirthYear)
+  (B1=BirthMonth)(B2=BirthDay)(B3=Custom1)(B4=Custom2)(B5=Custom3)
+  (B6=Custom4)(B7=Notes)>
+
+<(91=4)(81=Simpson)(82=)(80=0)(89=1407526575)(83=1)(8A=homer@example.com)
+  (85=generic)(86=Marge)(8B=1407526584)(8C=marge@example.com)(88=2)
+  (8D=Bart)(8E=bart@foobar.invalid)(8F=3)(90=lisa@example.com)>
+{1:^80 {(k^BC:c)(s=9)} 
+  [1:^82(^BB=4)]
+  [1(^87^81)(^86=)(^C3=)(^A1=)(^AE=)(^C4=)(^C5=)(^B2=)(^A7=)(^9A=)(^B3=)
+    (^C6=)(^A4=)(^A5=)(^A3=)(^C7=)(^85=)(^B0=)(^8E=0)(^C8=)(^A0=)(^A2=)
+    (^B7=)(^8D=0)(^9E=)(^B8^89)(^83=)(^A8=)(^C9=1)(^84=)(^92=)(^CA=)
+    (^B6=)(^9D=)(^89^8A)(^A6=)(^AF=)(^CB^85)(^8F=)(^CC=)(^8B=)(^93=)
+    (^90=)(^CD=1)(^B1=)(^CE=)(^9B=)(^9C=)(^91=)(^B4=)(^9F=)(^99=)(^88=)
+    (^CF=)(^B5=)(^8A^8A)(^B9=1)]
+  [2(^87^86)(^86=)(^C3=)(^A1=)(^AE=)(^C4=)(^C5=)(^B2=)(^A7=)(^9A=)(^B3=)
+    (^C6=)(^A4=)(^A5=)(^A3=)(^C7=)(^85=)(^B0=)(^8E=0)(^C8=)(^A0=)(^A2=)
+    (^B7=)(^8D=0)(^9E=)(^B8^8B)(^83=)(^A8=)(^C9=1)(^84=)(^92=)(^CA=)
+    (^B6=)(^9D=)(^89^8C)(^A6=)(^AF=)(^CB^85)(^8F=)(^CC=)(^8B=)(^93=)
+    (^90=)(^CD=2)(^B1=)(^CE=)(^9B=)(^9C=)(^91=)(^B4=)(^9F=)(^99=)(^88=)
+    (^CF=)(^B5=)(^8A^8C)(^B9=2)]
+  [3(^87^8D)(^86=)(^C3=)(^A1=)(^AE=)(^C4=)(^C5=)(^B2=)(^A7=)(^9A=)(^B3=)
+    (^C6=)(^A4=)(^A5=)(^A3=)(^C7=)(^85=)(^B0=)(^8E=0)(^C8=)(^A0=)(^A2=)
+    (^B7=)(^8D=0)(^9E=)(^B8=0)(^83=)(^A8=)(^C9=1)(^84=)(^92=)(^CA=)
+    (^B6=)(^9D=)(^89^8E)(^A6=)(^AF=)(^CB^85)(^8F=)(^CC=)(^8B=)(^93=)
+    (^90=)(^CD=3)(^B1=)(^CE=)(^9B=)(^9C=)(^91=)(^B4=)(^9F=)(^99=)(^88=)
+    (^CF=)(^B5=)(^8A^8E)(^B9=3)]
+  [4(^87=)(^86=)(^C3=)(^A1=)(^AE=)(^C4=)(^C5=)(^B2=)(^A7=)(^9A=)(^B3=)
+    (^C6=)(^A4=)(^A5=)(^A3=)(^C7=)(^85=)(^B0=)(^8E=0)(^C8=)(^A0=)(^A2=)
+    (^B7=)(^8D=0)(^9E=)(^B8=0)(^83=)(^A8=)(^C9=1)(^84=)(^92=)(^CA=)
+    (^B6=)(^9D=)(^89^90)(^A6=)(^AF=)(^CB^85)(^8F=)(^CC=)(^8B=)(^93=)
+    (^90=)(^CD=4)(^B1=)(^CE=)(^9B=)(^9C=)(^91=)(^B4=)(^9F=)(^99=)(^88=)
+    (^CF=)(^B5=)(^8A^90)(^B9=4)]}
+
+@$${5{@
+< <(a=c)> // (f=iso-8859-1)
+  (D0=Address1)(D1=Address2)(D2=Address3)(D3=Address4)>
+
+<(93=5)(92=simpson)>
+{1:^80 {(k^BC:c)(s=9)} 
+  [-1:^81(^BD^92)(^C1^92)(^BE=)(^BF=)(^C0=4)(^D0=1)(^D1=2)(^D2=3)(^D3=4)
+    (^B9=5)]}
+[4:^80(^87^90)]
+[1:^82(^BB=5)]
+@$$}5}@
+
+@$${6{@
+
+<(96=6)(94=marge)(95=marges own list)>
+{1:^80 {(k^BC:c)(s=9)} 
+  [-2:^81(^BD^94)(^C1^94)(^BE=)(^BF^95)(^C0=2)(^D0=1)(^D1=2)(^B9=6)]}
+[1:^82(^BB=6)]
+@$$}6}@
new file mode 100644
--- /dev/null
+++ b/mailnews/compose/test/unit/test_expandMailingLists.js
@@ -0,0 +1,71 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+
+/**
+ * Tests nsMsgCompose expandMailingLists.
+ */
+
+const MsgComposeContractID = "@mozilla.org/messengercompose/compose;1";
+const MsgComposeParamsContractID = "@mozilla.org/messengercompose/composeparams;1";
+const MsgComposeFieldsContractID = "@mozilla.org/messengercompose/composefields;1";
+const nsIMsgCompose = Components.interfaces.nsIMsgCompose;
+const nsIMsgComposeParams = Components.interfaces.nsIMsgComposeParams;
+const nsIMsgCompFields = Components.interfaces.nsIMsgCompFields;
+
+Components.utils.import("resource://gre/modules/Services.jsm");
+Components.utils.import("resource:///modules/mailServices.js");
+
+/**
+ * Helper to check population worked as expected.
+ * @param aTo - text in the To field
+ * @param aCheckTo - the expected To addresses (after possible ist population)
+ */
+function checkPopulate(aTo, aCheckTo)
+{
+  let msgCompose = Components.classes[MsgComposeContractID]
+                             .createInstance(nsIMsgCompose);
+
+  // Set up some basic fields for compose.
+  let fields = Components.classes[MsgComposeFieldsContractID]
+                         .createInstance(nsIMsgCompFields);
+
+  fields.to = aTo;
+
+  // Set up some params
+  let params = Components.classes[MsgComposeParamsContractID]
+                         .createInstance(nsIMsgComposeParams);
+
+  params.composeFields = fields;
+
+  msgCompose.initialize(params);
+
+  msgCompose.expandMailingLists();
+  equal(fields.to, aCheckTo);
+}
+
+function run_test() {
+  // Test setup - copy the data files into place
+  let testAB = do_get_file("./data/listexpansion.mab");
+
+  // Copy the file to the profile directory for a PAB
+  testAB.copyTo(do_get_profile(), kPABData.fileName);
+
+  // XXX Getting all directories ensures we create all ABs because mailing
+  // lists need help initialising themselves
+  MailServices.ab.directories;
+
+  // Test expansion of list with no description.
+  checkPopulate("simpson <simpson>", "Simpson <homer@example.com>,Marge <marge@example.com>,Bart <bart@foobar.invalid>,\"lisa@example.com\" <lisa@example.com>");
+
+  // Test expansion fo list with description.
+  checkPopulate("marge <marges own list>", "Simpson <homer@example.com>,Marge <marge@example.com>");
+
+  // Test we don't mistake an email address for a list, with a few variations.
+  checkPopulate("Simpson <homer@example.com>", "Simpson <homer@example.com>");
+  checkPopulate("simpson <homer@example.com>", "simpson <homer@example.com>");
+  checkPopulate("simpson <homer@not-in-ab.invalid>", "simpson <homer@not-in-ab.invalid>");
+
+  checkPopulate("Marge <marge@example.com>", "Marge <marge@example.com>");
+  checkPopulate("marge <marge@example.com>", "marge <marge@example.com>");
+  checkPopulate("marge <marge@not-in-ab.invalid>", "marge <marge@not-in-ab.invalid>");
+
+};
--- a/mailnews/compose/test/unit/xpcshell.ini
+++ b/mailnews/compose/test/unit/xpcshell.ini
@@ -4,16 +4,17 @@ tail = tail_compose.js
 support-files = data/*
 
 [test_attachment.js]
 [test_attachment_intl.js]
 [test_bug155172.js]
 [test_bug235432.js]
 [test_bug474774.js]
 [test_detectAttachmentCharset.js]
+[test_expandMailingLists.js]
 [test_mailtoURL.js]
 [test_nsMsgCompose1.js]
 [test_nsMsgCompose2.js]
 [test_nsMsgCompose3.js]
 [test_nsMsgCompose4.js]
 [test_nsSmtpService1.js]
 [test_saveDraft.js]
 [test_sendBackground.js]