163783 - sending message adds line break at its end

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Shotaro Kamio]

In sending message, line break is added at it's end.

To confirm this is to capture sent data from mozilla via SMTP.
Another way is sending to IMAP mailbox and saving it as a file.
(not POP. see Bug 163433)

In my diagnosis, nsMsgAsyncWriteProtocol::PostDataFinished() is bad.

1043 nsresult nsMsgAsyncWriteProtocol::PostDataFinished()
1044 {
1045   SendData(nsnull, CRLF "." CRLF);

if sent data is ended by CRLF, it adds one more CRLF.
We need to check if data is ended by CRLF or not.

Comment 1

[Shotaro Kamio]

hmm, RFC821 defines the end of mail data as "<CRLF>.<CRLF>".
Does it means that nsMsgAsyncWriteProtocol::PostDataFinished() is ok and 
we must remove last CRLF from data to send?

Comment 2

[http://tinymailto.com/oliversl]

I think the problem is in the DOM and the new line is inserted when you send the
email.

Comment 3

[Mike Cowperthwaite]

As with bug 163433, I believe this bug is invalid, for the reason stated in 
comment 1.

I think the first CRLF in that SendData() call is there because the text coming 
out of the Composer window does not necessarily end in a newline.

Comment 4

[Karsten Düsterloh]

skamio: yes, the "<CRLF>.<CRLF>" at the end is correct.

The problem is that Mozilla adds a line break at the end of the *data* every
time the data is edited! 

Mike: I strongly disagree with "minor" - let's keep it at "normal".

Comment 5

[Christian Eyrich]

Citing RFC 2821, chapter 4.1.1.4 DATA:

"Note that the first <CRLF> of this terminating sequence is also the <CRLF> that
ends the final line of the data (message text) or, if there was no data, ends
the DATA command itself.  An extra <CRLF> MUST NOT be added, as that would cause
an empty line to be added to the message."

I changed the function call to
SendData(nsnull, "." CRLF);
and did a little testing. I couldn't generate any situation in which the message
bodys last line didn't end with a <crlf>. So the first <crlf> in the function
call is unnecessary resp. wrong.

And if there would be a situation where we have no <crlf> at the end of a line,
that would be another bug.

Comment 6

[Christian Eyrich]

Attachment: patch removing the first <crlf>

I really encourage to apply this patch to become RFC compliant.

Comment 7

[Scott MacGregor]

Comment on attachment 131343 [details] [diff] [review]
patch removing the first <crlf>

a simple but nevertheless scary change. I'm hoping you'll volunteer to monitor
incoming bug complaints if this breaks something :)

Comment 8

[Christian Eyrich]

Thanks for rv. And yes, I monitor MailNews bugs.
I used this patch for some time without problems but there might be servers with
relevant bugs - let's knock on wood.

Assigning this bug to me for complaints.

Comment 9

[Scott MacGregor]

checked into the trunk for Christian.

Comment 10

[Kai Engert (:KaiE:)]

For tracking purposes, this caused regression bug 224138.

Comment 11

[Kai Engert (:KaiE:)]

So we have learned, there ARE places in the code where adding a newline is
necessary. S/Mime if the first place we found, and there might be additional
situations where it is necessary.

In my humble opinion, I think the checked in patch should be backed out and
replaced with a different logic.

I think the code you are modifying should be extended to automatically detect,
whether the data is currently ending with a newline or not.

If data is not ending with CRLF, add CRLF . CRLF
If data is already ending with CRLF, add . CRLF

Comment 12

[Christian Eyrich]

I don't agree. A flexible PostDataFinished() would work, but it would only be a
workaround.

Sending the period on a single line is a transportation task. And only adding
the period, nothing more. Ending the lines, including the final line, with CRLF
is a task of building the message body.

And of course, there might be additional situations without a CRLF at the bodys
end. But to repeat my comment #5, that would be another bug, to be fixed in
other places.

Comment 13

[Kai Engert (:KaiE:)]

Well, one could argue, since you broke it, it's your responsibility to find all
places that are incompatible with your change. If you are not able to fix it
yourself, delay your patch until all other places have been fixed.
A broken product is worse than an additional blank line in some messages.

Comment 14

[Christian Eyrich]

One could argue, yes.
But I'm sure you know that it's impossible to say that "I've found all
incompatible places". And an existing workaround will make it unlikely to find
such an incompatibility.
We can of course back out my fix until the S/MIME code adds the CRLF - if this
is really that difficult and lengthy.

Comment 15

[Kai Engert (:KaiE:)]

I don't know yet how soon I (or somebody else) will come up with a fix to the
other bug.

I propose to back out this patch, and make the other bug block this one, and
check this patch in again one the other one is fixed.

This way we avoid broken functionality.

Comment 16

[Christian Eyrich]

Ok, the last I want is a broken Mozilla.
Do you have CVS access, or who should back it out?

Comment 17

[Mike Cowperthwaite]

*** Bug 215134 has been marked as a duplicate of this bug. ***

Comment 18

[Mike Cowperthwaite]

*** Bug 211475 has been marked as a duplicate of this bug. ***

Comment 19

[Mike Cowperthwaite]

A couple weeks ago, someone said this patch broke something -- what's going on 
with that?  Kai, Christian asked you to back it out -- have you done so?

Comment 20

[Christian Eyrich]

No, Mike. Soon after making comment 18 I found a patch for the problem. This bug
(bug 224138) is fixed since two weeks.