466730 - file compact folders isn't compacting imap offline stores

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[David :Bienvenu]

this is really a backend bug, but I need to set the target milestones.

Comment 1

[David :Bienvenu]

I accidentally checked this in with an other patch - Standard8, can you look over this diff and see if it looks OK? The change is to make it so we compact the offline stores once the online expunges have finished; if the caller hasn't passed in an array of offline stores to compact, compact the same array as we were compacting online;

Comment 2

[David :Bienvenu]

I believe this is fixed - I meant to paste a url to the diff but somehow never did - http://hg.mozilla.org/comm-central/diff/735b67ea9ed6/mailnews/base/src/nsMsgFolderCompactor.cpp

Comment 3

[Mark Banner (:standard8)]

Yep this looks fine to me. r/sr=me.

Comment 4

[David :Bienvenu]

this needs to be backed out - it can crunch your local mailboxes. I'll back it out right now and post to the newsgroup.

Comment 5

[Nomis101]

I don't know, but is it possible that this has something to do with the issue I have now in 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081127 Shredder/3.0b1pre (tinderbox-build 1227783600)
If I double click on a new mail to open it, the opening window is now blank.

I doesn't have this issue in a build I've made after "Follow up to bug 456818". And there are not so much code changes after this.

Comment 6

[David :Bienvenu]

I believe the stand-alone message window issue is related to bug 456818, and nothing to do with this bug.

For this bug, it's fairly simple to fix, but I'm going to try to write some tests to catch this problem first. The issue is that the front end is asking to compact all offline stores, even for local folders.

Comment 7

[David :Bienvenu]

I don't think we're going to fix this for b1 - I'm still working on the unit tests. I'll fix this for b2 asap...

Comment 8

[David :Bienvenu]

Unit tests for folder compaction can't be done without reworking the compaction interfaces and code to notify listeners on completion. So this is going to be a bit involved.

Comment 9

[David :Bienvenu]

Attachment: proposed fix

I've changed the compaction interfaces to make them a bit clearer, documented them, and made sure that url listeners actually get called.

the change to bugmail11 is to add an x-mozilla-keywords header so that compacting doesn't change the size of the message, so our size calculations in the test come out right.

And I've added a test that makes sure that compaction results in a file the same size as the total of the existing messages, after deletion.

Comment 10

[[e-mail]]

No suite frontend changes?

Comment 11

[David :Bienvenu]

no, suite goes through the rdf command handler, as near as I could tell, so nsMsgFolderDataSource does the compaction. I'm not sure what suite's plan is re the new js folder pane. If it uses the new js folder pane, then the folder data source can go away...

Comment 12

[David :Bienvenu]

Attachment: un-bittrotted patch

previous patch was bit-rotted today - this patch fixes that, and removes some dump statements from the new test, otherwise unchanged.

We're going to need this patch if we're going to try to compact offline stores automatically for the user, so I'd like to get this patch moving again.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 356741 [details] [diff] [review]
un-bittrotted patch

>-/*  Use this for any object that wants to handle compacting folders */
>+/*  Use this for any object that wants to handle compacting folders.
>+ *  Currently, the folders themselves create this object.
>+ */

nit: as you're fixing it, I'd prefer:

/**
 * Use this...

>+   /**
>+   * Compact the given folder, or its offline store (imap/news only)
>+   *
>+   * @param aFolder        The folder to compact
>+   * @param aOfflineStore  Just compact the offline store?
>+   * @param aListener      Notified of completion, can be null.
>+   * @param aMsgWindow     Used for progress/status, can be null
>+   */

nit: need to shift the * one space across.

>+  void compactFolders(in nsIArray aArrayOfFoldersToCompact,
>+                  in nsIArray aOfflineFolderArray,
>+                  in nsIUrlListener aListener,
>+                  in nsIMsgWindow aMsgWindow);

nit: misaligned

>+  nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(m_folderArray,
>+                                                   m_folderIndex, &rv);

nit: alignment.

