Closed Bug 1069790 Opened 10 years ago Closed 9 years ago

Email addresses with parenthesis are not pretty-printed anymore

Categories

(MailNews Core :: MIME, defect)

defect
Not set
normal

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

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

People

(Reporter: shill, Assigned: mnyromyr)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 SeaMonkey/2.29
Build ID: 20140904220358

Steps to reproduce:

As far as I understand, there are two accepted formats for "enhanced" email addresses (providing name, along with the address itself)

1) Angle brackets: $NAME <$ADDRESS>
2) Parenthesis: $ADDRESS ($NAME)

Mozilla uses format 1.

In message lists, format 1 is pretty-printed (showing $NAME only, omitting $ADDRESS) while format 2 is printed in full (no pretty-printing)

I am 60% sure that format 2 used to be pretty-printed (showing $NAME only).

Is it possible to pretty-print format 2?
May be vaguely related to Bug 140693
The issue can be seen easily on Usenet, where many posters use format 2.

For example, the most recent thread in comp.unix.programmer

The original poster "gazelle@shell.xmission.X (Kenny McCormack)" appears as "gazelle@shell.xmission.X (Kenny McCormack)" in the From column, while the first respondent "John McCue <jmccue@jmcnet1.bstma.east.verizon.X>" appears only as "John McCue" in the From column.
Can anyone confirm this bug?
(This part is easy; it might even be a dupe, though I didn't find a similar report.)

Does anyone remember if previous versions used to pretty-print format 2?
(I suppose I could install an old version in a VM, if this part is important.)
I'm thinking Seamonkey and Thunderbird share the code for that part?
If that is the case, marking it as a Thunderbird bug would probably attract much more attention than lingering as a Seamonkey bug? Thoughts?
Thunderbird does not seem to exhibit this issue.
Starting in safe mode, with extensions disabled etc, the issue is still present.
(In reply to Mason from comment #3)
> Does anyone remember if previous versions used to pretty-print format 2?
> (I suppose I could install an old version in a VM, if this part is
> important.)

If you suggest a "regression", i.e. a behavior change in the recent version, it would be great help to hear in which version the now missing behavior worked.
(In reply to :Hb from comment #7)

> If you suggest a "regression", i.e. a behavior change in the recent version,
> it would be great help to hear in which version the now missing behavior
> worked.

OK, but could you confirm the issue on your end? (Before I start downloading several releases to bisect the regression.)
So you want to hide the email addresses in the printout of a single email?

Do my knowledge this was never possible in the Mozilla Suite, Thunderbird and Seamonkey.
OK, I bisected the bug (using the comp.lang.c newsgroup)

PRESENT in SM 2.29.1 / NOT PRESENT in SM 2.26

Load Seamonkey 2.29.1
Download the 200 most recent messages
=> observe bug
Quit Seamonkey 2.29.1

Remove the comp.lang.c.msf file

Load Seamonkey 2.26
Download the 200 most recent messages
=> problem does not occur
Quit Seamonkey 2.26


IMPORTANT NOTE : if I start SM 2.29.1 using the msf file created by SM 2.25, then the problem DOES NOT OCCUR.
In other words, the problem occurs when downloading the messages, and putting information in the msf file.

$ diff -u comp.lang.c.msf.SM26 comp.lang.c.msf.SM29
--- comp.lang.c.msf.SM26	2014-10-26 00:15:47.523023406 +0200
+++ comp.lang.c.msf.SM29	2014-10-26 00:16:35.687265751 +0200
@@ -569,7 +569,7 @@
     =MPG.2eb268153390c61998a90a@news.newsguy.com)(1D9=1c7c)(1DA=6f)
   (1DB=5448c8b0)(1DC=m2ahbb\$f3h\$1@dont-email.me)(1DD=1310)(1DE=42)
   (1E0=Fortran Guy Learning C)(1E1=jeffkrob16@gmail.com)(1E2=54490407)
-  (1E3=5273da65-921c-4655-9a94-469cfd49ac19@googlegroups.com)(81=)(1E4=e33)
+  (1E3=5273da65-921c-4655-9a94-469cfd49ac19@googlegroups.com)(82=)(1E4=e33)
   (1E5=54490c46)(1E6=I%72w.629560\$Zu6.535208@fx06.am4)(1E7
     =<5273da65-921c-4655-9a94-469cfd49ac19@googlegroups.com>)(1E8=8d3)
   (1E9=1d)(1EA=54490e70)(1EB=L882w.559117\$9R5.365466@fx29.am4)(1EC
@@ -1634,20 +1634,20 @@
   [1FDA0(^95^31F)]
   [1FDBE(^95^394)]}
 
-<(522=1-74025)(82=Sun Oct 26 00:15:00 2014)(83=d6ce)(3DD=130312-130511)
+<(81=Sun Oct 26 00:16:19 2014)(522=1-74025)(83=d6ce)(3DD=130312-130511)
   (520=c8)(521=1fdcf)(51F=1fdce)>
 {1:^9F {(k^A0:c)(s=9)} 
-  [1(^AC=1)(^AD=1)(^AE^522)(^B6^82)(^AA^83)(^A9^83)(^B7^3DD)(^A1=c8)
+  [1(^AC=1)(^AD=1)(^B6^81)(^AE^522)(^AA^83)(^A9^83)(^B7^3DD)(^A1=c8)
     (^A2=c8)(^A6^521)(^B9^51F)]}
 
-@$${3{@
+@$${2{@
 < <(a=c)> // (f=iso-8859-1)
   (BA=MRUTime)>
-<(523=1414275309)>[-1:^9F(^AC=1)(^AD=1)(^AE^522)(^B6^82)(^AA=0)(^A9=0)
+<(523=1414275382)>[-1:^9F(^AC=1)(^AD=1)(^B6^81)(^AE^522)(^AA=0)(^A9=0)
     (^B7^3DD)(^A1=c8)(^A2=c8)(^A6^521)(^B9^51F)(^BA^523)]
-@$$}3}@
+@$$}2}@
 
-@$${4{@
+@$${3{@
 < <(a=c)> // (f=iso-8859-1)
   (BB=sortColumns)(BC=customSortCol)(BD=keywords)(BE=imageSize)
   (BF=junkscore)(C0=charSet)(C1=sender_name)>
@@ -1661,11 +1661,12 @@
 [1FD18:^80(^C1^525)]
 <(526=0|Nobody)>[1FD23:^80(^C1^526)]
 <(52B=0|jeffkrob16@gmail.com)>[1FD4F:^80(^C1^52B)]
-<(528=0|Stefan Ram)>[1FD55:^80(^C1^528)]
+<(528=0|ram@zedat.fu-berlin.de (Stefan Ram\))>
+[1FD55:^80(^C1^528)]
 <(529=0|jacob navia)>[1FD58:^80(^C1^529)]
 [1FD74:^80(^C1^528)]
 <(52A=0|Rick C. Hodgin)>[1FDA0:^80(^C1^52A)]
-<(527=$121)>[-1:^9F(^AC=1)(^AD=1)(^AE^522)(^B6^82)(^AA=0)(^A9=0)(^B7^3DD)
+<(527=$121)>[-1:^9F(^AC=1)(^AD=1)(^B6^81)(^AE^522)(^AA=0)(^A9=0)(^B7^3DD)
     (^A1=c8)(^A2=c8)(^A6^521)(^B9^51F)(^BA^523)(^B0=12)(^B1=1)(^B3=1)
     (^B2=0)(^BB^527)(^B5=0)(^B4=0)]
-@$$}4}@
+@$$}3}@


The important thing to note is that Stefan Ram prints his address like this:
ram@zedat.fu-berlin.de (Stefan Ram)

For some reason, SM26 translates it to Stefan Ram (which is what is displayed)
whereas SM29.1 leaves it as-is.

Do you have an idea where this is coming from?
(In reply to :Hb from comment #9)

> So you want to hide the email addresses in the printout of a single email?

No, I'm not talking about the email addresses in the printout of a single email.

I'm talking about the information displayed in the "From" column, in the message-list pane, in the mail/news 3-pane window. This information consists of only the Name (when it was provided by the sender) otherwise it's an email address.

Starting with SM29, this field started showing ALSO the email address for people
using the "$ADDRESS ($NAME)" syntax.

Regards.
Any chance that someone with knowledge of this part of the code base could take a look at the issue, to figure out where the regression comes from?
I can confirm this with Linux 2.32 and trunk; and I already wondered what happened when reading usenet. ;-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: SeaMonkey 2.29 Branch → Trunk
What's weird is that this is a regression in Seamonkey, but Thunderbird does not seem affected. So the bug seems to be in Seamonkey code modified during the 2.29 time frame.
This is not actually SeaMonkey-specific but a backend problem.
Component: MailNews: Message Display → MIME
Product: SeaMonkey → MailNews Core
This patch alters jsmime.js in two ways:

1. getHeaderTokens will now only tokenize comment delimiters for comment nesting level 1, everything else will just be regarded as content of that comment.
Since comments have no syntactical meaning other than being a comment, this does no harm and makes token processing easier.

2. parseAddressingHeader now recognizes old addressee naming conventions as specified in RfC 5322 3.4 Note:
  a@b (joe)   →   name: Joe, email: a@b

There are certain edge cases, though, where it's hard to tell if a comment is part of the name, the email, or what else, like:
  joe <a@b> (c)
The patch tries to keep the existing logic in this cases as far possible (or even useful).
Assignee: nobody → mnyromyr
Attachment #8567575 - Flags: review?(Pidgeot18)
Comment on attachment 8567575 [details] [diff] [review]
recognize RfC 5322 3.4 Note adressee names (incl. tests)

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

::: mailnews/mime/jsmime/jsmime.js
@@ +723,5 @@
>    let results = [];
>    // Temporary results
>    let addrlist = [];
>  
> +  // support special email name scheme as per RfC 5322 3.4 Note

I think this line needs a more detailed comment as to its purpose, which ought to at least include the fact that you're trying to support addr-spec (display) as syntax. It also needs to illustrate the role of this variable as containing the latter comment if it exits.

Also, use "RFC 5322 §3.4" as the reference (in particular, use RFC as capitalization, NOT RfC).

@@ +725,5 @@
>    let addrlist = [];
>  
> +  // support special email name scheme as per RfC 5322 3.4 Note
> +  let finalComment = '';
> +  function AddToAddrList(name, address) {

The style here is camelCase, not CamelCase. Please also document what the function does with /** - */ style comments (cf. the decode2047Token inner function somewhere else in this file).

@@ +727,5 @@
> +  // support special email name scheme as per RfC 5322 3.4 Note
> +  let finalComment = '';
> +  function AddToAddrList(name, address) {
> +    if (name === '' && finalComment !== '' && address.endsWith(finalComment)) {
> +      // cut off last comment and take its content as the addressee's name

Nit: capitalization and punctuation.

@@ +730,5 @@
> +    if (name === '' && finalComment !== '' && address.endsWith(finalComment)) {
> +      // cut off last comment and take its content as the addressee's name
> +      address = address.substr(0, address.length - finalComment.length);
> +      let offset = finalComment[0] === ' ' ? 2 : 1;
> +      name = finalComment.substr(offset, finalComment.length - offset - 1);

It would probably behoove you to also have address and name get cleared to '' in this function as well.

@@ +763,5 @@
> +      finalComment = comment;
> +      if (inAngle || address !== '')
> +        address += comment;
> +      else
> +        name += comment;

You really oughtn't append comment text to an address--you want
"John Doe <(legal)but(evil)@example.com>" to translate to {name: "John Doe", email: "but@example.com" (and, actually, while you're mucking around here, please do add that to the test suite of email addresses).

I'd suggest, too, that you only add finalComment if it comes after the address--i.e., !inAngle && address !== ''.

@@ +764,5 @@
> +      if (inAngle || address !== '')
> +        address += comment;
> +      else
> +        name += comment;
> +      // enforce a space behind the comment

Nit: comments should be sentences, with capitalization and punctuation.

::: mailnews/mime/jsmime/test/test_header.js
@@ +328,5 @@
>        ["Name <not an email>", [{name: "Name", email: "not an email"}]],
>        ["=?UTF-8?Q?Simple?= <a@b.c>",
>          [{name: "=?UTF-8?Q?Simple?=", email: "a@b.c"}]],
>        ["No email address", [{name: "No email address", email: ""}]],
> +      // legacy addressee name handling as per RfC 5322 3.4 Note

Nit: capitalization, punctuation. In particular, "RFC 5322 §3.4" would be better.

@@ +329,5 @@
>        ["=?UTF-8?Q?Simple?= <a@b.c>",
>          [{name: "=?UTF-8?Q?Simple?=", email: "a@b.c"}]],
>        ["No email address", [{name: "No email address", email: ""}]],
> +      // legacy addressee name handling as per RfC 5322 3.4 Note
> +      ["name <a@b.c> (co(mm)ent)", [{name: "name", email: "a@b.c(co(mm)ent)"}]],

This kind of test is a clear example of something you DON'T want to do. Email ought to be "a@b.c" in this case--that it's not is a bug in your patch (and possibly pre-existing code).

@@ +333,5 @@
> +      ["name <a@b.c> (co(mm)ent)", [{name: "name", email: "a@b.c(co(mm)ent)"}]],
> +      ["<a@b.c> (co(mm)ent)", [{name: "co(mm)ent", email: "a@b.c"}]],
> +      ["a@b.c (co(mm)ent)", [{name: "co(mm)ent", email: "a@b.c"}]],
> +      ["a@b.c (co(mm)ent)(c2)", // is this legal anyway?
> +        [{name: "c2", email: "a@b.c (co(mm)ent)"}]],

Ditto here.

@@ +334,5 @@
> +      ["<a@b.c> (co(mm)ent)", [{name: "co(mm)ent", email: "a@b.c"}]],
> +      ["a@b.c (co(mm)ent)", [{name: "co(mm)ent", email: "a@b.c"}]],
> +      ["a@b.c (co(mm)ent)(c2)", // is this legal anyway?
> +        [{name: "c2", email: "a@b.c (co(mm)ent)"}]],
> +      ["jl1@b.c (=?ISO-8859-1?Q?Joe_L=F6we?=)", [{name: "=?ISO-8859-1?Q?Joe_L=F6we?=", email: "jl1@b.c"}]],

You'll like to want to add a test or two that the display name is properly decoded in the 2047-supported variant of this method below.

@@ +337,5 @@
> +        [{name: "c2", email: "a@b.c (co(mm)ent)"}]],
> +      ["jl1@b.c (=?ISO-8859-1?Q?Joe_L=F6we?=)", [{name: "=?ISO-8859-1?Q?Joe_L=F6we?=", email: "jl1@b.c"}]],
> +      ['"A,B" <a@b.c>,gg:jl2@b.c (=?ISO-8859-1?Q?Joe_L=F6we?=);',
> +        [{name: "A,B", email: "a@b.c"},
> +         {name: "gg", group: [{name: "=?ISO-8859-1?Q?Joe_L=F6we?=", email: "jl2@b.c"}]}]],

You're missing at least (comment) a@b.c as a test case.
Attachment #8567575 - Flags: review?(Pidgeot18) → feedback+
Hello Karsten,

(In reply to Karsten Düsterloh from comment #15)
> This is not actually SeaMonkey-specific but a backend problem.

Do you mean that Thunderbird is also affected?

Did you pinpoint when/where the regression occurred?
Keywords: regression
(In reply to Mason from comment #18)
> (In reply to Karsten Düsterloh from comment #15)
> > This is not actually SeaMonkey-specific but a backend problem.
> 
> Do you mean that Thunderbird is also affected?

Yes. I can't say which actual release is/was affected, but current trunk is. 
And this is not surprising, since jsmime.js is used by both SM and TB here.
 
> Did you pinpoint when/where the regression occurred?

The introduction of jsmime.js, obviously. ;-)
Changes in v2:
- comments in local-part productions will be dropped and handled as if they did never exist, eg. <(c1)a(c2)@b> → <a@b>
- other comments will be attached to the display-name incl. the brackets, eg. "name" <a@b> (c) → {name: "name (c)", email: "a@b"}
- the last comment after an email address is interpreted as per legacy note in RFC 5322 §3.4, eg. (a)a@b(c)(d) → {name: "d", email: "a@b"}
- fixed capitalization and comments
- better tests
Attachment #8567575 - Attachment is obsolete: true
Attachment #8568322 - Flags: review?(Pidgeot18)
Comment on attachment 8568322 [details] [diff] [review]
recognize RfC 5322 3.4 Note adressee names (incl. tests) , v2

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

This is only a partial review--I need more thought on the needsSpace stuff. I feel like it oughtn't be necessary, but the obvious tweaks didn't get tests passing. It may be that the code would end up clearer if this were more rigorously parsed. In the meanwhile, there is a good list of nits I've found.

::: mailnews/mime/jsmime/jsmime.js
@@ +480,5 @@
> +        tokenIsEnding = true;
> +        isSpecial = true;
> +      } else {
> +        tokenIsStarting = true;
> +      }

You'll want to tweak the wording of the documentation comment for this method with respect to comments.

@@ +729,2 @@
>    // Indicators of current state
> +  let inAngle = false, inComment = false, needsSpace = false, preserveSpace = false;

80-character violation here.

@@ +737,5 @@
> +  // want to recognize it, though.
> +  // Furthermore, despite allowing comments in addresses, RFC 5322 §3.4 notes
> +  // that legacy implementations may interpret the comment, and thus it
> +  // recommends not to use them. (Also, they may be illegal as per RFC 5321.)
> +  // While we do not create adress fields with comments, we recognize such

sp: address

@@ +739,5 @@
> +  // that legacy implementations may interpret the comment, and thus it
> +  // recommends not to use them. (Also, they may be illegal as per RFC 5321.)
> +  // While we do not create adress fields with comments, we recognize such
> +  // comments during parsing and (a) either drop them if inside addr-spec or
> +  // (b) preseerve them as part of the display-name if not.

sp: preserve

@@ +749,5 @@
> +  // local-part of an addr-spec (where we ignore comments) until we find an
> +  // '@' or an '<' token. Thus, we collect both variants until the fog lifts,
> +  // plus the last comment seen.
> +  let lastComment = '';
> +  

EWHITESPACE

@@ +755,5 @@
> +   * Add the parsed mailbox object to the address list.
> +   * If it's in the legacy form above, correct the display-name.
> +   * Also reset any faked flags.
> +   * @param {String} displayName   display-name as per RFC 5322
> +   * @param {sTring} addrSpec      addr-spec as per RFC 5322

Nit: String

@@ +756,5 @@
> +   * If it's in the legacy form above, correct the display-name.
> +   * Also reset any faked flags.
> +   * @param {String} displayName   display-name as per RFC 5322
> +   * @param {sTring} addrSpec      addr-spec as per RFC 5322
> +   */  

ditto

@@ +761,5 @@
> +  function addToAddrList(displayName, addrSpec) {
> +    if (displayName === '' && lastComment !== '') {
> +      // Take last comment content as the display-name.
> +      let offset = lastComment[0] === ' ' ? 2 : 1;
> +      displayName = lastComment.substr(offset, lastComment.length - offset - 1);

Mightn't this work better with lastComment.trim()?

@@ +790,5 @@
> +      lastComment = '';
> +    } else if (token === '(') {
> +      inComment = true;
> +      // The needsSpace flag may not always be set even if it should be,
> +      // eg. for a comment behind an angle-addr.

Nit: e.g.

::: mailnews/mime/jsmime/test/test_header.js
@@ +328,5 @@
>        ["Name <not an email>", [{name: "Name", email: "not an email"}]],
>        ["=?UTF-8?Q?Simple?= <a@b.c>",
>          [{name: "=?UTF-8?Q?Simple?=", email: "a@b.c"}]],
>        ["No email address", [{name: "No email address", email: ""}]],
> +      // handling of comments and legacy display-names as per RFC 5322 §3.4

Nit: Handling

@@ +329,5 @@
>        ["=?UTF-8?Q?Simple?= <a@b.c>",
>          [{name: "=?UTF-8?Q?Simple?=", email: "a@b.c"}]],
>        ["No email address", [{name: "No email address", email: ""}]],
> +      // handling of comments and legacy display-names as per RFC 5322 §3.4
> +      ["(c1)n(c2) <(c3)a(c4)@(c5)b(c6).(c7)d(c8)> (c9(c10)c11)", [{name: "(c1) n (c2) (c9(c10)c11)", email: "a@b.d"}]],

Nit: several violations of the 80-char rule.
Comment on attachment 8568322 [details] [diff] [review]
recognize RfC 5322 3.4 Note adressee names (incl. tests) , v2

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

Many apologies for taking so long to get to this review. The truth of the matter is that I'm personally uncomfortable with having the needsSpace and preserveSpace variables, since it feels like there ought to be some cleaner way to unify them, but the simple modifications failed to achieve this. My only other idea was to move more to a hand-coded LR parser model, which is a more invasive change that I won't burden you with achieving.

Addressing fields REALLY suck. :-(
Attachment #8568322 - Flags: review?(Pidgeot18) → review+
Although comment 5 claims TB is not affected, a TB bugged was duped to this, so I assume TB IS affected, right?
(In reply to Kent James (:rkent) from comment #24)
> Although comment 5 claims TB is not affected, a TB bugged was duped to this,
> so I assume TB IS affected, right?

Right. See comment 19.
What is the holdup in getting this landed?
Probably needs update to fix nits from comment 21.
Status: NEW → ASSIGNED
Flags: needinfo?(mnyromyr)
Attached patch Cleanup with jcranmer comments (obsolete) — Splinter Review
This patch incorporate jcranmer's comments. Since this has a review, it is as far as I am concerned ready to land. If I don't hear any objections, I will land this soon as it is needed for Thunderbird 38, and should be in the next beta.
Oops, I missed one issue (the documentation change). I'll try again.
(In reply to Kent James (:rkent) from comment #26)
> What is the holdup in getting this landed?

Just /me having a new day time job. Please just go ahead and land it.
Flags: needinfo?(mnyromyr)
Comment on attachment 8597536 [details] [diff] [review]
Cleanup with jcranmer comments (including documentation change)

Carrying over jcranmer review. Patch to checking.
Attachment #8597536 - Flags: review+
Keywords: checkin-needed
Attachment #8568322 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Attachment #8597536 - Flags: approval-comm-beta?
Attachment #8597536 - Flags: approval-comm-aurora?
Attachment #8597536 - Flags: approval-comm-beta?
Attachment #8597536 - Flags: approval-comm-beta+
Attachment #8597536 - Flags: approval-comm-aurora?
Attachment #8597536 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.