1289939 - display name truncated if no blank at end

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Marc Auslander]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160726073904

Steps to reproduce:

send mail to an address of the form:

display name<emailname@emaildomain>


Actual results:

the to fields was truncated to:

display nam<emailname@emaildomain>


Expected results:

all other mailers insert a blank to yield:


display name <emailname@emaildomain>

Comment 1

[Jorg K (CEST = GMT+2)]

I can reproduce this. Kent, another JSMime bug?
(NI just to attract attention ;-))

Comment 2

[Kent James (:rkent)]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #1)
> I can reproduce this. Kent, another JSMime bug?
> (NI just to attract attention ;-))

Done Germany have the fairy tale of the little boy who cried wolf?

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Tests showing that it's NOT a JSMime problem

(In reply to Kent James (:rkent) from comment #2)
> Done Germany have the fairy tale of the little boy who cried wolf?
No, it's not a common story in Germany:
https://en.wikipedia.org/wiki/The_Boy_Who_Cried_Wolf

*Sorry*, it's not a JSMime problem as the enclosed test shows which passes without problem. The problem is already present pre-JSMime in TB 31. Sorry again.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Debug patch

Debug shows:
huh<[e-mail]>======= fieldValue
hu <[e-mail]>======= recipient
There's your problem. Now we need to fix it.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Debug patch (v2).

More debug:
huh<[e-mail]>======= fieldValue
hu <[e-mail]>======= displayAddr
hu <[e-mail]>======= recipient
So clearly, the fault is in makeFromDisplayAddress().

Actually, the fault is here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeJSComponents.js#379
This code assumes that it can cut away a space between the display name and the <xx>, so if there is no space, it will chop the last character of the display name instead. Jolly good.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a).

Removed the ridiculous blind cutting off of one character and replaced with proper trim. Hopefully nothing in the system relied on this nonsense.

Just to be sure, try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6cf810604c0f

A very quick review would be appreciated since Monday is branch day ;-)
Patch will be landed without the debug, I left it in for the reviewer to try. (How's that for service?)

Comment 7

[:aceman]

Comment on attachment 8776309 [details] [diff] [review]
Proposed solution (v1a).

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

::: mailnews/mime/src/mimeJSComponents.js
@@ +376,2 @@
>        return this.makeMailboxObject(
> +        lbracket == 0 ? '' : aDisplayName.slice(0, lbracket).trim(),

So why is it now OK to remove all the spaces instead of one?

Comment 8

[Jorg K (CEST = GMT+2)]

Because no one should rely on the number of spaces, could be 0 to many, the result is always
aa <[e-mail]> (with one space).

BTW, JSMime parsing removes spaces as well, see attachment 8775721 [details] [diff] [review].

Comment 9

[:aceman]

Comment on attachment 8776309 [details] [diff] [review]
Proposed solution (v1a).

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

OK

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks, look, I'll wait for the try, I don't want to break anything ... further than it's already broken.

If it's any consolation, here some history:
https://hg.mozilla.org/comm-central/diff/456daa4ede27/mail/components/compose/content/addressingWidgetOverlay.js#l1.12

https://hg.mozilla.org/comm-central/diff/a8d1e10d39b7/mailnews/mime/src/mimeJSComponents.js#l1.199

So Joshua coded that. But it will be very hard to get an answer from him.

If you feel saver, we can only remove the last space, but not remove anything if there is no space.

Comment 11

[Jorg K (CEST = GMT+2)]

Maybe the idea was this:
makeMailboxObject("a", "[e-mail]") returns "a <[e-mail]>".
so if you start with "a   <[e-mail]>" (three spaces), the function will trim one away and call
makeMailboxObject("a  ", "[e-mail]"). The trimmed space gets added and you end up with what you had.

Comment 12

[Jorg K (CEST = GMT+2)]

There's also:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/test/unit/test_nsIMsgHeaderParser4.js#
but the test is very very simple, not to say deficient:
    { displayString: "John Doe <[e-mail]>",
      addresses: [["John Doe", "[e-mail]"]] },
    { displayString: "Doe, John <[e-mail]>",
      addresses: [["Doe, John", "[e-mail]"]] },

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a) with additional tests.

Kindly review again. Nothing defined the outcome when multiple/no spaces were present. Let's do it now.

Comment 14

[Jorg K (CEST = GMT+2)]

I forgot to say: To run the test locally:
mozilla/mach xpcshell-test mailnews/mime/test/unit/test_nsIMsgHeaderParser4.js

Comment 15

[Jorg K (CEST = GMT+2)]

I forgot to say:
https://hg.mozilla.org/comm-central/diff/a8d1e10d39b7/mailnews/mime/src/mimeJSComponents.js#l1.199
is part of https://hg.mozilla.org/comm-central/rev/456daa4ede27 from bug 842632 which landed on TB 29.

That's why it's already broken in TB 31. I'll try TB 24 now.

Comment 16

[Jorg K (CEST = GMT+2)]

Worked OK in TB 24. So regression from bug 842632. CC'ing Wayne since he wants to hear about regressions.

Comment 17

[:aceman]

Comment on attachment 8776313 [details] [diff] [review]
Proposed solution (v1a) with additional tests.

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

Great, thanks.

Comment 18

[Marc Auslander]

For what it's worth, the two email clients I tried - gmail web and lotus notes, but trim to a single blank.

Comment 19

[Jorg K (CEST = GMT+2)]

Landed with additional tests:
https://hg.mozilla.org/comm-central/rev/a760ec4fc9fd

Kent, should you pass by:
Re. your comment #2: Not strictly a JSMime bug (as of regression of bug 858337) but a surely a regression of the JSMime preparation work (bug 842632). So did I cry "wolf" too early?

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8776313 [details] [diff] [review]
Proposed solution (v1a) with additional tests.

[Approval Request Comment]
Regression caused by (bug #): Bug 842632
User impact if declined: Some e-mail addresses entered get silently corrupted (dataloss), not real good in a corporate environment.
You enter:
To: Pope<[e-mail]>
This gets sent:
To: Pop <[e-mail]>
Testing completed (on c-c, etc.): Manual and test suite.
Risk to taking this patch (and alternatives if risky):
Low, simple change for rare use case.

Comment 21

[Jorg K (CEST = GMT+2)]

Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/bbc8353aec50

Comment 22

[Jorg K (CEST = GMT+2)]

Also baked in TB 49 beta.