Closed Bug 19600 Opened 25 years ago Closed 25 years ago

[DOGFOOD] Messages display appended using POP accounts

Categories

(SeaMonkey :: MailNews: Message Display, defect, P1)

Product:
SeaMonkey
Component:
MailNews: Message Display
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: nbaca, Assigned: waterson)

References

Details

(Whiteboard: [PDT+] fix by 11/24)

Attachments

(1 file)

proposed fix
3.11 KB, patch
Details | Diff | Splinter Review 
Build 1999112208M12: Win/NT4, Linux/Redhat 6.0, Mac 8.5.1

Overview: A message created with Seamonkey, when viewed using a POP account,
displays its content and the content of all messages that follow it in the
folder. In other words, the messages are being appended.

Steps to reproduce:
1. Launch Mail using a POP account (i.e. qatest20)
2. Send 4 new messages to the POP account
3. Get messages
4. View the fourth to last message (message a)
5. View the third to last message (message b)
6. View the second to last message (message c)
7. View the last message (message d)

Actual Results:
- View "message a": It displays its content and appends the content of messages
b, c, and d.
- View "message b": It displays its content and appends the content of messages
c and d.
- View "message "c": It displays its content and appends the content of message
d.
- View "message "d": It dispalys as expected only showing its content.

Expected Results: After selecting any message it should only display the content
of the message selected. It should not append any other messages.

Additional Information:
1. The problem does not occur with IMAP accounts.
2. When a message is sent from 4.7 and viewed in Seamonkey then the message
displays correctly, no appending appears. For instance:

a. Create and send messages by:
- Using Seamonkey and calling it "A"
- Using Communicator 4.7 and calling it "4.7"
- Using Seamonkey and calling it "B"
- Using Seamonkey and calling it "C"
- Using Seamonkey and calling it "D"

b. Select message "A". It displays its content and every message that follows
it, including the "4.7" message.

c. Select message "4.7". It displays correctly only showing its content.

d. Select message "B". It displays its content and every message that follows
it.

e. Select message "C". It displays its content and every message that follows
it.

f. Select message "D". It displays its content correctly. No other messages are
appended because it is the last message.
OS: Windows NT → All
QA Contact: lchiang → pmock
Summary: Messages display appended using POP accounts → [DOGFOOD] Messages display appended using POP accounts
Results: Viewing one message can be very slow because of all the appending work
that is being done.
Assignee: phil → mscott
Assigning to mscott, thanks!
Jeff, do you have any ideas on this? nbaca says that the problem occurss with
messages created using 5.0 that are in your mail folder.


I just tried this out on my release build from today and I see it to.
Over to Warren. Warren, it looks like the file transport is no longer honoring
the read count variable passed into AsyncRead.

I stepped through it briefly and saw the mailbox protocol request 852 bytes
for one of my messages. Yet I received ODA events where the size of the
data available was much greater than 852. In fact I got enough ODA events
were the size == the total size of my mailbox if I tried to display the first
message
.

Definetly a dogfood stopper.


I'll keeping trying to debug it to. It's possible the bug is in the pipe code
that the file transport uses and not necessarily the file transport itself.
Assignee: mscott → warren
More info for you warren. The problem is in nsFileTransport::Process
under the READING switch statement.


The first time through we correctly read the correct number of bytes
I asked for (mTransferAmount). This then becomes 0 bytes because we
read out the same number of bytes. However, we don't kick out of the
read loop. Instead we come through it again. This time we try to
read 0 bytes (that's the new value of mTransferAmount) out of mBufferOuputStream
and this actually returns more data in the stream. So now we are reading
out and forwarding extra data that the caller didn't want.

Seems like if we get to the end of READING and mTransferAmount == 0, we should
kick out READ_END.
Assignee: warren → waterson
Sounds related to Waterson's recent changes.

Chris: Can you take a look?
I just looked at waterson's changes to nsFileTransport. Yup, he definetly
looks like the culprit =). Chris, your changes don't kick us out of the read
loop if mTransferAmount == 0. It used to do this before. Instead, you change
mTransferAmount to -1 if it is zero.

This breaks the case where you tell the file transport you only want to read
'n' bytes out of it.

What was the scenario where you had a caller passing 0 bytes for the number
of bytes to read into AsyncRead? Seems like the caller should really be passing
in -1 directly instead of nsFiletransport morphing 0 to -1 before calling
WriteFrom on the buffer output stream....
The case is where you are either reading a directory stream, in which case you
don't know the length of the stream beforehand, or you are reading a special
file, whose length per stat() is zero. I guess we could use '-1' as the special
case value, and if stat() returns a file length of zero, then just immediately
change that value to '-1' in nsLocalFileSystem.
I think that sounds like the right thing to do. Then we can just back out the
changes to nsFileTransport.cpp.
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: M12
allright, i'll try that.
Whiteboard: [PDT+]
Putting on PDT+ radar.
*** Bug 19995 has been marked as a duplicate of this bug. ***
Whiteboard: [PDT+] → [PDT+] fix by 11/24
Attached patch proposed fix — — Splinter Review 

    
    

    
  
changed to us '-1' as special value:

1. Check result of stat(); if == 0, set content length to -1.

2. Fix assertions that want mTransferLength to be positive.

3. Add back code that terminates transfer when mTranfserLength == 0.

warren, could you review this?

thanks.

    
    

    
  
Looks good, although can we eliminate the GetFileSize call entirely? In another
bug someone was complaining about all the stat calls that we do, and attributed
it to this. If we always use the -1 codepath for the "all the file" case then we
can eliminate this. Can you try it?

    
    

    
  
I tried the patch, and it fixes the problem in my local folders (and also cures
19996, where reading a particular mail message crashes my X server).

    
    

    
  
warren: in that case, should we also eliminate the 'out' parameter from the
nsIFileSystem::open() interface?

    
    

    
  
Maybe eliminating the out parameter is the best thing if it works for all the
cases (including jar).

    
    

    
  
warren: it seems to work fine w/o the filesize check. this'll probably suck a
little bit when we're laying out Very Large Files, because we won't be able to
show progress information.

i'd kinda recommend checking in without it: let's let the memory cache and
other stuff land before we decide not to stat files and stuff.

    
  
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED

    
    

    
  
ok, i checked in a fix with the stat() so that I can get this bug off my plate
and get a toe-hold into the tree. warren: assign the other performance bug to
me. i'm gonna try to remove the "Content length" parameter and see what breaks.

    
  
QA Contact: pmock → nbaca

    
    

    
  
Ninoschka - can you verify this today?  Peter is out on vacation and we'd
like to get the PDT+ resolved bugs verified by tomorrow.  Thanks.

    
    

    
  
I'm working on this now.

    
  
Status: RESOLVED → VERIFIED

    
    

    
  
Build 1999112909M12: NT4, Linux, Mac
Verified Fixed.

    
    

    
  
*** Bug 19996 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey

    
    

    
      
  

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: