17204 - [FEATURE] mail.purge_threshhold "Automatically compact folders..."

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[laurel]

This is a bug to track implementation of the feature to automatically compact
folders upon a user-specified threshhold.

Currently the associated prefs mail.prompt_purge_threshhold and
mail.purge_threshhold do migrate with the proper values to the prefs.js file.

The Prefs UI doesn't currently work to write these changes to the prefs.js file
(covered under bug #17201).

Comment 1

[jefft]

This sounds like a fair amount of works. 

Comment 2

[Scott MacGregor]

Hey Jeff, since we are past feature freeze and this isn't on the beta2 list,
should we move this off to a future release? either that or we need to nominate
it for beta2.

Comment 3

[jefft]

Future release, M20....

Comment 4

[scottputterman]

reassigning jefft's bugs to naving

Comment 5

[David :Bienvenu]

Nominating for beta - also need this for cleaning up offline storage as well.
Are we planning on doing this post beta1?

Comment 6

[scottputterman]

Yeah, we should look into this.  

Comment 7

[Peter Lairo]

SPAM: please implement this before all our hard drives explode ;)

Comment 8

[scottputterman]

moving to 0.9.3.  We should remove the UI for this until this gets implemented.
I will file a bug.

Comment 9

[scottputterman]

bug 83391 created to track UI removal.

Comment 10

[Navin Gupta]

scott, I was working on this and will have a fix soon. 

Comment 11

[scottputterman]

ok, but if it looks like it's going to take a long time we should put this back
in 0.9.3

Comment 12

[Navin Gupta]

Attachment: proposed fix, v1

Comment 13

[Navin Gupta]

The fix is to check if the pref is checked off and if it is checked, then check 
the number of expunged bytes at intervals of 1 hr. If the total expunged 
bytes exceed the limit, compact all folders. 

cc david for review. 

Comment 14

[David :Bienvenu]

initial comments:

need newline at end of local/resources/locale/en-US/localMsgs.properties

what's 36000000000000? Need a comment or a #define

This doesn't do anything for compaction of offline news or imap stores (which is
going to be a bit of work that I'll need to talk to you about).

Comment 15

[David :Bienvenu]

Attachment: patch to make offline stores keep track of expunged bytes

Comment 16

[David :Bienvenu]

The attached patch makes it so that imap and news db's will keep track of
expunged bytes as the total byte size of offline message bodies whose msg hdrs
were deleted. This will make expungedBytes for news and imap folders correct,
except that existing imap and news db's will have bogus counts because up til
now, they've been counting every msg hdr delete as expunged bytes. It would be
good to fix this, but not absolutely neccessary since it will just cause an
initial compaction of offline stores.

There are more details to deal with for offline. CompactAll will need to compact
offline stores, or we'll need to add a call to compact offline stores as well.
The method to compact an offline store is CompactOfflineStore(). If this method
doesn't set the expunged bytes to 0 for a folder, I'll need to fix that. Cc'ing
Seth so we can start getting reviews for these patches.

Comment 17

[scottputterman]

How much work do you think is left on this?  David mentioned that doing this for
offline is going to be a bit of work.  If it's a lot of work I think we should
wait on this (or just get the pop/local folders part in)

Comment 18

[David :Bienvenu]

I don't think it's very much work - basically, instead of iterating over only
local folders, we need to iterate over imap and news folders if any of them are
configured for offline use.

Comment 19

[David :Bienvenu]

Attachment: patch to fix offline folder compaction

Comment 20

[Navin Gupta]

Attachment: proposed fix, v2, combined w/ david's last patch

Comment 21

[Navin Gupta]

the fix is that I have added a function AutoCompact() to nsMsgDBFolder which 
gets called when you UpdateFolder , local/news/imap. For imap and news it gets
called only when we are offline. So it prompts the user if he/she wants to 
compact all folders if they exceed limit. For offline just check if the folder
is for offline use. Start compaction if the user says OK! 

Comment 22

[Navin Gupta]

I am still trying to make it more modular and will attach a new patch. 

Comment 23

[Navin Gupta]

Attachment: proposed fix

Comment 24

[Navin Gupta]

So this new patch makes the folderCompactor more modular. request for review 
from david and seth. 

