Closed Bug 1008718 Opened 10 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
major

Tracking

(thunderbird32 fixed, thunderbird33 fixed, thunderbird34 fixed, thunderbird_esr24 unaffected, thunderbird_esr3133+ fixed)

RESOLVED FIXED
Thunderbird 34.0
Tracking Status
thunderbird32 --- fixed
thunderbird33 --- fixed
thunderbird34 --- fixed
thunderbird_esr24 --- unaffected
thunderbird_esr31 33+ fixed

People

(Reporter: craig, Assigned: mkmelin)

References

Details

(Keywords: regression, Whiteboard: [needs documentation])

Attachments

(1 file, 1 obsolete file)

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'
Severity: normal → major
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.
Blocks: TB31found
I don't seem able to reproduce.
Craig, did you have any further info that could help track this down. xref/dupe bug 1044559
Keywords: regression
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.
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/
Flags: needinfo?(galpeaceman)
Flags: needinfo?(eunall)
Flags: needinfo?(craig)
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 ?
Flags: needinfo?(galpeaceman)
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.)
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: magnus@netti.fi
   * contatc2: display name: Magnus, email: magnus.melin@hut.fi
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 <magnus.melin@hut.fi>
5) add some subject, and message, hit Send Later
6) check Outbox, the source of the message has (only)
To: m2 <magnus@netti.fi>

... which is the address(es) on the list, not the address entered on the To line
Blocks: 970118
Status: UNCONFIRMED → NEW
Component: Untriaged → Composition
Ever confirmed: true
Flags: needinfo?(eunall)
Flags: needinfo?(craig)
OS: Windows 8.1 → All
Product: Thunderbird → MailNews Core
Hardware: x86_64 → All
Summary: Reply-to is sending to wrong email if "name" is in address book twice → sending to wrong email (list) if "name" is in address book twice and one of them is a list
Looks like I have a patch, looking into tests now.
Assignee: nobody → mkmelin+mozilla
So basically the nsMsgMailListComparator is just wrong, and since it's used to differentiate mail addresses from mail lists...
Attachment #8470467 - Flags: review?(Pidgeot18)
The try run looked ok I think, given the state of the tree - https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b2ca7fc2787d
Status: NEW → ASSIGNED
Joshua: ping on the review, we should make sure to get this in soon so it can go into 31.1.
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).
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.
Attachment #8470467 - Attachment is obsolete: true
Attachment #8470467 - Flags: review?(Pidgeot18)
Attachment #8479316 - Flags: review?(Pidgeot18)
Attachment #8479316 - Flags: review?(Pidgeot18) → review+
https://hg.mozilla.org/comm-central/rev/148e7a1145c4 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Attachment #8479316 - Flags: approval-comm-esr31?
Attachment #8479316 - Flags: approval-comm-beta?
Attachment #8479316 - Flags: approval-comm-aurora?
Attachment #8479316 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment #8479316 - Flags: approval-comm-esr31?
Attachment #8479316 - Flags: approval-comm-esr31+
Attachment #8479316 - Flags: approval-comm-beta?
Attachment #8479316 - Flags: approval-comm-beta+
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 <homer@example.com>,Marge <marge@example.com>" - 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
Attachment #8479316 - Flags: approval-comm-esr31+ → approval-comm-esr31-
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.
Attachment #8479316 - Flags: approval-comm-esr31- → approval-comm-esr31?
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 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.
Attachment #8479316 - Flags: approval-comm-esr31? → approval-comm-esr31+
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.
Summary: sending to wrong email (list) if "name" is in address book twice and one of them is a list → sending to wrong email (list) if "name" is in address book twice and one of them is a mailing list
Whiteboard: [needs documentation]
You need to log in before you can comment on or make changes to this bug.