1428104 - Renaming IMAP folders that have subfolders may fail (Seamonkey only)

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[[e-mail]]

After renaming a folder, the IMAP protocol then attempts to subscribe to the new folder name and unsubscribe from the old folder name. When renaming a leaf folder, any errors from the subscribe operations are ignored (presumably in case the server already updated the subscriptions automatically). However if the folder being renamed contains subfolders then errors from the subscribe operations cause the whole rename to bail, leaving the client out of sync. Side effects include UI not being up-to-date and .msf files not being renamed.

Comment 1

[[e-mail]]

Attachment: 1428104.diff

(The suite change is a port of a fix that Thunderbird incidentally happened to make as part of their front-end rewrite. It prevents the .msf for the old folder from being recreated.)

Comment 2

[Jorg K (CEST = GMT+2)]

Kent hasn't done any reviews in ages, so you might want to find a different reviewer.

Comment 3

[Jorg K (CEST = GMT+2)]

Another bad choice of reviewer :-(

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939903 [details] [diff] [review]
1428104.diff

Neil, neither Kent not Joshua do timely reviews (if at all) these days. Gene has been very active in IMAP lately, so I'm asking him to try this out. I'm happy to r+ the change if I can get his positive feedback. I hope you agree on this way forward.

Gene, could you please give this a spin.

Comment 5

[[e-mail]]

(In reply to Jorg K (GMT+1) from comment #3)
> Another bad choice of reviewer :-(

Somebody needs to give Bugzilla better reviewer choices...

Comment 6

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8939903 [details] [diff] [review]
1428104.diff

f+ for the suite part. Renaming an imap folder with an unread mail in a subfolder fails miserably without the patch in SeaMonkey.

The other part looks ok too. RenameMailboxRespectingSubscriptions returns the return code from LastCommandSuccessful() and it doesn't seem to make sense to first ignore subscribe/unsubsribe errors and then trying to retrieve them later.

Tested with TB 56 and SeaMonkey 2.53. Might be a good candidate for the next ESR 52 release.

Comment 7

[gene smith]

Is there a way to force a failure on this to test the fix? I tried a rename on non-leaf folders and see no problems in tb. (Don't have SeaMonkey.)

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to gene smith from comment #7)
> Is there a way to force a failure on this to test the fix? I tried a rename
> on non-leaf folders and see no problems in tb. (Don't have SeaMonkey.)
Yes, a verbal description of a test case would be nice. Maybe "lock" a file somehow that needs renaming?

Comment 9

[gene smith]

Comment on attachment 8939903 [details] [diff] [review]
1428104.diff

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

Apparently this patch really only affects suite/SeaMonkey. I always see renameSucceeded as true regardless of whether it comes directly from the renaming function or from "last command succeeded". Even with the folder.sdb and folder.msf locked with attribute immutable, when renamed, renameSucceeded is set true. It actually creates a new folder, with the new name, with the the same folders and emails below it and leaves the older folder structure in place and tb flags errors when accessed. But the older folders are no longer on the server. When unlocked, the older structure can be deleted in thunderbird and all seems OK.
So this really fixes nothing on Thunderbird that I can see.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +6817,5 @@
>          newChildName += (currentName + PL_strlen(oldParentMailboxName));
> +        renameSucceeded =
> +          RenameMailboxRespectingSubscriptions(currentName,
> +                                               newChildName.get(),
> +                                               nonHierarchicalRename);

In the comment following this I would suggest changing the xNonHierarchicalRename to exclude the leading 'x' (unless it means something I don't understand). Also, I would move the comment to before the function. 
Also, as I mentioned before, in my simple tests I have only seen renameSucceeded be set true regardless of how it is set. So other than making the code a bit more efficient, this does nothing in thunderbird.

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks, Gene. Neil, how would you like to proceed? Apparently the suite/ hunk fixes the problem for SM. Should we fix the mailnews hunk (comment) as suggested or drop it?

Comment 11

[[e-mail]]

> It [...] leaves the older folder structure in place and tb flags errors when accessed.

Except that the old folders are supposed to be renamed on disk, but that only happens if Thunderbird thinks that the IMAP rename succeeded.

Also, the issue might depend on your IMAP server; I see the problem with Gmail for instance.

Comment 12

[Jorg K (CEST = GMT+2)]

Neil, would be be OK with taking the patch and moving the comment up (and losing the x)?

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939903 [details] [diff] [review]
1428104.diff

OK, let's go with this.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/20758564d6cb
Renaming IMAP folders that have subfolders may fail. r=jorgk,GeneSmith, f=frg

Comment 15

[Frank-Rainer Grahl (:frg)]

Assuming it survives a Beta can we take this to ESR52.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939903 [details] [diff] [review]
1428104.diff

Sure.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939903 [details] [diff] [review]
1428104.diff

That only landed on TB 60 and would ship in TB 52.8 which comes out together with TB 60 ESR.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939903 [details] [diff] [review]
1428104.diff

Let's reconsider this for the sake of SM which seems to have a benefit from this. It could go into 52.8.

Comment 19

[Jorg K (CEST = GMT+2)]

TB 52.8 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/3df0989644bb5fc2df4769bfc5a53d650fd8fe15