Closed Bug 1130248 Opened 9 years ago Closed 9 years ago

|To: "foo@example.com" <foo@example.com>| becomes |"foo@example.comfoo"@example.com| when I compose mail to it

Categories

(MailNews Core :: MIME, defect)

defect
Not set
normal

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed, thunderbird41 fixed, thunderbird_esr31 unaffected)

VERIFIED FIXED
Thunderbird 41.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
thunderbird41 --- fixed
thunderbird_esr31 --- unaffected

People

(Reporter: masayuki, Assigned: rkent)

References

Details

(Keywords: regression)

Attachments

(8 files)

1. Drag&Drop the attachment to Thunderbird after you save it on your computer.
2. Open the message on Thunderbird
3. Click "To"'s email address under "From".
4. Click "Compose Mail To..."

Actual Result:

The To field in the message compose window becomes |"receiver@example.comreceiver"@example.com|

Expected Result:
It should be "receiver@example.com" <receiver@example.com> or something.

I tested on the latest Daily on Win8.1.
likely fallout from jsmime work, bug 858337 maybe
OS: Windows 8.1 → All
Hardware: x86_64 → All
So the full email address is processed with parseAddressingHeader() at http://mxr.mozilla.org/comm-central/source/mailnews/mime/jsmime/jsmime.js#707 and then the node get a "fullAddress" attribute with the value of the full address, however it has lost the quotes around sender name. So '"receiver@example.com" <receiver@example.com>' becomes 'receiver@example.com <receiver@example.com>'. And this is mangled in the compose window to ""receiver@example.comreceiver"@example.com".

So why are the quotes lost? Is that the intentional behaviour?

