Closed Bug 57614 Opened 24 years ago Closed 24 years ago

...but copying to your sent folder failed

Categories

(MailNews Core :: Backend, defect, P1)

x86
Windows NT
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jar, Assigned: jefft)

Details

(Whiteboard: [rtm++] r=bienvenu sr=alecf)

Attachments

(2 files)

I'm not dead sure of the circumstances, but I keep seeing this now and again. I try to send a reply, and I get the dialog saying "the send operation succeeded, but the copy to the sent folder failed, would you like to return to the editor?" (I'm paraphrasing the latter section). I *think* this happens when I reply to a message that includes a combination of plain text and html. I tend to send my replay in "html only," and I think that triggers the problem. I don't know if this is associated with forwarding (replying) to messages with attachments... so I'm copying bienvenu as well. I thought it would be good to get this on the radar even before I can figure out what the exact trigger is. I'm using the optimized branch build 2000 10 20 08, whic I pulled from sweetlou on Friday. I'm running on NT, with service pack 6.
Putting on the rtm keyword.
Keywords: rtm
I tried replying to the same message several times... I found that it "failed to copy to sent folder" 2 out of 3 ways: HTML and Plain Text: Fails HTML only: Fails Plain Text only: Success (copying to sent folder). IN all three case, the email was sent, even though the copy failed. I've sent the "failing" message to selmer, mscott, and bienvenu, hoping the'll see the evil within the message.
Even more incredible... teh message that I sent in "plain text only" actually arrived with a "blue bar" indent of the replied to text, which I *THOUGHT* was html markup. I suspect that there is something very wrong in this area :-( :-(. I'll send a version to the CC list
reassigning to ducarroz. Do you know if it actually copies it to the sent folder in these cases. While looking into another bug, I found that that error message is out default error message when we don't know went wrong even if it's not regarding copying to the sent folder.
Assignee: putterman → ducarroz
I just checked my sent folder, and the messages are NOT being placed in there, so the dialog is accurate. I also just realized that N6 must have a "feature" that allows it to guess that ASCII mark-up is a "reply" indent, and it actually displays a Plain Text reply indented with a bluish bar... so my worries in that area are not founded. The basic bug still stands.
For those of you who got the message jar sent, are you able to reproduce it and if you can tell, does it look like it's something that will happen often enough that it needs to get fixed?
yes, I can reproduce it. I doubt it's that hard to run into - there's nothing much going on besides a couple simple text attachments. I wonder if this is related to the XIF landing, like the last time we saw problems like this. I'll debug a little.
We're failing just like the previously described XIF problem - nsRandomAccessInputStream::readline in nsIOFileStream is failing in the following code: else if (!eof() && n-1 == bytesRead) bufferLargeEnough = PR_FALSE; because we couldn't find a CRLF but we're not at eof - it turns out that n-1 == bytesRead, and n is 0x2800 (the buf size). So, my guess is that the XIF code is generating a really long line (but that's really just a guess - I don't know anything about this code). I'm pretty sure the recent change to this file did not cause this problem - it only makes us less likely to return an error from this routine.
I thought this was fixed by adding the check for n-1 == bytesRead. There was a duplicate bug which has been fixed and verfied, I believed. The must the odd case of that the last bytes read is exactly the same size of bytes we read from PRInt32 bytesRead = read(s, n - 1); I don't have a good solution at the moment. I'll take this bug, again.
Assignee: ducarroz → jefft
If that's true, would fixing the XIF code to output a trailing CRLF, as the output code did previously, fix this problem? Perhaps we should explore that.
I looked at the temp file - it does have a line longer than 2048 according to VC (and I bet longer than 0x2800). I think the XIF output code (if that's the relevant code) should wrap some lines. Otherwise, we'd have ignore the readline error, and check for eof ourselves, or something similary ugly.
Beth, could you have the NOXIF experts help look at this? Thanks!
Whiteboard: [rtm need info]
Pulling in jst and vidur (who played heavilly on the noxif landing).
I am not sure that NOXIF should wrap long lines. Wrapping long lines should be a responsibility of a mail client. Because of the NOXIF explores the ill implementation of nsFileStream class. Anther dramatic option we can consider is to abandon the use of nsFileStream class.
It would seem less risky to put the fix in our own code at this point. A prettier fix can go on the trunk when it's ready...
David, can you attachment you test messages to the bug? I think I have a fix. Thanks,
Attached patch A fix (deleted) — Splinter Review
I am rearranging the code in nsFileStream.cpp a little bit in order to work around of not detecting eof when calling readline. Seek() tends to set the eof correctly to the position. I am getting rid of the n-1 == bytesRead check. There is no guarantee the check works on all circumstances. In nsMsgSend.cpp I am adding a check for empty buffer read to make sure we are not bailing out too soon. Can I get a r= and sr= before I check in to the trunk for extensive tests?
Status: NEW → ASSIGNED
Jeff, do you really need to add the seeks, or is that part of an earlier, abandoned fix? In other words, what's the least fix that works?
David, the seek is needed in order to work around the false EOF flag. If you look into the following nsInputStream::read() method you will see that even you have read all of your data and reach the end of the stream your EOF flag is not set to true. The previous fix I had to check for n-1 == bytesRead apparently does not work for all cases. For example if the buffer size is small than the file size nsRandomAccessInputStream::readline() tends to return bufferLargeEnough == false which is wrong. seek() does the right thing to set the EOF flag correctly depends on where the position you are seeking into. Adding the seek() prior checking the *tp == "\r" or "\n" is to gurantee that we have EOF set correctly if we were at the end of the file and there isn't any CRLF. So, for the nsFileStream.cpp part this should be way to correctly fix the EOF flag and readline problem. For the nsMsgSend.cpp part we have a bad logic there.
You'll need to run this past someone who knows about the file stream code, then, cuz I sure don't. Cc'ing doug t.
Here is the same bug I tried to fix previously, bug 56129.
If I just apply the last part of your patch (the patch to nsMsgSend.cpp), this bug goes away. So, I don't think we need the other changes to nsIOFIleStream. However, I worry a little that we're adding an extra CRLF in this special case (where readline doesn't return a whole line) that we didn't used to before.
Is seek cheap on all platforms (esp. the Mac?) Is there any reason to worry about performance degradation with this fix?
I am completely unconvinced that the seek calls are needed for this fix, as I said before. The only reason I am unconvinced is that the empirical evidence showed that I just needed the part of the fix in nsMsgSend.cpp to not run into this bug.
The check for n-1 == bytesRead isn't 100%. However, additional seek guaratee it works 100%.
I don't seek will cost much of the performance hit. We are not actually trying to read anothing from the file system. Seek should simply playing with some memory structure and flags.
The bottom line is we need a conservative fix... and we need it *soon*. Please settle on it, and get a set of reviews and approvals... there just won't be many more changes to get into a limbo effort... and so we need closure Very Soon (TM). Thanks, Jim
Can I get people with knowledge of nsFileStream.cpp to review the patch? Please? The check in the nsMsgSend.cpp indeed fix most of the problem in case of buffer size equals to the read size and we couldn't find any CRLF within the buffer. Extra CRLF shouldn't be matter in the HTML message case since it's treated as a soft line break instead of <br>.
I'm happy to say r=bienvenu for the nsMsgSend.cpp change (and doesn't it fix it for the cases where the line length is >= buffersize? i.e., it's not the unlikely edge case of the line length being exactly the same size as the read buffer).
Yes. The nsMsgSend.cpp change should be sufficient. The changes in nsFileStream.cpp is for the rare edge case when the last read size equal to the buffer size and there is no CRLF read in the buffer.
Attached patch fix with nsMsgSend.cpp only (deleted) — Splinter Review
Can someone give a sr= in addition to r=bienvenu?
Whiteboard: [rtm need info] → [rtm need info] r=bienvenu sr=???
ok, sr=alecf
Fix checked in to trunk. Adding [rtm+] ...
Priority: P3 → P1
Whiteboard: [rtm need info] r=bienvenu sr=??? → [rtm+] r=bienvenu sr=alecf
This bug is in candidate limbo. We will reconsider this fix once we have a candidate in hand, but we can't take this fix before then.
rtm++, please checkin ASAP so we can build today.
Whiteboard: [rtm+] r=bienvenu sr=alecf → [rtm++] r=bienvenu sr=alecf
Fix checked in to the branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
The message wasn't attached to this bug, so I created a message using 4.7 with both HTML and plain text. I opened this message in Netscape 6 build 2000-11-01-20 and replyied to it using all three formats. All sent and all have copies in the Sent folder. I did this with the Sent folder assigned to the Local Sent folder and my IMAP Sent folder and it worked OK. If someone has the original "bad" message please send it to me otherwise, I will verify this per my test case.
I don't have the message, but it was described as having been created as plaintext in NS6 mail compose. If I'm remembering correctly, the problem happens when you use reply to such a message and you have html compose set.
Using 2000-11-03 branch on win98 OK, I tried this again sending in Plain text compose using Netscape 6, the opened the message and clicked Reply with HTML set for my compose window. I added HTML text and Sent, I just type in plain text and sent. both ok now errors. Marking verified, if anyone sees this problem again
verified
Status: RESOLVED → VERIFIED
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

Creator:
Created:
Updated:
Size: