1524076 - Pasted recipient email address a@b autocompletes to more popular primary email address c@d

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P2)

Description

[amos]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

I'm composing a new message
I paste an email address of the form [e-mail] into the 'to' field of the email.
I click back into the message area to edit the email.

Actual results:

Thunderbird amends the addressee to [e-mail]

Note that both [e-mail] and [e-mail] are in my address book, accounts@ as an additional email address to the main address xx@

Expected results:

Thunderbird should not modify the valid email address I paste into the 'to' field.

This happens reproducibly and has been going on for many months. (It's why I've not been getting paid. ;-) )

In case it's relevant, I'm also using gContactSync.

This is similar to, but probably not identical with this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1130858

Comment 1

[Wayne Mery (:wsmwk)]

Does it also happen when started in safe mode?
https://support.mozilla.org/en-US/kb/safe-mode-thunderbird

Comment 2

[amos]

Yes, I've just tried it, it still happens in safe mode.

Comment 3

[Wayne Mery (:wsmwk)]

But if you type it rather than paste it, it sticks?

Comment 4

[amos]

Interesting question.

I just tried – I get as far as typing 'accounts@', at which point autocomplete, to my mind prematurely, inserts the (correct) address from my address book - if I continue typing it continues after the already inserted address.

A little further experimentation reveals that
a) this only happens when I pause (even fleetingly) as I search for the AltGr key to type @ (the @ symbol on my [German] keyboard is AltGr+Q). If I'm quick enough, it doesn't interrupt my typing.
b) it doesn't happen if I pause elsewhere in the address – it offers me addresses from my address book, but it doesn't insert them automatically.

This looks like an additional bug/feature.

Comment 5

[Jorg K (CEST = GMT+2)]

Ben, can you try this.

Comment 6

[Benjamin Flanagin]

I've failed to replicate this issue in 60.8 or 69b3

First Test

Created two accounts based on the available info provided by the bug owner.
Copied and pasted [e-mail] in the to address
no issue
Manually typed in accounts@ and the auto complete suggests the correct account.
no issue

Second Test

Using two of my own addresses from individuals that share a domain I went through the same steps with no issue.

Comment 7

[Benjamin Flanagin]

Amos,

Have you looked through your address book to make sure that there isn't a address that contains both accounts and the xx address? That would cause the issue you are seeing

Comment 8

[amos]

So, as set out in my original post, that's exactly my situation. I'm entering an address set up as an additional email, and it's reverting it, on pasting (and not if I type it in, but see comment 4 above), to the 'main' email address.

I take it from your response that you consider this a feature, not a bug? Not at all sure I agree with that asessment, but at least I know how to fix it for me now.

Comment 9

[Jorg K (CEST = GMT+2)]

We missed that:

Note that both [e-mail] and [e-mail] are in my address book, accounts@ as an additional email address to the main address xx@

At least I didn't think that it was on the same contact. But OK, you said "additional".

Comment 10

[amos]

Sorry if I was unclear. "Additional email" is the term used in the address book in my version of Thunderbird.

To clarify, in case things are still unclear:
I have a single contact (customer-company-name), for which the "Email" field is set to [e-mail] and the 'Additional email' field is set to [e-mail]. When writing an email, on pasting [e-mail] into the recipient field, Thunderbird silently changes it to [e-mail].

As a result, I have been sending emails to the wrong address.

You can (quite reasonably) argue that I've got my contacts set up 'wrong', but this still seems like it shouldn't be the desired behaviour.

Comment 11

[Jorg K (CEST = GMT+2)]

As a result, I have been sending emails to the wrong address.

Hmm, "wrong address". Well, it was sent to the "main" address of the contact, how can that be wrong? For different recipients, I guess you should two different address book entries.

WONTFIX, Magnus?

Comment 12

[amos]

As I noted, I wouldn't disagree that in this case I have (had) my contacts set up incorrectly.

But I'm nonetheless not sure I would agree with you that this is expected or desirable behaviour.

