122361 - downloaded mail gets messed up when read/deleted at the same time

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[gaele]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.7) Gecko/20011221
BuildID:    2001122106

My inbox is messed up.

This has happened:

1. I'm downloading mail with POP3, about 100 messages.
2. At the same time I am reading the newly downloaded messages and deleting some
of them
3. At some point several messages seem to be mixed up:
   - the body contains (parts of) several other messages
   - the Subject/Sender is that of another message
See below for an example.

I'm not sure if this is part of the problem:
I have a filter that puts all messages containing "To: edwintersport@" in a
separate folder. Messages in this folder are messed up as well.

I checked my Inbox file, and it looked alright to me. So maybe just the
Inbox.msf is corrupt. (I'm not publishing these files here as they contain
private messages, and besides my Inbox is 6.5 MB.)



Reproducible: Didn't try

This is an example. The data in the summary pane is not related to the data in
the body pane. In the body pane the Subject is empty, and there is no From, no
To, no Date. The body data contains two parts of other messages.


This is the data in the summary pane:

Subject                                               Sender
Singer Island Beach Ultimate Tournament is going hat  Beach Ultimate Tournament
Alert


This is the data in the body pane:

Subject: 

http://bugzilla.mozilla.org/show_bug.cgi?id=121839





------- Additional Comments From [e-mail] 2002-01-28 12:57 -------
filed bug 122270 as a spinoff ...


From - Tue Jan 29 11:35:46 2002
X-UIDL: 5af5d218629735be80de4816056a9907
X-Mozilla-Status: 0000
X-Mozilla-Status2: 00000000
>From [e-mail] Mon Jan 28 21:57:34 2002
Return-Path: <[e-mail]>
Received: from c015.snv.cp.net by hees.nijmegen.internl.net
	via c015-mx000.c015.snv.cp.net [209.228.35.109] with SMTP for <[e-mail]>
id VAA02350 (8.8.8/1.3); Mon, 28 Jan 2002 21:57:33 +0100 (MET)
Received: (cpmta 28484 invoked from network); 28 Jan 2002 12:57:49 -0800
Delivered-To: [e-mail]
Received: (cpmta 28479 invoked from network); 28 Jan 2002 12:57:49 -0800
Received: from 128.121.214.228 (HELO s0276.pm0.net)
  by smtp.c015.snv.cp.net (209.228.35.109) with SMTP; 28 Jan 2002 12:57:49 -0800
X-Received: 28 Jan 2002 20:57:49 GMT
Received: (from s0276@localhost)
	by s0276.pm0.net (8.8.8pmg/8.8.5) id UAA75609
	for
:include:/usr/home/s0276/pmgs/users/Transatlantic/delivery/1012248559.21296/rblk.38;
Mon, 28 Jan 2002 20:47:51 GMT
Message-Id: <[e-mail]>
From: The Atlantic Online <[e-mail]>
To: [e-mail]
X-Info: Message sent by Mindshare Design customer with ID "Transatlantic"
X-Info: Report abuse to list owner at [e-mail]
X-PMG-Userid: Transatlantic
X-PMG-Msgid: 1012248559.21296
X-PMG-Recipient: [e-mail]
Subject: The February issue: Nazi saboteurs, councils of war; and much more...
Date: Mon, 28 Jan 2002 15:48:02 EST
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
X-UIDL: 5af5d218629735be80de4816056a9907

TransAtlantic | The Atlantic Online=20

January 28, 2002

-------------------------------------------------------
+ In THE ATLANTIC MONTHLY | February 2002 | Digital Edition

Please note that only selected articles from the current issue of the
magazine will be made available for free on the Web.

THE KEYSTONE KOMMANDOS
by Gary Cohen
In June of 1942 eight Nazis bent on sabotage were set ashore on
American beaches. Their mission came to naught, undermined by
confusion and betrayal. The one (inadvertent) accomplishment: creation
o

Comment 1

[David :Bienvenu]

Navin has some code that's supposed to prevent this sort of thing.

Comment 2

[Navin Gupta]

We had an old bug when copy was corrupting destination folder -  hdrs body
don't match, but that was fixed long time back. 

We also had some similar filter bugs where filtering resulted in the same hdrs body
don't match but we were unable to reproduce it (IIRC). 

laurel, have you seen this recently?

Comment 3

[laurel]

No, I haven't.

Comment 4

[David Balch]

