1197686 - jsmime fails on long references header and e-mail gets sent and stored in Sent without headers. "Error: Cannot encode ... due to length."

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Wayne Mery (:wsmwk)]

Attachment: problem-message-Re Auto check for new messages not working.eml

+++ This bug was initially created as a clone of Bug #1154521 +++

Like Bug #1154521, using the attached message I see 

Error: Error: Cannot encode <[e-mail]><[e-mail]><[e-mail]><[e-mail]><[e-mail]><[e-mail]><[e-mail]> due to length.
Source File: resource:///modules/jsmime.jsm -> resource:///modules/jsmime/jsmime.js
Line: 2740

In Drafts folder the message is saved with no subject. When "sent" the message also has no subject. 

The oroginal message is seen in mozilla.support.thunderbird but but came to the newsgroup via mailman [e-mail].  The sender was using 
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0

Comment 1

[Jorg K (CEST = GMT+2)]

Bug 1154521 was different. In bug 1154521 we had the case of references being created by MSN/Outlook/Hotmail that contained commas as a separator instead of blanks. This was fixed by changing these commas to spaces.

In your attached example, there aren't any separators. The header reads:
References: <11><22><33><44><55><66><77><tab><88>
<tab><99>
Please check the .eml file in an editor to see for yourself.

No separators between the first 7 references, then two tabs!

The obvious fix is changing tabs to spaces.

Comment 2

[Rob Stradling]

> The obvious fix is changing tabs to spaces.

Since I upgraded from TB 31.7.0 to 38.1.0 on 17th July, I've been encountering the "Cannot encode...due to length" error frequently, with each affected message being "sent and stored in Sent without headers".

Every References header I've looked at contains no separators.  Zero commas, zero tabs and zero whitespace.  i.e. "References: <11><22><33><44><55><66><77><88><99>"
AFAICT from RFC2822, this is acceptable, since the header is supposed to contain (emphasis mine) "one or more unique message identifiers, *optionally* separated by CFWS."

So that "obvious fix" won't have any effect for me.  :-(

I just tried manually changing "><" to "> <".  That worked.  :-)

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Rob Stradling from comment #2)
> "one or more unique message identifiers, *optionally* separated by CFWS."
Thanks for pointing this out.

> So that "obvious fix" won't have any effect for me.  :-(
Sorry, I was wrong.

> I just tried manually changing "><" to "> <".  That worked.  :-)
Yes.

Kent, we need to do something about this, an e-mail client which can't send e-mail is no good!

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Not sure if Joshua commented on this outside of the bug, but I think he is the better person to at least give us an analysis on what is going wrong.

Comment 5

[Jorg K (CEST = GMT+2)]

Sure, but given Joshua's current participation, this is a little wishful thinking.

I'd like to make Kent aware. He was prepared to block TB38 on the predecessor bug 1154521. This is equally severe.

Comment 6

[Rob Stradling]

Here's a patch to jsmime.js that works for me.  Not requesting r? deliberately, 'cos it's far too crude to commit.  :-)

@@ -260,7 +260,7 @@
 // separate values using commas. Temporarily replace commas with spaces until
 // full references header parsing is implemted. See bug 1154521.
 function replaceCommasWithSpaces(values) {
-  return values[0].replace(/,/g, " ");
+  return values[0].replace(/,/g, " ").replace(/></g, "> <");
 }
 structuredDecoders.set("References", replaceCommasWithSpaces);
 structuredDecoders.set("In-Reply-To", replaceCommasWithSpaces);

Comment 7

[Kent James (:rkent)]

We can at least track this.

Jorg it would be great if you could evaluate the proposed fix and make your recommendations.

Comment 8

[Jorg K (CEST = GMT+2)]

Hmm, I was thinking about the same fix, changing "<11><22" to "<11> <22>". It's a little crude, but if Joshua doesn't give us a better fix, we might not have an option. He already complained about the fix to bug 1154521, but never revisited it to give us something better.

I'd do this: Wait until just before TB 38.3 and then ship this, if there is not better fix.

Comment 10

[Magnus Melin [:mkmelin]]

Seems like a good fix to me. Can we please get that landed on trunk, with a test (similar to the one in bug 1154521)?

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Here is Rob Stradling's fine patch ;-)

May ask for review whoever thinks that this is a valid solution ;-)
I've just done the leg-work in Rob Stradling's name.

Comment 13

[Joshua Cranmer [:jcranmer]]

(In reply to Rob Stradling from comment #6)
> @@ -260,7 +260,7 @@
>  // separate values using commas. Temporarily replace commas with spaces
> until
>  // full references header parsing is implemted. See bug 1154521.
>  function replaceCommasWithSpaces(values) {
> -  return values[0].replace(/,/g, " ");
> +  return values[0].replace(/,/g, " ").replace(/></g, "> <");

Might you try (still hacky, but ought to work until I actually implement detecting message ids properly):
var msgId = /<[^>]*>/g;
var match, ids = [];
while ((match = msgId.exec(str)) !== null) {
  ids.push(match[0]);
}
return ids.join(' ');

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Patch according to Joshua's instructions

Hi Joshua, thanks for the input. Here you get to review your own code ;-)
I changed msgId.exec(str) to msgId.exec(values) ;-)

Comment 15

[Rob Stradling]

Comment on attachment 8658020 [details] [diff] [review]
Patch according to Joshua's instructions

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

This patch works for me.  Thanks guys.

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8658020 [details] [diff] [review]
Patch according to Joshua's instructions

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

r+, assuming you make the following changes:

::: mailnews/mime/jsmime/jsmime.js
@@ +260,5 @@
> +// separate values using commas. Also, some clients don't separate References
> +// with spaces, since these are optional accordint to RFC2822. So here we
> +// preprocess these headers (see bug 1154521 and bug 1197686).
> +function preprocessMessageIDs(values) {
> +  dump (values + "--val\n");

Remove the dump lines.

@@ +261,5 @@
> +// with spaces, since these are optional accordint to RFC2822. So here we
> +// preprocess these headers (see bug 1154521 and bug 1197686).
> +function preprocessMessageIDs(values) {
> +  dump (values + "--val\n");
> +  var msgId = /<[^>]*>/g;

Replace the var with let, please.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Patch according to Joshua's instructions (review issues fixed)

Carrying forward Joshua's r+. Fixed review issues.
(Sorry about the left-over "dump" statements, very embarrassing)

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8659740 [details] [diff] [review]
Patch according to Joshua's instructions (review issues fixed)

[Approval Request Comment]
Regression caused by (bug #): 858337
User impact if declined: Severe, mail sent without headers.
Testing completed (on c-c, etc.): mozilla/mach xpcshell-test mailnews/mime

Comment 19

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/38e7cc56ac4ff4d9cf38b6d6341112fbf5f768b4
Bug 1197686 - Preprocess References header. r=jcranmer

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 8659740 [details] [diff] [review]
Patch according to Joshua's instructions (review issues fixed)

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

::: mailnews/mime/jsmime/jsmime.js
@@ +257,5 @@
>  structuredEncoders.set("Content-Transfer-Encoding", writeUnstructured);
>  
>  // Some clients like outlook.com send non-compliant References headers that
> +// separate values using commas. Also, some clients don't separate References
> +// with spaces, since these are optional accordint to RFC2822. So here we

Nit/fyi: "according" is typoed here.

Comment 21

[Kent James (:rkent)]

Comment on attachment 8659740 [details] [diff] [review]
Patch according to Joshua's instructions (review issues fixed)

https://hg.mozilla.org/releases/comm-aurora/rev/a02f56d7a8d7
https://hg.mozilla.org/releases/comm-beta/rev/48e8ee8813e3
https://hg.mozilla.org/releases/comm-esr38/rev/fb8c831a4997