Bugzilla

Comment 9

•

11 years ago

Compact in IMAP consists of "request expunge" and "removal of unused mail data from offline-stor file".
So, in berkleystore/IMAP/Offline-Use=Off, "removal of unused mail data from offline-stor file" is job of Compact.
Culprit is "removal of Compact from maildir even though IMAP", isn't it?

Summary: deleting or moving messages doesn't delete messages from the filesystem when using maildir (lite) → deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (no one can delete files under "cur" used as offline-store file when maildirstore, because Compact menu is removed even though IMAP)

Comment 10

•

11 years ago

"Compact" in maidirstore/IMAP/Offline-Use=On may be;
- Expunge menu to issue EXPUGE command.
  Expunge in conext menu of message if "selective expunge" will be
  suported, in addition to "per mail Undelete".
  (per mail re-fetch header and body may be needed)
- Automatic removel of associated file when "Expunged" is detected.

Because msDBHdr for "Flaged as \Deleted" mail is removed from Msgdatabase when "Move to trash" model and "Remove immediately" model, relation between UID(currently messageKey is UID, perhaps) and "file name under cur"(storeToken) is lost by "store +Flag \Deleted".
So, new mechinis to keep relation between UID and filename is needed.
- Mbox/cur/nnnnnn.UID (if gmail IMAP, Mbox/cur/nnnnnn.(X-GM-MSGID)
- Mbox/UID directory in addition to cur and tmp.
           filename=UID,
           file content=storeToken value of mail with \Deleted flag
  This is similar to popstate.dat in POP3.
If relation is kept at somewhere consistently, file under Mbox/cur can be deleted when "Expunged" is detected.

However, I think mechanism of "clean up of garbage" is needed in case of Mbox.msf corruption or intensional deletion of Mbox.msf by user.

Rebuild Index, Compact, and Force Re-Fetch for IMAP, in maildirstore.
- Repair Folder :
   - local folder :
       Re-creation of Mbox.msf content from existent Mbox/cur files,
       with reparsing message headers.
   - IMAP/Offline-Use=On :
       Re-creation of Mbox.msf content from existent Mbox/cur files,
       with reparsing message headers.
- Repair Folder :
  - local folder :
      Re-creation of Mbox.msf content from existent Mbox/cur files,
      with reparsing message headers.
  - IMAP/Offline-Use=On :
      Re-creation of Mbox.msf content from existent Mbox/cur files,
      with reparsing message headers.
      (similar to Don't RETRIEVE if states of UIDL is "k".
- Compact Folder :
  If IMAP, issue EXPUGE command.
  Based on Mbox.msf content, clean up Mbox/cur.
- Force Refetch in IMAP(New. per account, per folder, per mail)
  Discard current data relevant to an UID, fetch header, and fetch
  message body if required.

Comment 11

•

11 years ago

supportsCompaction=false was set in nsIMsgPluggableStore of maidirstore/IMAP/Offline-Use=On folder.
Dump of property of msgStore.
> msgFolder.msgStore : [xpconnect wrapped nsIMsgPluggableStore]
>  msgFolder.msgStore / Property
>    supportsCompaction = false
As for Compact, problem was "Compact is completly killed in maildirstore even though IMAP", instead of "Compact menu is killed or hidden".

"Delete" case and "Move" and is slightly different, although "Mbox/cur/nnnn is not deleted" is same.

In any case, MoveSourceFolder==maildirstore/IMAP/Offline-Use=On folder. 
(a-1) Delete, IMAP delete model : Move to Tash, Remove immediately
    "uid xx store *Flags \Deleted" is normally issued.
    msDBHdr is removed from Msgdatabase.
    However, Mbox/cur/nnnn is not deleted.
(a-2) Delete, IMAP delete model : Just mark it as deleted.
    "uid xx store *Flags \Deleted" is normally issued.
    msDBHdr is NOT removed from Msgdatabase.
    Instead, IMAPDeleted flag of msgDBHdr is set to true.
    Message is still shown with strike-thru line, Undelete is possible.
    So, Mbox/cur/nnnn is kept correctly.
(b-1) Move to maildirstore/IMAP/Offline-Use=Off, 
              maildirstore/local folder
              berkleystore/any folder type
    For "Move Source Folder", same as "Delete".
(b-2) Move to maildirstore/IMAP/Offline-Use=On
      IMAP delete model : Move to Tash, Remove immediately
    Mbox/cur/nnnn is moved to MoveTarget/cur.
    At thread pane, mail disappears once.
    "uid xx store *Flags \Deleted" is not issued.
    By folder access, message body is fetched to Mbox/tmp/nnnn
    and moved to Mbox/tmp/nnnn, msgDBHdr is updated, and shown again.
    ==> Delete operation at Move Source is not invoked as expected.
(b-3) Move to maildirstore/IMAP/Offline-Use=On
      IMAP delete model : Jus mark it as deleted
    Mbox/cur/nnnn is moved to MoveTarget/cur.
    At thread pane, mail disappears once.
    "uid xx store *Flags \Deleted" is not issued.
    By folder access, message body is fetched to Mbox/tmp/nnnn
    and moved to Mbox/tmp/nnnn, msgDBHdr is updated, and shown again.
    IMAPDeleted flag of msgDBHdr is not set true.
    ==> Moved mail is not show as "Deleted".

So, even if "delete of Mbox/cur/nnnn" will be executed upon end of Delete operation or end of Move operation, problem of "Mbox/cur/nnnn will remain.

I think followig is an simplest way.
(1) Keep relation between UID and cur/nnnn file(storeToken) somewhere,
    as postate.dat holds "k", "p", "d" states.
(2) Upon folder re-sync(folder open), delete all cur/nnnn files(and
    directory, garbage) which is not pointed by existent UID(call Table)
    1. Scan msgDBHdrs, add UID to Table if UID is not held in Table.
    2. Delete all uknown cur/nnnn based on Table.
    3. Refresh Table with current states to keep consistency with msgDB.
    This is same as "issue RETR if UIDL=k/p is not held in popsate.dat.
(3) Upon unsol "EXPUNGED xxx" response, delete obsolete cur/nnnn file
    used for the UID.
(4) In case of garbage, and for explicit expunge, provide a way to force removal of unknown cur/nnnn files.
Name of "Compact" is not appropriate for maildirstore. And "Compact Folders" like capability(do on all folders of the account at once) is not mandatory.
How about "Expunge Folder" button near"Repair Folder" in Folder Property/General?

Severity: normal → major

OS: Linux → All

Hardware: x86_64 → All

Summary: deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (no one can delete files under "cur" used as offline-store file when maildirstore, because Compact menu is removed even though IMAP) → deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (no one can delete files under "cur" used as offline-store file when maildirstore, because Compact is completely killed even though IMAP)

Comment 12

•

11 years ago

For "Table" I call in previous comment.
If implemnted by JavaScript, I think JavaScript Object, saved as JSON data which is held in BLOB data of, SQLite DB with utilizing journal, is most convenient one. No need to care for file loss due to safe but not-absolutely-safe file writing, no need to parse not-so-well structured text data, no need to use Mork, no need to use RDF, ...

Updated

•

11 years ago

Summary: deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (no one can delete files under "cur" used as offline-store file when maildirstore, because Compact is completely killed even though IMAP) → deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (no one can delete files under "cur" used as offline-store file when maildirstore, because "Compact" is completely killed by maldirstore even though IMAP)

Updated

•

11 years ago

Summary: deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (no one can delete files under "cur" used as offline-store file when maildirstore, because "Compact" is completely killed by maldirstore even though IMAP) → deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (No way to expunge/delete files under "cur" as offline-store file when maildirstore, because "Compact" is completely killed by maldirstore even though IMAP)

Comment 13

•

11 years ago

FYI.

As for "move from maildirstore/IMAP/Offline-Use=On to maildirstore/LocalMailFolder" case, phenomenon was never "IMAP/cur/nnnn is not removed by delete of mail", as seen in bug 856532.
Phenomenon was;
- IMAP/cur/nnnn file is successfully moved to Target/cur/nnnn
  because it's "Move", and msgDBHdr is successfully removed from
  msgDatabase for maildirstore/LocalMailFolder because it's "Move".
  i.e. IMAP/cur/nnnn file is pretty normally deleted by "Move".
- But, "uid xx store +Flags \Delete" is not issued even though IMAP.
  (Perhaps, because of "Move" != "Delete")
  So, upon folder open, mail data of the unknown UID is fetched,
  then stored in offline-store file(IMAP/cur/xxxx) if Offline-use=On.

Because IMAP/cur/nnnn file for deleted mail or moved mail should be kept with IMAPDeleted flag of msgDBHddr.flags = true when Offline-use=On and IMAP delete model = Just mark it as deleted, removal of IMAP/cur/nnnn file should be executed according to EXPUNGE of an UID at server's Mbox.
This is difference of "Just mark it as deleted" case from problem of "IMAP-OfflineUseOn/cur/nnnn file is not removed by Delete of mail" with "Move to trash" model and "Remove it immediately" model.

Updated

•

11 years ago

Summary: deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite (No way to expunge/delete files under "cur" as offline-store file when maildirstore, because "Compact" is completely killed by maldirstore even though IMAP) → deleting or moving messages doesn't delete messages from the filesystem when using Maildir Lite(IMAP. No way to expunge/delete files under cur as offline-store file when maildirstore, because "Compact" is completely killed by maldirstore even though IMAP)

Updated

•

11 years ago

Blocks: 857003

Updated

•

11 years ago

Blocks: 859011

Updated

•

11 years ago

No longer blocks: maildirblockers

Jakob Bohm

Comment 14

•

11 years ago

I think the handling of MOVE with IMAP involved here is fundamentally wrong.

MOVE is not COPY+DELETE from the user perspective, even though that may be the implementation in some backend combinations. MOVE was always "make it appear in folder2, not folder1, make sure mail is never lost in the process. The direct comparison is that in the file system, move is a different operation from copy+delete on all current file systems (including FAT since 1983), and the trash folders in file UIs only contains files that were deleted, not ones that were moved and survive in their new home.

So MOVE on IMAP should be (regardless of delete mode):
- If same account and server supports the extension, execute the move server side.
- Otherwise copy the message, retrieve its header back to confirm copy successfully committed server side, then delete and SELECTIVELY EXPUNGE (do not expunge any pending delete mails, those need to be preserved for user undelete until use empties the trash). This needs to be efficiently batched when doing multiple mails, especially when moving a large batch (such as moving all mailing list threads from 2012 to a "2012" folder).
- If success mark action as locally complete (e.g. by moving file if maildirstore).
- If something goes wrong, refresh folder contents of both folders to find out what is really where now.

COPY on IMAP should be (regardless of delete mode):
- If same account and server supports the extension, execute the copy server side.
- Otherwise copy by retrieve and upload, the retrieve back headers to confirm successfully committed server side. This needs to be efficiently batched when doing multiple mails, especially when copying a batch (such as all correspondence on a case being copied to or from a shared folder).
- If success mark action as locally complete (e.g. by copying file if maildirstore).
- If something goes wrong, refresh folder contents of both folders to find out what is really where now.

USER DELETE on IMAP with just-mark-as-delete should be
- Execute UID delete.
- If success mark action as locally complete (e.g. by updating the deleted flag in local index).
- If something goes wrong, refresh folder contents to find out what is really where now.

USER DELETE on IMAP with move-to-trash should be
- Follow procedure (and code) for regular message move (see above).

USER DELETE on IMAP with delete-flag-as-trash should be
- Execute UID delete.
- If success mark action as locally complete (e.g. by moving file if maildirstore).
- If something goes wrong, refresh folder contents to find out what is really where now.

"delete-flag-as-trash" would be a new IMAP mode, where mails with the "Deleted" IMAP flag are shown in a corresponding virtual Trash folder in the UI, but remain in their original location server side. Emptying the virtual trash folder executes a server side expunge for its associated non-trash folder. But adding such a mode is a topic for another bug (if somebody cares to file it).

Wayne Mery (:wsmwk)

Comment 15

•

11 years ago

xref bug 302205 Need IMAP Purge/Expunge (without compact)

Comment 16

•

10 years ago

FYI.
nsMsgMaildirStore::GetSupportsCompaction() always returns false.
  http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgMaildirStore.cpp#1087
So msgFolder.msgStore.supportsCompaction = false is always returned if Maildir, then "Compact of Maildir" is always killed.

Assignee

Comment 17

•

9 years ago

Confirming that this still occurs even after the fixes for bug 1113275

STR:

1) In maildir IMAP with offline storage enabled, and IMAP delete model set to move to Trash. Select one or more messages in an IMAP folder, and delete.

Expected:

1) messages disappear from the source folder, appear in the Trash folder
2) in the maildir offline file storage, the files in the source folder disappear

