56129 - Save As Draft and Save to Sent folder are broken

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[Phil Peterson]

Using 2000-10-11-08 RTM branch build on NT

1. Compose a mail message
2. Save as draft

Expected: draft saved
Actual: error msg: Send was successful but saving to Sent folder failed. Draft
not saved

Comment 1

[Phil Peterson]

rtm keyword

Comment 2

[pmock]

Changing platform to all... :(

This problem is reproducible on today builds on all platforms.
 win32 commercial seamonkey build 2000-101109-mn6 installed on P500 Win98
 linux commercial seamonkey build 2000-101109-mn6 installed on P200 RedHat 6.2
 macos commercial seamonkey build 2000-101110-mn6 installed on G3/400 OS 9.04

Comment 3

[selmer (gone)]

Adding regression keyword.  Can anyone figure out what recent change broke this?
 (adding some people to the CC who may be able to help answer that question)

Comment 4

[Phil Peterson]

For me, I can send mail, but can't FCC either. Which makes sense since I bet
saving to Sent and saving to Drafts use some of the same code. Resummarizing.

BTW, both my Drafts and Sent folder are on the IMAP server.

Comment 5

[Jean-Francois Ducarroz]

I'll take a look as soon my build is done...

Comment 6

[scottputterman]

 It's failing here:

nsMsgComposeAndSend::MimeDoFCC line 4091

   if (!inputFile.readline(ibuffer, ibuffer_size))
    {
      status = NS_ERROR_FAILURE;
      goto FAIL;
    }

this status then gets returned and we bail on our current action.  Interestingly
enough, we have a default error case that we hit in

nsMsgComposeAndSend::NotifyListenersOnStopCopy that puts up the copy to sent
message folder failed alert. This probably shouldn't happen in the Save as Draft
case.

Comment 7

[scottputterman]

*** Bug 56108 has been marked as a duplicate of this bug. ***

Comment 8

[Jean-Francois Ducarroz]

I am working on it...

Comment 9

[Jean-Francois Ducarroz]

cc'ing rhp in case he has an idea.

Comment 10

[jefft]

Couple problems found here:

1) nsFileStream::readline() falsefully returns bufferNotLarge enough if it could 
find "\r\n" token, another thing the implementation of readline is really time 
consuming

2) for some reason when writing out the final temp rfc822 message file we no 
longer writing our "\r\n" for the last line.

Comment 11

[jefft]

I meant "could NOT find \r\n".

Comment 12

[(not reading, please use [e-mail] instead)]

nsMsgComposeAndSend::MimeDoFcc() is what is failing.

I'll continue to debug.  (my dollar is on the noxif stuff.)

Comment 13

[jefft]

nsInputStream::read() has a false login there. It set_at_eof(PR_TRUE) only when
the mInputStream->Read() result is equal to 0 bytes. In the case of reading the
last remaining bytes of the file we failed to set the eof flag.

Comment 14

[jefft]

Attachment: A safe fix and a right thing to do

Comment 15

[Jean-Francois Ducarroz]

R=ducarroz

Comment 16

[Jean-Francois Ducarroz]

reassign to jeff

Comment 17

[(not reading, please use [e-mail] instead)]

r=sspitzer.

the fix looks good, and fixes the problem for me.

we should get more eyes on it.

who owns the input stream code these days?  (warren?)

Comment 18

[Scott MacGregor]

I thought I had already cc'ed warren on this. I think bugzilla dropped my
comments =(.

I'd like to wait until Warren does a code review on this before we check it in.
He knows the nsInputStream code best of all.

Comment 19

[jefft]

dougt? I think.

Comment 20

[Phil Peterson]

Does anyone know why this just started happening? Would be good to pinpoint the 
cause, before changing something so fundamental as the streaming code.

Comment 21

[(not reading, please use [e-mail] instead)]

the noxif landing appears to have exposed this problem.  but the noxif landing
did not cause the bug.

before the noxif landing, the file was terminated with "\r\n"
after the noxif landing, the file was not terminated with "\r\n" which exposed a
problem with read / readline.

mail news uses the input stream code, and the failures bubble up and cause weird
bugs.

Comment 22

[Phil Peterson]

Adding jst to cc list. jst or jfrancis, can you review the patch please? We 
should definitely understand why this broke, and what else could be broken.

Comment 23

[Joe Francis]

I'm not the right person to review this.  I looked at it and it lokos ok to me, 
but we need someone else.  

Comment 24

[Warren Harris]

mscott: Actually I don't know much about this code. It's the old mcmullen stuff 
that's really obsolete now.

Comment 25

[Jean-Francois Ducarroz]

*** Bug 56336 has been marked as a duplicate of this bug. ***

Comment 26

[Scott MacGregor]

Okay if there's no owner then I'll super review it because the change makes
sense and looks correct to me. sr=mscott

Comment 27

[(not reading, please use [e-mail] instead)]

marking rtm+, since we have r= and sr=.

Comment 28

[Jim Roskind]

Marking rtm-double-plus. Please land on branch asap.

I spoke to jst, and he doesn't think he's the guy to be reviewing this (re:
Phil's comment above).
...but he did offer to help with the landing this on the branch if no one is
around tonight to do so.

Comment 29

[Johnny Stenback (:jst)]

I checked in the attached patch so that this problem is fixed in tomorrows builds.

Comment 30

[pmock]

Verified as fixed on branch builds of win32, linux, and macos using the builds:
 win32 commercial seamonkey build 2000-101810-mn6 installed on P500 Win98
 linux commercial seamonkey build 2000-101809-mn6 installed on P200 RedHat 6.2
 macos commercial seamonkey build 2000-101808-mn6 installed on G3/400 OS 9.04

Comment 31

[Asa Dotzler [:asa]]

Verified Fixed on trunk builds
linux 101808 RedHat 6.2
win32 101804 NT 4
mac 101804 Mac OS9
Setting bug to Verified and removing vtrunk keyword