1222046 - Forwarding out of filter rules result in SMTP timeout if email doesn't end with <CR><LF>

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Christoph Nickel]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

I created a filter rule, that forward an email to another recipient.
The email body is base64 encoded.



Actual results:

Thunderbird tries to send the email, but runs into a SMTP timeout.

With a sniffer tool, I see that thunderbird tries to forward the email decoded.
To signal the end of the DATA command, thunderbird only gives a ".<CR><LF>", but it should be "<CR><LF>.<CR><LF>". So the mail server gives a timeout, because it is still waiting for "end of data".


Expected results:

Email should be forwarded.

Comment 1

[Wayne Mery (:wsmwk)]

Are you suggesting this is a regession?
If so, can you find the one day regression range?

Comment 2

[Christoph Nickel]

I don't know, if it is a recession.

But I can give you some futher information:

Thunderbird seems to decodes the base64 coded mail for filtering the body.

The problem occurs, when the original email source code, generated by the sender, doesn't end with <CR><LF>. This seems to happen with some automate generated emails like paypal.
The generated email source code will be base 64 encoded and the email will be send to the recipient.

If I forward this email manually, there will be no problem, because the email is base64 encoded.

Now, I try to filter this email. The email body will be decoded and Thunderbird tries to forward the email. Because of the missing <CR><LF> at the end of the decoded email, the SMTP server runs into a timeout.

In my opinion, there are two solutions.

Solution a):
Thunderbird attaches <CR><LF>.<CR><LF> at every email. If the decoded email doesn't end with <CR><LF> the email could be forwarded anyway.

Solution b):
After filtering the body, Thunderbird encodes the email again and forward it to the recipient.

Comment 3

[John Murray]

I confirm the continuing existence of this bug in Thunderbird 52.3.0.

Comment 4

[bgz]

This looks like the root cause of the problem I am having with Thunderbird 52.6.0 (64 bit) on macOS 10.11.6

The emails in question are notifications from 23andme which are base64 encoded.
I can manually forward them without a problem, but a filter rule does not forward them. After a period of time there is a timeout message from the smtp server. No sent message appears in the Sent folder.

A workaround is to change mail.forward_message_mode = 0
(i.e. forward messages as an .eml attachment, not inline)
However, this is not satisfactory for me due to unreliability in being able to read such .eml attachments.

Comment 5

[IV2KBMoFxYIA]

This is still a valid bug as of TB 60.4.0 on Ubuntu 16.04.

Comment 6

[hirameki]

TB 60.6.1(32bit on Windows10) still has this bug. And bgz's workaround [mail.forward_message_mode = 0] still working.

Comment 7

[hirameki]