Comment 25

[scottputterman]

adding PDT+, but we need to really test this before going in and make sure that
we don't know of any side effects. 

Comment 26

[Navin Gupta]

David, could you review the last patch. I have tested your 2nd patch and
compact offline works. r=naving on both your patches. 

Comment 27

[David :Bienvenu]

are you intending that the purge threshhold should be global or per-server? I'm
not sure what the UI is looking like at this moment. It seems like you've made
it pretty easy to make it per-server, so maybe we should just go that way, in
which case you should use the macros for server prefs, and make sure the UI
people (Diane and Mohan) get and set those same prefs. Also, checking uri's for
"nntp" or "imap" is not the cleanest way of doing this - I think bhuvan was
going to add an attribute to the server which indicated whether it had offline
support, you should check into that or ask him. Sorry for forgetting to review
this - ping me if I don't get to a review in 24 hours, because if I haven't done
it in 24 hours, I've probably forgotten.

Comment 28

[Navin Gupta]

Purge_threshold is global right now 

#define PREF_MAIL_PROMPT_PURGE_THRESHOLD "mail.prompt_purge_threshhold"
#define PREF_MAIL_PURGE_THRESHOLD "mail.purge_threshhold"

but it should be on per-server basis, because the account manager wizard allows
you to set the boolean and limit on a per-server basis. 

Comment 29

[Navin Gupta]

It looks like these profiles will be on a global level (Edit | Preferences), 
dianesun/mohanb could you confirm ?

Comment 30

[Diane Sun]

Yes,  they are global level preferences.

Comment 31

[Navin Gupta]

Could we have them as server-prefs. I think it is more logical and compact All 
folders also works on a per-server basis. 

Comment 32

[scottputterman]

moving to 0.9.3

Comment 33

[Peter Lairo]

Please make sure that the hourly autocompacting of on-/offline imap folders
doesn't remove messages that have only been *marked as deleted*. 

It would suck if someone marked a msg as deleted and the autocompact kicked-in
and permanently removed the msg, and then the user decides he wanted the file
after all :(

Comment 34

[Navin Gupta]

Attachment: proposed fix, v3

Comment 35

[Navin Gupta]

I have made it such that we check for all the accts and see if the total
expunged bytes > purge_threshold (global pref), then we compact all local 
folders and also once the local folders are done, compact offline folders. On 
the way, found some problems so fixed those problems (like m_keyArray was not 
being cleared, m_messageUri was not being reset between compacts of folders) and 
it works for both local and offline. requesting reviews from david and seth.

Comment 36

[Peter Lairo]

Navin, will your fix delete msgs that were "marked as deleted"? If so, your fix
is not finished.

Comment 37

[David :Bienvenu]

Peter, aren't you talking about imap messages/folders? This is only for local
folders, isn't it, Navin?

Comment 38

[David :Bienvenu]

r=bienvenu

Comment 39

[Peter Lairo]

Yes, I am talking about IMAP folders - whose messages that are "marked as
deleted" ahould NOT be purged.

bienvenu, did you confirm with Navin that the patch doesn't BREAK this, before
you r='ed?

Comment 40

[David :Bienvenu]

Peter, I don't see any code that will issue an online compact of imap folders.
This code is to compact the offline store. Do you have any evidence to back up
your claim that this breaks something, or are you just idly speculating?

Comment 41

[Peter Lairo]

... just "idly" speculating. Thank you for explicitly confirming that IMAP
folders are unaffected.

Comment 42

[Navin Gupta]

It will not affect your online imap stuff. 

Comment 43

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

1) interCaps

void StartCompactingAll()

should be

void startCompactingAll()

2) extra space in autoCompactAllFolders property

"threshold ?" should be "threshhold?"

3) can m_size ever be 0?

double check that, since if ever is zero, you'll crash.

4)  kStringBundleServiceCID

+static NS_DEFINE_CID(kStringBundleServiceCID, NS_STRINGBUNDLESERVICE_CID);

use the string bundle contract id instead of defining this static CID

5)  if promptPurgeThreshold and purgeThreshold are global prefs, don't add them
to the nsIMsgIncomingServer
interface.

you should remove this code:

