The default bug view has changed. See this FAQ.

display name truncated if no blank at end

RESOLVED FIXED in Thunderbird 50.0

Status

MailNews Core
MIME
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: Marc Auslander, Assigned: Jorg K (GMT+1))

Tracking

({regression})

Thunderbird 50.0
regression
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr38 wontfix, thunderbird_esr4549+ fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 months ago
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>
(Assignee)

Comment 1

8 months ago
I can reproduce this. Kent, another JSMime bug?
(NI just to attract attention ;-))
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rkent)

Comment 2

8 months ago
(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?
Flags: needinfo?(rkent)
(Assignee)

Comment 3

8 months ago
Created attachment 8775721 [details] [diff] [review]
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.
(Assignee)

Comment 4

8 months ago
Created attachment 8776295 [details] [diff] [review]
WIP: Debug patch

Debug shows:
huh<h@h.com>======= fieldValue
hu <h@h.com>======= recipient
There's your problem. Now we need to fix it.
(Assignee)

Comment 5

8 months ago
Created attachment 8776297 [details] [diff] [review]
WIP: Debug patch (v2).

More debug:
huh<h@h.com>======= fieldValue
hu <h@h.com>======= displayAddr
hu <h@h.com>======= 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.
Attachment #8775721 - Attachment is obsolete: true
Attachment #8776295 - Attachment is obsolete: true
(Assignee)

Comment 6

8 months ago
Created attachment 8776309 [details] [diff] [review]
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?)
Assignee: nobody → mozilla
Attachment #8776297 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8776309 - Flags: review?(acelists)

Comment 7

8 months ago
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?
(Assignee)

Comment 8

8 months ago
Because no one should rely on the number of spaces, could be 0 to many, the result is always
aa <aa@aa.com> (with one space).

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

Comment 9

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

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

OK
Attachment #8776309 - Flags: review?(acelists) → review+
(Assignee)

Comment 10

8 months ago
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.
(Assignee)

Comment 11

8 months ago
Maybe the idea was this:
makeMailboxObject("a", "a@a.com") returns "a <a@a.com>".
so if you start with "a   <a@a.com>" (three spaces), the function will trim one away and call
makeMailboxObject("a  ", "a@a.com"). The trimmed space gets added and you end up with what you had.
(Assignee)

Comment 12

8 months ago
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 <test@foo.invalid>",
      addresses: [["John Doe", "test@foo.invalid"]] },
    { displayString: "Doe, John <test@foo.invalid>",
      addresses: [["Doe, John", "test@foo.invalid"]] },
(Assignee)

Comment 13

8 months ago
Created attachment 8776313 [details] [diff] [review]
Proposed solution (v1a) with additional tests.

Kindly review again. Nothing defined the outcome when multiple/no spaces were present. Let's do it now.
Attachment #8776313 - Flags: review?(acelists)
(Assignee)

Comment 14

8 months ago
I forgot to say: To run the test locally:
mozilla/mach xpcshell-test mailnews/mime/test/unit/test_nsIMsgHeaderParser4.js
(Assignee)

Comment 15

8 months ago
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.
(Assignee)

Comment 16

8 months ago
Worked OK in TB 24. So regression from bug 842632. CC'ing Wayne since he wants to hear about regressions.
Blocks: 842632
Keywords: regression

Comment 17

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

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

Great, thanks.
Attachment #8776313 - Flags: review?(acelists) → review+

Updated

8 months ago
Component: Untriaged → MIME
Flags: in-testsuite+
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Version: 45 Branch → 29
(Reporter)

Comment 18

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

Comment 19

8 months ago
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-thunderbird48: --- → wontfix
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr38: --- → wontfix
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Updated

8 months ago
Attachment #8776309 - Attachment is obsolete: true
(Assignee)

Comment 20

8 months ago
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<pope@vatican.it>
This gets sent:
To: Pop <pope@vatican.it>
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.
Attachment #8776313 - Flags: approval-comm-esr45?
Attachment #8776313 - Flags: approval-comm-aurora+
(Assignee)

Comment 21

8 months ago
Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/bbc8353aec50
status-thunderbird49: affected → fixed
(Assignee)

Comment 22

6 months ago
Also baked in TB 49 beta.
Flags: needinfo?(rkent)

Updated

6 months ago
Flags: needinfo?(rkent)
Attachment #8776313 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

6 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 49+
You need to log in before you can comment on or make changes to this bug.