Closed Bug 1154521 Opened 9 years ago Closed 9 years ago

jsmime fails on long references header and e-mail gets sent and stored in Sent without headers

Categories

(MailNews Core :: MIME, defect)

x86_64
Windows 7
defect
Not set
major

Tracking

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

RESOLVED FIXED
Thunderbird 41.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
thunderbird41 --- fixed
thunderbird_esr31 --- unaffected

People

(Reporter: jorgk-bmo, Assigned: Fallen)

References

Details

(Keywords: regression, Whiteboard: [regression:TB38?])

Attachments

(2 files, 2 obsolete files)

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]
@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
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.
The main part of jsmime was landed for thunderbird32, bug 858337.

I can confirm, the sent message is totally horked.
Whiteboard: [regression:TB38] → [regression:TB38?]
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.)
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>
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 ;-)
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)
Attached patch Fix - v1 (obsolete) — Splinter Review
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)
Attached patch Fix - v2Splinter Review
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 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+
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Egads! I said replace the commas in nsMsgCompUtils.cpp, not jsmime.js!
You didn't say it in the bug ;-(
Do you have a line number?
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 → ---
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.
(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>
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.
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?
Fallen, why did you change the status flags from affected to fixes? I backed out the patch.
Sorry, seems to be a bug with the new bugzilla UI. Reverting that change (hopefully).
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).
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.
Attached patch This patch does NOT work ... (obsolete) — Splinter Review
... 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.
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??
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.
(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.
I added In-Reply-To since rfc 822 says this field is also *(phrase / msg-id), so it can have multiple values too.
So a week has gone by since the backout, no progress. Is there any hope of getting the fix in compose to work?
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
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.
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
Relanded as https://hg.mozilla.org/comm-central/rev/aa1c5c5e9bcb -> FIXED
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 40.0 → Thunderbird 41.0
Blocks: 1197686
Flags: needinfo?(Pidgeot18)
You need to log in before you can comment on or make changes to this bug.