Closed Bug 1289939 Opened 4 years ago Closed 4 years ago

display name truncated if no blank at end


(MailNews Core :: MIME, defect)

Not set


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

Thunderbird 50.0
Tracking Status
thunderbird48 --- wontfix
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr38 --- wontfix
thunderbird_esr45 49+ fixed


(Reporter: marcausl, Assigned: jorgk-bmo)



(Keywords: regression)


(1 file, 4 obsolete files)

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>
I can reproduce this. Kent, another JSMime bug?
(NI just to attract attention ;-))
Ever confirmed: true
Flags: needinfo?(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?
Flags: needinfo?(rkent)
(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:

*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.
Attached patch WIP: Debug patch (obsolete) — Splinter Review
Debug shows:
huh<>======= fieldValue
hu <>======= recipient
There's your problem. Now we need to fix it.
Attached patch WIP: Debug patch (v2). (obsolete) — Splinter Review
More debug:
huh<>======= fieldValue
hu <>======= displayAddr
hu <>======= recipient
So clearly, the fault is in makeFromDisplayAddress().

Actually, the fault is here:
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
Attached patch Proposed solution (v1a). (obsolete) — Splinter Review
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:

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
Attachment #8776309 - Flags: review?(acelists)
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?
Because no one should rely on the number of spaces, could be 0 to many, the result is always
aa <> (with one space).

BTW, JSMime parsing removes spaces as well, see attachment 8775721 [details] [diff] [review].
Comment on attachment 8776309 [details] [diff] [review]
Proposed solution (v1a).

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

Attachment #8776309 - Flags: review?(acelists) → review+
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:

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.
Maybe the idea was this:
makeMailboxObject("a", "") returns "a <>".
so if you start with "a   <>" (three spaces), the function will trim one away and call
makeMailboxObject("a  ", ""). The trimmed space gets added and you end up with what you had.
There's also:
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"]] },
Kindly review again. Nothing defined the outcome when multiple/no spaces were present. Let's do it now.
Attachment #8776313 - Flags: review?(acelists)
I forgot to say: To run the test locally:
mozilla/mach xpcshell-test mailnews/mime/test/unit/test_nsIMsgHeaderParser4.js
I forgot to say:
is part of from bug 842632 which landed on TB 29.

That's why it's already broken in TB 31. I'll try TB 24 now.
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 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+
Component: Untriaged → MIME
Flags: in-testsuite+
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Version: 45 Branch → 29
For what it's worth, the two email clients I tried - gmail web and lotus notes, but trim to a single blank.
Landed with additional tests:

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?
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
Attachment #8776309 - Attachment is obsolete: true
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<>
This gets sent:
To: Pop <>
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+
Also baked in TB 49 beta.
Flags: needinfo?(rkent)
Flags: needinfo?(rkent)
Attachment #8776313 - Flags: approval-comm-esr45? → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.