+  readonly attribute boolean promptPurgeThreshold; 
+  readonly attribute long purgeThreshold;

+nsMsgIncomingServer::GetPromptPurgeThreshold(PRBool *aPrompt)
+{
...
+NS_IMETHODIMP
+nsMsgIncomingServer::GetPurgeThreshold(PRInt32 *aThreshold)

and fix this code to use prefs:

+       PRBool prompt;
+       server->GetPromptPurgeThreshold(&prompt);
+       PRInt32 purgeThreshold;
+       server->GetPurgeThreshold(&purgeThreshold);


is there UI for this?  from the code, it looks like it should be under the "Edit
| Preferences | Offline & Diskspace" panel, and would be global (a check box for
the bool pref, a text field for the threshold.)

6)  AutoCompact(aWindow);

you never check the return value of AutoCompact() (it returns a nsresult)

Comment 44

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

also, where'd you get that prompt string from?  if you made it up (and didn't
steal it from 4.x) you should run it by [e-mail] (tech writer) or robinf

Comment 45

[Navin Gupta]

I have made changes for 1, 2 and 4. 3 is already taken care of because we 
check m_size for > 0 in StartCompacting(). 5 is a global pref and i have 
kept in the server because later on we might change it to be a server pref. 
for 6 the return value is not important. 

Comment 46

[Navin Gupta]

jatin, do you think this is correct text 

Do you wish to compact all local and offline folders because the total wasted 
space in all accounts exceeds the purge threshhold ?

Comment 47

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

> 5 is a global pref and i have kept in the server because later on we might 
> change it to be a server pref. 

that seems like a bad argument to me.  if later we make it a server specific
pref, we should change it then.  all you've done is add more code when we don't
need it.  the rest of your code goes through all servers and adds up the total
bytes (meaning the prefs are global, not per server)

your entire fix should be consistent.

it really depends on the UI.  it seems like the UI for this is (or is going to
be) global.

the spec doesn't seem to cover this (see
http://www.mozilla.org/mailnews/specs/prefs/Preferences.html)

jglick, can you confirm this will be a global, and not per server thing?

Comment 48

[David :Bienvenu]

that's my fault about leaving the code per-server - Seth convinced me we should
just make it consistent, and the pref is global now, so the code should be global.

Comment 49

[Navin Gupta]

Attachment: proposed fix, v4

Comment 50

[Navin Gupta]

The patch size has increased because I have combined david's patch with my 
patch. seth, please review it again. 

Comment 51

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

1)

don't use static comptrs.  (see bug #85225, for example, on how this can cause
crashers)

static nsCOMPtr<nsISupportsArray> gOfflineFolderArray;

see http://www.mozilla.org/hacking/portable-cpp.html#dont_use_static_constructors

according to alecf:  "don't use static comptrs because the constructors might
not get called, plus it varies from platform to platform when the constructors
AND destructors will be called."

2)  use do_getService() and contract ids, instead of NS_WITH_SERVICE() and k*CID

NS_WITH_SERVICE(nsIMsgAccountManager, accountMgr, kMsgAccountManagerCID, &rv); 

3)

purgeThreshold*1000

shouldn't that be *1024 (it's kb, right?)

4)

+             PRUnichar *alertString;
+            
bundle->GetStringFromName(NS_ConvertASCIItoUCS2("autoCompactAllFolders").GetUnicode(),
+                            &alertString);

you leak alertString, and you don't need to use NS_Convert*().

do something like this:

nsXPIDLString alertString
bundle->GetStringFromName(NS_LITERAL_STRING("autoCompactAllFolders").get(),getter_Copies(alertString));
...
dialog->Confirm(nsnull, alertString.get(), &okToCompact);

5)

you still aren't checking the return result of AutoCompact();

Comment 52

[Navin Gupta]

Ok, so i will not use static nsCOMPtrs but instead i will pass the 
offlineFolderArray. All other changes also done. patch coming up...

Comment 53

[Navin Gupta]

Attachment: proposed fix, v5

Comment 54

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

you really want bienvenu to do the final review, as he understand this backend 
stuff more than I do.  (I can do the sr, though.)

questions:

1)  can you add a comment explaining what supportsArray (poorly named) is for?
maybe change the name so that a comment isn't necessary.  (aOfflineFolderArray 
is an array of all offline folders, right?)