I can report getting POP3 messages getting jumbled together, on build 2002020406
(0.9.8) on WindowsME (and on previous builds, but I'm not sure of the numbers).

I could believe it's something to do with filters, but am not certain.

Will add an example...

Comment 5

[David Balch]

Attachment: Txt showing a jumbled message displayed

This text is as appears in the message pane (or window). It is comprised of
several emails concatenated together:

- something from W3C style mailing list
- several from bugzilla
- one from a website status checker (which looks truncated).

The message was identified by the subject, sender (and most likely, date) of
the last message in this file.

Comment 6

[Navin Gupta]

same problem as 125503. 

*** This bug has been marked as a duplicate of 125503 ***

Comment 7

[Navin Gupta]

This may have nothing to do with dropping connection. 

Comment 8

[David :Bienvenu]

we should not allow you to read/delete new mail while downloading messages,
because that can result in two open streams on the file, both writing data at
the same time into the file (because reading/deleting messages updates the
mozilla status line).

Comment 9

[Jonas Jørgensen]

Nominating for nsbeta1.

Comment 10

[Michael Buckland]

Discussed at Mail News bug meeting with Engineering, QA Mktng and PjM.  Decided
to plus this bug.  We propose the fix as described in comment #8.

Comment 11

[Michael Buckland]

Discussed in Mail News bug mtg with Engineering QA Mktng PjM.  Decided to ADT2
this bug. 

Comment 12

[Navin Gupta]

Take the case where we have a filter - label set up. When we download the mail
and it matches filter criteria, we again open a stream in mailDatabase to change
moz-status line, so opening two streams to the berkeley mailbox can happen
without reading/deleting mail as we are downloading pop3 mail. 

I think we may have to close the downloading pop3 stream before publishing header
and then reopen after the header is published. 

Comment 13

[David :Bienvenu]

that would solve the label problem, but not the general problem of the user
reading an unread message while downloading was going on. It would also slow
down pop3 downloading since opening the stream can be slow, especially for large
files. Another approach would be to share the stream between pop3 downloading
and changing a msg flag in the folder, and make sure we seek to the end before
using it for pop3 download. This would solve the general problem, and not have a
performance degradation.

Comment 14

[Navin Gupta]

Attachment: proposed fix

The general idea is to not have two outputStream to the mailbox writing at the
same time. So when we are downloading pop3 mail, once we have downloaded a
message and we are publishing the hdr we will share pop3 inbox stream with
mailDatabase by setDBFolderStream. This will be used for filter requirements
like labelling, marking message read/flagged etc. So the lowest level of flag
setting routine SetHdrFlag will check for existing m_folderStream and if it is
not there, it will try to get a folder lock because we are creating
folderStream in UpdateFolderFlag. Once filtering is done we reset the file pos
of the fileStream to end of file. 

Now during download if the user tries marking message read/flagged/label then 
we have null m_folderStream so it fails. For delete we do batching and we
create m_folderStream in StartBatch so I had to get the lock in
nsMsgLocalMailFolder::DeleteMessage 

I have tested this patch for a while and it works fine. David, need review

Comment 15

[Navin Gupta]

Attachment: proposed fix

The general idea is to not have two outputStream to the mailbox writing at the
same time. So when we are downloading pop3 mail, once we have downloaded a
message and we are publishing the hdr we will share pop3 inbox stream with
mailDatabase by setDBFolderStream. This will be used for filter requirements
like labelling, marking message read/flagged etc. So the lowest level of flag
setting routine SetHdrFlag will check for existing m_folderStream and if it is
not there, it will try to get a folder lock because we are creating
folderStream in UpdateFolderFlag. Once filtering is done we reset the file pos
of the fileStream to end of file. 

Now during download if the user tries marking message read/flagged/label then 
we have null m_folderStream so it fails. For delete we do batching and we
create m_folderStream in StartBatch so I had to get the lock in
nsMsgLocalMailFolder::DeleteMessage 

I have tested this patch for a while and it works fine. David, need review

Comment 16

[David :Bienvenu]

cool.

+      //PublishMsgHeader would have changed the postion of file stream pointer,
reset it back.

this should be "could have" or "might have" 

looks like tabs here:

+    PRBool isLocked;
+    supports = do_QueryInterface(NS_STATIC_CAST(nsIMsgDatabase*, this));
+
  m_folder->GetLocked(&isLocked);
+
  if(!isLocked)
+    {
+      m_folder->AcquireSemaphore(supports);
+      locked = PR_TRUE;

I really hate NS_ERROR_FAILURE; it's useless.

+  if (!SetHdrFlag(msg, PR_TRUE, MSG_FLAG_EXPUNGED)) // tell mailbox (mail)
+    return NS_ERROR_FAILURE;	
   
   if (m_newSet)	// if it's in the new set, better get rid of it.
     m_newSet->Remove(key);
@@ -1692,6 +1693,9 @@
   (void)msgHdr->GetMessageKey(&key);
   msgHdr->GetFlags(&oldFlags);
   
+  if (!SetHdrReadFlag(msgHdr, bRead))
+    return NS_ERROR_FAILURE;

There are a few error codes that might be more applicable in msgCore.h, like
NS_MSG_ERROR_WRITING_MAIL_FOLDER - also, note NS_MSG_FOLDER_BUSY


shouldn't this be an assertion too?
+NS_IMETHODIMP nsMsgDatabase::SetFolderStream(nsIOFileStream *aFileStream)
+{
+  return NS_OK;
+}

and what if m_mailDB is not set? Is that an error? Or worthy of an assertion?

+NS_IMETHODIMP nsParseMailMessageState::SetDBFolderStream(nsIOFileStream
*fileStream)
+{
+  if (m_mailDB)
+    m_mailDB->SetFolderStream(fileStream);
+  return NS_OK;
+}
+

Comment 17

[Navin Gupta]

SetHdrFlag/SetHdrReadFlag both return either true or false. So converting to 
nsresult can be NS_OK or NS_ERROR_FAILURE. We can later on change SetHdrFlag
to return nsresult rv, instead of boolean. 

Comment 18

[David :Bienvenu]

yes, but WHY would it return FALSE? Actually, looking at it, it looks like it
would return FALSE if the flag was already set, so I'm not sure bailing out with
NS_ERROR_FAILURE is the right thing to do at all.

Also, are we seeking to the end of the file before we write each chunk of data
to the file? If we don't, couldn't the following happen:?

We get a chunk of data in a big message, write it out the local store
The user marks some other message read before the next chunk of data comes,
moving the file stream ptr
The next big chunk of data comes, and gets written out to the middle of the
message we just marked read.

Comment 19

[Navin Gupta]

If we are not setting the flag then there is nothing to notify, so bailing out
is the correct thing to do. 

Once we have written an entire message to the Inbox, we flush it and then
publish the header so that would work ok. we are seeking to the end of file

m_outFileStream->seek(PR_SEEK_END, 0);

Comment 20

[Navin Gupta]

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

Comment 21

[Joel Nelson (don't send email)]

Change platform and OS to All/All.

Comment 22

[Navin Gupta]

There is a basic problem with this patch as pointed out by bienvenu. I 
will attach a new patch. 

Comment 23

[Navin Gupta]

Attachment: proposed fix

This patch uses the shared stream created for downloading pop3 messages, for 
reading/deleting messages (as long as we are downloading msgs). If mailDatabase
is not the owner of the stream it should not close the stream - I added a
boolean to take care of this. Also before writing a line to the Inbox we are
seeking to the end of file.

david, need review.

Comment 24

[David :Bienvenu]

Comment on attachment 76790 [details] [diff] [review]
proposed fix

r=bienvenu, I think this looks good. Now, we just need to test the heck out of
it before checking it in. This is a good canditate for test builds, especially
on the mac.

Comment 25

[Navin Gupta]

ok, I have been testing all common operations like reading/deleting, changing
flags - label, flagged etc on mac and win32 and everything seems ok. I
will test on linux also.

Comment 26

[Navin Gupta]

I tested this on linux and it worksfine. 

Comment 27

[Cavin Song]

Comment on attachment 76790 [details] [diff] [review]
proposed fix

r=cavin.

Comment 28

[Navin Gupta]

Comment on attachment 76790 [details] [diff] [review]
proposed fix

carrying r=bienvenu to sr

Comment 29

[David :Bienvenu]

actually, I'd feel much more comfortable if mscott or sspitzer looked at this too.

Comment 30

[Navin Gupta]

I did request mscott but looks like he is busy/forgotten.

Comment 31

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

some comments / questions:

1)

I don't see anyone ever using (or implementing) GetFolderStream()

so this should be:

+  attribute nsIOFileStream folderStream;

+  void setFolderStream(in nsIOFileStream aFileStream);

and all the references to GetFolderStream() should be removed, right?

2)
 
+NS_IMETHODIMP nsMailDatabase::SetFolderStream(nsIOFileStream *aFileStream)
+{
+  m_folderStream = aFileStream;
+  return NS_OK;
+}

Do you want to set m_ownFolderStream = PR_FALSE there if aFileStream is null?

3)

+NS_IMETHODIMP nsImapMailDatabase::SetFolderStream(nsIOFileStream *aFileStream)
+{
+  return NS_OK;
+}

Does this ever get called?  It doesn't look like it.  If it isn't supposed to be
called, but it needs to be implemented, I'd suggest this instead:

+NS_IMETHODIMP nsImapMailDatabase::SetFolderStream(nsIOFileStream *aFileStream)
+{
+  NS_ASSERTION(0, "Trying to get folderStream, not implemented");
+  return NS_ERROR_NOT_IMPLEMENTED;
+}

4)

