1008718 - sending to wrong email (list) if "name" is in address book twice and one of them is a mailing list

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Craig Dearden]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.99 Safari/537.36

Steps to reproduce:

In my Personal Address book - I have a mailing list called - Internet Team
I received an email from one of my team, in my address book named as 'Molly Internet Team', I've clicked reply.
The message compose window appears normal, however the 'to' says 'Internet Team' <her email>


Actual results:

When sending, the 'Internet Team' changes to the Mailing List in my address book.


Expected results:

Expected to reply to 'Molly Internet Team'

Comment 1

[rogers]

I have a very similar situation.  I have several email accounts and when I send a test message between accounts, when received the email has the wrong "From" account.  When I click reply it also replies to the wrong account (not the one I sent it from originally).  I just noticed this recently but in the past I have always wondered why, when the recipient of an email replies, it doesn't always reply to the account I sent it from.. This seems to be a serious problem as far as I am concerned.

Comment 2

[Magnus Melin [:mkmelin]]

I don't seem able to reproduce.

Comment 3

[Magnus Melin [:mkmelin]]

Craig, did you have any further info that could help track this down. xref/dupe bug 1044559

Comment 5

[Magnus Melin [:mkmelin]]

eunall: could you attach the problem mail (as .eml). 
Bug 1044559 have sample mails in hebrew, but if you have a more readable testcase that could be useful.

Comment 6

[Magnus Melin [:mkmelin]]

If I had to suspect a regression it would be bug 970118, bug 842632 or a combination of those.

Those who can reproduce: could you try a nightly build from before bug 970118 landed, and report back on the results?

2014-03-03: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/03/2014-03-03-03-02-04-comm-central/
2014-03-05: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/03/2014-03-05-03-02-02-comm-central/

Comment 7

[Gal Ish Shalom]

Don't know why this is a duplicate of 1044559
Original problem refers mainly to people that don't appear in the address book
Furthermore, original problem sends emails to non-existing Mailing Lists...

But, if this could help resolve the issue, I didn't understand your more info request
What should I do exactly ?

Comment 8

[Magnus Melin [:mkmelin]]

Ok, let's reopen bug 1044559 then, so put the info there. 
If you can reproduce, what I'd like you to do is to check it happened to regress around 2014-03-04 by first trying the 
2014-03-03 build (download and install), then the 2014-03-05 build and see if there one reproduces the problem and the other not. (If not you could of course go back way further and find the regression range.)

Comment 10

[Magnus Melin [:mkmelin]]

Alexander confirmed in bug 1047305 that it indeed regressed in the range I guessed. So I think it's save to say this is fallout from bug 970118.

I couldn't reproduce in an old profile (not sure why yet), but in a clean profile I did the following, and can now reproduce (on trunk).

1) open the Address Book, select the personal address book
2) add two contacts:
   * contact1: display name: m2, email: [e-mail]
   * contatc2: display name: Magnus, email: [e-mail]
3) add a list, with list name: magnus
   * add contact1 to the list
4) open a new compose window, enter this in the To-line:
Magnus <[e-mail]>
5) add some subject, and message, hit Send Later
6) check Outbox, the source of the message has (only)
To: m2 <[e-mail]>

... which is the address(es) on the list, not the address entered on the To line

Comment 13

[Magnus Melin [:mkmelin]]

Looks like I have a patch, looking into tests now.

Comment 14

[Magnus Melin [:mkmelin]]

Attachment: bug1008718_sends_to_wrong_addr.patch

So basically the nsMsgMailListComparator is just wrong, and since it's used to differentiate mail addresses from mail lists...

Comment 15

[Magnus Melin [:mkmelin]]

The try run looked ok I think, given the state of the tree - https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b2ca7fc2787d

Comment 17

[Magnus Melin [:mkmelin]]

Joshua: ping on the review, we should make sure to get this in soon so it can go into 31.1.

Comment 20

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8470467 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch

Review of attachment 8470467 [details] [diff] [review]:
-----------------------------------------------------------------

A comment about the test database:

