Closed Bug 163783 Opened 23 years ago Closed 22 years ago

sending message adds line break at its end

Categories

(MailNews Core :: Networking: SMTP, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: skamio, Assigned: ch.ey)

References

Details

Attachments

(1 file)

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.
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?
I think the problem is in the DOM and the new line is inserted when you send the email.
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.
Severity: normal → minor
Summary: sending message adds line break at it's end → sending message adds line break at its end
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".
Severity: minor → normal
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.
I really encourage to apply this patch to become RFC compliant.
Attachment #131343 - Flags: review?(scott)
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 :)
Attachment #131343 - Flags: review?(scott) → review+
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.
Assignee: mscott → ch.ey
checked into the trunk for Christian.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
For tracking purposes, this caused regression bug 224138.
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
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.
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.
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.
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.
Ok, the last I want is a broken Mozilla. Do you have CVS access, or who should back it out?
*** Bug 215134 has been marked as a duplicate of this bug. ***
*** Bug 211475 has been marked as a duplicate of this bug. ***
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?
No, Mike. Soon after making comment 18 I found a patch for the problem. This bug (bug 224138) is fixed since two weeks.
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: