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

RESOLVED FIXED in Thunderbird 41.0

Status

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

People

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

Tracking

({regression})

Thunderbird 41.0
x86_64
Windows 7
regression

Thunderbird Tracking Flags

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed, thunderbird41 fixed, thunderbird_esr31 unaffected)

Details

(Whiteboard: [regression:TB38?])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8592531 [details]
Archive containing message that can't be replied to properly

Import the attached message into a folder, then reply to it.
Change the e-mail address so you don't reply to "someone" but to yourself.
Send the message just like that, no need to add any text.
Now check your sent items and received message.

Result: Message sent without any decent headers, no subject, sender, recipient.

Fails in TB 38, 39, 40 (daily).

Daily gives the following debug:
JavaScript error: resource:///modules/jsmime.jsm -> resource:///modules/jsmime/j
smime.js, line 2609: Error: Cannot encode <DUB131-W686B3A035BE37C1F1CE8E4D5000@p
hx.gbl>,<5509425A.3080904@jorgk.com>,<DUB131-W486812B60CDD807960076DD5000@phx.gb
l>,<5509913D.80506@jorgk.com>,<DUB131-W33299DECEBBE42D775DBFDD5F40@phx.gbl>,<551
A73E9.9070704@jorgk.com>,<DUB131-W146D42C9F443413A581917D5FA0@phx.gbl>,<552800D7
.3040906@jorgk.com>,<DUB131-W39EAC5DE45C565C04B5F99D5F90@phx.gbl>,<552CE7A6.2010
101@jorgk.com> due to length.
Thanks for filing this. If it is not a regression from TB37 please adjust accordingly
Component: General → MIME
Keywords: regression
Product: Thunderbird → MailNews Core
Whiteboard: [regression:TB38]
(Reporter)

Comment 2

2 years ago
@Wayne:
I was looking for the correct Product/Component, but didn't see it, sorry.
It's a regression, sure, it works in TB31.x but not TB38.
I'm not fully aware of the history of JSMIME. To my knowledge it was introduced at TB31.x but it has not been as widely used in TB 31.x as it is used in TB38, so despite being present in TB31.x, some problems only show in versions later than 31.x, for example bug 1141446.
I don't know when exactly the regression came in.

I raised the importance to "major" since this bug causes silent data loss and bad messages being sent in some (admittedly) rare cases.
Severity: normal → major
status-thunderbird38: --- → affected
tracking-thunderbird38: --- → ?
(Reporter)

Comment 3

2 years ago
I forgot to mention: The debug comes from a local build.

The error is raised here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/jsmime/jsmime.js#2609
and is due to some length overrun, see here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/jsmime/jsmime.js#2447
It doesn't seem to be an option simply to increase the limits.

The error handling needs to be improved, since the throw is silently ignored and the message gets sent without headers.

It would be good to inform the user of the problem or at least only skip the offending header instead of all headers.

Comment 4

2 years ago
The main part of jsmime was landed for thunderbird32, bug 858337.

I can confirm, the sent message is totally horked.
tracking-thunderbird38: ? → +
Whiteboard: [regression:TB38] → [regression:TB38?]

Comment 5

2 years ago
Joshua, this is one of our likely-jsmime blockers. Could you give some comments?
Flags: needinfo?(Pidgeot18)
WTF? The References header is supposed to be *space*-separated, not comma-separated (cf. RFC 5322, §3.6.4), so whichever client was sending this header originally is broken. Arguably, the best way for me to fix this is to actually implement the References header parsing and emission properly, which is a bit too complex for me to do quickly.
Flags: needinfo?(Pidgeot18)
(Also, for future reference, it helps to upload messages as text/plain--it allows me to look at them within bugzilla.)
(Reporter)

Comment 8

2 years ago
The sending client is Microsoft's web interface of MSN/Hotmail/Outlook.com, so we're likely to see more of these failures. We know that Microsoft have their own standards, but sadly, we can't argue with them :-((

Here are a few more examples:

From: XXXX <someone@hotmail.com>
To: =?iso-8859-1?B?SvZyZyBLbm9ibG9jaA==?= <zzzzz@zzzzz.com>
Subject: RE: Malware infection
Date: Thu, 23 Apr 2015 05:06:59 +0000
Importance: Normal
In-Reply-To: <5537F96C.8040505@jorgk.com>
References:
 <552658E0.9080900@jorgk.com>,<BLU171-W72F1DA25EB02C504B23412C1FB0@phx.gbl>,<55377A1A.4060204@jorgk.com>,<BLU171-W130AAFEC3814B9C2C0067A3C1EE0@phx.gbl>,<5537F96C.8040505@jorgk.com>

From: YYY <someone@msn.com>
To: Jorg <zzzzk@zzzzz.com>
Subject: RE: Update of your laptop - complete
Date: Sun, 26 Apr 2015 10:13:36 +0200
Importance: Normal
In-Reply-To: <553C8515.1040300@jorgk.com>
References:
 <553BAE8D.1090809@jorgk.com>,<SNT151-W3565C32F1B10703F9362B6A2EB0@phx.gbl>,<553BB86B.60206@jorgk.com>,<SNT151-W17C0A6F645EE4022DC8155A2EB0@phx.gbl>,<553BBE01.3020309@jorgk.com>,<SNT151-W92036621E8504D454E8705A2EB0@phx.gbl>,<553C8515.1040300@jorgk.com>
