Closed Bug 1152045 Opened 5 years ago Closed 5 years ago

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

Categories

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

38 Branch
defect
Not set
major

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed

People

(Reporter: peter.reynolds, Assigned: neil)

References

Details

(Keywords: regression, Whiteboard: [regression:TB38])

Attachments

(1 file, 2 obsolete files)

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.
Severity: normal → major
Component: Untriaged → Account Manager
Whiteboard: [dupeme?]
Sounds like bug 1135610
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1135610
Keywords: regression
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.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
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.
Whiteboard: [dupeme?] → [regression:TB38][dupeme?]
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
May you have some special characters in your sending email address?
@ :aceman - no, no special characters!
Duplicate of this bug: 1152762
Have you tried in safe-mode (all extensions disabled)?
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.
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.
(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: <peter@peterreynoldsxxxxx.co.uk>
X-Original-To: mmokrejs@fold.natur.xxxx.cz
Delivered-To: mmokrejs@fold.natur.xxxx.cz
Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.131])
	by fold.natur.xxxx.cz (Postfix) with ESMTP id 59C9A10FC1E1
	for <mmokrejs@fold.natur.xxxx.cz>; 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
 <mmokrejs@fold.natur.xxxx.cz>; Thu, 09 Apr 2015 20:44:47 +0200
To: mmokrejs@fold.natur.xxxx.cz
Reply-To: peter@peterreynoldsxxxxx.com
From: Peter Reynolds
Subject: Thunderbird email problem
message-id: <5526C89D.4090005@peterreynoldsxxxxx.co.uk>
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: peter@peterreynoldsxxxxx.co.uk


Why user-agent: and mime-version: in lowercase, btw?
> 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?
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.
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.
(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...
Summary: Email address missing from "From" field on emails sent through Thunderbird 38 → Email address missing from "From" field on emails sent through Thunderbird 38 (involving length of "From:" field value?)
@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.
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.
Summary: Email address missing from "From" field on emails sent through Thunderbird 38 (involving length of "From:" field value?) → Email address missing from "From" field on emails sent through Thunderbird 38
(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
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).
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?
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?
Blocks: 1135610
Status: UNCONFIRMED → NEW
Component: Account Manager → Message Compose Window
Ever confirmed: true
Flags: needinfo?(neil)
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [regression:TB38][dupeme?] → [regression:TB38]
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.
(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.)
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk 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.
(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.
(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).
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #8591351 - Flags: review?(mkmelin+mozilla)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Summary: Email address missing from "From" field on emails sent through Thunderbird 38 → Email address missing from "From" field on emails sent through Thunderbird 38 if the identityName pref was set
Comment on attachment 8591351 [details] [diff] [review]
Proposed patch

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

Looks good thx! r=mkmelin
Attachment #8591351 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8591351 [details] [diff] [review]
Proposed patch

a=me for SeaMonkey CLOSED TREE
https://hg.mozilla.org/comm-central/rev/a634e04c93db
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
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): ?
Attachment #8591351 - Flags: approval-comm-beta?
Attachment #8591351 - Flags: approval-comm-aurora?
Please set the Target Milestone to when the patch first landed, which in this case is TB 40.
Target Milestone: Thunderbird 38.0 → Thunderbird 40.0
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
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8591351 - Flags: approval-comm-beta+
Attachment #8591351 - Flags: approval-comm-aurora+
Blocks: 318495
Attached patch With test fix (obsolete) — Splinter Review
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).
Attachment #8591351 - Attachment is obsolete: true
Attachment #8593028 - Flags: review?(mkmelin+mozilla)
(In reply to neil@parkwaycc.co.uk 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.
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 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?
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
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.
Attachment #8593930 - Flags: review?(mkmelin+mozilla)
Attachment #8593930 - Flags: feedback?(acelists)
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.
Attachment #8593930 - Flags: feedback?(acelists) → feedback+
(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.)
OK, then I'm happy now.
Status: REOPENED → ASSIGNED
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();
Attachment #8593930 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8593028 - Attachment is obsolete: true
Attachment #8593028 - Flags: review?(mkmelin+mozilla)
Neil please just update the patch and land it. I have a mozmill test for this in bug 318495.
Flags: in-testsuite+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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):
Attachment #8593930 - Flags: approval-comm-beta?
Attachment #8593930 - Flags: approval-comm-aurora?
Comment on attachment 8593930 [details] [diff] [review]
Alternative approach

https://hg.mozilla.org/releases/comm-aurora/rev/7e07f36a4477
Attachment #8593930 - Flags: approval-comm-aurora? → approval-comm-aurora+
Comment on attachment 8593930 [details] [diff] [review]
Alternative approach

https://hg.mozilla.org/releases/comm-beta/rev/ec4c11836db9
Attachment #8593930 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.