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

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: 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>,<[e-mail]>,<[e-mail]
l>,<[e-mail]>,<[e-mail]>,<551
[e-mail]>,<[e-mail]>,<552800D7
[e-mail]>,<[e-mail]>,<552CE7A6.2010
[e-mail]> due to length.

Comment 1

[Wayne Mery (:wsmwk)]

Thanks for filing this. If it is not a regression from TB37 please adjust accordingly

Comment 2

[Jorg K (CEST = GMT+2)]

@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.

Comment 3

[Jorg K (CEST = GMT+2)]

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

[Magnus Melin [:mkmelin]]

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

I can confirm, the sent message is totally horked.

Comment 5

[Kent James (:rkent)]

Joshua, this is one of our likely-jsmime blockers. Could you give some comments?

Comment 6

[Joshua Cranmer [:jcranmer]]

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.

Comment 7

[Joshua Cranmer [:jcranmer]]

(Also, for future reference, it helps to upload messages as text/plain--it allows me to look at them within bugzilla.)

Comment 8

[Jorg K (CEST = GMT+2)]

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 <[e-mail]>
To: =?iso-8859-1?B?SvZyZyBLbm9ibG9jaA==?= <[e-mail]>
Subject: RE: Malware infection
Date: Thu, 23 Apr 2015 05:06:59 +0000
Importance: Normal
In-Reply-To: <[e-mail]>
References:
 <[e-mail]>,<[e-mail]>,<[e-mail]>,<[e-mail]>,<[e-mail]>

From: YYY <[e-mail]>
To: Jorg <[e-mail]>
Subject: RE: Update of your laptop - complete
Date: Sun, 26 Apr 2015 10:13:36 +0200
Importance: Normal
In-Reply-To: <[e-mail]>
References:
 <[e-mail]>,<[e-mail]>,<[e-mail]>,<[e-mail]>,<[e-mail]>,<[e-mail]>,<[e-mail]>

Comment 9

[Jorg K (CEST = GMT+2)]

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

[Kent James (:rkent)]

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

References:
 <[e-mail]>,<[e-mail]>,<[e-mail]>

We cannot ship Thunderbird 38 with this serious regression. What do we need to do to fix this?

Comment 11

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

Attachment: 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?

Comment 12

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

Attachment: Fix - v2

Missed the /g on the regex, this patch fixes. Also added more comments to test and code as suggested by rkent.

Comment 13

[Kent James (:rkent)]

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).

Comment 14

[Jorg K (CEST = GMT+2)]

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

[Kent James (:rkent)]

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

Comment 16

[Joshua Cranmer [:jcranmer]]

Egads! I said replace the commas in nsMsgCompUtils.cpp, not jsmime.js!

Comment 17

[Jorg K (CEST = GMT+2)]

You didn't say it in the bug ;-(
Do you have a line number?

Comment 18

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

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.

Comment 19

[Kent James (:rkent)]

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.

Comment 20

[Joshua Cranmer [:jcranmer]]

(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>

Comment 21

[Jorg K (CEST = GMT+2)]

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.

Comment 22

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

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?

Comment 23

[Kent James (:rkent)]

Fallen, why did you change the status flags from affected to fixes? I backed out the patch.

Comment 24

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

Sorry, seems to be a bug with the new bugzilla UI. Reverting that change (hopefully).

Comment 25

[Jorg K (CEST = GMT+2)]

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).

Comment 26

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

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.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 28

[Jorg K (CEST = GMT+2)]

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??

Comment 29

[Jorg K (CEST = GMT+2)]

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.

Comment 30

[Jorg K (CEST = GMT+2)]

(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.

Comment 31

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

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

[Kent James (:rkent)]

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

Comment 33

[Jorg K (CEST = GMT+2)]

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.

Comment 34

[Kent James (:rkent)]

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.

Comment 35

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

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.

Comment 36

[Magnus Melin [:mkmelin]]

Relanded as https://hg.mozilla.org/comm-central/rev/aa1c5c5e9bcb -> FIXED

Comment 37

[Kent James (:rkent)]

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