Closed
Bug 460636
Opened 15 years ago
Closed 15 years ago
nsMsgSaveAsListener sometimes inserts extra LF characters
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b1
People
(Reporter: mozbgz, Assigned: mozbgz)
Details
Attachments
(3 files, 1 obsolete file)
8.72 KB,
message/rfc822
|
Details | |
2.24 KB,
patch
|
Details | Diff | Splinter Review | |
12.63 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
When saving messages from an IMAP account to a local .eml file, nsMsgMailNewsUrl/nsMsgSaveAsListener may spuriously insert additional LF characters, *if* a CRLF line ending happens to occur at a multiple of 8192. This is due to nsMsgSaveAsListener::OnDataAvailable reading a message in chunks of 8192 bytes (SAVE_BUF_SIZE), but not handling the case where a CRLF occurs exactly at the boundary. The attached patch fixes that. Reproducing the problem is somewhat tricky, but I will attach a message file which should allow to trigger the effect.
Attachment #343786 -
Flags: superreview?(bienvenu)
Attachment #343786 -
Flags: review?(bienvenu)
To reproduce the problem, store this message in an IMAP folder and save it to an .eml file afterwards. It will then have a size of 8934 bytes, with an extra LF inserted after line 110. (Make sure that the respective folder is not selected for offline use - otherwise the "From -" and "X-Mozilla-Status*" lines will interfere.)
Comment 2•15 years ago
|
||
Kaspar, would you consider trying to write a unit test for this? See https://wiki.mozilla.org/MailNews:Automated_Testing for more info. David and I can help you with locating the appropriate functions (which would seem to be nsIMsgMailNewsURL::getSaveAsListener).
Flags: in-testsuite?
(In reply to comment #2) > Kaspar, would you consider trying to write a unit test for this? I can try, but would certainly need assistance. Do you think it's doable with an xpcshell test (and without requiring access to an IMAP server)?
Comment 4•15 years ago
|
||
(In reply to comment #3) > (In reply to comment #2) > I can try, but would certainly need assistance. Do you think it's doable with > an xpcshell test (and without requiring access to an IMAP server)? Given that the code you are touching affects all servers (afaict) then I think it would be quite doable. I'd expect the actual test to be a subset of that in nsMessenger::SaveAs, starting from http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp#1125 So you could write a listener class to implement nsIUrlListener, and then call nsIMsgMessageService::SaveMessageToDisk on an appropriate object. Once the listener gives you the OnStopRunningUrl notification, you can check the saved version against the real version. This test looks like a nice simple test to mimic: http://mxr.mozilla.org/comm-central/source/mailnews/local/test/unit/test_msgCopy.js It creates a local mail account, and copies an email into a folder. So you'd still need this bit, but then just have a call to SaveMessageToDisk. This test has a bit at the end which loads a file from disk, and compares its data: http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_getMsgTextFromStream.js
Comment 5•15 years ago
|
||
I think getMsgTextFromStream maintains line endings, but I wouldn't swear to it, so it may or may not allow you to test this fix. I'm going to try this patch this morning.
Comment 6•15 years ago
|
||
Comment on attachment 343786 [details] [diff] [review] Proposed patch thx for the patch; it fixes the problem. I'm going to change the var name to be a little clearer, and change the initialization to be '\0' instead of nsnull before checking in.
Attachment #343786 -
Flags: superreview?(bienvenu)
Attachment #343786 -
Flags: superreview+
Attachment #343786 -
Flags: review?(bienvenu)
Attachment #343786 -
Flags: review+
Comment 7•15 years ago
|
||
this is what I landed as changeset: 694:b52bc0e1b57a Thx again for the patch. One thing I noticed while testing this is that we're holding onto a lock on the saved file after the save finishes (perhaps we're not closing the file). I'll have a quick look at that and file a spin-off bug.
Attachment #343786 -
Attachment is obsolete: true
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Hardware: PC → All
Target Milestone: --- → Thunderbird 3.0b1
Comment 8•15 years ago
|
||
(In reply to comment #5) > I think getMsgTextFromStream maintains line endings, but I wouldn't swear to > it, so it may or may not allow you to test this fix. I don't think it does, as it does a NS_ReadLine and then appends \r\n later. http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp?mark=4769-4774#4769 Well, if extra LF characters come in, presumably NS_ReadLine will have a blank line there, so maybe you could use that to test.
Thanks for testing and applying the patch, David. (In reply to comment #4) > Given that the code you are touching affects all servers (afaict) then I think > it would be quite doable. My knowledge of the mailnews code is very limited - but from what I see, that statement is not true. Cf. http://mxr.mozilla.org/comm-central/search?string=GetSaveAsListener&find=&findi=&filter=&hitlimit=&tree=comm-central I.e., it's not used by nsMailboxService (nsMailboxService::SaveMessageToDisk doesn't use GetSaveAsListener, in contrast to nsNntpService and NsImapService). This also reflects my experience when testing the patch - you can't use local folders... I had to use an IMAP account (and run it under Windows, i.e. on a platform where messages come in with CRLF as the default line ending).
Comment 10•15 years ago
|
||
This FIXED bug is flagged with in‑testsuite? It would be great if assignee or someone else can clear the flag if a test is not appropriate. And if appropriate, create a test and plus the flag to finish off the bug.
Assignee | ||
Comment 11•15 years ago
|
||
As I tried to explain in comment 9, I don't think it's doable without access to an IMAP (or NNTP server), so I'm clearing the flag.
Flags: in-testsuite?
Comment 12•15 years ago
|
||
(In reply to comment #11) > As I tried to explain in comment 9, I don't think it's doable without access to > an IMAP (or NNTP server), so I'm clearing the flag. We do have fake IMAP and NNTP servers in the tree now. You can look at some of the tests in mailnews/imap/test/unit and mailnews/news/test/unit to get started. The test definitely looks doable. From what I understand, you need to copy a message with a particular line ending format to the server -- see addMessagesToServer() in test_nsIMsgFolderListenerIMAP.js for this.
Flags: in-testsuite?
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #12) > We do have fake IMAP and NNTP servers in the tree now. You can look at some of > the tests in mailnews/imap/test/unit and mailnews/news/test/unit to get > started. Thanks for the pointer - I wasn't aware of that. I'll look into it more closely in the next few days.
Assignee | ||
Comment 14•15 years ago
|
||
Ok, I think I've figured it out... and am proposing the attached unit test. Is anybody volunteering for a review? The test should also run successfully on platforms where CRLF is not the default line ending (Unix, e.g.), since I'm forcing canonicalLineEnding to true when calling SaveMessageToDisk(). [Hopefully hg will preserve the line endings in the test message when it's checked in.] If I un-apply attachment 344379 [details] [diff] [review] and run the test, then I'm getting a failure... as expected. Would be glad if someone else could give it a try as well, though.
Attachment #360057 -
Flags: review?
Comment 15•15 years ago
|
||
Comment on attachment 360057 [details] [diff] [review] [checked in] Proposed unit test I'll take a look soon, thanks.
Attachment #360057 -
Flags: review? → review?(bugzilla)
Updated•15 years ago
|
Attachment #360057 -
Attachment description: Proposed unit test → [checked in] Proposed unit test
Attachment #360057 -
Flags: review?(bugzilla) → review+
Comment 16•15 years ago
|
||
Comment on attachment 360057 [details] [diff] [review] [checked in] Proposed unit test Excellent works nicely, thanks for doing this, it'll cover another area so that we don't regress. r=me. I've checked this in already: http://hg.mozilla.org/comm-central/rev/fd3b1f075fa1
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•