CopyFileMessage truncates messages with no ending cr/lf

RESOLVED FIXED in Thunderbird 3.0b4

Status

MailNews Core
Backend
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Phil Lacy, Assigned: Phil Lacy)

Tracking

({dataloss})

Trunk
Thunderbird 3.0b4
dataloss
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
copy an email message file that doesn't have cr/lf and it will give an error and not copy over last line of message.
cause of problem is here:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2382
error is from left over buffer reported here:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2516
doesn't happen when downloading IMAP messages, but a cr/lf is added to downloaded imap messages.

Existing unit test may prove this, I'll check later if not addressed in meantime.
(Assignee)

Updated

9 years ago
Assignee: nobody → philbaseless-firefox
(Assignee)

Updated

9 years ago
Depends on: 499304
(Assignee)

Comment 1

9 years ago
Created attachment 384326 [details] [diff] [review]
copies over the last line that is 'leftover'

This takes away the value of saving a partial line, and adding to new data on the next call.  The CopyFileMessage doesn't do this so it needs CopyData to do the whole thing, crlf at eof or not.
I need to pass through a variable for those calls that don't want the wholeFile functionality but to save partial lines.
I left it alone for now to see if you have a better idea. So far I think there is only one other call to CopyData.
Attachment #384326 - Flags: review?(bienvenu)
(Assignee)

Updated

9 years ago
Attachment #384326 - Flags: review?(bienvenu)
(Assignee)

Comment 2

9 years ago
Comment on attachment 384326 [details] [diff] [review]
copies over the last line that is 'leftover'

mbox format requires a blank line at end of message so I'll fix this and resubmit patch to add a blank line to files as needed.  This will make the CopyData work for all callers.
(Assignee)

Comment 3

9 years ago
Created attachment 384797 [details] [diff] [review]
adds test for complete file copy and checks for crlf

This takes care of CopyFileMessage to account for files that do not have a crlf at the eof.
But I'm not sure if the users of CopyData need this functionality. David, please check and see if we want to set this boolean value on the end of other CopyData uses.  Otherwise the init is setting it to false.
Attachment #384326 - Attachment is obsolete: true
Attachment #384797 - Flags: review?(bienvenu)

Comment 4

9 years ago
Comment on attachment 384797 [details] [diff] [review]
adds test for complete file copy and checks for crlf

This file doesn't use K&R braces, so this patch shouldn't...

m_completeFile is an ambiguous var name - I assume the sense of it is whether we should a trailing blank line? Could you use a name that reflects that?

On Mac/Linux, don't we just want a LF, not a CRLF?

Updated

9 years ago
Attachment #384797 - Flags: review?(bienvenu) → review-
(Assignee)

Comment 5

9 years ago
Created attachment 385498 [details] [diff] [review]
per review comments

variable suggestion makes much better sense. Other comments fixed.
Attachment #384797 - Attachment is obsolete: true
Attachment #385498 - Flags: review?(bienvenu)

Comment 6

9 years ago
Comment on attachment 385498 [details] [diff] [review]
per review comments

this patch doesn't apply cleanly, but more seriously, I think it adds a blank line at the end of every block, not just at the end of the message.
(Assignee)

Comment 7

9 years ago
(In reply to comment #6)
> (From update of attachment 385498 [details] [diff] [review])
> this patch doesn't apply cleanly, but more seriously, I think it adds a blank
> line at the end of every block, not just at the end of the message.

I had this depending on bug 499304. Both bugs are important to CopyFileMessage. But I think I can change the patch order if needed.

CopyFileMessage sets the m_addNewLineToEndOfMsg and calls CopyData with length of complete file (that was original intent of using m_completeFile as name, although I did change it to that from my first idea of m_wholeFile).

The 'while (1)' block has to copy the whole file, a line at a time, and if the last line of data is not terminated with a linebreak it adds it.
(Assignee)

Updated

9 years ago
Flags: blocking-thunderbird3?
Keywords: dataloss

Updated

9 years ago
Flags: blocking-thunderbird3? → blocking-thunderbird3+

Comment 8

9 years ago
The CopyData method is called from an other place, in a loop, so it's a bit scary to add code that could add newlines in the middle of a message. I can see why you did it there, because you want to make sure the added CRLF goes through the normal line parsing stuff...

What might make this a lot cleaner is a comment explaining what the situation is, and what the code is doing. Otherwise, it just looks like magic.

Something like this, where you handle the case of not finding a cr or lf:

In the case of copying a whole file (i.e., called from CopyFileMessage), if we don't find a trailing LINEBREAK, we must be at the last line of the file, and it must not have a LINEBREAK, so we're going to add it to the buffer.
(Assignee)

Comment 9

9 years ago
Created attachment 387813 [details] [diff] [review]
adds comment and changes variable name--again

I quasi adapted your comment suggestion but feel free to edit it and the variable name if it's still not clear enough.
Attachment #385498 - Attachment is obsolete: true
Attachment #387813 - Flags: review?(bienvenu)
Attachment #385498 - Flags: review?(bienvenu)
(Assignee)

Comment 10

9 years ago
Created attachment 387814 [details] [diff] [review]
header file change see other version for comments
Attachment #387813 - Attachment is obsolete: true
Attachment #387814 - Flags: review?(bienvenu)
Attachment #387813 - Flags: review?(bienvenu)

Comment 11

8 years ago
ok, thx, yes, I'm going to edit this a little to make it clear that the whole message is being copied in a single call to CopyData - that's really the crucial point.  I'll tweak this when the tree opens again, and land it; thx for the patch! I'll mark it r/sr=me when I attach the tweaked version later.
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> crucial point.  I'll tweak this when the tree opens again, and land it; thx for

I don't know if you applied bug 499304 but remember that needs to go in first or else you need to have me fix this patch to build on trunk.
(Assignee)

Comment 13

8 years ago
Created attachment 389324 [details] [diff] [review]
patch diff to trunk

just to save time when you are ready to land this.  I did a diff off the trunk as of today.
Whiteboard: [needs bienvenu to move forward]
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED

Comment 14

8 years ago
Comment on attachment 389324 [details] [diff] [review]
patch diff to trunk

I tweaked the variable name and added some comments to make it clearer what's going on, and checked in. Thx for the patch.
Attachment #389324 - Flags: superreview+
Attachment #389324 - Flags: review+

Comment 15

8 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [needs bienvenu to move forward]

Comment 16

8 years ago
Comment on attachment 387814 [details] [diff] [review]
header file change see other version for comments

clearing request
Attachment #387814 - Flags: review?(bienvenu)
Target Milestone: --- → Thunderbird 3.0b3
oups
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4

Updated

8 years ago
OS: Windows XP → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.