1152045 - Email address missing from "From" field on emails sent through Thunderbird 38 if the identityName pref was set

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Peter Reynolds]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.60 Safari/537.36 OPR/29.0.1795.30 (Edition beta)

Steps to reproduce:

Sent an email


Actual results:

My email was received without an email address in the "From" box, or with an email address supplied by the server.


Expected results:

The email address listed under "Email Address" in "Account Settings" should appear, as it does when Thunderbird 37 is installed.

Comment 1

[Magnus Melin [:mkmelin]]

Sounds like bug 1135610

Comment 2

[Peter Reynolds]

Having skimmed through bug 1135610 I don't see how this relates to it.  This is something completely new in Thunderbird 38, so much so that when I reinstalled 37 from a download, the problem disappeared, and when I allowed 38 to install again through the Help About pull down the problem came back.  Whilst I have a number of accounts in my Thunderbird I don't have one with the email address blank, and my mails are being sent through the correct servers, just without the sender's (my) email address.

Comment 3

[Peter Reynolds]

I've now checked and Thunderbird 38 downloaded from here: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/releases/38.0b1/win32/en-GB/Thunderbird%20Setup%2038.0b1.exe has the same problem.

Comment 4

[Magnus Melin [:mkmelin]]

How about trunk? http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-central/

Comment 5

[Peter Reynolds]

Magnus

I just used the installer:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-central/thunderbird-40.0a1.en-US.win64.installer.exe
and the issue is the same there.  I will reinstall 37.

Peter

Comment 6

[:aceman]

May you have some special characters in your sending email address?

Comment 7

[Peter Reynolds]

@ :aceman - no, no special characters!

Comment 9

[Magnus Melin [:mkmelin]]

Have you tried in safe-mode (all extensions disabled)?

Comment 10

[Peter Reynolds]

Rather than reinstalling Thunderbird 38 yet again, I have just tried Safe Mode on Thunderbird Daily 40 (which was still on my computer from 2015-04-08) and the result is the same.  Very relieved to see that I am not totally crazy and that the originator of bug 1152762 has the same problem.

Comment 11

[Peter Reynolds]

Incidentally the email address has disappeared by the time the message appears in the Sent folder, not just after it's gone across the internet.

Comment 12

[Martin Mokrejs]

(In reply to Peter Reynolds from comment #11)
> Incidentally the email address has disappeared by the time the message
> appears in the Sent folder, not just after it's gone across the internet.

That's what I reported in bug #1152762. I forgot that I could work around the bug by setting a Reply-To: value, which seems you did (see below). Other than that, also your From: line does not contain your email address.


Return-Path: <[e-mail]>
X-Original-To: [e-mail]
Delivered-To: [e-mail]
Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.131])
	by fold.natur.xxxx.cz (Postfix) with ESMTP id 59C9A10FC1E1
	for <[e-mail]>; Thu,  9 Apr 2015 20:44:48 +0200 (MEST)
Received: from [192.168.1.94] ([86.155.99.177]) by mrelayeu.kundenserver.de
 (mreue003) with ESMTPA (Nemesis) id 0LfWft-1ZDSXG2BZZ-00p8UL for
 <[e-mail]>; Thu, 09 Apr 2015 20:44:47 +0200
To: [e-mail]
Reply-To: [e-mail]
From: Peter Reynolds
Subject: Thunderbird email problem
message-id: <[e-mail]>
Date: Thu, 9 Apr 2015 19:44:45 +0100
user-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101
 Thunderbird/40.0a1
mime-version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: [e-mail]


Why user-agent: and mime-version: in lowercase, btw?

Comment 13

[Magnus Melin [:mkmelin]]

> Why user-agent: and mime-version: in lowercase, btw?

Bug 1138220.

Hmm, still can't reproduce this. Both of you have emails with multi-sub-domains. Could that be it, crazy as it sounds? Have you tried with some other email account?

Comment 14

[Peter Reynolds]

I'm not using subdomains at all on either my .com address or my .co.uk address, but the domain name on the email addresses on which the problem occurs *are* long, the main part of the domain name before the dot being 18 letters long.  I've sent a more detailed email to Magnus showing the details.

Comment 15

[Peter Reynolds]

Appears to be a field length issue on the whole email address, not the domain.  A short yahoo.co.uk address works, a long one doesn't.

Comment 16

[Thomas D. (:thomas8)]

(In reply to Peter Reynolds from comment #15)
> Appears to be a field length issue on the whole email address, not the
> domain.  A short yahoo.co.uk address works, a long one doesn't.

Peter, can you narrow down from which length exactly they start to fail? That might provide some clues...

Comment 17

[Peter Reynolds]

@Thomas D. Maybe this is not the issue as I appear to have a length of 28 that did not go through, but one of 30 that did.  Both on the basis of the Sent folder, so not anything that could have been inserted by the SMTP server.

Comment 18

[Peter Reynolds]

I have a new theory which seems to hold good:

The ones that don't get their email address propagated in "From" are ones that have an entry for identityName in prefs.js, whether this entry has the same value as that for fullName or not.

Some of my accounts have no entry for identityName at all in prefs.js and these work correctly.

Comment 19

[Joe Sabash [:JoeS1]]

(In reply to Peter Reynolds from comment #18)
> I have a new theory which seems to hold good:
> 
> The ones that don't get their email address propagated in "From" are ones
> that have an entry for identityName in prefs.js, whether this entry has the
> same value as that for fullName or not.
> 
> Some of my accounts have no entry for identityName at all in prefs.js and
> these work correctly.

Peter
are you seeing that exact preference name of "identityName"
because all of my prefs related to identity are in the form of
mail.identity.idx.fullName
x being the identity number
I see none like identityName

Comment 20

[Peter Reynolds]

Yes, here's an example:
user_pref("mail.identity.id1.fullName", "Peter Reynolds");
user_pref("mail.identity.id1.identityName", "Peter Reynolds");

There's one where the values are different and Thunderbird picked the identityName value when sending mail (and left the email address out, as it did in all the cases where there was an identityName).

Comment 21

[:aceman]

Prefs with .identityName do exist if the account was imported from another client (e.g. Outlook or Eudora). Is that the case for you?

By default there is not .identityName pref, but the attribute can be queried inside TB code. If it is empty, a string of "fullname <email>" is returned. This .identityName is used in the compose code at some places.

Peter, do you say TB used the value of .identityName as the "From" value in the message and that failed?

Comment 22

[:aceman]

I think I see the problem. This was introduced in http://hg.mozilla.org/comm-central/rev/35d9101cb3b1 (bug 1135610). Yes, it uses .identityName as the From field in outgoing messages.
This shouldn't be done as .identityName can by any arbitrary string (even with private info). We even have a bug filed for TB to allow setting the value in the UI.

Neil, can you tweak this to use some proper string for the From field?

Comment 23

[Peter Reynolds]

I converted from Outlook Express 6 to Thunderbird 1.5 on 13 January 2006.  I have never had a problem with outgoing email addresses disappearing until Thunderbird 38.

I am quite happy to go through prefs.js and delete the identityName lines if it is safe to do so, but no doubt there are other folks out there who are going to be very puzzled when this goes live as a release version rather than a Beta, so I think it would be better for me to stick with Thunderbird 37 and you folks fix the issue as there will be many people who at some time in the past have converted from another email program.  Incidentally my very oldest email address is not affected, so it seems strange that it is suggested that this should be an issue related to my conversion from Outlook Express over 9 years ago.

Aceman, Thunderbird 38-40 used the "identityName" value as the "From" name in place of the "fullName" value, and omitted the "From" email address altogether.

Comment 24

[[e-mail]]

(In reply to aceman from comment #22)
> I think I see the problem. This was introduced in
> http://hg.mozilla.org/comm-central/rev/35d9101cb3b1 (bug 1135610). Yes, it
> uses .identityName as the From field in outgoing messages.
> This shouldn't be done as .identityName can by any arbitrary string (even
> with private info). We even have a bug filed for TB to allow setting the
> value in the UI.

Sorry, I didn't realise that you could override the identity name.
When you fetch the identityName and it isn't overridden, it will try to use fullName <email>, which I guess is also wrong in the case of there being no full name, so I suppose I need to make a MIME address manually. (And then file a bug to provide a readonly accessor on the identity, so people don't make this mistake in future.)

Comment 25

[:aceman]

(In reply to [e-mail] from comment #24)
> Sorry, I didn't realise that you could override the identity name.
You can't yet in the UI, but you can change the prefs any time. The bug for exposing it in the UI and allowing it to be used as some label/memo/name for the identity is bug 318495 which I have in progress.

> When you fetch the identityName and it isn't overridden, it will try to use
> fullName <email>, which I guess is also wrong in the case of there being no
> full name, so I suppose I need to make a MIME address manually. (And then
> file a bug to provide a readonly accessor on the identity, so people don't
> make this mistake in future.)
Yes please, that seems right to me.

Comment 26

[:aceman]

(In reply to Peter Reynolds from comment #23)
> I am quite happy to go through prefs.js and delete the identityName lines if
> it is safe to do so, but no doubt there are other folks out there who are
> going to be very puzzled when this goes live as a release version rather
> than a Beta
Yes, it should be safe to delete identityName. It is not critical information and is not used in TB for anything important (we cope with it being empty by default). That is, until this bug :) You can try to do it for one identity and check if it fixes the problem. You can then even put the indetityName back.
Of course we plan to fix this for TB38 as many other converting users may be hit.

Comment 27

[Peter Reynolds]

(In reply to :aceman from comment #26)
Yes, I've deleted the identityName line from prefs.js for an address I don't normally use, and I can confirm it fixes the issue of a missing outgoing email address (tested by sending from Daily 40).

Comment 28

[[e-mail]]

Attachment: Proposed patch

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 8591351 [details] [diff] [review]
Proposed patch

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

Looks good thx! r=mkmelin

Comment 30

[Philip Chee]

Comment on attachment 8591351 [details] [diff] [review]
Proposed patch

a=me for SeaMonkey CLOSED TREE

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/a634e04c93db

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8591351 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): bug 1135610
User impact if declined: Invalid From: header
Testing completed (on c-c, etc.): Manual edit similar to patch
Risk to taking this patch (and alternatives if risky): ?

Comment 33

[Kent James (:rkent)]

Please set the Target Milestone to when the patch first landed, which in this case is TB 40.

Comment 34

[Kent James (:rkent)]

Comment on attachment 8591351 [details] [diff] [review]
Proposed patch

https://hg.mozilla.org/releases/comm-aurora/rev/dbb131256ac1
https://hg.mozilla.org/releases/comm-beta/rev/9d0181083ebb

Comment 35

[Kent James (:rkent)]

Backed out from comm-beta due to a suspected mozmill test failure. Backouts from aurora and central to follow if test failure is confirmed.

https://hg.mozilla.org/releases/comm-beta/rev/7cb2c130de12

Comment 36

[Kent James (:rkent)]

Backed out from comm-aurora https://hg.mozilla.org/releases/comm-aurora/rev/69d873e28d3a

Comment 37

[Kent James (:rkent)]

Backed out: https://hg.mozilla.org/comm-central/rev/2e70c3813774

TEST-START | /builds/slave/test/build/mozmill/composition/test-newmsg-compose-identity.js | setupModule
TEST-PASS | /builds/slave/test/build/mozmill/composition/test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::setupModule
TEST-START | /builds/slave/test/build/mozmill/composition/test-newmsg-compose-identity.js | test_compose_from_composer
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/composition/test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::test_compose_from_composer

Comment 38

[[e-mail]]

Attachment: With test fix

Bah, if only this test had looked for the current identity in the correct way in the first place (my preference would be to call getCurrentIdentityKey but I don't know if that's possible in a mozmill test) then there wouldn't have been anything to fix... (no I'm not fixing it that would probably more than double the size of this patch).

Comment 39

[[e-mail]]

Oh, by the way, the try run passed, as far as I can tell:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-fc21af86f380/try-comm-central-macosx64-debug/try-comm-central_yosemite-debug_test-mozmill-bm107-tests1-macosx-build7.txt.gz

Comment 40

[:aceman]

(In reply to [e-mail] from comment #38)
> Created attachment 8593028 [details] [diff] [review]
> With test fix
> 
> Bah, if only this test had looked for the current identity in the correct
> way in the first place (my preference would be to call getCurrentIdentityKey
> but I don't know if that's possible in a mozmill test) then there wouldn't
> have been anything to fix... (no I'm not fixing it that would probably more
> than double the size of this patch).

I can look at that. However we actually may want to check the generated label there as we touch it in these bugs :) Bug 318495 will also touch it again.

Comment 41

[:aceman]

But the existing tests do not need to check the label, just the key so I can do that. In bug 318495 I'll add tests that check label as it is important for the feature.

Comment 42

[:aceman]

Comment on attachment 8593028 [details] [diff] [review]
With test fix

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3218,5 @@
> +      let address = MailServices.headerParser
> +                                .makeMailboxObject(identity.fullName,
> +                                                   identity.email).toString();
> +      let item = menulist.appendItem(address,
> +                                     address,

Would there be a problem keeping this as menulist.appendItem(identity.identityName, address, ...) ?
In bug 1135610 you seem to use the menulist.value for the From field. (And key is another attribute.) So why do you touch the label? I think the change as is would regress Thunderbird. And I plan to put .identityName back into the label in bug 318495 so it may be unnecessary jumping around.

::: mail/test/mozmill/composition/test-newmsg-compose-identity.js
@@ +45,5 @@
>   * composer window.
>   */
>  function checkCompIdentity(composeWin, expectedFromEmail) {
>    let identityList = composeWin.e("msgIdentity");
> +  assert_equals(identityList.selectedItem.label, expectedFromEmail,

Would this change be needed if you kept using .identityName as I propose?

Comment 43

[Joe Sabash [:JoeS1]]

Magnus Ping for review
Is there any way you can get to this test review within the next few hours
It would be great if we could get this in the next beta

Comment 44

[[e-mail]]

Attachment: Alternative approach

I was worried that this approach would regress my patch for bug 87987 but in fact my testing showed a bug in that patch so I'll need to tweak that anyway.

Comment 45

[:aceman]

Comment on attachment 8593930 [details] [diff] [review]
Alternative approach

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3221,2 @@
>        let item = menulist.appendItem(identity.identityName,
> +                                     address,

Yes, this is what I wanted.

::: suite/mailnews/compose/MsgComposeCommands.js
@@ +2384,5 @@
>      var prevIdentity = gCurrentIdentity;
>  
>      if (identityElement) {
> +        identityElement.value = identityElement.selectedItem.value;
> +

Is this change not needed for the Thunderbird version? It does not contain this at this spot.
Also, will identityElement.selectedItem always be non-null (this property can be null in general, according to XUL menulist docs)? Later code does check if it is non-null.

Comment 46

[[e-mail]]

(In reply to aceman from comment #45)
> > +        identityElement.value = identityElement.selectedItem.value;
> Is this change not needed for the Thunderbird version?
A similar change is needed as part of my patch to bug 87987.

> Also, will identityElement.selectedItem always be non-null
It gets set before the first call to LoadIdentity, and subsequent calls only happen as a result of the selected item changing. (It can only be null if you set it to null in script, give it a value which doesn't have an item, or have no items in the menulist.)

Comment 47

[:aceman]

OK, then I'm happy now.

Comment 48

[Magnus Melin [:mkmelin]]

Comment on attachment 8593930 [details] [diff] [review]
Alternative approach

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3216,5 @@
>      for (let i = 0; i < identities.length; i++) {
>        let identity = identities[i];
> +      let address = MailServices.headerParser
> +                                .makeMailboxObject(identity.fullName,
> +                                                   identity.email).toString();

nit: would imho be prettier to make all this fit on two rows, like

      let address = MailServices.headerParser.makeMailboxObject(
        identity.fullName, identity.email).toString();

Comment 49

[:aceman]

Neil please just update the patch and land it. I have a mozmill test for this in bug 318495.

Comment 50

[[e-mail]]

Pushed comm-central changeset d674ea621dbf.

Comment 51

[[e-mail]]

Comment on attachment 8593930 [details] [diff] [review]
Alternative approach

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 52

[Kent James (:rkent)]

Comment on attachment 8593930 [details] [diff] [review]
Alternative approach

https://hg.mozilla.org/releases/comm-aurora/rev/7e07f36a4477

Comment 53

[Kent James (:rkent)]

Comment on attachment 8593930 [details] [diff] [review]
Alternative approach

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