136784 - Problems while compacting local folders (only in certain cases)

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Navin Gupta]

On linux : we lose messages
On win2000 : we leave the nstmp file around.

Comment 1

[Navin Gupta]

I am working on a fix, will attach a patch soon.

Comment 2

[Navin Gupta]

Attachment: proposed fix

This patch fixes two problems

1. It prevents issuing a compact if it finds a db and finds that the summary is

is invalid. Such can happen if the user selects a folder or the folder is
selected by default and the use right clicks to issue a compact or does File |
Compact Folders. 

2. For local folders we try to get a db without parse and if we don't get a db
because summary is not in sync then we issue a parseFolder and listen for 
completion. In nsMsgMailboxParser::OnStartRequest we were opening the db
directly
that was not setting m_folder for the db - had to do OpenFolderDB. This is
necessary because later when parsing is over and we try to compact the folder
we try to get the folder from header. The header tries to get it from the db.
So I added a SetFolder method on nsMsgMailboxParser and made it private so 
that no other class can use it. I made it a weakPtr, to prevent any leaks.

I also cleaned up nsMsgFolderCompactor code so I tested all different ways to 
compact

Tests:

1. Compact This Folder while building summary file and w/o building summary
file
2. File | Compact Folders while building summary file and w/o building summary
file
3. Compacting local and offline stores (both imap and news offline store). 

All these test run as expected (it works). 

david, can you r=?, thx

Comment 3

[Navin Gupta]

See comment #2 for detailed description of the problems. 

Comment 4

[David :Bienvenu]

What happens if I do a compact all and none of the summary files are up to date?
It used to reparse all the db's to create the summary files, and then compact
all the folders. Does it still do that?

Comment 5

[Navin Gupta]

It still does that - compacts all folders. Parses one, compacts it and then next
and so on. I have just tested it. This is working as expected after fixing 
problem #2.

Comment 6

[David :Bienvenu]

I guess I don't quite understand how that works:

+       PRBool valid;  
+       rv = db->GetSummaryValid(&valid); 
+       if (!valid) //we are probably parsing the folder because we selected it.
+       {
+         folder->NotifyCompactCompleted();
+         if (m_compactAll)
+           return CompactNextFolder();
+         else
+           return NS_OK;
+       }

Doesn't this code make it so we skip the folders with invalid db's? What am I
missing?

Comment 7

[Navin Gupta]

