Closed
Bug 57614
Opened 24 years ago
Closed 24 years ago
...but copying to your sent folder failed
Categories
(MailNews Core :: Backend, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: jar, Assigned: jefft)
Details
(Whiteboard: [rtm++] r=bienvenu sr=alecf)
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•24 years ago
|
||
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.
Reporter | ||
Comment 3•24 years ago
|
||
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
Comment 4•24 years ago
|
||
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
Reporter | ||
Comment 5•24 years ago
|
||
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.
Comment 6•24 years ago
|
||
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?
Comment 7•24 years ago
|
||
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.
Comment 8•24 years ago
|
||
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
Comment 10•24 years ago
|
||
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.
Comment 11•24 years ago
|
||
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.
Comment 12•24 years ago
|
||
Beth, could you have the NOXIF experts help look at this? Thanks!
Whiteboard: [rtm need info]
Reporter | ||
Comment 13•24 years ago
|
||
Pulling in jst and vidur (who played heavilly on the noxif landing).
Assignee | ||
Comment 14•24 years ago
|
||
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.
Comment 15•24 years ago
|
||
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...
Assignee | ||
Comment 16•24 years ago
|
||
David, can you attachment you test messages to the bug? I think I have a fix.
Thanks,
Assignee | ||
Comment 17•24 years ago
|
||
Assignee | ||
Comment 18•24 years ago
|
||
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
Comment 19•24 years ago
|
||
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?
Assignee | ||
Comment 20•24 years ago
|
||
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.
Comment 21•24 years ago
|
||
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.
Assignee | ||
Comment 22•24 years ago
|
||
Here is the same bug I tried to fix previously, bug 56129.
Comment 23•24 years ago
|
||
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.
Comment 24•24 years ago
|
||
Is seek cheap on all platforms (esp. the Mac?) Is there any reason to worry
about performance degradation with this fix?
Comment 25•24 years ago
|
||
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.
Assignee | ||
Comment 26•24 years ago
|
||
The check for n-1 == bytesRead isn't 100%. However, additional seek guaratee it
works 100%.
Assignee | ||
Comment 27•24 years ago
|
||
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.
Reporter | ||
Comment 28•24 years ago
|
||
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
Assignee | ||
Comment 29•24 years ago
|
||
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>.
Comment 30•24 years ago
|
||
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).
Assignee | ||
Comment 31•24 years ago
|
||
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.
Assignee | ||
Comment 32•24 years ago
|
||
Assignee | ||
Comment 33•24 years ago
|
||
Can someone give a sr= in addition to r=bienvenu?
Whiteboard: [rtm need info] → [rtm need info] r=bienvenu sr=???
Comment 34•24 years ago
|
||
ok, sr=alecf
Assignee | ||
Comment 35•24 years ago
|
||
Fix checked in to trunk. Adding [rtm+] ...
Priority: P3 → P1
Whiteboard: [rtm need info] r=bienvenu sr=??? → [rtm+] r=bienvenu sr=alecf
Comment 36•24 years ago
|
||
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.
Comment 37•24 years ago
|
||
rtm++, please checkin ASAP so we can build today.
Whiteboard: [rtm+] r=bienvenu sr=alecf → [rtm++] r=bienvenu sr=alecf
Assignee | ||
Comment 38•24 years ago
|
||
Fix checked in to the branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 39•24 years ago
|
||
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.
Comment 40•24 years ago
|
||
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.
Comment 41•24 years ago
|
||
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
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•