460636 - nsMsgSaveAsListener sometimes inserts extra LF characters

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Kaspar]

Attachment: Proposed patch

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.

Comment 1

[Kaspar]

Attachment: Sample message (8933 Bytes)

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

[Mark Banner (:standard8)]

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).

Comment 3

[Kaspar]

(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

[Mark Banner (:standard8)]

(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

[David :Bienvenu]

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

[David :Bienvenu]

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.

Comment 7

[David :Bienvenu]

Attachment: fix as checked in.

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.

Comment 8

[Rain [:rain1] (inactive)]

(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.

Comment 9

[Kaspar]

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

[Wayne Mery (:wsmwk)]

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.

Comment 11

[Kaspar]

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.

Comment 12

[Rain [:rain1] (inactive)]

(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.

Comment 13

[Kaspar]

(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.

Comment 14

[Kaspar]

Attachment: [checked in] Proposed unit test

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.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 360057 [details] [diff] [review]
[checked in] Proposed unit test

I'll take a look soon, thanks.

Comment 16

[Mark Banner (:standard8)]

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