Actual: 1) works, but 2) the files continue to appear in the source folder.

Assignee

Comment 18

•

9 years ago

Attached patch modified test_imapMove that fails as described in this bug (obsolete) — Details — Splinter Review

Assignee: acelists → kent

Status: NEW → ASSIGNED

Assignee

Comment 19

•

9 years ago

Attached patch Implement DeleteStoreMessages for imap offline stores (obsolete) — Details — Splinter Review

I could find no plan to delete store messages for the non-compacting maildir. For mbox, we do not need to delete messages, because the status is stored in the msgHdr, and "deleting" does really affect the offline store until compact.

So this is an implementation of a store delete for the offline store, done at the time that the message is deleted from the database.

Attachment #8543547 - Attachment is obsolete: true

Attachment #8543574 - Flags: review?(neil)

Assignee

Comment 20

•

9 years ago

This patch is applied on top of the current patches for bug 1113275 (and also bug 856519 though I don't think that one matters here.)

Depends on: 1113275

Comment 21

•

9 years ago

Comment on attachment 8543574 [details] [diff] [review]
Implement DeleteStoreMessages for imap offline stores

>+                  // I believe we only reach here for same-server operations,
That would be the !m_isCcrossServerOp check above.

>+                if (!supportsCompaction & numHdrs)
&&

>+  void DeleteStoreMessages(nsTArray<nsMsgKey> &aMessages, nsIMsgFolder* aFolder);
Nit: static

>     // let's check if the folder exists
>     bool exists;
>     path->Exists(&exists);
>-    path->Remove(false);
>+    rv = path->Remove(false);
>+    if (!exists || NS_FAILED(rv))
>+    {
>+      PR_LOG(MailDirLog, PR_LOG_ALWAYS,
>+             ("DeleteMessages - file does not exist !!\n"));
>+      // perhaps an offline store has not downloaded this particular message
>+      continue;
>+    }
Not sure what you're trying to achieve here with the rv check. (Not sure what the old code was trying to achieve with the exists check!)

Assignee

Comment 22

•

9 years ago

(In reply to neil@parkwaycc.co.uk from comment #21)
> Comment on attachment 8543574 [details] [diff] [review]
> >     // let's check if the folder exists
> >     bool exists;
> >     path->Exists(&exists);
> >-    path->Remove(false);
> >+    rv = path->Remove(false);
> >+    if (!exists || NS_FAILED(rv))
> >+    {
> >+      PR_LOG(MailDirLog, PR_LOG_ALWAYS,
> >+             ("DeleteMessages - file does not exist !!\n"));
> >+      // perhaps an offline store has not downloaded this particular message
> >+      continue;
> >+    }
> Not sure what you're trying to achieve here with the rv check. (Not sure
> what the old code was trying to achieve with the exists check!)

The previous path->Exists() I believe was pointless, probably a leftover copy-and-paste.

What I am trying to accomplish is the correct balance of error reporting. I can imagine cases in which we would reach the previous code (fileName.IsEmpty()) under normal operation. So while it is an interesting error, it should not really be a warning. That's why I removed the NS_ERROR_FAILURE from it, but left the PR_LOG. For the ->Remove(), a failure is also interesting and should not just be neglected, though I am less sure of cases where it would be reached in normal operation. But it should not abort, since the method has accomplished its mission (delete the message) since the message does not exist. So I also added PR_LOG reporting here as well. (Though in practice, I have never used PR_LOG with maildir, but perhaps I would if the implementation was more complete).

Comment 23

•

9 years ago

(In reply to Kent James from comment #22)
> (In reply to comment #21)
> > (From update of attachment 8543574 [details] [diff] [review])
> > >     path->Exists(&exists);
> > (Not sure what the old code was trying to achieve with the exists check!)
> The previous path->Exists() I believe was pointless, probably a leftover
> copy-and-paste.
> 
> What I am trying to accomplish is the correct balance of error reporting.

OK but I think checking both exists and rv might be overkill in that case.

Assignee

Comment 24

•

9 years ago

Are you done with your initial review, and waiting for me to implement the changes from your last comment? I'd like to get all of the maildir patches landed.

Comment 25

•

9 years ago

(In reply to Kent James from comment #24)
> Are you done with your initial review, and waiting for me to implement the
> changes from your last comment?

Are you planning to implement any changes to the code that you requoted in comment #22?

Assignee

Comment 26

•

9 years ago

(In reply to neil@parkwaycc.co.uk from comment #25)
> (In reply to Kent James from comment #24)
> > Are you done with your initial review, and waiting for me to implement the
> > changes from your last comment?
> 
> Are you planning to implement any changes to the code that you requoted in
> comment #22?

I'm happy to do the change you suggested in comment 23. I'll do a revised patch when you want, are you done with your initial review and ready for a revised patch?

Assignee

Comment 27

•

9 years ago

Attached patch Un-bitrotted with requested changes — Details — Splinter Review

This patch had bit-rotted significantly, so I fixed that as well as incorporated the suggested changes.

Attachment #8543574 - Attachment is obsolete: true

Attachment #8543574 - Flags: review?(neil)

Attachment #8560022 - Flags: review?(neil)

Comment 28

•

9 years ago

Comment on attachment 8560022 [details] [diff] [review]
Un-bitrotted with requested changes

>+                  // I believe we only reach here for same-server operations,
>+                  // so we can assume that the src is also imap that uses
>+                  // offline storage.
Revise this to mention m_isCcrossServerOp?

>-                if (notifier)
>+                if (notifier && numHdrs)
>                 {
>                   // XXX Currently, the DeleteMessages below gets executed twice on deletes.
>                   // Once in DeleteMessages, once here. The second time, it silently fails
>                   // to delete. This is why we're also checking whether the array is empty.
Hmm, I guess the comment still makes sense here.

>+    if (exists)
>+    {
>+      path->Remove(false);
>+    }
>+    else
>+    {
>+      PR_LOG(MailDirLog, PR_LOG_ALWAYS,
>+             ("DeleteMessages - file does not exist !!\n"));
>+      // Perhaps an offline store has not downloaded this particular message.
>+      continue;
>+    }
The way you've written this with the continue in the else block looks awkward. Could you change it to
if (!exists)
{
  ...
   continue;
}
path->Remove(false);

Attachment #8560022 - Flags: review?(neil) → review+

Assignee

Comment 29

•

9 years ago

Pushed with nits fixed:

https://hg.mozilla.org/comm-central/rev/baa8c010ffb3

Status: ASSIGNED → RESOLVED

Closed: 9 years ago

Resolution: --- → FIXED

Target Milestone: --- → Thunderbird 38.0

Comment 30

•

9 years ago

Excuse-me about being naive. I'm interested and testing Thunderbird with Maildir-like storage and I found this bug on my testing. I'm surprised to see the milestone set to version 38, as we are at 31.5 at the moment. Why can't this fix be pushed before, let say for version 32? Testing and validation issues?

By the way, big thanks to all developers that put some work to give us this function. It's getting close and I'm excited about the moment of switching to Thunderbird (We are blocked by the one file per mail functionality, a must for us).

Assignee

Comment 31

•

9 years ago

There were many issues with maildir in TB 31, and fixing them required changes that were beyond what would be appropriate for back-porting.

Current aurora Thunderbird 38 has all of the critical maildir patches, and I would encourage you to use that for testing.

It is not trivial to setup though. Start a new profile, skip the add account dialog, and go to Tools/Options/Advanced/General and set "Message Store Type for New Accounts" to maildir.

Comment 32

•

9 years ago

Thanks for that answer! As I see we misunderstood each other. I wasn't talking about backporting to TB 31, but releasing the fixes for the next version TB 32. As I understand your answer, next version will be TB 38, and not 32. Excuse my ignorance about the version numbering. I suppose this jump is to have the same number as Firefox at the date of it's release, am I right?

Many thanks again and congratulations for your hard work. Will have a try at the testing version. Cheers!

:aceman

Comment 33

•

9 years ago

(In reply to macuserfr from comment #32)
> Thanks for that answer! As I see we misunderstood each other. I wasn't
> talking about backporting to TB 31, but releasing the fixes for the next
> version TB 32. As I understand your answer, next version will be TB 38, and
> not 32. Excuse my ignorance about the version numbering. I suppose this jump
> is to have the same number as Firefox at the date of it's release, am I
> right?
Well, there is no jump, the versions of TB 32-37 do exist, but they can only be downloaded as "beta" state, never a stable "release". Yes, TB 38 will be released at the same time as Firefox 38. This is intentional and worked this way at least since TB 24 (the next release was then TB31).

Comment 34

•

9 years ago

Thanks again! Sorry about spamming Bugzilla, didn't find a better place to talk directly with people envolved. That's cool. So in 2 major releases we can expect to have maildir working in the stable version.