>+  if (NS_SUCCEEDED(rv) && folder)
>+    rv = Compact(folder, m_compactingOfflineFolders, m_listener, m_window);                    

nit: lots of whitespace on the end of this line.

>-  nsCOMPtr<nsISupportsArray> m_folderArray; // to store all the folders in case of compact all
>+  nsCOMPtr<nsIArray> m_folderArray; // folders we are compacting, if compacting multiple.

Yay! Its just reminded me: as a result of this patch, nsIMsgFolderCompactor.idl no long needs to include nsISupportsArray.idl

>-             for (PRUint32 i=0; i< cnt;i++)
>+             for (PRUint32 i = 0; i < cnt;i++)

nit: space after the second ; (two places)

>+  nsCOMPtr<nsIMsgFolderCompactor> folderCompactor;
>   folderCompactor = do_CreateInstance(NS_MSGOFFLINESTORECOMPACTOR_CONTRACTID, &rv);

This should be one statement.

>+    NS_ENSURE_SUCCESS(rv,rv);

nit: there's a lot of these where I'd prefer to have a space after the comma.

>+    if (cnt == 0 )
>+      return NotifyCompactCompleted();

nit: no space after 0.

>+  return folderCompactor->CompactFolders(folderArray,
>+                                      nsnull,
>+                                      aListener, aMsgWindow);

nit: alignment.

>diff --git a/mailnews/test/data/bugmail11 b/mailnews/test/data/bugmail11
>@@ -1,11 +1,12 @@ From - Mon Jun 02 19:00:00 2008
> From - Mon Jun 02 19:00:00 2008
> X-Mozilla-Status: 0001
> X-Mozilla-Status2: 00000000
>+X-Mozilla-Keys:                                                                                 

Is the whitespace on the line essential?

Comment 14

[[e-mail]]

Comment on attachment 356741 [details] [diff] [review]
un-bittrotted patch

>+      folder.compactAll(null, msgWindow, isImapFolder || folder.server.type == "news");
Don't local folders ignore that parameter anyway?

>   nsresult rv = NS_OK;
>   m_window = aMsgWindow;
>+  m_listener = aUrlListener;
>   if (aArrayOfFoldersToCompact)  
>-    m_folderArray =do_QueryInterface(aArrayOfFoldersToCompact, &rv);
>+    m_folderArray = aArrayOfFoldersToCompact;
>   else if (aOfflineFolderArray)
>   {
>-    m_folderArray = do_QueryInterface(aOfflineFolderArray, &rv);
>+    m_folderArray = aOfflineFolderArray;
>     m_compactingOfflineFolders = PR_TRUE;
>     aOfflineFolderArray = nsnull;
>   }
>   if (NS_FAILED(rv) || !m_folderArray)
>     return rv;
Nit: rv is now guaranteed to be NS_OK at this point.

>+void nsFolderCompactState::CompactCompleted(nsresult exitCode)
>+{
>+  if (m_listener)
>+    m_listener->OnStopRunningUrl(nsnull, NS_OK);
>+  ShowDoneStatus();
> }
exitCode unused?

>+    if (m_compactOfflineAlso)
>+    {
>+     m_compactingOfflineFolders = PR_TRUE;
>+     nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(m_folderArray,
>+                                                       m_folderIndex-1, &rv);
>+     if (NS_SUCCEEDED(rv) && folder)
>+       folder->CompactAllOfflineStores(this, m_window, m_offlineFolderArray);
>+    }
>+    else
>+    {
>+      CompactCompleted(NS_OK);
>+      return rv;
>+    }
Weird logic here. Better to write
if (!m_compactOfflineAlso)
{
  CompactCompleted(NS_OK);
  return rv;
}
m_compactingOfflineFolders = PR_TRUE;
etc.

>+     
Nit: whitespace

>+        rv = folder->CompactAll(nsnull, window, PR_FALSE);
Why PR_FALSE?

>+    PRUint32 cnt =0;
Nit: not enough whitespace!

