Closed Bug 857436 Opened 7 years ago Closed 5 years ago

When Mbox is renamed, Mbox directory of maildirstore/IMAP/Offline-Use=On folder(container of cur, tmp directory) is created under Parent directory instead of under Parent.sbd directory

Categories

(MailNews Core :: Database, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WORKSFORME
Thunderbird 38.0

People

(Reporter: World, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [maildir])

Attachments

(2 files, 3 obsolete files)

[Build ID]
> Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20100101 Thunderbird/22.0a1
> Application Build ID : 20130325031104

When Mbox is renamed, Mbox directory of maildirstore/IMAP/Offline-Use=On folder(container of cur, tmp directory) is created under Parent directory instead of under Parent.sbd directory

[Steps to reproduce]
(1) Create maildirstore/IMAP, copy 3 mails to Offline-Use=On folder,
    call "Parent", copy a mail and click mail to force creation of 
    "Parent.msf" file, "Parent" directory, "Parent\cur" directory.
(2) Create Offline-Use=On folder under Parent, call ChildA,
    copy a mail and click the mail to force creation of
    "ChildA.msf" file, "ChildA" directory, "ChildA\cur" directory.
(3) Repeat rename o the "Child" folder.
    Rename ChildA to ChildB, click mail
    Rename ChildA to ChildA, click mail
                  |         |
    Rename Childx to Childy, click mail
                  |         |
    Rename ChildB to ChildA, click mail
(4) Childx.msf file, Childx directory, Childx\cur directory,
    are created under "Parent" directory for "cur" directory,
    instead of under expected "Parent.sbd" directory for subfolder.
(5) Because "Childx" directory and "Childx\cur" directory
    is not deleted by rename and is re-used by rename,
    bug 857003 is always observed.
No longer depends on: 857003
See Also: → 857003
Summary: Mbox/cur directory size of maildirstore/IMAP/Offline-Use=On folder increases infinitely by simple/cyclic rename of Mbox → When Mbox is renamed, Mbox directory of maildirstore/IMAP/Offline-Use=On folder(container of cur, tmp directory) is created under Parent directory instead of under Parent.sbd directory
Blocks: 857443
No longer blocks: 857443
When "rename back to previously used Mbox name" is involved in problem, "folder cache data for relation between Mbox name and file path" is also relevant to problem, in addition to "file/directory is cleanly closed or not" and "old file/directory is cleanly deleted or not".

Cleanup upon (a) folder delete, (b) folder rename, (c) unsubscribe, and cleanup due to (c) Mbox doesn't exist at server, (d) Mbox is hidden by IMAP server directory: setting, should be done on all of next;
- Mbox.msf
- Mbox (directory if MaildirStore, file if BerkleyStore)
- Mbox/cur, Mbox/tmp directory under Mbox directory
- Mbox.sbd directory
- Folder Cache(data saved in panacea.dat)

One of main causes of problems by bug 520437(suffixed name by unsubscribe/re-subscribe) is "Mbox.msf file is not cleanly closed", "Mbox.msf file is not cleanly deleted", "relation between Mbox name and Mbox.msf file is not cleared from panacea.dat" etc.

And, when IMAP, all of old Mbox.msf file, old Mbox directory, old Mbox.sbd directory should be deleted by cleanup step, unless "rename old Mbox.msf to new Mbox.msf", "rename old Mbox to new Mbox", and "old Mbox.sbd to new Mbox.sbd", of all subfolders, is implemented in a proper fashion.
Blocks: 859011
No longer blocks: maildirblockers
Attached patch modifyTest (obsolete) — Splinter Review
While going through test_nsIMsgLocalMailFolder.js with maildir enabled
I found that after folder.rename(), it left the folder with orphaned and unnecessary duplicated files.

So, I have tried to modify this test to confirm the cause(s) of failure.
Can you please verify and confirm if this test is able to highlight the problem
and the problem is the same that this bug is about and we need to modify the rename() method?

Thanks.
Attachment #8424412 - Flags: feedback?(m-wada)
Attached file A test run for the same (obsolete) —
As a test run, I got the folder states as shown in the test run description after
a few rename() calls, as shown in the description.
OS: Windows XP → All
Hardware: x86 → All
Version: Trunk → unspecified
(In reply to Suyash Agarwal (:sshagarwal) from comment #2)
> While going through test_nsIMsgLocalMailFolder.js with maildir enabled
> I found that after folder.rename(), it left the folder with orphaned and
> unnecessary duplicated files.

I believe that root cause is same in both local mail folder case and IMAP/Offline-Use=On case.
However, I also believe separate test case is needed for local mail folder case, IMAP/Offline-Use=On case, and IMAP/Offline-Use=Off case(no msgStore file is used. .msf file only is used. however, .sbd is used for sub folder. if changed to offline-use=On to offline-use=off, msgStore file should be removed at least upon Repair Folder).
Comment on attachment 8424412 [details] [diff] [review]
modifyTest

Review of attachment 8424412 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I can say nothing on test script and test result.
Attachment #8424412 - Flags: feedback?(m-wada)
Attached patch Patch v1 (obsolete) — Splinter Review
Okay so here is an attempt to modify nsMsgMaildireStore::RenameFolder()
so that the test passes with maildir.

I have also added another test to check closely, the rename() method.
Attachment #8424412 - Attachment is obsolete: true
Attachment #8424413 - Attachment is obsolete: true
Attachment #8430652 - Flags: feedback?(kent)
I tried to simply disconnect the server and remove it, leaving the files
behind so that one can look at it.
And the root folder was being created nicely at every iteration
"Local Folders" and "Local Folders-1"
but in the second iteration, in subtest_folder_operations(), creation of "folder3"
was being done in "Local Folders" instead of "Local Folders-1" even when
root had the leafname "Local Folders-1". Though, "folder1" was being created fine.

I couldn't figure out the cause for this. So I am doing it this way, and leaving the
files of the last iteration so that we can look at the directory structure at the end
of the test and verify that it is correct.
So, please let me know how I can fix this.

Thanks.
(In reply to WADA from comment #6)
> Sorry, I can say nothing on test script and test result.

That's fine, thanks.
Depends on: 1017939
Depends on: 1028548
Attachment #8430652 - Attachment is obsolete: true
Attachment #8430652 - Flags: feedback?(kent)
Can you please confirm if this bug still exists after the patch on bug 1017939 ?

Thanks.
Flags: needinfo?(m-wada)
(In reply to Suyash Agarwal (:sshagarwal) from comment #10)
> Can you please confirm if this bug still exists after the patch on bug 1017939 ?

Does it mean "you are never able to verify that the 'patch on bug 1017939' can resolve this bug"?
If so, what kind of tests did you do for the 'patch on bug 1017939' can resolve this bug"?
What is actual result of your fix verification tests for the 'patch on bug 1017939'?
Flags: needinfo?(m-wada) → needinfo?(syshagarwal)
Hi WADA,

Suyash, and in some cases myself, have fixed a number of maildir bugs this summer. Those fixes arose from attempting to get existing xpcshell tests working rather than from tracing out issues from bug reports. However, we expect that many of the issues in bug reports have been resolved from those fixes.

So a question like "If so, what kind of tests did you do for the 'patch on bug 1017939' can resolve this bug?" in most cases the answer is going to be "We fixed an issue that looks like it might have caused your bug, can someone retest the bug and confirm that it is now resolved?"

In the specific case of bug 1017939, the fix here https://bugzilla.mozilla.org/attachment.cgi?id=8437928&action=diff#a/mailnews/local/src/nsMsgMaildirStore.cpp_sec3 specifically addressed the issue of this current bug, so we believe this current bug could probably now be resolved as a dup of bug 1017939, bu we would appreciate an independent verification of that. See the test in bug 1017939 as our verification that the issue is fixed.
Attached patch Patch v2Splinter Review
Okay this bug still existed. The msf file was created but the folder wasn't renamed.

This patch fixes the issue for me.
Attachment #8470561 - Flags: review?(kent)
Flags: needinfo?(syshagarwal)
Comment on attachment 8470561 [details] [diff] [review]
Patch v2

Review of attachment 8470561 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review, with the tree closed it seemed pointless and difficult.

Generally I don't think this is the correct approach. The root cause of these rename bugs are that the pluggable store renameFolder method (which should be doing the rename) is not being, used, and the msgFolder is making assumptions about the structure. The whole concept of a .sbd file is an artifact of the old scheme, so any time we see that it is a sign that a conversion to using pluggable stores has not been fully implemented.

As soon as nsImapMailFolder::RenameLocal starts getting file references for the folder, it is going astray. We need to figure out how to do this using pluggable store calls instead.

So the goal should be to replace the bulk of the code of RenameLocal to use the pluggable store RenameFolder. That was done for local, but not for imap.
Attachment #8470561 - Flags: review?(kent) → review-
I have tried to make the changes to use msgStore. But the current IMAP rename seems to be broken. It isn't working even for mbox without any of my patches. It needs further investigation as to what's wrong now.
Suyash, can you give steps to reproduce for IMAP rename with mbox issues? I have not tried that, but if there are problems there they need fixing.
STR:

1) Create a folder under inbox (IMAP account).
2) Copy a mail in the folder.
3) Create another subfolder under the folder created in the step above.
4) Copy a mail to this folder too.
5) Try to rename the first created folder.