void startCompactingAll(in nsISupportsArray supportsArray, in nsIMsgWindow 
aMsgWindow, in boolean compactOfflineAlso, in nsISupportsArray 
aOfflineFolderArray);

2)  casting to (void) and not checking rv?

+    (void)msgHdr->GetOfflineMessageSize(&size);
+    (void)msgHdr->GetMessageSize(&size);
+    (void)msgHdr->GetOfflineMessageSize(&size);

do we really not care about the return value?  if not, that implies we are 
expecting error in some cases and we want to proceed anyways.  if that is the 
case (I'm not sure if it is), you should add a comment.  

if error shouldn't happen, we should do something like:

rv = msgHdr->GetFoo()
NS_ENSURE_SUCCESS(rv,rv);

Comment 55

[Navin Gupta]

For 1) isn't the variable name aOfflineFolderArray self-explanatory. 
For 2) those changes are from david's patch. 

Comment 56

[David :Bienvenu]

Seth is complaining about this line:
NS_IMETHODIMP nsFolderCompactState::StartCompactingAll(nsISupportsArray
*aSupportsArray

and was hoping you'd replace aSupportsArray with something like
aArrayOfFoldersToCompact or something like that.

Otherwise, it looks OK to me. r=bienvenu

Comment 57

[Navin Gupta]

fix checked in. Changed supportsArray to aArrayOfOfflineFoldersToCompact before
checking in. 

Comment 58

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

thanks for change it to aArrayOfOfflineFoldersToCompact

I gave a sr=sspitzer to naving over aim.

Comment 59

[Navin Gupta]

per esther reassigning to sheela

Comment 60

[Sheela Ravindran]

I spoke to esther as to how this bug needs to be verified. We understand that 
this fix is for the local and pop folders compaction. By checking the 
preference in offline and diskspace I should be prompted to compact local 
folders if it exceeds the size limit.  Here are some questions which I was not 
too clear on.

-Do we need to verify this bug both while online and offline for local folders? 
-Does this bug fix the time limit? Do we have any set time limit when the dlg 
has to appear if the folders need to be compacted again in the same session.  
-For example: When I select the pop account which needs to compact as per the 
preference and I click ok to the dlg and the folders are compacted. In the same 
session when I delete more messages  should I expect to be prompted with the dlg 
again? Does the compaction happen in the background without the dlg appearing?  
Or is it only once per session? 
-Should be this be tested by migrating a pop account from 4.x and checking the 
pref and size are migrated to 6.x?
-Does this involve any regression checks to be done with imap and news with 
offline?

Comment 61

[Peter Lairo]

Attempting to answer Sheela's questions:
1. Compaction should occurr both while on- and offline
2. There should be no time limit. If a folder needs to be compacted again, then
the user should be informed (that's what he set in his prefs). This shouldn't
happen too often and therefore a consisten behaviour is preferable (i.e.,
compact when threshold is exceeded).
3. A dlg and subsequent compaction should occurr every time it is needed. See #2
above.
4. Testing against 4.x: good idea.
5. Definetely checks are needed to make sure that IMAP is NOT compacted ("marked
as deleted"). For newsgoups: How are those "disk space" settings triggered (same
as this compaction? on load? etc.)?

Comment 62

[Sheela Ravindran]

As per my conversation with navin this bug does not matter if it online or 
offline
The compaction is triggered if it is required after 1 hr in the same session. 

verified this using 2001-09-12-05 trunk build on win98
still need to verify on mac and linux

Comment 63

[Sheela Ravindran]

verified on linux, mac- 2001-09-17-05 branch build. Fix was checked in july 
before the 0.9.4 branch so I guess this should be ok. 

Comment 64

[Sheela Ravindran]

Forgot to mention checked the profile migration too when verifying this bug. 

Comment 65

[Peter Lairo]

If compaction only occurs after an hour; what about the many people that are
regularly online for less than an hour? 

Does the compaction also occurr at the *beginning* of a session?

If not, should we be reminding the user somehow that a compaction is overdue?

Comment 66

[Navin Gupta]

Compaction occurs at the beginning of the session if there is any room and then
at intervals of one hr.