Email addresses with parenthesis are not pretty-printed anymore

RESOLVED FIXED in Thunderbird 40.0

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: shill, Assigned: mnyromyr)

Tracking

({regression})

Trunk
Thunderbird 40.0

Thunderbird Tracking Flags

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
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?
Reporter

Comment 1

5 years ago
May be vaguely related to Bug 140693
Reporter

Comment 2

5 years ago
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.
Reporter

Comment 3

5 years ago
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.)
Reporter

Comment 4

5 years ago
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?
Reporter

Comment 5

5 years ago
Thunderbird does not seem to exhibit this issue.
Reporter

Comment 6

5 years ago
Starting in safe mode, with extensions disabled etc, the issue is still present.

Comment 7

5 years ago
(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.
Reporter

Comment 8

5 years ago
(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.)

Comment 9

5 years ago
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.
Reporter

Comment 10

5 years ago
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?
Reporter

Comment 11

5 years ago
(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.
Reporter

Comment 12

5 years ago
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?
Assignee

Comment 13

4 years ago
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
Reporter

Comment 14

4 years ago
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.
Assignee

Comment 15

4 years ago
This is not actually SeaMonkey-specific but a backend problem.
Component: MailNews: Message Display → MIME
Product: SeaMonkey → MailNews Core
Assignee

Comment 16

4 years ago
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+
Reporter

Comment 18

4 years ago
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
Assignee

Comment 19

4 years ago
(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. ;-)
Assignee

Comment 20

4 years ago
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.
Duplicate of this bug: 1072204
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?
Reporter

Comment 25

4 years ago
(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?

Comment 27

4 years ago
Probably needs update to fix nits from comment 21.
Status: NEW → ASSIGNED
Flags: needinfo?(mnyromyr)
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.
Assignee

Comment 31

4 years ago
(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+

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Attachment #8568322 - Attachment is obsolete: true

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0

Updated

4 years ago
Attachment #8597536 - Flags: approval-comm-beta?
Attachment #8597536 - Flags: approval-comm-aurora?

Updated

4 years ago
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.