several of the changes are in code wrapped with #ifndef XP_MAC

Does this problem still exist on the mac, or am I mis-reading the patch?

Comment 32

[David :Bienvenu]

I agree with the comments about the getter not being needed, etc.

Re the mac question, this problem definitely occurs on the mac - in fact, it's
worst on the mac. The #ifndef MAC code is from 4.x and was to try to prevent
similar problems on the mac. It might be that this approach will make it so we
don't need to have the #ifndef MAC code.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Please resolve/respond to sspitzer's comments (and preferably get an r/sr from
him) and then re-request an a= from drivers.

Comment 34

[Navin Gupta]

I don't see anyone ever using (or implementing) GetFolderStream()

>so this should be:

+  attribute nsIOFileStream folderStream;

+  void setFolderStream(in nsIOFileStream aFileStream);

>and all the references to GetFolderStream() should be removed, right?


This suggestion doesn't work, it doesn't link !!


>Do you want to set m_ownFolderStream = PR_FALSE there if aFileStream is null?

m_ownFolderStream is false by default so we don't need to set it again. 

Regarding the mac question it doesn't like two streams writing at the same time. 
Those ifndef MAC are still needed, they are used for batch copying/deleting msgs
This is because we disable counts notification for both src and destination 
folder and thereby we open the stream to both src folder and destination folder
in StartBatch() and for destination folder, the local folder code needs a stream 
to copy the msgs and mac doesn't like this. I see a bug right here, will 
elaborate and file separately.