(Reporter)

Comment 9

2 years ago
I assume that anyone, who has a long conversation with someone on MSN/Hotmail/Outlook.com will eventually run into this problem.
So wouldn't it be possible to implement a quick hack that can be removed when the correct solution is implemented? Users don't care what's under the cover, as long as it works ;-)

Comment 10

2 years ago
With a brand-new outlook.com account, sending several emails gives me these references:

References:
 <SNT152-W43964E1772BF4E97419EFACAD70@phx.gbl>,<5541511E.1070706@caspia.com>,<SNT152-W850339738596D7D728BD06CAD70@phx.gbl>

We cannot ship Thunderbird 38 with this serious regression. What do we need to do to fix this?
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 11

2 years ago
Created attachment 8600632 [details] [diff] [review]
Fix - v1

No idea if this is the right way to fix it, but from a brief tests with the steps from comment 0 it seems to work for me. What do you think?
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Flags: needinfo?(Pidgeot18)
Attachment #8600632 - Flags: review?(Pidgeot18)
(Assignee)

Comment 12

2 years ago
Created attachment 8600645 [details] [diff] [review]
Fix - v2

Missed the /g on the regex, this patch fixes. Also added more comments to test and code as suggested by rkent.
Attachment #8600632 - Attachment is obsolete: true
Attachment #8600632 - Flags: review?(Pidgeot18)
Attachment #8600645 - Flags: review?(Pidgeot18)

Comment 13

2 years ago
Comment on attachment 8600645 [details] [diff] [review]
Fix - v2

I'm stealing this review since jcranmer seems to be tied up at the moment.

Given that the ideal fix is "a bit too complex for me to do quickly" and this is blocking us, I think that this workaround is acceptable. I'll also do the checkin (with one minor typo fixed in the comments).
Attachment #8600645 - Flags: review?(Pidgeot18) → review+
(Reporter)

Comment 14

2 years ago
Good man, Kent, let's get the show on the road and let the pragmatists rule over the purists. Users don't disassemble the C++ code or watch the JS in the debugger, they only want to have it working.

Comment 15

2 years ago
Comment on attachment 8600645 [details] [diff] [review]
Fix - v2

https://hg.mozilla.org/releases/comm-aurora/rev/f8df0f149bda
https://hg.mozilla.org/releases/comm-beta/rev/4621293acff7
Attachment #8600645 - Flags: approval-comm-beta+
Attachment #8600645 - Flags: approval-comm-aurora+

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-thunderbird38: affected → fixed
status-thunderbird39: --- → fixed
status-thunderbird40: --- → fixed
status-thunderbird_esr31: --- → unaffected
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Egads! I said replace the commas in nsMsgCompUtils.cpp, not jsmime.js!
(Reporter)

Comment 17

2 years ago
You didn't say it in the bug ;-(
Do you have a line number?
(Assignee)

Comment 18

2 years ago
It seems he did on IRC though, looks like I missed that. I wasn't quite sure I was doing the right thing, hence I would have preferred jcranmer's review. I'll see if I can upload a new patch on the weekend.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 19

2 years ago
OK, backed out:

https://hg.mozilla.org/comm-central/rev/bcc467b4c5a5
https://hg.mozilla.org/releases/comm-aurora/rev/b838face42ae
https://hg.mozilla.org/releases/comm-beta/rev/a52dfd4f91fe