If you look at the code above this code we try to getDBWOReparse. that call
returns us a null db with rv = out_of_date if the db is out of date so we issue
a ParseFolder. Basically these lines

     rv=localFolder->GetDatabaseWOReparse(getter_AddRefs(db));
     if (NS_FAILED(rv) || !db)
     {
       if (rv == NS_MSG_ERROR_FOLDER_SUMMARY_MISSING || rv ==
NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE)
       {
         m_folder =folder;  //will be used to compact
         m_parsingFolder = PR_TRUE;
         rv = localFolder->ParseFolder(m_window, this);
       }

Only if we get a db and if it is not valid - meaning someone has called
GetDatabase and issued a parse in case of local folders, so skip this one. 
We will do it next time. 

Comment 8

[David :Bienvenu]

what does "next time" mean? The next time the user does a compact all? Or
somehow we'll go back to this folder while we're compacting all?

Comment 9

[Navin Gupta]

"next time" - next time user does Compact all | Compact this folder | AutoCompact.

Comment 10

[David :Bienvenu]

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

OK, r=bienvenu. I think the confusing is coming because we're using different
definitions of "work" - you're using it in the sense of not crashing or
corrupting data, and I meant it in the sense of having the user's command
succeed (also w/o crashing or corrupting data). IIUC, this patch makes it not
crash or corrupt data, which is an improvement, but I think we should have a
bug filed for making it work the first time, if we don't already have a bug
like that.

Comment 11

[Navin Gupta]

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

Comment 12

[Scott MacGregor]

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

Navin, can you list the spin off bug you filed to make it the command work as
opposed to just avoiding the crash & corruption?


sr=mscott

Comment 13

[Navin Gupta]

bug 137210 takes care of that. 

Comment 14

[Navin Gupta]

fixed

Comment 15

[scottputterman]

How likely are either case 1 or 2 to happen?

Comment 16

[Navin Gupta]

Case 1 could happen if someone issues a compact for a folder whose summary file
is being built. This leads to dataloss. 

>On linux : we lose messages
>On win2000 : we leave the nstmp file around.


Case 2 was preventing compacting of local stores that did not have up-to-date
summary files. 

Comment 17

[Sheela Ravindran]

cc-ing gchan since navin mentions compaction to be tested on imap and news
offline store.

Comment 18

[grylchan]

tested compacation as it relates to offline mail/news in
Navin's test 3 in comment 2.
Using commercial trunk:
2002-05-21-08-trunk/ nt 4.0
2002-05-21-07-trunk linux 2.2
2002-05-21-08-trunk/ mac 10.1.4

AFter downloading mail or news and then deleting mail/canceling a news mesg,
I get the prompt to compact folders when I go back online and click on
Local Folders account. Verified the offline store file (ex inbox or mcom.test)
does indeed become smaller after the prompt to compact folders appears.

Comment 19

[Sheela Ravindran]

Testing this on win98 using 05-21-08 branch build. Here is what I see.
I have pref to auto compact folders turned on.

1. I delete messages from inbox on a pop account.  Restart application
Prompted with auto compact dlg. Ok to compact. Login prompt.  Login 
successfully. Compaction was done fine.  
As a result:  I was not able to delete any messages from any one of my local 
folders.

2.Next test:  Deleted messsages from inbox.  Close application.  Open inbox.msf 
and save it again.  Restart application
Result: Was never prompted for auto compact.  I logged into the account. Then 
did a manual compact folders. Manual compact seemed to work.

3.Next test:  Deleted messages from inbox. Close application.
Remove msf file for inbox and two other local folders(from which I was not able 
to delete messages at all after first compaction test)
Restart application.  Login to the account.  No prompt for auto compact.  Nor 
the compaction did go through.  It build the summary file for inbox since the 
inbox was auto selected. I did a compact folders from file menu.  Compacted 
inbox folder.  But lost messages on one of the local folders.  I see the 
messages are there but not visible in the thread pane. 

Scenarios stated by Navin is not working for me.  

Comment 20

[Sheela Ravindran]

Ignore my last comments. I happened to test all these scenarios on the branch 
build.  I will check the same on the trunk build.  Sorry for the confusion.  

Comment 21

[Sheela Ravindran]

Ok tested all the scenarios listed in my comments #19.
This time yes i did check them on the trunk build from 2002-05-21-08 on win98, 
linux RH6.2 and Mac os x.  Compaction worked successfully without any problems 
that I have listed while testing it on branch build.

Marking this bug as verified.  

Comment 22

[Navin Gupta]

I am able to apply trunk patch on branch but I have not been able to test it. 
I don't see any dependicies from other bug fixes on trunk. It should work like
trunk. 

Comment 23

[Jaime Rodriguez, Jr.]

does this patch impact only compacting local folders only? what could this
break, if it was chekced in?

Comment 24

[Navin Gupta]

We can take this for branch. It is pretty safe. I haven't seen any regressions
on trunk.

Comment 25

[David :Bienvenu]

this patch only affects compacting local folders (in theory, it could affect
compacting offline local stores as well, though the problems it fixes don't
arise when compacting offline local stores). It will not affect compacting of
online imap folders. Since this results in dataloss, I think we should try to
get this in the branch.

Comment 26

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on adt's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls checkin this in asap, the add the "fixed1.0.1" keyword.

Comment 27

[Scott MacGregor]

adding Jaime's plus for real. 

Comment 28

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 29

[Navin Gupta]

fixed on branch

Comment 30

[Sheela Ravindran]

Verified on commercial branch build: 06-25-08 on win98, linux
Compaction on local folders and pop folder(including inbox and user defined 
folder)
Did the following test: auto compact, Compact folders from menu, compact this 
folder, compact folder with invalid summary file, compact folder while building 
summary files.

Still needs verification on mac os x... 

Comment 31

[Sheela Ravindran]

Verified on mac os x. Commercial branch build: 06-25-13.
Verified the scenarios specific to this bug as mentioned above.  This 
verification does not cover compacting folders that are imported from other 
clients and some other situations the user might run into. 
The folders I compacted did not have any subfolders.  Also did not try the 
scenarios on folders which has illegal characters(like: /,\,%,&,!) etc. I tested 
with inbox and one of the local folders on a pop account.

changing keywords to verified1.0.1.  

Comment 32

[Navin Gupta]

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