There are many perfectly legitimate reasons to want to send an email specifically to a contact's 'main' or 'additional' email address. The obvious one is where you have both a work and non-work email address for a contact. There are many reasons why you might not wish to accidentally send a non-work email to that contact's work email address – I assume I don't need to spell these out.

I don't think it's correct that it doesn't matter which email address an email gets sent to as long as it's the same contact. (Which is what I understand you to be saying – apologies if I've misunderstood.)

Comment 13

[Magnus Melin [:mkmelin]]

Agreed it's a bug. A little unexpected!
Skimming over the code, I think the problem may be that we don't have a previous result, so the scoring won't properly give a score to the pasted address. Should be around here: https://searchfox.org/comm-central/rev/d55fb7f2405fd4e07cb267851340626a63a8c270/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#367

Comment 14

[Ellen (Yufei) Chen]

Hi there! I am a University of Toronto compsci student interested in picking this bug up as my first Mozilla bug. Can I claim this? Also, I would love if anyone can mentor me on this!

Comment 15

[Jorg K (CEST = GMT+2)]

All yours. Since Magnus claimed that this was easy, he can mentor you.

Comment 16

[Magnus Melin [:mkmelin]]

Ellen, let me know if you need any advice. Comment 13 should get you started.

Comment 17

[Ellen (Yufei) Chen]

Hi Magnus,

I've looked over the code and I noticed that while the bulk of the startSearch function is collecting search results, it is at the very end of the function that the search results are sorted with the result._searchResults.sort() call, so I think this is a good place to target my inquiry. Currently, if the search results contain two results that belong to the same contact card, the primary email is preferred over any secondary emails, and the sort function does not take into account the actual entered search query (the query is only used for compiling the search results). I am thinking that this is the reason why the primary email is being autofilled in this case, even though the search query is exactly the same as the secondary email search result that is not selected.

I am in the process of building Thunderbird, and I will get into implementing and testing my theory after this. Please let me know if I am on the right track!

Comment 18

[Ellen (Yufei) Chen]

Actually, scratch that, I just realized that the score should take care of that, and the problem is that the correct email address (the secondary email address) is not being placed into the results. I will proceed to look into this more!

Comment 19

[Magnus Melin [:mkmelin]]

Before testing other hypothesis, I'd recommend taking a close look into comment 13.

Comment 20

[Ellen (Yufei) Chen]

(In reply to Magnus Melin [:mkmelin] from comment #19)

Before testing other hypothesis, I'd recommend taking a close look into comment 13.

Will do! Is there an easy way to debug the application, or log messages to the console/terminal?

Comment 21

[Ellen (Yufei) Chen]

I was able to log messages to the console, and I've found the root of the problem:

When assigning scores, the _getScore() function doesn't actually take into consideration that the search query could be complete email addresses. It first discards the domain name of the potential match email (so as to not try to match the domain name), and then uses indexOf() to try to find the search query inside that string, which would now result in a -1 as the domain name portion is not found. This means that both the primary and the secondary email now has a score of 0 (as confirmed by logging into the console), and the emails are compared by their primary/secondary status in the address card, resulting in the primary email being ranked higher than the secondary email that matched the query completely.

There are a couple of ways to fix this I can think of: I can either add a special case to compare the email directly with the search query and return a high score if they match, or I can extend this to also consider partial email address search queries that match a substring of the target email address, but I am not sure if this would be a use case that we need to consider.

Comment 22

[Magnus Melin [:mkmelin]]

If that's the case, yeah do a direct compare of the email.

To log messages to console: console.log("foo"), to log to terminal use dump("foo\n");

Comment 23

[Magnus Melin [:mkmelin]]

Ellen, any update?

Comment 24

[Ellen (Yufei) Chen]

(In reply to Magnus Melin [:mkmelin] from comment #23)

Ellen, any update?

So sorry - I lost my MFA device and couldn't access Bugzilla for a while. I have a patch ready to go from a bit ago but I'll need to update it and make sure it still works, and I need some help with getting it tested on the Try servers/reviewed. Can you be my reviewer on Phabricator?

Comment 25

[Magnus Melin [:mkmelin]]

Sure!

Comment 26

[Wayne Mery (:wsmwk)]

Still have the patch?

Comment 27

[Ellen (Yufei) Chen]

Assuming there hasn't been any changes for this file, I have a commit pushed to my personal fork on Github for this.

https://github.com/BlackSpade741/releases-comm-central/commit/9a1b20ee28c9604ab4e93390ea5dfa32b7b8b5d2

Comment 28

[Thomas D. (:thomas8)]

(In reply to Ellen (Yufei) Chen from comment #27)

Hi Ellen, thanks for providing this WIP patch on your Github! Great start! Sorry that we didn't come back to you here. The best way to get the attention of your mentor (between thousands of other bugmail) is to use Request information from: Mentor below the comment input box.

Would you be able to provide another iteration of the patch? (see my tentative review comments below).
Could you attach your new patch on phabricator?

Assuming there hasn't been any changes for this file, I have a commit pushed to my personal fork on Github for this.

https://github.com/BlackSpade741/releases-comm-central/commit/9a1b20ee28c9604ab4e93390ea5dfa32b7b8b5d2

  _getScore(aCard, aAddress, aSearchString) {
    const BEST = 100;

    // First check whether the search string matches the email for the card
    const addressStartIdx = aAddress.indexOf("<") + 1;
    const address = aAddress.substring(addressStartIdx, aAddress.length - 1);
    if (address == aSearchString) {
      return BEST + 1;
    }

That looks promising! I haven't tested the patch, but I think this would fail if there's a plain vanilla email address on the card (without names/display name), as in that case, aAddress probably wouldn't have the angle brackets <...>, so your way of retrieving the plain vanilla email address would fail. Conversely, if the pasted email address did have a display name, your comparison would also fail.

Maybe Magnus (mentor) would know how to parse either of John <[e-mail]> or [e-mail] as [e-mail]. We have some helpers for that.

Comment 30

[Thomas D. (:thomas8)]

This is a pretty serious and cunning privacy violation, also for enterprise. No user will expect that Thunderbird can be bold enough to change a complete pasted plain email address into another address, even if that other address may belong to the same person. E.g., a mixup between work and private address of the recipient may have significant problem potential.

Comment 31

[Thomas D. (:thomas8)]

Comment 6 misunderstood STR comment 0, so this was definitely happening at the time of reporting.
Bug 1725849 looks like a duplicate and seems reported against TB 91, so I'll mark 91 affected.
This won't be easy to reproduce iiuc, as it involves popularityIndex, a hidden property of AB cards. Is there any way of viewing or changing popularityIndex for testing purposes?

Comment 32

[Magnus Melin [:mkmelin]]

Attachment: Bug 1524076 - for a direct search hit on email address, make sure not to autocomplete to the primary address for matching card. r=freaktechnik

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c035e9a54368
for a direct search hit on email address, make sure not to autocomplete to the primary address for matching card. r=freaktechnik

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9255897 [details]
Bug 1524076 - for a direct search hit on email address, make sure not to autocomplete to the primary address for matching card. r=freaktechnik

[Approval Request Comment]
Regression caused by (bug #): not a regression
User impact if declined: autocompletes to unexpected address in some cses
Testing completed (on c-c, etc.): beta for a while
Risk to taking this patch (and alternatives if risky): low risk

Comment 35

[Wayne Mery (:wsmwk)]

Comment on attachment 9255897 [details]
Bug 1524076 - for a direct search hit on email address, make sure not to autocomplete to the primary address for matching card. r=freaktechnik

[Triage Comment]
Approved for esr91

Comment 36

[Rob Lemley [:rjl]]

The calendar import code has changed; the patch does not apply on c-esr91. getItemsFromIcsFile() doesn't exist. Can you make a esr91 version?

Comment 37

[Magnus Melin [:mkmelin]]

Let's not uplift then.

Comment 38

[Magnus Melin [:mkmelin]]

Wrong bug, I see. So let's uplift.

Comment 39

[Rob Lemley [:rjl]]

Thunderbird 91.7.0:
https://hg.mozilla.org/releases/comm-esr91/rev/a5ce61323b37