If feels more appropriate to me under compose/test/unit/data than a unified mailnews/test/data. Even better, though, I think would be to build the database manually in the test code (it's not too complex and it's better documentation within the test file itself).

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4604,5 @@
>      }
>    }
>    return rv;
>  }
> +/**

Nit: add a blank line, please.

@@ +4605,5 @@
>    }
>    return rv;
>  }
> +/**
> + * Comparator; with this we can use IndexOf to find a recipient in an

Nit: "Comparator for use with nsTArray::IndexOf to find a recipient" or something else flows better.

@@ +4622,5 @@
>                const nsMsgRecipient &recipient) const {
> +    if (!mailList.mName.Equals(recipient.mName,
> +                               nsCaseInsensitiveStringComparator()))
> +      return false;
> +    return mailList.mDescription.IsEmpty() ? 

Nit: trailing whitespace

@@ +4624,5 @@
> +                               nsCaseInsensitiveStringComparator()))
> +      return false;
> +    return mailList.mDescription.IsEmpty() ? 
> +      mailList.mName.Equals(recipient.mEmail, nsCaseInsensitiveStringComparator()) :
> +      mailList.mDescription.Equals(recipient.mEmail, nsCaseInsensitiveStringComparator());

(/me wonders why the name for case-insensitive string comparison is sooo bloody long).

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: bug1008718_sends_to_wrong_addr.patch

Updated location and nits.
I didn't make it add the list dynamically. It shouldn't be that hard, but I don't have time to do it right now, and with an actual file we can be sure it's  a "real" situation we're testing.

Comment 22

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/148e7a1145c4 -> FIXED

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 8479316 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch

[Approval Request Comment]
Regression caused by (bug #): 970118
User impact if declined: thunderbird sends mail to a mail list if a displayname happens to be the list name, or if the list name is empty and recipeint display name is empty
Testing completed (on c-c, etc.): works locally and on try, just landed on trunk
Risk to taking this patch (and alternatives if risky): not very risky

Comment 24

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/37f5cd53ae9a

Comment 25

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-beta/rev/15a9f27b2157

Comment 26

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr31/rev/e7d00a26719e

Comment 27

[Mark Banner (:standard8)]

I had to back this out from esr due to test bustage:

https://hg.mozilla.org/releases/comm-esr31/rev/1eeeb979e2b3

The builders are showing:

TEST-UNEXPECTED-FAIL | /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js | ReferenceError: equal is not defined at /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js:42 - See following stack:

JS frame :: /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js:42 :: checkPopulate :: line 3
JS frame :: run_test@/Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js:57:3
JS frame :: /Users/mark/tb/esr31/mozilla/testing/xpcshell/head.js:387 :: _execute_test :: line 5
JS frame :: @-e:1:1

However, even when I change the 'equal' to 'Assert.equal', this fails with:

TEST-UNEXPECTED-FAIL | /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js | "marge <\\"marges own list\\">" == "Simpson <[e-mail]>,Marge <[e-mail]>" - See following stack:
JS frame :: /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js :: checkPopulate :: line 42
JS frame :: /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js :: run_test :: line 60
JS frame :: /Users/mark/tb/esr31/mozilla/testing/xpcshell/head.js :: _execute_test :: line 387
JS frame :: -e :: <TOP_LEVEL> :: line 1

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 8479316 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch

Given the severity of this bug, may I suggest we just land the fix without the test for ESR (or disable the test). I have a hard time thinking the code wouldn't work as that code is same on trunk, and there try was ok with it at some point, so it's probably a test issue of some sort.

Comment 29

[Magnus Melin [:mkmelin]]

Tested a build from https://ftp-ssl.mozilla.org/pub/mozilla.org/thunderbird/tinderbox-builds/comm-esr31-linux64/1409226347/ (which had the fix) and works using manual testing.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8479316 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch

Ok, given a manual test works, we'll re-land with the test disabled.

Comment 31

[Mark Banner (:standard8)]

Re-landed: https://hg.mozilla.org/releases/comm-esr31/rev/644a0bcaeffd

Comment 33

[Thomas D. (:thomas8)]

FTR: How recipient autocomplete deals with mailing list names (or not) should be documented on TB's support pages. Reminiscent of Nicknames, where preferential treatment is explicitly wanted and required.