90186 - POP .msf corruption when copying msgs to another newly created POP folder

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[esther]

Using windows build 2001-07-10 branch and 2001-07-09 trunk we corrupt a POP 
folder when copying a lot of msgs to a newly created POP folder.  Not sure how 
much of this is necessary, but this scenario is reproducible.  

1. Launch APP, create a New Profile (don't Activate)
2. Create a POP account (for those inside the firewall use 3qatest07 there are 
1,000 plus msgs in that account) Make sure you don't Get Msgs until you check 
the box to LEAVE MSGS on SERVER.  Also uncheck Biff and check messages on 
Startup.
3. Now, do Get Msg and let all the messages download.
4. Read the first 10 Msgs by clicking on them or down arrow key.
5. Create (2) new folders in this POP account, I created testfolder1 and 
testfolder2.
6. Select the 1st msg in the Inbox, holding the Shift key down to multiple 
select about 1/2 of the messages in the Inbox. 
7. Using menu item Message|Copy Message copy the selected msgs to testfolder1 
8. Waiting until the status and msg count finish, (the selection doesn't seem to 
unselect when done)click on a message in the Inbox to unselect all the messages.
9. Click on testfolder1 and select the first message.
10. Down arrow through the messages, before the 10th message you will see 
headers and displayed msgs not matching what you have selected in the Thread 
pane.  
Note: with the branch the first time I did this I crashed on the 6th msg, but 
when I relaunched I didn't crash but the headers are still not in sync.

Comment 1

[esther]

reassigning to navin

Comment 2

[esther]

nominating for nsbeta1, perceived data loss and basic mis-matched messages with 
headers.

Comment 3

[Navin Gupta]

investigating....

Comment 4

[Navin Gupta]

Deleting the .msf file seems to help, but the copy should not mess-up causing 
mismatch of headers. 

Comment 5

[Navin Gupta]

cc bienvenu, This could be a mail database problem 

Comment 6

[David :Bienvenu]

My guess is that it's not a database problem - my guess is that the copy code is
writing the wrong data into the database.

Comment 7

[scottputterman]

Adding nsbranch. If we can get a fix for the branch, I'll bring it to PDT.

Comment 8

[esther]

Tested on Mac and Linux, this is on all platforms.  Note, I logged a new bug 
about corrupting a mail msg during the copy. That particualr mail message 
happens to be in the 10 msgs selected in this scenario.  In the case of the 
specific mail message, it's OK when first viewed, then becomes an Unknown error 
when viewed after copy. If a message becomes corrupt during the copy (bug 
90217), could this be the reason the whole .msf file is out of sync as what 
happened in this bug?   

Comment 9

[Navin Gupta]

I have made some progress, the msgKey on a Copy changes and so it is showing
mismatch headers but interestingly I thought when you download the messages
the keys should be in the increasing order as the msg appear in the berkeley
mailbox but that it is not true here. Still investigating how this can be 
possible...

Comment 10

[Navin Gupta]

Either flush or tell is not working or message_size are all wrong. 

Comment 11

[David :Bienvenu]

why would it matter if message_size is all wrong? If message_size is wrong, that
tends to make messages get chopped off, or include other messages. But if tell
or flush doesn't work, then the offsets are wrong and we see the symptoms that I
understand we're seeing.

Comment 12

[Navin Gupta]

Got it finally. The Quicksort of the keys in the mailboxService is 
causing all the problems. This is because in LocalMailFolder we have the 
messageArray that does not corresspond to the keysArray once you have 
sorted it. 

Either we should sort and make sure the messageArray is also sorted or 
don't sort at all. I can attach the patch once we decide which way to go.

Comment 13

[David :Bienvenu]

cool. The msg key array should be sorted since it speeds up the move/copy - and
the message array should be build up from the already sorted msg key array.
Which msg array in the local mail folder are you talking about?

Comment 14

[Navin Gupta]

Attachment: proposed fix.

Comment 15

[Navin Gupta]

The fix is not to sort the keys so that the messageArray stays in sync with 
msgKeyArray. I am guessing we won't lose so much on performance. request for
review from david. 

Comment 16

[Navin Gupta]

oops didn't see david's comment, will attach a new patch. 

Comment 17

[Navin Gupta]

Attachment: proposed fix

Comment 18

[Navin Gupta]

Ok, I have added a function that will sort the messages before starting the 
copy. I didn't want to fetch the msgHdrs again from the key in this function
so I am appending the hdrs sorted based on key at the end and then deleting 
the previous unsorted hdrs from the messages array. I will add check to do
this sort only if needed ie number of hdrs > 1

requesting review. 

Comment 19

[David :Bienvenu]

Good find, but in your patch, you've removed the sorting of the msg keys, so the
msg key array and the hdr array will be out of sync again. Let me think if
there's a better way of sorting the msg hdr array.

Comment 20

[David :Bienvenu]

whoops, never mind, I see why you want to remove the sort in the message
service. It won't do anything, but it's dangerous to leave it in there.

However, I think the sorting code would be much cleaner if you did actually just
get the headers from the db after sorting the extracted key array. Getting
headers from db's is just a hash table lookup, and will actually be faster than
the loop you've written, I believe. Also, this code could be moved into
CopyMessagesTo, so that you could take advantage of the fact that it already has
code to extract the msg keys out of the message hdr array. Then, all you'd have
to do is clear the msg hdr array and build it up in a little look asking
mDatabase for the hdr for each key in the sorted key array.

Comment 21

[Navin Gupta]

Thanks for clarifying that getting the hdrs from the database would not be 
any costlier. will attach a new patch. 

Comment 22

[Navin Gupta]

Attachment: proposed fix, v2

Comment 23

[David :Bienvenu]

looks good, except this comment isn't valid anymore, is it?

 NS_IMETHODIMP
 nsMsgLocalMailFolder::CopyMessages(nsIMsgFolder* srcFolder, nsISupportsArray*
@@ -1674,6 +1699,7 @@
 
   // don't update the counts in the dest folder until it is all over
   EnableNotifications(allMessageCountNotifications, PR_FALSE);
+  //sort the keys

Comment 24

[Navin Gupta]

"sr-mscott. I was unable to add my review to the bug because bugzilla is 
blocking changes made from today's trunk bugzilla build.
http://bugzilla.mozilla.org/show_bug.cgi?id=90363
-Scott"

Comment 25

[scottputterman]

If you can get this checked into the trunk we can get this tested tomorrow.

Comment 26

[Navin Gupta]

fix checked on trunk

Comment 27

[Navin Gupta]

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

Comment 28

[esther]

Using Trunk build 2001-07-13 on win, mac and linux and the original scenario,
this is fixed.  trunkverified

Comment 29

[Oliver Klee]

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

Comment 30

[Navin Gupta]

shouldn't this one go on branch also ? why are we delaying ?

Comment 31

[selmer (gone)]

PDT+.  Please checkin on the *branch* tonite!

Please do any extra testing you can think of to ensure no new problems crop up.

Comment 32

[Navin Gupta]

fix checked on branch. 

Comment 33

[esther]

Using branch builds 2001-07-17 on win, mac and linux and the original scenario,
this is fixed. Verified.

Comment 34

[Oliver Klee]

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

Comment 35

[Dwayne]

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

Comment 36

[Navin Gupta]

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