This bug is a blocker to our release, and per the original schedule we were supposed to have done our final beta this week, so it is critical that this get resolved quickly.
status-thunderbird38: fixed → affected
status-thunderbird39: fixed → affected
status-thunderbird40: fixed → affected
(In reply to Jorg K from comment #17)
> You didn't say it in the bug ;-(
> Do you have a line number?

<https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompUtils.cpp#481>
(Reporter)

Comment 21

2 years ago
Tried this, ain't any better ;-(

   nsAutoCString references;
   finalHeaders->GetRawHeader("References", references);
   if (!references.IsEmpty())
   {
+    // Some clients like outlook.com send non-compliant References headers that
+    // separate values using commas.
+    nsAutoCString::iterator start;
+    nsAutoCString::iterator end;
+    references.BeginWriting(start);
+    references.EndWriting(end);
+    while (start != end) {
+      if (*start == ',') {
+        *start = ' ';
+      }
+      ++start;  
+    }
+
     // The References header should be kept under 998 characters: if it's too
     // long, trim out the earliest references to make it smaller.
(Assignee)

Comment 22

2 years ago
Thanks for testing that Jörg, much appreciated! Joshua, any ideas how we can proceed here? The jsmime patch seemed to work for me, maybe its a valid workaround in case you don't have time to fix this for TB38?
status-thunderbird38: affected → fixed
status-thunderbird39: affected → fixed
status-thunderbird40: affected → fixed
Flags: needinfo?(Pidgeot18)

Comment 23

2 years ago
Fallen, why did you change the status flags from affected to fixes? I backed out the patch.
(Assignee)

Comment 24

2 years ago
Sorry, seems to be a bug with the new bugzilla UI. Reverting that change (hopefully).
status-thunderbird38: fixed → affected
status-thunderbird39: fixed → affected
status-thunderbird40: fixed → affected
(Reporter)

Comment 25

2 years ago
I'm a little confused here.
The jsmime change which I didn't test was committed and then later backed out.
What I reported in comment #21 DOES NOT WORK (sorry about the slang "ain't any better").
So we're basically back to square one (back at the beginning).
(Assignee)

Comment 26

2 years ago
Yes, we are. We'd need input from jcranmer if its ok to re-push the jsmime fix at least for beta/aurora or if he is aware of a different fix. We are back to square one.
(Reporter)

Comment 27

2 years ago
Created attachment 8603752 [details] [diff] [review]
This patch does NOT work ...

... but perhaps someone can tell me why. Well, I know why, I saw it in the debugger:

  nsAutoCString references;
  finalHeaders->GetRawHeader("References", references);
  if (!references.IsEmpty())
  {
    // Some clients like outlook.com send non-compliant References headers that
    // separate values using commas.
    bool changed = false;

references must be returned empty, and the code I added is not run.
I don't understand how it can be empty if the References header is a fairly long string as shown in comment #8.
(Reporter)

Comment 28

2 years ago
Hmm, I don't think we're looking at the right spot here. The call stack is this:

xul.dll!mime_generate_headers() Line 482	C++
xul.dll!nsMsgComposeAndSend::GatherMimeAttachments() Line 763	C++
xul.dll!nsMsgAttachmentHandler::UrlExit() Line 1234	C++

Attachments??
(Reporter)

Comment 29

2 years ago
Confirming what we know already: Philipp's patch works. However, I think there is nothing to do for the In-Reply-To header, since it's a single entry.

+function replaceCommasWithSpaces(values) {
+  return values[0].replace(/,/g, " ");
+}
+structuredDecoders.set("References", replaceCommasWithSpaces);
+structuredDecoders.set("In-Reply-To", replaceCommasWithSpaces);  <--- lose this line

In absence of a better fix, I think we have to go with this, with or without the line I'd lose.
(Reporter)

Comment 30

2 years ago
(In reply to Kent James (:rkent) from comment #19)
> OK, backed out:
> 
> https://hg.mozilla.org/comm-central/rev/bcc467b4c5a5
> https://hg.mozilla.org/releases/comm-aurora/rev/b838face42ae
> https://hg.mozilla.org/releases/comm-beta/rev/a52dfd4f91fe

Not sure why this was backed out. It might be suboptimal, but it works. I'd land it again and let Joshua worry about it in due course. I wasn't able to provide a better fix following his hint in comment #20.
(Assignee)

Comment 31

2 years ago
I added In-Reply-To since rfc 822 says this field is also *(phrase / msg-id), so it can have multiple values too.

Comment 32

2 years ago
So a week has gone by since the backout, no progress. Is there any hope of getting the fix in compose to work?
(Reporter)

Comment 33

2 years ago
Comment on attachment 8603752 [details] [diff] [review]
This patch does NOT work ...

(In reply to Kent James (:rkent) from comment #32)
> Is there any hope of getting the fix in compose to work?

YES.

Just land Philipp's fix which you landed before and then backed out. It works.
Land it as it is, forget my criticism from comment #29.

Joshua doesn't like it, so he can fix it later.
I tried following his suggestion from comment #16 and comment #20, but my fix (attachment 8603752 [details] [diff] [review]) DOES NOT work.

So let's be pragmatic here, use what works and worry about being purist later.
We can't delay forever and have no other choice.
Attachment #8603752 - Attachment is obsolete: true

Comment 34

2 years ago
Phillip, you were unhappy with my review and push of you original patch, but you have not come up with an alternative. I really need to know what your plans are here. A fix for this must land in the next few days.
(Assignee)

Comment 35

2 years ago
Jörg has tested the alternative which did not work and I haven't been able to get a hold of jcranmer for a comment. I think we should re-land the bug for now on all branches, we can open a followup bug for jcranmer to do the real fix.
Keywords: checkin-needed

Comment 36

2 years ago
Relanded as https://hg.mozilla.org/comm-central/rev/aa1c5c5e9bcb -> FIXED
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-thunderbird41: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 40.0 → Thunderbird 41.0

Comment 37

2 years ago
Comment on attachment 8600645 [details] [diff] [review]
Fix - v2

http://hg.mozilla.org/releases/comm-aurora/rev/6fef69796656
(TB 39): http://hg.mozilla.org/releases/comm-beta/rev/854d2b310255
(TB 38): http://hg.mozilla.org/releases/comm-beta/rev/433866b2dc10

Updated

2 years ago
status-thunderbird38: affected → fixed
status-thunderbird39: affected → fixed
status-thunderbird40: affected → fixed
Blocks: 1197686
Flags: needinfo?(Pidgeot18)
You need to log in before you can comment on or make changes to this bug.