Problems while compacting local folders (only in certain cases)

VERIFIED FIXED in mozilla1.0.1

Status

MailNews Core
Backend
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: Navin Gupta, Assigned: Navin Gupta)

Tracking

({dataloss})

Trunk
mozilla1.0.1
x86
Windows NT
dataloss

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM] [ETA 06/21])

Attachments

(1 attachment)

15.64 KB, patch
Bienvenu
: review+
Scott MacGregor
: superreview+
Judson Valeski
: approval+
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
On linux : we lose messages
On win2000 : we leave the nstmp file around.
(Assignee)

Comment 1

16 years ago
I am working on a fix, will attach a patch soon.
Status: NEW → ASSIGNED
Keywords: dataloss, nsbeta1
(Assignee)

Comment 2

16 years ago
Created attachment 78806 [details] [diff] [review]
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
(Assignee)

Comment 3

16 years ago
See comment #2 for detailed description of the problems. 
Summary: Compacting while building summary file results in msgs getting lost → Problems while compacting local folders

Comment 4

16 years ago
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?
(Assignee)

Comment 5

16 years ago
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.
(Assignee)

Updated

16 years ago
Summary: Problems while compacting local folders → Problems while compacting local folders (only in certain cases)

Comment 6

16 years ago
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?
(Assignee)

Comment 7

16 years ago
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

16 years ago
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?
(Assignee)

Comment 9

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

Comment 10

16 years ago
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.
Attachment #78806 - Flags: review+
(Assignee)

Updated

16 years ago
Blocks: 137206
(Assignee)

Updated

16 years ago
No longer blocks: 137206
(Assignee)

Comment 11

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

Updated

16 years ago
QA Contact: gayatri → sheelar

Comment 12

16 years ago
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
Attachment #78806 - Flags: superreview+
(Assignee)

Comment 13

16 years ago
bug 137210 takes care of that. 
(Assignee)

Comment 14

16 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 15

16 years ago
How likely are either case 1 or 2 to happen?
(Assignee)

Comment 16

16 years ago
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

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

Comment 18

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
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.  
Status: RESOLVED → VERIFIED
(Assignee)

Comment 22

16 years ago
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. 
(Assignee)

Updated

16 years ago
Keywords: adt1.0.1

Comment 23

16 years ago
does this patch impact only compacting local folders only? what could this
break, if it was chekced in?
Whiteboard: [adt2 RTM]
(Assignee)

Comment 24

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

Comment 25

16 years ago
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

16 years ago
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.
Blocks: 143047
Keywords: nsbeta1 → approval, mozilla1.0.1, nsbeta1+
Whiteboard: [adt2 RTM] → [adt2 RTM] [ETA 06/21]
Target Milestone: --- → mozilla1.0.1

Comment 27

16 years ago
adding Jaime's plus for real. 
Keywords: adt1.0.1 → adt1.0.1+

Updated

16 years ago
Attachment #78806 - Flags: approval+

Comment 28

16 years ago
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
(Assignee)

Comment 29

16 years ago
fixed on branch
Keywords: approval, mozilla1.0.1 → fixed1.0.1

Comment 30

16 years ago
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

16 years ago
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.  
Keywords: fixed1.0.1 → verified1.0.1
(Assignee)

Comment 32

16 years ago
*** Bug 146414 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.