(In reply to hirameki from comment #6)

TB 60.6.1(32bit on Windows10) still has this bug. And bgz's workaround [mail.forward_message_mode = 0] still working.

sorry. not TB 60.6.1. TB 60.5.1 is correct.

Comment 8

[gene smith]

(In reply to hirameki from comment #7)

(In reply to hirameki from comment #6)

TB 60.6.1(32bit on Windows10) still has this bug. And bgz's workaround [mail.forward_message_mode = 0] still working.

sorry. not TB 60.6.1. TB 60.5.1 is correct.

hirameki, Can you or other reporters on this bug attach a minimal sized email that has this problem. That will make it a lot easier to debug. Or you can just send it to me if you prefer. Thanks.

Comment 9

[gene smith]

(In reply to bgz from comment #4)

The emails in question are notifications from 23andme which are base64
encoded.
I can manually forward them without a problem, but a filter rule does not
forward them. After a period of time there is a timeout message from the
smtp server. No sent message appears in the Sent folder.

Thanks, I received the example .eml files via email. It appears every line ends in LF instead of the rfc2822 standard CRLF. Not sure if base64 encoding should really matter since it's all text, just unreadable. Anyhow, I will try to forward these via a filter and see what happens.

Comment 10

[gene smith]

I haven't been able to duplicate the problem yet. I think the mail maybe has to come from 23&me or paypal or whatever with just /n for end of lines and fetched as-is into a mailbox. When I append the .eml to a mailbox, tb seems to "repair" the file and put /r/n on the lines so when it is forwarded or sent to another account all is OK, even via a filter.

Assuming the problem you see is due to line ending /n instead of /r/n, it may depend on the imap server containing the mailbox (folder) where you are doing the forward from. Can you tell my the type of imap and smtp server you are seeing the problem on? For example, dovecot, gmail, yahoo, or whatever. (You might be able to tell by looking at an IMAP or SMTP log, see below.)

Also, assuming the problem is line endings, tb just sends the email file as-is for the most part to the smtp server. It assumes that the last line of the email will end with /r/n so, after the file is sent to the smtp server, tb just sends a terminating "./r/n". So the effective terminating sequence might be just "\n.\r\n" instead of the correct "\r\n.\r\n".

Again, assuming the problem is the /n, tb can be changed so it sends the full terminating sequence "\r\n.\r\n" and not assume that the last line of the file already ends in \r\n. I didn't see any ill effects when I made this code change when I sent emails. (It may cause an extra blank line at the end of some text only emails, not sure.)

Another thing that might help would be for you to produce and attach at the link above an smtp log that records the activities when you attempt to forward the email and it fails. This may show where in the send sequence the timeout occurs. We are assuming it is due to the ending sequence but maybe not. See https://wiki.mozilla.org/MailNews:Logging. Like this for windows:

set MOZ_LOG=SMTP:5,timestamp
set MOZ_LOG_FILE=%USERPROFILE%\Desktop\smtp.log
"%ProgramFiles(x86)%\Mozilla Thunderbird\thunderbird.exe"

Note: You can record imap by changing SMTP to IMAP above, or you can record both together into the same log like this:
set MOZ_LOG=SMTP:5,timestamp,IMAP:5
Imap log data tends to be a bit more verbose.

Comment 11

[gene smith]

bgz, I got your email with the smtp log. Looks like tb tries to send the terminating period but nothing happens after that. Do you see an error pop up or anything listed in activity manager about send failure?

So if there's a timeout, it is after the period is sent, it seems. So apparently the smtp server is waiting for more data.

I don't see anything in the smtp log about server type. But I am interested in knowing the IMAP server type since the problem might be due to the mailbox the filter is forwarding from, probably not the smtp process itself. You might be able to tell the imap server type by looking at the imap log for responses to imap command ID or possibly CAPABILITY responses (toward the end of the capability list).

There is another recent bug where failure to send the QUIT command after the period was reported. I haven't been able to duplicate that either: Bug 1529009. But no more info received from the reporter.

I will look closer at the file with base64 since you say only it gives you problems.

Comment 12

[bgz]

I get the following pop-up after about 2-3 minutes from running the filter rule:

Send Message Error
Sending of the message failed.
The message could not be sent because the connection to Outgoing server (SMTP) mail.gol.com
timed out. Try again.

I'll email you with some more info from logs but it looks like the IMAP servers are IMAP4rev1

Comment 13

[gene smith]

I'm looking for something more like this; Tb sends the server its version ID and then the server, if it's nice, responds with its (lines may not be contiguous in log):

[26398:Unnamed thread 0x7270fdf7d700]: I/IMAP 0x7270fba8b000:mobile.charter.net:A:SendData: 5 ID ("name" "Thunderbird" "version" "60.4.0")
[26398:Unnamed thread 0x7270fdf7d700]: I/IMAP 0x7270fba8b000:mobile.charter.net:A:CreateNewLineFromSocket: * ID ("name" "Email Mx" "version" "M.9.00.023.01.5 201-2473-194" "vendor" "Openwave Messaging" "support-url" "http://support.openwave.com" "date" "Wed Nov 14 12:28:00 EST 2018")
[26398:Unnamed thread 0x7270fdf7d700]: I/IMAP 0x7270fba8b000:mobile.charter.net:A:CreateNewLineFromSocket: 5 OK ID completed

The IMAP4rev1 is just the imap RFC version that the server conforms to. Almost all say that. The server type also sometimes appears after the capability string is sent, but not always. The ID response is better but not all servers support ID command (not nice).

Comment 14

[bgz]

I sent you the full IMAP log as an email attachment.
I can see Thunderbird announcing it's version number but no version information coming back.

Comment 15

[IV2KBMoFxYIA]

@gene smith were you able to decipher this from @bgz 's log?

Comment 16

[gene smith]

(In reply to bgz from comment #14)

I sent you the full IMAP log as an email attachment.
I can see Thunderbird announcing it's version number but no version information coming back.

@gene smith were you able to decipher this from @bgz 's log?

I can't find the imap log sent via email from reporter bgz. I did find a message dated 2019-03-02 from bgz but it was empty (edit the next day: now see it, must have been a network problem.). Anyhow, not sure how helpful the imap log would be anyhow. bgz, if you still have the imap log you sent, it is probably better to just attach it using the link above on this bugzilla page.

I did work on this some back in late Feb and early March but other thing came up and haven't gotten back to it. I will mark this as "need info" from myself as a reminder. I do have the sample emails from bgz that have the problem.

I suspect that the problem is due to email lines not ending in the required /r/n. However, tb should probably be more tolerant of malformed emails when trying to send/forward them.

Comment 17

[bgz]

FYI I am still here. I believe I had reproduced the issue on Windows 7 with a different mail server. I didn't upload the logs here because I am not very clear what information is buried in them - thinking of privacy/security aspect.
I had put one of the logs on OneDrive and given a link at the bottom of my email of 2-Mar-19 05:34 (GMT)... that link is still active.

Comment 18

[gene smith]

bgz, I now see the 3-2-2019 email today. Also the link to the imap log (labeled smtp.log) works too. But I don't see anything about the actual imap server type or version in the CAPABILITY or ID imap responses. But that's OK.

In the 3-2 email you said you were forwarding a message from Local Folders using a filter rule. If you haven't already send me the .eml that caused the problem, please do. Via the OneDrive link is fine now that I have the link. Thanks.

Comment 19

[bgz]

gene,

In an email of 22-Feb-19 I attached two .eml files. One was from macos and one from Windows 7. At least one of those is believed to cause the problem (at some point I think 23andme changed the formatting of their emails such that I think the problem no longer occurs with more recent emails).

In case you can't find that email I have put the two .eml files in the same folder as the log on OneDrive (I guess you can see the entire "Public Videos" folder). If you can't get them from there then please let me know.

I also put a file "About the eml files.txt" in that folder which contains the text from my email of 22-Feb.

Comment 20

[gene smith]

bgz, Yes, I see the .eml files on the onedrive link. So are you saying that the problem no longer occurs for you since the 23/me has changed their format?

But if still a problem let me ask some more details:

0. Problem only occurs for (old format) 23/me emails?
1. When email originally received via imap, I assume it is received in INBOX. If not, which folder.
2. Is the email moved or copied via filter or manually form inbox to another folder? Imap folder, or Local Folder?
3. When you manually forward the message and it works OK, which folder is it in? Imap or local?
4. When you forward using a rule, which folder is the message in? Again, imap or local folder?
5. Are you forwarding to a remote person or to another account you have setup in tb?
6. Any other details that might be helpful?

Note: If forwarding to a another imap account in tb, it might be better to just MOVE the message using the filter rule. This will avoid using SMTP and will instead will just do an imap APPEND to the destination mailbox and it is less sensitive to malformed emails. Of course, this is not relevant if forwarding to a remote account.

Comment 21

[landsteph]

(In reply to Christoph Nickel from comment #2)

The Bug is confirmed on :

• 60.8.0 (32 bits) on Windows 10 Pro 1903 (64bits)
• 68.0b4 (32 bits) on Windows 10 Pro 1903 (64bits)

If I send this mail as Text (from WebMail or TB), thunderbird will FAIL to forward it.
If I send this mail as HTML (from WebMail or TB), thunderbird will forward it.

I think it's linked with https://bugzilla.mozilla.org/show_bug.cgi?id=161806

Comment 22

[gene smith]

(In reply to landsteph from comment #21)

(In reply to Christoph Nickel from comment #2)

The Bug is confirmed on :

• 60.8.0 (32 bits) on Windows 10 Pro 1903 (64bits)
• 68.0b4 (32 bits) on Windows 10 Pro 1903 (64bits)

If I send this mail as Text (from WebMail or TB), thunderbird will FAIL to forward it.
If I send this mail as HTML (from WebMail or TB), thunderbird will forward it.

Can you attach, provide a link to or maybe send me the .eml file you refer to as "this" mail? Thanks.

I think it's linked with https://bugzilla.mozilla.org/show_bug.cgi?id=161806

Maybe.

Comment 23

[landsteph]

@ gene smith
Wrong syntax, my mistake.
A simple mail with just "Test" as Subject, and "Test" as body will do the job...
The first one as plaintext => FAIL
The second one written in color (html) => OK

the workaround above : "mail.forward_message_mode = 0" is still working, but is not satisfying.

Comment 24

[IV2KBMoFxYIA]

FWIW the linked workaround from the linked bug fixed the issue for me: https://bugzilla.mozilla.org/show_bug.cgi?id=161806#c6

" As a workaround, set "mail.file_attach_binary = true" "

Comment 25

[gene smith]

(In reply to landsteph from comment #23)

@ gene smith
Wrong syntax, my mistake.
A simple mail with just "Test" as Subject, and "Test" as body will do the job...
The first one as plaintext => FAIL

I just forwarded in tb a text-only message and it worked. Are you saying the problem is windows specific?

Comment 26

[gene smith]

(In reply to IV2KBMoFxYIA from comment #24)

FWIW the linked workaround from the linked bug fixed the issue for me: https://bugzilla.mozilla.org/show_bug.cgi?id=161806#c6

" As a workaround, set "mail.file_attach_binary = true" "

I think this will cause the "bad" email to be forwarded as an attachment and not try to send it inline. May not be "satisfying" either.

Comment 27

[IV2KBMoFxYIA]

Once I did the mail.file_attach_binary = true setting I also changed the forwarding to be inline. Now it does the inline forwarding without the error message. For my purposes I have nothing else that this ticket would accomplish for me. Give it a try. It may sufficient for your needs too.

Comment 28

[Philippe Mignard]

I have the same problem with tb 60+. I reverted to tb 52.8.0 and no problem anymore. Maybe this can help...
Regards

Comment 29

[gene smith]

Comment 3 above claims 52.3 has the problem but comment 28 (Phillippe) says it's OK at 52.8. That would mean it was bad at or before 52.3, got fixed between 52.3 and 52.8 and then broke again after 52.8.
Phillippe, do you have a reliable way to duplicate the problem with tb 60+ ?
Thanks.

Comment 30

[Philippe Mignard]

Well, in new 68 version, a filter witch forwards mail always fail to connect to SMTP server. In 60.x, sometimes it fails, sometime not. I tested with 2 smtp servers: my own (running Kerio mail server) and smtp.free.fr. The result is the same: timeout. I've no log to provide, sorry.

I've not tested with 52.3 because 52.8 was the first version I used to do this filters. To downgrade from 68 to 52.8, I had to restore old personnal folder, because datas seems corrupted with a script which hangs 52.8 after 68 version upgrade.

I send the mail inline. If I use the attached format, no problem even in tb 68.

Regards

Comment 31

[Philippe Mignard]

Note: I've forgotten: I use the 32 bits version. Not tested with 64 bits.

Comment 32

[Philippp]

I have this problem, too. At least it looks very much like this problem: Forwarding a message manually is no problem, but having a filter forward it results in a SMTP timeout after some time, and no mail being sent.
This is TB 68.2.2 (64-Bit) on Mac OS X 10.14.6 (18G1012).
The affected messages are reports generated by Backup on a Synology NAS. If I look at the source (cmd-U), it seems like the message does have two empty lines at the end. I attach the message, hth (though editied, so I cannot guarantee the encoding is the same).

Comment 33

[Philippp]

Attachment: Raw text of an affected message (copied into text editor)

Comment 34

[gene smith]

Philippp, looks like the file you attached has line ending only 0x0a or LF instead of 0x0d,0x0a or CRLF as required by SMTP. But this may be the result of editing and saving with a unix style editor.
Was it saved before or after it was forwarded? Can you check the line ending using binary mode editing before editing/saving the file? (But I'm not sure this will help since tb on unix may on save with LF line endings.)
I have an idea for fixing the problem real soon and will provide a "try" build for reporters and commenters on this bug to test, hopefully will provide it tomorrow evening.

Comment 35

[gene smith]

The link to the "try" builds for the various architectures is here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=03725623e619ab2a5c0f9c5586fa31d1f53236ca&selectedJob=278366690
This adds my patch to the latest ESR 68 release.

If the reporter and commenters on this bug could test the "try" build and see if it fixes this bug it would be appreciated. Just click on the green "B" next to the appropriate architecture. When you click the "B" you should then see a selection for "Job Details" in a black bar about half way down the screen. Clicking on "Job Details" you should see a long list of items, but the item you probably want differs by architecture:

"Windows 2012 x64 opt" for 64-bit window:   artifact uploaded: target.installer.exe
"Windows 2012 opt" for 32-bit windows:      artifact uploaded: target.installer.exe
"Linux x64 opt":                            artifact uploaded: target.tar.bz2
"OS X Cross Compiled opt":                  artifact uploaded: target.dmg

The patch attempts to examine the message being sent or forwarded for standard line endings CRLF. If the line ending are not standard (i.e., just LF) the terminating sequence sent is "CRLF.CRLF" instead of the just ".CRLF"
When testing this please use env. var MOZ_LOG=SMTP:5,timestamp. The patch also adds some new logging to record if the line endings detected and the termination sequence sent. It also now show whether "dot stuffing" occurs on the message which may be of relevant to the bug. Please attach the generated log when the timeout error occurs.

I/SMTP Line endings are CRLF, sent ".CRLF" to terminate
I/SMTP Did dot stuffing occurred at least once in sent message? NO

I have not been able to duplicate the problems so far but I think improper termination when the message lines end in just LF is causing the problem. Please let me know if this helps.

Comment 36

[gene smith]

Another thing to check if a message times out while sending is to look in the system temp directory (e.g., /tmp on linux) for a file or files like this:

nsemail.eml
nsemail-1.eml
nsemail-2.eml
etc.

This is the file that is read and sent out via smtp verbatim. If there are line ending problems we might be able to see something by looking at the file and check to see if lines in the file end with /r/n or just /n (0x0d,0x0a or just 0x0a). If the message sends OK, this temp file is deleted, but if there is a problem with the send sometimes it sticks around and can be examined.

This can be checked without using the "try" build.

Also, I may be able to create another "try" build that doesn't delete the nsemail*.eml temp file at all which may be useful for more debugging for user who can duplicate the issue of this bug easily.

Comment 37

[Philippp]

Attachment: temp file as found when filter was stuck

Comment 38

[Philippp]

Thanks gene for the quick reply! I managed to find the temp file on my OSX, and I attach it (replaced some info by xxx using a hex editor, but did not change anything else). Seems to end without a line break.

Comment 39

[gene smith]

Yes, the attached nsemail-5.eml would cause the problem since no CRLF at the end (but has CRLF on other lines). Can you describe the exact filter rule you use to forward the message?

Also, if what you show applies to the other commenters (missing CRLF only at end), my "try" build above won't fix the problem since it assumes that no CRLF line ending is found in the file.

Comment 40

[Philippp]

Looks like my filter converts this into HTML, and thereby forgets to insert the linebreaks. I just saved the original message to eml, and opened it with a text editor - it is simple plaintext, not HTML.
It has 5 empty lines at the end, equivalent to the <br><br><br><br><br> at the end of the tmp file!

My gilter is like:
ALL of:
from = xxxx
subject contains xxx
subject contains xxx
ACTIONS:
forward to: [e-mail]

Comment 41

[Philippp]

Just to rule them out, the problem persists with all my Add-Ons disabled.

The Add-On "Mail Redirect" (which I guess has similar internal functionality) has no problem with these messages.

Comment 42

[gene smith]

Can you send me the original unmodified plaintext message as an email attachment via my bugzilla profile address? If not, can I assume it is similar to your 1st attachment: attachment 9111214 [details]
So the filter is changing the message to html? Do you have compose as html set? Thanks.

Comment 43

[Philippp]

Yes, the message is exactly like attachment 9111214 [details]. I checked with hex editor: All line endings are 0D0A, and the file ends with "0D0A0D0A 0D0A0D0A 0D0A".
Yes looks like it is doing that!

I could not find this setting in the new settings UI, but I am definitely composing in HTML, so probably yes. :-)
Maybe related somehow:
In Compose / Sending Options I have: [x] "If possible, send as plain text"; when sending to people who dont want to receive html: "Send as plaintext and HTML". (all translated from german)

Comment 44

[gene smith]

Ok, I have now duplicated what Philippp sees. I modified his attachment 9111214 [details] with a From field and set up a filter to forward the message. It fails with a timeout and leaves behind the /tmp/nsemail.eml file with no CRLF end of line marker after the html email body.
This happens only when setting account to compose as html. If the account is set to not compose as html, the message is forwarded as is, plain text and works OK with no timeout (and no /tmp/nsemail.eml is left behind) so I assume the message body ends with CRLF.

Not sure if this is similar to what other reporters are seeing, but I suspect it is and I think an easy solution is for tb always send "CRLF.CRLF" as the smtp termination sequence and not just ".CRLF" like it does now in this function: https://searchfox.org/comm-central/rev/7bf30c9070215a6fcb51fe51fc89bfd5e8c7e6fb/mailnews/base/util/nsMsgProtocol.cpp#1216
Edit: Found the root cause of the problem and fixing it there (see comment 49).

Note again that the "try" build linked above in comment 35 won't fix the problem if what Philippp observed applies to the other commenters.

Comment 45

[gene smith]

If you are forwarding a message inline automatically via a filter, it seem you would want to not change the format even if you normally compose messages as html. So is converting plaintext to html when forwarding via filter also a (new) bug? Also, if you compose in text you might want html messages auto-forwarded via filter go out as html, but I assume the html is converted to text.

Anyhow, was hoping detect the missing last CRLF at the protocol file stream level rather than trying to find exactly where the missing CRLF is occurring with the various application scenarios described in this bug. That hasn't worked so I'm now looking that the applications (filtering, forwarding, converting to html, base64 encoding, etc).

If I can't find the problem at the application, I may still try to fix at an even lower protocol level by inserting a usually redundant CRLF at the end of each sent message rather than assuming the application always ends the message with CRLF. https://searchfox.org/comm-central/rev/7bf30c9070215a6fcb51fe51fc89bfd5e8c7e6fb/mailnews/base/util/nsMsgProtocol.cpp#1216
Edit: Found the root cause of the problem and fixing it there (see comment 49).

Comment 46

[Philippp]

Yes, I think as a user I would assume that a filter is forwarding the message as-is without any changes (but other opinions might vary).
My guts feeling says (I do a little bit of programming) your findings looks like the implementation uses the same mechanism as when manually forwarding a message, just skipping the UI (message editor) part. So maybe the HTML editor UI usually inserts the CRLF at the end? But sure know better than me :-) Thanks for looking into it!

Comment 47

[Tarcisio Scotta]

I was having the same problem forwarding with filters. I switched to "don't use HTML formatting" and it worked.
Thanks for the info

Comment 48

[gene smith]

Bug 437494 provides some documentation on how the the smtp protocol elements and the full message in /tmp/nsemail.eml are sequenced and sent, mostly pertaining to code here: https://searchfox.org/comm-central/rev/07984c67130608158f9b801527f1667978c3d4ef/mailnews/base/util/nsMsgProtocol.cpp#997

Comment 49

[gene smith]

Attachment: 1222046-review-changes.patch

This fixes the problem of attempting to forward plaintext messages via filters that gets converted to html because the user has selected the "compose as html" setting.
I'm not sure I am using the optimal or preferred string functions in this patch, but it does seem to work. If the message body doesn't end in CRLF, a CRLF is added.

Comment 50

[gene smith]

Comment on attachment 9114411 [details] [diff] [review]
1222046-review-changes.patch

Missing closing bracket, ignore this patch.

Comment 51

[gene smith]

Attachment: 1222046-review-changes(v1).patch

This should be better.
This fixes the problem of attempting to forward plaintext messages via filters that gets converted to html because the user has selected the "compose as html" setting.
I'm not sure I am using the optimal or preferred string functions in this patch, but it does seem to work. If the message body doesn't end in <CRLF>, a <CRLF> is added ahead of the terminating <.CRLF>.

Comment 52

[:aceman]

Comment on attachment 9114415 [details] [diff] [review]
1222046-review-changes(v1).patch

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +1250,5 @@
> +    if (m_composeHTML && !msgBody.IsEmpty()) {
> +      uint32_t bodyLen = msgBody.Length();
> +      const char16_t* body = static_cast<const char16_t*>(msgBody.get());
> +      if (bodyLen < 2 || body[bodyLen-2] != '\r' || body[bodyLen-1] != '\n')
> +        msgBody.InsertLiteral(u"\r\n", bodyLen);

Can you try: if (!StringEndsWith(msgBody, "\r\n")) { msgBody.AppendLiteral("\r\n") } ?

Comment 53

[gene smith]

Attachment: 1222046-review-changes(v2).patch

Thanks, that way is much better! Works if I also add NS_LITERAL_STRING.

Comment 54

[Magnus Melin [:mkmelin]]

Comment on attachment 9114442 [details] [diff] [review]
1222046-review-changes(v2).patch

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +1246,5 @@
>      rv = m_editor->OutputToString(contentType, flags, msgBody);
>      NS_ENSURE_SUCCESS(rv, rv);
>    } else {
>      m_compFields->GetBody(msgBody);
> +    if (m_composeHTML && !msgBody.IsEmpty()) {

Do we need these check at all, and both of them? From the comments sounds like that was the reproducible case, but would it hurt to do the check for all cases? Also, please add a code comment about why the \r\n is added

Comment 55

[gene smith]

Attachment: 1222046-review-changes(v3).patch

Do we need these check at all, and both of them? From the comments sounds like that was the reproducible case, but would it hurt to do the check for all cases? Also, please add a code comment about why the \r\n is added

Moved the check for missing ending CRLF on message body down some so it occurs for all cases. Works with all variations. Also, added a comment.

For the record, a missing final CRLF caused timeouts only when the message body was the last message content sent via SMTP. If attachments follow the body they always have terminating CRLF. Also, a missing CRLF on the message body only occurs when forwarding the message as HTML via a filter (no editor involved). Forwarding manually as HTML results in no missing CRLF. Forwarding as text, manually or via filter, also results in a terminating CRLF on the body.

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 9114997 [details] [diff] [review]
1222046-review-changes(v3).patch

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

Looks good, thx Gene! r=mkmelin

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/680552a461aa
Fix SMTP server responding with timeout due to missing CRLF at end of forwarded HTML message. r=mkmelin

Comment 58

[Magnus Melin [:mkmelin]]

Had to back this out due to test failures in comm/mailnews/compose/test/unit/test_longLines.js
See https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281694235&repo=comm-central&lineNumber=2279

Comment 59

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bc449b862ba6
Backed out changeset 680552a461aa for test failures in comm/mailnews/compose/test/unit/test_longLines.js. DONTBUILD

Comment 60

[gene smith]

I'll look into it ASAP.

Comment 61

[gene smith]

Attachment: 1222046-feedback-changes(v0).patch

Sorry for the delay in getting back to this. I was unable to find a fix for the unit test failures so I am proposing a different approach. Instead of trying to determine where the missing final <CRLF> occurs while composing the message, I now just check the final line of the message before it is sent to the output socket and add a <CRLF> if one is missing. It require a "peek" into each line before outputting it and uses the mozilla function ReadSegments() from https://searchfox.org/comm-central/rev/f0eb632f3e70c9c7c30624d49e0d8922f5b4a562/mozilla/xpcom/io/nsIInputStream.idl#132 to access the internal buffer.

This also allows the actual message being sent to be printed with SMTP:5 logging which is not currently possible since the buffer containing the message lines is not accessible. I had previously proposed doing this in bug 1602281.

(The attached diff still contains some printf since at this point I am just asking for feedback.)

Comment 62

[Magnus Melin [:mkmelin]]

Comment on attachment 9119989 [details] [diff] [review]
1222046-feedback-changes(v0).patch

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

I don't think I like this approach. It would be hard to test too. Basically what's in the Outbox should be what's sent and it looks like this would change that story.

Comment 63

[gene smith]

This only adds a <CRLF> at the end right before sending if the composing code somehow doesn't include it, which it does not under some condition. Anyhow, before Christmas I looked and looked and couldn't figure out why that test failed with my simple fix so maybe someone else who better understands that code should look at this.
Also, I tried not to do anything that would affect the "dot stuffing" things in the code which makes the code in this area complex. As far as testing goes, I don't think there is any unit test for this now that causes the dot stuffing to occur.

Comment 64

[Magnus Melin [:mkmelin]]

If you change the message even so slightly, it could cause problems with things like crypto signature.

Comment 65

[gene smith]

Good point. The code also can add a dot (period) to the raw message. I guess that gets filtered out and not seen at the receiving end.

Comment 66

[gene smith]

Attachment: 1222046-feedback-changes(v1).patch

Here's another try at fixing this. The affected code occurs while writing the temp file containing the message to be sent. There are already attempts in this code to ensure a CRLF ending but sometimes, obviously, it doesn't work. So as each line is written to the file, I check the ending for CRLF and if it is not found on the last write, I force a write of CRLF. However, when the line is written encrypted, I assume the CRLF is present so this should preclude a problem.

This fixes the bug as reported and also passes the tests under mailnews/compose/test/unit/ including the long lines test that failed before. (FWIW, the v0 diff also passes the tests.)

Comment 67

[Jorg K (CEST = GMT+2)]

Attachment: Simple solution with one test fixed

I'm still wondering whether the very first solution wouldn't be best. Of course tests need to be adjusted. With these additional changes, test_longLines.js passes on Windows. Since different platform have different line endings (with Mac mostly following Linux), we need to check on Linux first and then send to try to avoid surprises. Also, I'm wondering whether nsMsgCompose::SendMsg() we should add a LF or a CRLF. In the DOM linebreaks are done with LF only, and as far as I can see, the message is set from a DOM string. It would be worth checking at the call site whether there are CRLF or only LF in the string.

Also, I'm sure Gene checked that adding the CRLF in nsMsgCompose::SendMsg() fixes the forwarding bug. Looks like nsMsgCompose::SendMsg() is the central sending function (I should know really, but I haven't touched that stuff in a while).

Comment 68

[Jorg K (CEST = GMT+2)]

P.S.: Of course we need to check whether the test changes are legitimate. For example, I added a space just to force the test to pass :-(

Comment 69

[gene smith]

A few weeks ago I tried similar changes to the long lines test and could only get the first sub-test that compares the strings to pass. This was on linux. I'll try your changes since maybe you did it different than I did (pretty sure I didn't try adding spaces since I didn't see errors about them).

Would a "try" build run the tests on all platforms? Maybe I'll try that if it works for me here.

Comment 70

[Jorg K (CEST = GMT+2)]

Yes, you can run a try on all platforms. I messed up a bit. If you look a few lines up, you'll see that we serialise with CRLF, so you're code was right to start with. Let me fix this in 10 minutes.

Comment 71

[Jorg K (CEST = GMT+2)]

Attachment: Simple solution with one test fixed (JK, v2)

OK, this fixes the test on Windows with the original CRLF.

mach xpcshell-test comm/mailnews/db/gloda/test/unit/test_smime_mimemsg_representation.js also passes which bumped out here:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281694235&repo=comm-central&lineNumber=2279
or maybe that failure had another reason at the time.

You should try both tests on Linux, so
mach xpcshell-test comm/mailnews/db/gloda/test/unit/test_smime_mimemsg_representation.js
mach xpcshell-test comm/mailnews/compose/test/unit/test_longLines.js

and then we can check Mac on try. We'd still need to dig around a bit to see why I had to add a space as well.

Comment 72

[Jorg K (CEST = GMT+2)]

Check the interdiff to see what I've done with your original patch:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9114997&action=interdiff&newid=9120278&headers=1

Comment 73

[Jorg K (CEST = GMT+2)]

Comment on attachment 9120278 [details] [diff] [review]
Simple solution with one test fixed (JK, v2)

I think I understand the extra space. You add CRLF to the HTML part of a message and then run it to the plaintext serialiser, since the two cases needing the extra space a the ones where the message is sent as plaintext (`fields.forcePlainText = true;`). It's possible that the serialiser adds another space since we're doing format flowed.

So Magnus and I are happy with adding the CRLF to the body, and I'm happy with the test changes. If you can give your f+ if it works on Linux, we can close this issue.

Comment 74

[gene smith]

Attachment: longlines-passes-linux.diff

Your change to the longlines test attached in comment 71 passes on linux. The changes in your interdiff don't. I changed it to pass again on linux. Don't know if this will pass on win and osx.

Comment 75

[Jorg K (CEST = GMT+2)]

Attachment: Simple solution with one test fixed (JK+GS v3)

OK, how about this then.

Comment 76

[gene smith]

Comment on attachment 9120282 [details] [diff] [review]
Simple solution with one test fixed (JK+GS v3)

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

This passes on linux!

Comment 77

[Jorg K (CEST = GMT+2)]

Attachment: Simple solution with one test fixed (JK+GS v3b)

OK, thanks. Linting didn't like on of the lines, so I fixed it here.

Comment 78

[Jorg K (CEST = GMT+2)]

Do you want Magnus to take a look at the alternative solution, or should I land what we have now?

Comment 79

[gene smith]

No need for him to look at them. This is better and gets right to the problem. The others add runtime overhead (not much) and involve static/global vars which worried me since I'm not sure if more than one email can be in the process of being sent simultaneously.
Jorg, Thanks for your help on this. It was beginning to get me down a bit...

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/52d792dac3b9
Fix SMTP server responding with timeout due to missing CRLF at end of forwarded HTML message. r=mkmelin,jorgk

Comment 81

[Jorg K (CEST = GMT+2)]

Comment on attachment 9120216 [details] [diff] [review]
1222046-feedback-changes(v1).patch

OK then, this is also a little complicated for adding a missing CRLF.

Comment 82

[Jorg K (CEST = GMT+2)]

Damn, that still failed. And I also noticed that I fixed the tests inconsistently. Let me see what I can do.

Comment 83

[Jorg K (CEST = GMT+2)]

Attachment: follow-up.patch

I remember having lots of "fun" with this test when I first wrote it. This passes on Windows, let's see the other platforms:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ce4521d07282039ebdba526d24daa1b566c7fe94

Comment 84

[Jorg K (CEST = GMT+2)]

Attachment: follow-up.patch (v2)

That follow-up wasn't right. The CRLF needs to be added to the HTML regardless of the platform. Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=62797e87504907f81d18c13ea5f3c4456135ec34

Comment 85

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56f3244b4b9d
Follow-up: fix test properly this time. r=me

Comment 86

[Intermittent Failures Robot]

6 failures in 4472 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 6

Platform breakdown:

• linux64: 2
• linux32: 2
• macosx1014-64: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1222046&startday=2020-01-06&endday=2020-01-12&tree=all

Comment 87

[Jorg K (CEST = GMT+2)]

Comment on attachment 9120295 [details] [diff] [review]
follow-up.patch (v2)

Might as well get this on the road.

Comment 88

[Geoff Lankow (:darktrojan)]

73 beta 1:
https://hg.mozilla.org/releases/comm-beta/rev/1dcefcd94db905a5374efb3b7851fe3623436be4
https://hg.mozilla.org/releases/comm-beta/rev/8efbc97ee1181794e7292ff7b7702698c306af50

Comment 89

[Geoff Lankow (:darktrojan)]

68.4.2:
https://hg.mozilla.org/releases/comm-esr68/rev/004b0245854e02c50671e653acdf4f47a43fd636
https://hg.mozilla.org/releases/comm-esr68/rev/e5fbfe36443f9e995feac1db9819f08f1ebc8438