>+      nsCOMPtr<nsISupports> supports = getter_AddRefs(allDescendents->ElementAt(i));
Nit: prefer dont_AddRef. Well, prefer fixing ListDescendents ;-)

Comment 15

[David :Bienvenu]

> >diff --git a/mailnews/test/data/bugmail11 b/mailnews/test/data/bugmail11
> >@@ -1,11 +1,12 @@ From - Mon Jun 02 19:00:00 2008
> > From - Mon Jun 02 19:00:00 2008
> > X-Mozilla-Status: 0001
> > X-Mozilla-Status2: 00000000
> >+X-Mozilla-Keys:                                                                                 
> 
> Is the whitespace on the line essential?

Yes, that's the format used for berkeley mailbox messages - the keywords replace/write over the blanks, when added to messages. I'd like to have the test simulate what the core code does. It's conceivable that the test would work w/o the blanks, and with just an empty header, but I think there's more value in making the test true.

Comment 16

[David :Bienvenu]

Attachment: address Standard8 and Neil's comments

carrying forward Standard8's r+, requesting sr. I've addressed all the comments, except for Neil's first comment - yes, local folders ignore the parameter, but I got into a huge amount of trouble before for passing the wrong parameter in and I don't want to repeat that.

Comment 17

[David :Bienvenu]

fix checked in

Comment 18

[David :Bienvenu]

Attachment: follow on fix

we shouldn't be using offline folder compactor to compact local folders - bad things can happen. The local mail folder compactor will turn around and ask the imap folder to compact offline stores, which will create the offline compactor - this is still really convoluted and should be cleaned up.

The second part is a typo while trying to clean up whitespace - I don't know why this compiles, but I think it may be causing the news crash...

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 357396 [details] [diff] [review]
follow on fix

>+++ b/mail/base/content/mail3PaneWindowCommands.js
>+++ b/mail/base/content/messageWindow.js
>+++ b/mailnews/base/src/nsMsgAccountManager.cpp

I'm assuming these aren't meant to be part of this patch

>-               nsCOMPtr <nsIMsgFolderCompactor> folderCompactor =  do_CreateInstance(NS_MSGOFFLINESTORECOMPACTOR_CONTRACTID, &rv);
>+               nsCOMPtr <nsIMsgFolderCompactor> folderCompactor =  do_CreateInstance(NS_MSGLOCALFOLDERCOMPACTOR_CONTRACTID, &rv);

nit: we can drop the space between nsCOMPtr and <, and only one space after the equals.

Given the problems this seems to be causing with the crashing I'll land it in a couple of minutes so that it gets in for tomorrow's nightlies.

Comment 20

[Mark Banner (:standard8)]

Pushed: http://hg.mozilla.org/comm-central/rev/3d63461d9075

Comment 21

[Wayne Mery (:wsmwk)]

file>compact folders works both imap and local  using
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090118 Shredder/3.0b2pre

Comment 22

[WADA:World Anti-bad-Duping Agency]

Tb trunk still didn't reduce offline-store file size(tested with Gmail IMAP) upon "Compact Folder".
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090126 Shredder/3.0b2pre
Re-opening.

(Steps to produce)
 1. Disable auto-sync. 
    Enable "offile use" of an Gmail IMAP folder (IDLE support=On)
 2. "Rebuild Index", "Download Now"
 3. Repeat step 2 several times. File size increases upon each "Download Now".
 4. Move a mail from the IMAP folder to Trash folder
 5. Compact folder => file size didn't change (with Tb 2, size reduces)
When Gmail IMAP, "flag \Delete" is equivallent to "flag \Deleted + Expunge", if new Gmail's feature provided at Gmail Labs is not enabled.
(after "flag \Deleted", the mail disappears automattically from the IMAP folder)
Will it affects on Tb's "compact folder" behaviour?

Comment 23

[David :Bienvenu]

wfm - I deleted 10 MB of messages from my (non-gmail) imap inbox, did a file | compact folders, and the offline store shrunk by 10 MB...I don't know if  you're seeing a gmail specific issue (which would merit a different bug, I think).