Expected results:
Everything happens as expected.

Actual Results:
The folder doesn't seem to be renamed in the folderpane.
When this folder tries to sync with the server, [nonexistent] error
is encountered since rename was successful on the server side.
The aforementioned issue still exists.
Just re-checked on 31.4.0 release TB for Mac.

Will investigate it later today.
By "aforementioned", I mean the issue mentioned in comment 17.
I could not reproduce comment 17 on my current builds.

You mentioned this was TB 31.4.0  Very few of the maildir patches are there. Can you try it on current trunk? Though I run with a few more maildir fixes locally, I don't think they would affect this issue.
(In reply to Kent James (:rkent) from comment #20)
> I could not reproduce comment 17 on my current builds.

Okay, tried this with the trunk and rename succeeds. But, <prevname>.sbd/ is left
in the profile directory.

original state:
-folderA
 -<a mail>
 -<subfolderA>
  -<another mail>

profile top directory contents:
folderA.msf
folderA.sbd/
folderA

rename folderA to say, folderB

folderpane structure:
folderB
 -<a mail>
 -subfolderA
  -<another mail>

profile directory contents:

folderB
folderB.sbd/
folderB.msf
folderA.sbd/
Funny, but I get different behavior. Following the example in comment 21, what I see is that everything succeeds, but after the rename to folder B I see:

folderA
  cur (with emails)
  tmp
folderB.sbd
  subfolderA
    cur (with emails)
    tmp

But in any case, that is not exactly working as expected, so I'll investigate.
Assignee: nobody → rkent
Status: NEW → ASSIGNED
(In reply to Kent James (:rkent) from comment #12)
> So a question like "If so, what kind of tests did you do for the 'patch on bug 1017939' can resolve this bug?" in most cases the answer is going to be
> "We fixed an issue that looks like it might have caused your bug, can someone retest the bug and confirm that it is now resolved?"

Tested "Repeated/cyclic rename of newly created local Maildir mbox" case with following nigtly.
> Mozilla/5.0 (Windows NT 5.1; rv:38.0) Gecko/20100101 Thunderbird/38.0a1

   FolderX(top level)
       A
           Sub_of_A
   Repeat "Rename A B", "Rename B C", "Rename C A", "Rename A B", "Rename B C", "Rename C A", ...
   => no problem is found. As you say, many problems are already resolved.
   Delete A => Directory of A.sbd is also correctly moved to Trash.
   Empty Trash => Trash is normally cleared.

As for "Rename of local Maildir folder", I couldn't observe problem in recent nightly.

FYI.
Bug 520437 is IMAP specific issue, and "x.sbd and files under it is not removed by rename of IMAP folder" is already known phenomenon in Berkleystore.
When IMAP, no problem was observed on "newly created folder by rename". This is because:
In IMAP, rename consists of:
    Level1
        Level2 => rename to Level2X
            Level3
   1. "rename Level1/Level2 Level1/Level2X"
       => server renames  Level1/Leve2 => Level1/Level2X,   Level1/Leve2/Level3  => Level1/Leve2X/Level3
       Tb perhaps do unsubscribe of Level1/Leve2/Level3, Level1/Leve2, prior to rename.
   2. LIST   Level1/%/% => Level1/Level2X, Level1/Leve2X/Level3 is returned
        subscribe Level1/Level2X, Level1/Leve2X/Level3
   3. create associated files for Level1/Level2X, Level1/Leve2X/Level3.
       Offline-Use=On/Off is determined by mail.server.server#.offline_download=true/false
   4. Because Level1/Level2, Level1/Leve2/Level3. doesn't exist t server any more, remove it in Tb too.
i.e. "Rename in IMAP" is (a) "discard old mboxes" + (b) "request rename to server" + (c) "access newly creaated mboxes".
I couldn't observe problem in (b) and (c).
I could observe problem on (a) only. (Checked with "Show only subscribed folder" is disabled == based on LIST response instead of LSUB response.
(a-1) Level2(old name) won't disappear from folder pane until restart of Tb.
          It was not removed by folder re-discovery by "collapse/expand higer folder including accout folder(rootFolder)".
(a-2) "OldName.msf file corresponds to Mbox at server" is deleted after rename because Mbox doesn't exist at server(not returned to LIST).
          However, OldName directory, OldName.sbd directory, file directory under OldName.sbd directory, are not removed by Tb.

".sbd directory is not removed by Rename" is phenomenon which is observed in Berkleystore.
I think mjaority of rename relevant problems which is  "problem due to Maildir" is already resolved.
For IMAP rename.
When rename A1 to A2, rename A2 to A3, rename A3 to A1, rename A1 to A2, rename A2 to A3, rename A3 to A1, ... ,
(i) A is removed from folder pane by folder re-discovery(by collapse/expand account) when first rename cycle.
     If cyclic rename is repeated, previous one was not removed only by folder re-discovery.
(ii) An.msf file is not removed by folder re-discovery, "Go Work Offline/go back Work Online then folder click", and so on.
      So, if cyclic rename is repeated, suffix is added to .msf file name(X-Suffix.msf, then X-Suffix.sbd) upon each rename.
      This is phenomenon of Bug 520437.
       And, one of causes of problem (i) is "mismatch between MboxName and .msf file name due to Suffix".
Bug 520437 is not MailDir only problem. As for "Rename of MailDir folder", I believe this bug was resolved.
Additional observation.
Once Suffixed msf file is used by repeated cyclic rename,
(iii) All of A1, A2, A3 is shown at folder pane(from panacea.dat data and non-deleted An-Suffix.msf files) after restart,
       then non-existent one disppears at folder pane after connection to server.
(vi) However, non used An.msf file, An-Suffix.msf file, are not deleted by Tb.

If server supports case-sensitive Mbox name(Yahoo! IMAP is example) and if file system of client side is case-insensitive(MS Windows is example), following occurs(Tb's current design/implementation)
   abc <-> abc.msf
   Abc <-> Abc-1.msf
   ABC <-> ABC-2.msf
If rename to AbC, aBC, abC, like one is repeated and if cyclic rename, AbC-n.msf file perhaps increases.
When restart of Tb, if "relation between Mbox<->.msf file held in panacea,dat" is lost, following occurs because LIST response is sent in alphabetic order. 
   ABC -> abc,msf  (search ABC, ABC==abc, abc.msf is used)
   Abc -> Abc-1,msf (search Abc, abc.msf is used, search Abc-1, Abc-1==Abc-1, Abc-1.msf is used)
   abc -> ABC-2,msf (search abc, abc-1, abc-2, abc-2==ABC-2, ABC-2.msf is used)
In this case, UIDVALIDITY mismatch occurs, then re-sync from scratch. re-auto-sync from scratch occurs.

Is MailDir torelant with this kind of special case? :-)
"Is MailDir torelant with this kind of special case? :-)"

I don't know.
(In addition to comment #27)
> (vi) However, non used An.msf file, An-Suffix.msf file, are not deleted by Tb.

This was observed on both BerkleyStore and MailDieStore.
This was observed in both Tb 31.3.0 and recent Tb nightly.
IIRC, garbled X.msf file is removed when X is not returned to LIST in the past, at least when "Show subscribed folders only" is unchecked(relies on LIST, instead of LSUB). Perhaps regression, or design/implementtion change.

Anyway, I believe problem of this bug was fixed.
Problem of Bug 520437, problem of "XXX.msf file for non-existing Mbox at server is not deleted" is different/independent problem.
Closing as WORKSFOME, because I don't know which patch fixed the problem.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Target Milestone: --- → Thunderbird 38.0
FYI.
I've opened Bug 1132805 for problem of comment #29.
Assignee: rkent → nobody
You need to log in before you can comment on or make changes to this bug.