A suggestion to bienvenu, don't do whitespace changes as part of fix which 
are not relevant to the patch, makes cvs log less useful. 

Comment 35

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

>>+  void setFolderStream(in nsIOFileStream aFileStream);
>>and all the references to GetFolderStream() should be removed, right?
> This suggestion doesn't work, it doesn't link !!

Are you saying it's not possible to change the attribute to just a setter, even
if you wanted to?  I'm sure it's possible.

>>Do you want to set m_ownFolderStream = PR_FALSE there if aFileStream is null?
>m_ownFolderStream is false by default so we don't need to set it again. 

are there possible code paths where the filestream is owned, and then an
external caller sets it to null using SetFileStream()?  If so, I'd think we'd
have the case where m_ownFolderStream was PR_TRUE but the stream was null, which
doesn't seem correct.

Comment 36

[David :Bienvenu]

Navin, if you're having trouble with cvs blame showing whitespace changes, it's
trivial to get the cvs blame for the previous rev before the whitespace changes
by changing the &rev= to &rev=1.xxx in the url. You can also do your own cvs
diff -uw if you're getting confused. 

You'd need to remove the attribute completely, and just have a setter method -
that's what Seth meant to say, I'd guess.

Comment 37

[David :Bienvenu]

you an also ask for the  ignore whitespace diff right off the diff page

Comment 38

[Navin Gupta]

Attachment: patch with minor changes

The ifndef MAC issue is not related to this bug.

Comment 39

[David :Bienvenu]

Comment on attachment 77553 [details] [diff] [review]
patch with minor changes

r=bienvenu

Comment 40

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

Comment on attachment 77553 [details] [diff] [review]
patch with minor changes

sr=sspitzer

Comment 41

[Asa Dotzler [:asa]]

Comment on attachment 77553 [details] [diff] [review]
patch with minor changes

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 42

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0.

Comment 43

[Navin Gupta]

fixed

Comment 44

[Navin Gupta]

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

Comment 45

[laurel]

Seems OK to me using apr8 commercial trunk build: win98, linux rh6.2
Used a large mailbox, created POP account for it and a filter to filter 50-75%
of the inbox messages to a folder (folder on same account, folder on Local
Folders). While inbox was downloading, I read and deleted many of the newest
messages as they were arriving on a consistent basis.  Both the inbox and the
filter destination folders had no problems reading through mail, no mismatches
or corrupted messages as far as I could see.

Comment 46

[laurel]

Looks OK, too, with apr9 commercial trunk on Mac OS 10.1
Marking verified.

Comment 47

[Daniel Veditz [:dveditz]]

So is this the "fix" that's screwed up my mailbox? I was doing mostly fine (i.e.
not normally seeing this bug) until I upgraded to this week's builds. Now my
filtered mail mostly comes out in fragments, with the headers not at all
matching the fragmentary contents. The raw contents of my filtered mail folders
show the data is corrupt. If I delete the .msf files I can at least see all the
mail correctly in the inbox (in addition to the fragments I have to go delete
from the other boxes).

Comment 48

[Navin Gupta]

dveditz, the problem you are describing has been fixed on trunk (in 4/11 build
)and is ready for branch checkin, see bug 136636

Comment 49

[laurel]

marking verified on trunk

Comment 50

[laurel]

marking verified for branch, have done some testing on current branch, and am
getting off the radar.  But most of all, the master bug for the fix is bug
136636. So any further comments will appear there.