The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 43.0

Status

MailNews Core
MIME
--
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: wsmwk, Assigned: Jorg K (GMT+1))

Tracking

({regression})

Trunk
Thunderbird 43.0
x86_64
Windows 7
regression

Thunderbird Tracking Flags

(thunderbird40 wontfix, thunderbird41 fixed, thunderbird42 fixed, thunderbird43 fixed, thunderbird_esr3841+ fixed)

Details

(Whiteboard: [regression:TB38?])

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 8651626 [details]
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 <mailman.557.1440119111.18942.support-thunderbird@lists.mozilla.org><mailman.565.1440123689.18942.support-thunderbird@lists.mozilla.org><mailman.567.1440124850.18942.support-thunderbird@lists.mozilla.org><mailman.593.1440128019.18941.support-thunderbird@lists.mozilla.org><mailman.625.1440171783.18941.support-thunderbird@lists.mozilla.org><mailman.607.1440172761.18942.support-thunderbird@lists.mozilla.org><mailman.633.1440176554.18941.support-thunderbird@lists.mozilla.org> 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 support-thunderbird@lists.mozilla.org.  The sender was using 
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0
(Assignee)

Comment 1

2 years ago
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

2 years ago
> 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.  :-)
(Assignee)

Comment 3

2 years ago
(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!
Flags: needinfo?(rkent)
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.
Flags: needinfo?(rkent) → needinfo?(Pidgeot18)
(Assignee)

Comment 5

2 years ago
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.
Flags: needinfo?(rkent)

Comment 6

2 years ago
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

2 years ago
We can at least track this.

Jorg it would be great if you could evaluate the proposed fix and make your recommendations.
status-firefox43: affected → ---
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → +
Flags: needinfo?(rkent)
(Assignee)

Comment 8

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

Updated

2 years ago
Duplicate of this bug: 1188494

Comment 10

2 years ago
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)?
(Assignee)

Comment 11

2 years ago
Created attachment 8657536 [details] [diff] [review]
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.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Updated

2 years ago
Attachment #8657536 - Attachment description: Here it Rob Stradling's fine patch ;-) → Here is Rob Stradling's fine patch ;-)

Updated

2 years ago
Duplicate of this bug: 1196736
(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(' ');
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 14

2 years ago
Created attachment 8658020 [details] [diff] [review]
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) ;-)
Attachment #8657536 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #8658020 - Flags: review?(Pidgeot18)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla

Comment 15

2 years ago
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 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.
Attachment #8658020 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 17

2 years ago
Created attachment 8659740 [details] [diff] [review]
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)
Attachment #8658020 - Attachment is obsolete: true
Attachment #8659740 - Flags: review+
(Assignee)

Comment 18

2 years ago
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
Attachment #8659740 - Flags: approval-comm-esr38?
Attachment #8659740 - Flags: approval-comm-beta?
Attachment #8659740 - Flags: approval-comm-aurora?
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 19

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

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 43.0
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

2 years ago
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
Attachment #8659740 - Flags: approval-comm-esr38?
Attachment #8659740 - Flags: approval-comm-esr38+
Attachment #8659740 - Flags: approval-comm-beta?
Attachment #8659740 - Flags: approval-comm-beta+
Attachment #8659740 - Flags: approval-comm-aurora?
Attachment #8659740 - Flags: approval-comm-aurora+

Updated

2 years ago
status-thunderbird40: --- → wontfix
status-thunderbird41: --- → fixed
status-thunderbird42: --- → fixed
status-thunderbird43: --- → fixed
status-thunderbird_esr38: affected → fixed
tracking-thunderbird_esr38: + → 41+
You need to log in before you can comment on or make changes to this bug.