I have tested that putting the quotes back into "fullAddress" attribute of the "mail-emailaddress" node fixes the bug and the address is not damaged when composing.
Flags: needinfo?(Pidgeot18)
This affects tb38b2 as well
(In reply to :aceman from comment #4)

> '"receiver@example.com" <receiver@example.com>' becomes
> 'receiver@example.com <receiver@example.com>'. And this is *mangled* in the
> compose window to ""receiver@example.comreceiver"@example.com".
> 
> So why are the quotes lost? Is that the intentional behaviour?

I think this is intentional behaviour. Take a look at this test:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/jsmime/test/test_header.js#267
|"foo bar" <me@foo.invalid>| is parsed to name: |foo bar| and email: |me@foo.invalid|
(both with no quotes).

I don't understand why this is later *mangled* (as you say) in the compose window. It should just send to
receiver@example.com <receiver@example.com>
No quotes anywhere.
See Also: → 1162477
I'm sure Neil can help us here ;-) IMHO it's not a jsmime problem.
Flags: needinfo?(neil)
I cannot reproduce this on 38.0b4 or on a recent trunk. Could somebody (like Jorg) confirm that this is no longer an issue in 30.0b4?
Flags: needinfo?(neil)
Flags: needinfo?(mozilla)
Flags: needinfo?(Pidgeot18)
TB 38.0b4 (not 30.0b4, you gotta love Bugzilla for not being able to edit simple typos):
The error is still there. What are you doing, Kent?
The "Reply All" works, what doesn't work is right clicking on the 
To: receiver@example.com <receiver@example.com>
in the message header in the message pane and selecting the "Compose Message To".

Recent trunk (last refreshed 2015-05-07 10:42:18 PDT):
Still broken, recipient shows as: "receiver@example.comreceiver"@example.com

Let me know if you want a screen shot or further testing. Note: The patch to bug 1069790 is included in my "recent" build, since it landed on trunk on 2015-04-29 14:54:50 PDT.
Flags: needinfo?(mozilla)
Refreshed local build: $ hg pull -u ... (can't do python client.py checkout due to bug 777770)
added 10 changesets with 13 changes to 10 files (+1 heads)

Problem persists in locally compiled version.

I think we should ask for help.
Flags: needinfo?(neil)
Flags: needinfo?(Pidgeot18)
Sorry, my local build had a problem (I've cleaned it out now "hg up --clean -r default") but I still need to get TB to build, which is a pain due to bug 777770).
So perhaps I gave you the wrong answer and you are right that it's fixed.
To be continued in a few hours when I can check it again.
With M-C a rev 617dbce26726 (due to bug 777770) and C-C at rev d3090583a3cb (due to compile problems after landing bug 1163331) I observe the following:
The problem persists, recipient still:
"receiver@example.comreceiver"@example.com
Kent: I think you owe me one for the wild-goose chase you sent me on.

Let's not confuse bug 1162477 and this bug 1130248:
To reproduce bug 1162477 do "Reply" all, for this bug 1130248 do "Compose Message To".
The other way around it all works.
Result is:
"\"Uri Oliva urioliva\"@hotmail.com [Barcelona-Freecycle]Barcelona-Freecycle-noreply"@yahoogroups.com
OK, sorry, I see this now.
The problem here is that parseAddressingHeader in jsmime.js handles this string really badly:

receiver@example.com <receiver@example.com>
The header "receiver@example.com <receiver@example.com>" should parse correctly with name receiver@example.com and address receiver@example.com but does not. This value comes originally from this code ins SendMailToNode in msgHdrViewOverlay.js:

fields.to = addressNode.getAttribute("fullAddress");

Both esr31 and current trunk set "receiver@example.com <receiver@example.com>" to fields.to, so the regression did not happen upstream, but is a failure of the parser to correctly interpret this.
Assignee: nobody → rkent
Status: NEW → ASSIGNED
Attachment #8605070 - Flags: review?(Pidgeot18)
Comment on attachment 8605070 [details] [diff] [review]
Reinterpret address as name when we see a <

Philipp, if Joshua doesn't get to this, could you look at it? Thanks. 

Jorg, can you confirm that this works?
Attachment #8605070 - Flags: review?(philipp)
Attachment #8605070 - Flags: feedback?(mozilla)
Comment on attachment 8605070 [details] [diff] [review]
Reinterpret address as name when we see a <

Greetings from Barcelona Airport ;-)
The patch fixes the bug.
I'll cancel the "need info" requests.
Flags: needinfo?(neil)
Flags: needinfo?(mnyromyr)
Flags: needinfo?(Pidgeot18)
Attachment #8605070 - Flags: feedback?(mozilla) → feedback+
Actually, please try the test case from comment #15. Better, but still not perfect. Result of "Compose Message To" is:
"Uri Oliva urioliva"@hotmail.com [Barcelona-Freecycle] <Barcelona-Freecycle-noreply@yahoogroups.com>

It should really be:
"Uri Oliva urioliva@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>
since this is the From header:

From: "Uri Oliva urioliva@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>
Same problem:
"Bettina Röhricht Bettina_Roehricht"@web.de [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>

Should be:
"Bettina Röhricht Bettina_Roehricht@web.de" [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>

Not sure why the display in the header is:
"Bettina Röhricht" Bettina_Roehricht@web.de [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>.

Where do the quotes come from? There are no quote displayed in the test case from comment #15
Comment on attachment 8605070 [details] [diff] [review]
Reinterpret address as name when we see a <

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

Codewise the patch looks good with the following nits, but it seems there is still some discussion here w.r.t the right fix. r=philipp for the time being, I'm happy to review a new patch if needed.

::: mailnews/mime/jsmime/jsmime.js
@@ +822,5 @@
> +          name = address;
> +        localPart = address = '';
> +      }
> +      else
> +        inAngle = true;

if/else bracketing is a bit strange here, but for all I can see there are brackets for the else part when there are brackets for the if part. Could you add them here, using he } else { formatting.

::: mailnews/mime/jsmime/test/test_header.js
@@ +330,5 @@
>          [{name: "=?UTF-8?Q?Simple?=", email: "a@b.c"}]],
>        ["No email address", [{name: "No email address", email: ""}]],
> +      // Thought we were parsing an address, but it was a name.
> +      ["name@example.com <receiver@example.com>",
> +        [{name: "name@example.com", email: "receiver@example.com"}]],

I didn't check the whole file if we already have one, but maybe there should also be a test case for:

"\"last, first\" <name@example.com>"

as the double quotes seem to also be causing trouble here.
Attachment #8605070 - Flags: review?(philipp)
Attachment #8605070 - Flags: review?(Pidgeot18)
Attachment #8605070 - Flags: review+
Attachment #8605070 - Flags: feedback?(mozilla)
Attachment #8605070 - Flags: feedback-
Comment on attachment 8605070 [details] [diff] [review]
Reinterpret address as name when we see a <

Sorry for resetting that feedback flag, my bug view was outdated.
Attachment #8605070 - Flags: feedback?(mozilla) → feedback-
Attachment #8605188 - Attachment mime type: message/rfc822 → text/plain
Rkent, it seems you found out the same as I did in comment 4:) I just didn't start modifying that mime function before getting confirmation from jcranmer. I think you still should get the patch through him.
Well, perhaps I removed the feedback+ too hastily, or perhaps not, I'm undecided now.

The quotes around Bettina are here in the encoding, sorry my mistake.
From: "=?UTF-8?Q?=22Bettina_R=C3=B6hricht=22?= Bettina_Roehricht@web.de [freecycle-berlin]" <freecycle-berlin-noreply@yahoogroups.de>

So "Compose Message To" should NOT do this:
"Bettina Röhricht Bettina_Roehricht"@web.de [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>
but this:
"Bettina Röhricht" Bettina_Roehricht@web.de [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>

Also, as stated in comment #23, this is not right either:
"Uri Oliva urioliva"@hotmail.com [Barcelona-Freecycle] <Barcelona-Freecycle-noreply@yahoogroups.com>

However, as Kent's test show, his patch fixes the JSMIME problem. So the problem of the spurious quotes comes from elsewhere, and if it's not fixed here, we need another bug for it.

Can you please add a few more tests:
      ["name@huhu.com <receiver@example.com>",
        [{name: "name@huhu.com", email: "receiver@example.com"}]],
      ["\"name@huhu.com\" <receiver@example.com>",
        [{name: "name@huhu.com", email: "receiver@example.com"}]],
      ["\"Chaplin, Charlie\" <receiver@example.com>",
        [{name: "Chaplin, Charlie", email: "receiver@example.com"}]],
      ["\"name@huhu.com and name@haha.com\" <receiver@example.com>",
        [{name: "name@huhu.com and name@haha.com", email: "receiver@example.com"}]],

I am wondering whether the following test should fail or succeed:
      ["name@huhu.com and name@haha.com <receiver@example.com>",
        [{name: "name@huhu.com and name@haha.com", email: "receiver@example.com"}]],
It fails.

Can you also please include this test (from bug 1162477), name already obfuscated
      ["=?iso-8859-1?Q?Klaus_Eisschl=E4ger_=28k=2Eeisschlaeger=40t-onli?=" +
        "=?iso-8859-1?Q?ne=2Ede=29?= <k.eisschlaeger@t-online.de>",
      [{name: "Klaus Eisschläger (k.eisschlaeger@t-online.de)",
        email: "k.eisschlaeger@t-online.de"}]],
Comment on attachment 8605070 [details] [diff] [review]
Reinterpret address as name when we see a <

I'm clearing the feedback since I'm undecided and since feedback- looked funny coming from Philipp who at the same time gave review+.

I'd still like to see whether the test case mentioned in the previous comment should pass or fail.

Also, we need to see where the extra quotes in the wrong places come from.

Anyway, with the patch, it's a whole lot better than it was, so it will be a compromise if we don't come up with anything better.
Attachment #8605070 - Flags: feedback-
Chasing the spurious quotes with some debug:

When I click on the message in the message list I get a few of these prints:
parseAddressingHeader, header |"Uri Oliva urioliva@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>|
This is exactly the From header.

When I right click on the From and select "Compose Message To" I see:
parseAddressingHeader, header |Uri Oliva urioliva@hotmail.com [Barcelona-Freecycle] <Barcelona-Freecycle-noreply@yahoogroups.com>|
SendMailToNode, fields.to |"\"Uri Oliva urioliva\"@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>|
parseAddressingHeader, header |"\"Uri Oliva urioliva\"@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>|
parseAddressingHeader, header |"\"Uri Oliva urioliva\"@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>|
parseAddressingHeader, header |"\"Uri Oliva urioliva\"@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>|
Resulting address line: "Uri Oliva urioliva"@hotmail.com [Barcelona-Freecycle] <Barcelona-Freecycle-noreply@yahoogroups.com>
Resulting address line TB31.6: Uri Oliva urioliva@hotmail.com [Barcelona-Freecycle] <Barcelona-Freecycle-noreply@yahoogroups.com>

So somehow somewhere, two quotes get taken out from the original From header. I think, JSMIME does that in getHeaderTokens. Then they get put back, together with some extra ones.

In the original example the quote juggling happens too, but is less obvious:
parseAddressingHeader, header |receiver@example.com <receiver@example.com>|
SendMailToNode, fields.to |"receiver@example.com" <receiver@example.com>|
parseAddressingHeader, header |"receiver@example.com" <receiver@example.com>|
parseAddressingHeader, header |"receiver@example.com" <receiver@example.com>|
parseAddressingHeader, header |"receiver@example.com" <receiver@example.com>|
Resulting address line: receiver@example.com <receiver@example.com>
Resulting address line TB31.6: receiver@example.com <receiver@example.com>

If you want even more confusing, give Bettina a try, she comes with already embedded quotes and delivers this:
parseAddressingHeader, header |"Bettina Röhricht" Bettina_Roehricht@web.de [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>|
SendMailToNode, fields.to |=?UTF-8?Q?=22Bettina_R=c3=b6hricht_Bettina=5fRoehricht=22@web.de_[freecy?= =?UTF-8?Q?cle-berlin]?= <freecycle-berlin-noreply@yahoogroups.de>|
parseAddressingHeader, header |=?UTF-8?Q?=22Bettina_R=c3=b6hricht_Bettina=5fRoehricht=22@web.de_[freecy?= =?UTF-8?Q?cle-berlin]?= <freecycle-berlin-noreply@yahoogroups.de>|
parseAddressingHeader, header |"\"Bettina Röhricht Bettina_Roehricht\"@web.de [freecycle-berlin]" <freecycle-berlin-noreply@yahoogroups.de>|
parseAddressingHeader, header |"\"Bettina Röhricht Bettina_Roehricht\"@web.de [freecycle-berlin]" <freecycle-berlin-noreply@yahoogroups.de>|
Resulting address line: "Bettina Röhricht Bettina_Roehricht"@web.de [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>
Resulting address line TB31.6: Bettina Röhricht Bettina_Roehricht@web.de [freecycle-berlin] <freecycle-berlin-noreply@yahoogroups.de>

As far as I can see, with Kent's fix JSMIME is behaving as it should, but there are still other parts of the system which have the urgent need to do their own "parsing" and interpreting and adding and subtracting of quotes. Confusing!
Aceman: Where is the "mangling" happening that you talk about in comment #4?
Flags: needinfo?(acelists)
I haven't investigated that. I asked why we remove quotes that are there in the original.

There must be an inverse function called from compose that parses the address (now unquoted) again. It may be the same one rkent is touching.
Flags: needinfo?(acelists)
As mentioned in Comment 4 and others, the quotes are missing from the displayed To line.

The To line comes from the toString() method of an msgIAddressObject, which is clearly documented as:

31  * In general, the attributes of this interface are always meant to be in a form
32  * suitable for display purposes, and not in a form usable for MIME emission. In
33  * particular, ... the name or email parameters are unquoted.

Yet when we generate the fields.to that we send to compose, we just grab that display address which is "not in a form usable for MIME emission" and happily go about our way as though it is.

Yet this is what was done in TB 31 as well, the difference being that the previous MIME processing was able to take the mangled display field and generate email addresses from it.

There's a few possible alternate fixes that follow these directions. 1) add quotes in the displayed address field if appropriate, or 2) try to use address and name fields to generate addresses sent to compose rather than the displayed field. These are alternatives to my previous patches, which have followed the approach 3) try to get jsmime to handle the same non-encoded strings that the previous implementation did.
It turns out that jsmime has a routine that claims to generate proper addresses from display data. Use that in generating the compose .to field.

As usual, I would appreciate feedback.
Attachment #8605693 - Flags: review?(mkmelin+mozilla)
Thanks for the analysis and clarification. I would like to retrace the steps.

(In reply to Kent James (:rkent) from comment #35)
> The To line comes from the toString() method of an msgIAddressObject, which
> is clearly documented as:

I can see the comment in nsIMsgHeaderParser.idl but where is the toString() method implemented?
For newbies like myself it's hard to find things at times.

You said that the "display address" is grabbed to send mail to, however, after
fields.to = addressNode.getAttribute("fullAddress");
fields.to contains these
SendMailToNode, fields.to |"\"Uri Oliva urioliva\"@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>|
SendMailToNode, fields.to |=?UTF-8?Q?=22Bettina_R=c3=b6hricht_Bettina=5fRoehricht=22@web.de_[freecy?= =?UTF-8?Q?cle-berlin]?= <freecycle-berlin-noreply@yahoogroups.de>|
which are not the "displayed addresses" in the UI since they do contain extra quotes and they do contain undecoded RFC2047. Where is this assembled? Maybe there is a MIME-encode step somewhere, so far I've only seen the decode part that has been worked on (like in the patch here).

You explained the stripping of the quotes (not that I've seen the code so far), but where are they added again?

Can you describe the life of the original From header
|"Uri Oliva urioliva@hotmail.com [Barcelona-Freecycle]" <Barcelona-Freecycle-noreply@yahoogroups.com>|
on it's way through the system.

> There's a few possible alternate fixes that follow these directions. 1) add
> quotes in the displayed address field if appropriate, or 2) try to use
> address and name fields to generate addresses sent to compose rather than
> the displayed field. These are alternatives to my previous patches, which
> have followed the approach 3) try to get jsmime to handle the same
> non-encoded strings that the previous implementation did.

The tests show that JSMIME does parse the various inputs correctly into name and address, however, other parts of the system do their own thing. I'd like to see the entire picture, ie. have my questions above answered, before commenting on this further.

Another comment just arrived, I'll post this one anyway.
> As usual, I would appreciate feedback.

That works like a charm, Bettina comes out really nice like that.

Can you please add the tests from comment #30 (if required) and this one to the JSMIME tests:
      ["\"=?UTF-8?Q?=22Claudia_R=C3=B6hschicht=22?= Claudia_Roehschicht@web.de [freecycle-berlin]\" " +
        "<freecycle-berlin-noreply@yahoogroups.de>",
      [{name: "\"Claudia Röhschicht\" Claudia_Roehschicht@web.de [freecycle-berlin]",
        email: "freecycle-berlin-noreply@yahoogroups.de"}]],
(that's Bettina obfuscated).
Just for the record:
The original Bettina test with removed Reply-To: works well when replying. I assume that "Reply" and "Send Message To" are two code paths which should both work, as they do now. Thanks Kent!!
Comment on attachment 8605070 [details] [diff] [review]
Reinterpret address as name when we see a <

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

::: mailnews/mime/jsmime/jsmime.js
@@ +817,5 @@
>        addrlist = [];
>      } else if (token === '<') {
> +      if (inAngle) {
> +        // Interpret the address we were parsing as a name.
> +        if (address.length)

nit: address.length > 0 is the style here (a style I do like in general for numbers)
(In reply to Magnus Melin from comment #40)
> Comment on attachment 8605070 [details] [diff] [review]
> Reinterpret address as name when we see a <
> 
> Review of attachment 8605070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/mime/jsmime/jsmime.js
> @@ +817,5 @@
> >        addrlist = [];
> >      } else if (token === '<') {
> > +      if (inAngle) {
> > +        // Interpret the address we were parsing as a name.
> > +        if (address.length)
> 
> nit: address.length > 0 is the style here (a style I do like in general for
> numbers)

This is not the patch that we are hoping to land, please review the other one.
Comment on attachment 8605693 [details] [diff] [review]
Use headerParser.makeFromDisplayAddress

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

does feel a bit strange to semi-roundtrip it like this, but I have no better suggestsions and it seems to work so r=mkmelin
Attachment #8605693 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Kent James (:rkent) from comment #41)

> This is not the patch that we are hoping to land, please review the other one.

NO!!!

We need both patches. If you take away the first one, then you break the case from bug 1162477 again. You made me make the other one a duplicate, so it MUST be fixed here.

See test cases in comment #30, especially this one:
      ["=?iso-8859-1?Q?Klaus_Eisschl=E4ger_=28k=2Eeisschlaeger=40t-onli?=" +
        "=?iso-8859-1?Q?ne=2Ede=29?= <k.eisschlaeger@t-online.de>",
      [{name: "Klaus Eisschläger (k.eisschlaeger@t-online.de)",
        email: "k.eisschlaeger@t-online.de"}]],

I propose that you merge the two patches, fix the nits, add the test cases and be done with it.
[my testing was done with both patches applied]
Sure, because without the first part, the Klaus Eisschläger ain't working.
He has no quotes and the (xx@yy.com) is encoded in RFC2047.
What's confusing me is that none of this is working in my TB 38 builds. I think I may be missing a patch that c-c has.
OK everything looks good after I updated all of my directories. I'm going to take Jorg's patch.
Comment on attachment 8606447 [details] [diff] [review]
[checked-in] My proposal: Both patches combined, added tests

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

Carrying over reviews mkmelin, fallen
Attachment #8606447 - Attachment description: My proposal: Both patches combined, added tests → [checked-in] My proposal: Both patches combined, added tests
Comment on attachment 8606447 [details] [diff] [review]
[checked-in] My proposal: Both patches combined, added tests

[Triage Comment]

[Triage Comment]

http://hg.mozilla.org/releases/comm-aurora/rev/68d22375ec67
TB-39: http://hg.mozilla.org/releases/comm-beta/rev/49f543a16f82
TB-38: http://hg.mozilla.org/releases/comm-beta/rev/26334e467817
Attachment #8606447 - Flags: approval-comm-beta+
Attachment #8606447 - Flags: approval-comm-aurora+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
Blocks: 1165612
Thanks, I confirmed this is fixed in the latest Daily.

-> v.
Status: RESOLVED → VERIFIED
Depends on: 1180360
You need to log in before you can comment on or make changes to this bug.