857436 - 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

Status: RESOLVED WORKSFORME

(MailNews Core :: Database, defect)

Description

[WADA:World Anti-bad-Duping Agency]

Attachment: Directory list after multiple rename of Mbox

[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.

Comment 1

[WADA:World Anti-bad-Duping Agency]

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.

Comment 2

[Suyash Agarwal (:sshagarwal)]

Attachment: modifyTest

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.

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: A test run for the same

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.

Comment 4

[WADA:World Anti-bad-Duping Agency]

(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 5

[Suyash Agarwal (:sshagarwal)]

Okay so I think, basically both
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#880
and 
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#3994
need to be fixed.

(In reply to WADA from comment #4)
Thanks.

Comment 6

[WADA:World Anti-bad-Duping Agency]

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.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

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.

Comment 8

[Suyash Agarwal (:sshagarwal)]

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.

Comment 9

[Suyash Agarwal (:sshagarwal)]

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

That's fine, thanks.

Comment 10

[Suyash Agarwal (:sshagarwal)]

Can you please confirm if this bug still exists after the patch on bug 1017939 ?

Thanks.

Comment 11

[WADA:World Anti-bad-Duping Agency]

(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'?

Comment 12

[Kent James (:rkent)]

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.

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Okay this bug still existed. The msf file was created but the folder wasn't renamed.

This patch fixes the issue for me.

Comment 14

[Kent James (:rkent)]

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.

Comment 15

[Suyash Agarwal (:sshagarwal)]

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.

Comment 16

[Kent James (:rkent)]

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.

Comment 17

[Suyash Agarwal (:sshagarwal)]

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.

Comment 18

[Suyash Agarwal (:sshagarwal)]

The aforementioned issue still exists.
Just re-checked on 31.4.0 release TB for Mac.

Will investigate it later today.

Comment 19

[Suyash Agarwal (:sshagarwal)]

By "aforementioned", I mean the issue mentioned in comment 17.

Comment 20

[Kent James (:rkent)]

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.

Comment 21

[Suyash Agarwal (:sshagarwal)]

(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/

Comment 22

[Kent James (:rkent)]

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.

Comment 23

[WADA:World Anti-bad-Duping Agency]

(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.

Comment 24

[WADA:World Anti-bad-Duping Agency]

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.

Comment 25

[WADA:World Anti-bad-Duping Agency]

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".

Comment 26

[WADA:World Anti-bad-Duping Agency]

Bug 520437 is not MailDir only problem. As for "Rename of MailDir folder", I believe this bug was resolved.

Comment 27

[WADA:World Anti-bad-Duping Agency]

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? :-)

Comment 28

[Kent James (:rkent)]

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

I don't know.

Comment 29

[WADA:World Anti-bad-Duping Agency]

(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.

Comment 30

[WADA:World Anti-bad-Duping Agency]

FYI.
I've opened Bug 1132805 for problem of comment #29.