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

RESOLVED FIXED in Thunderbird 34.0

Status

defect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: craig, Assigned: mkmelin)

Tracking

({regression})

Dependency tree / graph

Thunderbird Tracking Flags

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

Details

(Whiteboard: [needs documentation])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
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'
Reporter

Updated

5 years ago
Severity: normal → major

Comment 1

5 years ago
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.

Updated

5 years ago
Blocks: TB31found
Assignee

Comment 2

5 years ago
I don't seem able to reproduce.
Assignee

Comment 3

5 years ago
Craig, did you have any further info that could help track this down. xref/dupe bug 1044559
Keywords: regression
Assignee

Updated

5 years ago
Duplicate of this bug: 1044559
Assignee

Comment 5

5 years ago
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.
Assignee

Updated

5 years ago
Assignee

Comment 6

5 years ago
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)

Comment 7

5 years ago
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)
Assignee

Comment 8

5 years ago
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.)
Assignee

Updated

5 years ago
Duplicate of this bug: 1047305
Assignee

Comment 10

5 years ago
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
Assignee

Updated

5 years ago
Duplicate of this bug: 1048194
Assignee

Updated

5 years ago
Duplicate of this bug: 1050775
Assignee

Comment 13

5 years ago
Looks like I have a patch, looking into tests now.
Assignee: nobody → mkmelin+mozilla
Assignee

Comment 14

5 years ago
So basically the nsMsgMailListComparator is just wrong, and since it's used to differentiate mail addresses from mail lists...
Attachment #8470467 - Flags: review?(Pidgeot18)
Assignee

Comment 15

5 years ago
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
Assignee

Updated

5 years ago
Duplicate of this bug: 1054743
Assignee

Comment 17

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

Updated

5 years ago
Duplicate of this bug: 1057539
Assignee

Updated

5 years ago
Duplicate of this bug: 1058005
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).
Assignee

Comment 21

5 years ago
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+
Assignee

Comment 22

5 years ago
https://hg.mozilla.org/comm-central/rev/148e7a1145c4 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Assignee

Comment 23

5 years ago
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?
Assignee

Updated

5 years ago
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-
Assignee

Comment 28

5 years ago
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?
Assignee

Comment 29

5 years ago
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+
Assignee

Updated

5 years ago
Duplicate of this bug: 1063955

Comment 33

5 years ago
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.