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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: skamio, Assigned: ch.ey)
References
Details
Attachments
(1 file)
666 bytes,
patch
|
mscott
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
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•23 years ago
|
||
I think the problem is in the DOM and the new line is inserted when you send the
email.
Comment 3•22 years ago
|
||
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
Comment 4•22 years ago
|
||
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
Assignee | ||
Comment 5•22 years ago
|
||
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.
Assignee | ||
Comment 6•22 years ago
|
||
I really encourage to apply this patch to become RFC compliant.
Assignee | ||
Updated•22 years ago
|
Attachment #131343 -
Flags: review?(scott)
Comment 7•22 years ago
|
||
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+
Assignee | ||
Comment 8•22 years ago
|
||
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
Comment 9•22 years ago
|
||
checked into the trunk for Christian.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 10•22 years ago
|
||
For tracking purposes, this caused regression bug 224138.
Comment 11•22 years ago
|
||
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
Assignee | ||
Comment 12•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 14•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 16•22 years ago
|
||
Ok, the last I want is a broken Mozilla.
Do you have CVS access, or who should back it out?
Comment 17•22 years ago
|
||
*** Bug 215134 has been marked as a duplicate of this bug. ***
Comment 18•22 years ago
|
||
*** Bug 211475 has been marked as a duplicate of this bug. ***
Comment 19•22 years ago
|
||
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?
Assignee | ||
Comment 20•22 years ago
|
||
No, Mike. Soon after making comment 18 I found a patch for the problem. This bug
(bug 224138) is fixed since two weeks.
Updated•21 years ago
|
Product: MailNews → Core
Updated•17 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•