Comment 24

[David :Bienvenu]

also works on my gmail imap account...

Comment 25

[WADA:World Anti-bad-Duping Agency]

Thanks for additional testing. 
Scenario in Comment #22 is for re-creation test of Bug 463359(Tb 2 issue).
(repeated "Rebuild Index"+"Download Now" to force phenomenon of "infinite increase of offline-store file size" until Compact)
  - Bug 463359 Comment #9 : My result is opposite (Tb 2 reduced size)
  - Comment #22           : My result is opposite (Tb Trunk doesn't reduce size)
Possibly due to my special/intentional test scenario.
If I can find what is problem, I'll open separate bug.

Changing back to RESOLVED/FIXED.

Comment 26

[WADA:World Anti-bad-Duping Agency]

Phenomenon of Comment #22 seems to be same as Bug 467305 Comment #2(report for Tb 3.0b2), because IMAP log was very similar to log attached to Bug 467305 Comment #4.

Comment 27

[WADA:World Anti-bad-Duping Agency]

We opened next bugs, which occurs on Tb Trunk even after fix of this bug.
  Bug 487992 : for file size growth upon each rebuild-index
  Bug 495862 : for never shrinking file size by "Compact Folder(s)"

Comment 28

[WADA:World Anti-bad-Duping Agency]

Comment #22 was Gmail IMAP partcular phenomenon.
I couldn't observer phenomenon of comment #22 with [Gmail]/All Mail. It's probably because \Deleted flag is kept if [Gmail]/All Mail. (probablly same for [Gmail]/Trash and [Gmail]/Spam).
If Gmail IMAP folder(except above special folders in which \Deleted is kept), "store flag \Deleted" is equivallent to next.
  1. uid XXX store flag \Deleted
  2. expunge by other client before Tb executes fetch at step 3
  3. fetch => mail of UID=XXX doesn't exist

Comment 29

[WADA:World Anti-bad-Duping Agency]

I've opened Bug 499630 for comment #22 of Gmail IMAP.

Comment 30

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #21, Wayne Mery, on 2009-01-18)
> file>compact folders works both imap and local  using
> Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090118 Shredder/3.0b2pre
(In reply to comment #23, David :Bienvenu, on 2009-01-31)
> wfm - I deleted 10 MB of messages from my (non-gmail) imap inbox, did a file |
> compact folders, and the offline store shrunk by 10 MB...
(In reply to comment #24, David :Bienvenu, on 2009-01-31)
> also works on my gmail imap account...

As I wrote in Bug 495862 and Bug 499630, we can see phenomenon of "compact folder doesn't reduce file size of offline-store" which started to occur from 2008/11/12 build(Bug 495862) and from 2008/11/22 build(Bug 499630). These phenomenon can be observed even with 2009/1/18 build and some builds after it, and can be observed with recent trunk builds.

To Wayne Mery and David:

What is difference from your WORKSFORMEs?
Delete model? "Comact Folders" vs. "Compact Folder"? "Shift+Delete" vs. "real Delete(really Moved to Trash)"? "really Moved to Trash" vs. "No deleted mail(Bug Bug 495862)"?
(I can observe problem by "Compact FolderS" too in some tests...)
Note:
"really Moved to Trash" case can't be checked using Gmail IMAP after XLIST support, because special [Gmail]/Trash is always set as trash folder.

Comment 31

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #24)
> also works on my gmail imap account...

I could see "it works with Gmail IMAP" at last with Tb 2009/6/30 build, using [Gmail]/Trash which supports/keeps \Deleted flag, using delete model of "Remove it immediately", using "Delete" instead of "Shift+Delete", using both of File/"Compact Folders" and "Compact Folder" of Folder Properties.
See Bug 499630 Comment #14 and Bug 499630 Comment #16 for it. 
Bug 499630 was Gmail IMAP particular issue, although Bug 499630 is possibly applicable to "Expunge by other client" case, and although "Shift+Delete" possibly has relation to some issues observed in Bug 499630.