437848 - Moving a local folder with subfolders causes infinite recursion

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: patch

When moving a local folder which has subfolders in it, an infinite recursion happens, causing the subfolder to be created inside itself, and again inside itself, and so on -- and this ultimately seems to stop and fail only when the FS limit for folder depth is reached.

Steps to reproduce:
1. Create the following folder structure
 Local Folders
- Inbox
-- Folder1
--- Folder2
- Trash
 <other folders>

2. Drag and drop Folder1 from Inbox to Trash

Observed:
The copy fails.

(in the filesystem)
 <Local mail directory>
- Trash.sbd
-- Folder1.sbd
--- Folder2.sbd
---- Folder2.sbd
----- Folder2.sbd
... and so on

This is a regression from bug 420614, according to blame.

The patch should fix this, by moving back to the old behaviour of calling CopyFolderLocal from the new folder instead of the subfolder itself.

Comment 1

[Rain [:rain1] (inactive)]

link: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/local/src/nsLocalMailFolder.cpp&rev=1.585&root=/cvsroot#1986

Comment 2

[Mark Banner (:standard8)]

Would you be willing to try extending the unit test test_localSubFolders.js to cover this case? I think it should be pretty easy.

I'll check the patch later, but at a first glance it seems right.

Comment 3

[[e-mail]]

Comment on attachment 324186 [details] [diff] [review]
patch

>+    nsCOMPtr<nsIMsgLocalMailFolder> localFolder(do_QueryInterface(newMsgFolder, &rv));
As an optimisation this could be moved outside the loop e.g. to line 1964.

Comment 4

[David :Bienvenu]

Comment on attachment 324186 [details] [diff] [review]
patch

thx, sid, that looks right.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 324186 [details] [diff] [review]
patch

As Neil suggested, please move that outside the loop with the appropriate change to the error check.

r=me with that fixed.

Comment 6

[Magnus Melin [:mkmelin]]

We should get this in for 3.0a2, given that it's a regression and have a patch basically ready for checkin.

Comment 7

[Rain [:rain1] (inactive)]

Attachment: update, move folders out + update unit test

Mark, could you please review the unit test and see if anything else should be added?

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 324437 [details] [diff] [review]
update, move folders out + update unit test

That's fine, seems to catch the error nicely if I back out the cpp fix, thanks for doing the test. r=me.

Comment 9

[Mark Banner (:standard8)]

Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.586; previous revision: 1.585
done
Checking in mailnews/local/test/unit/test_localSubFolders.js;
/cvsroot/mozilla/mailnews/local/test/unit/test_localSubFolders.js,v  <--  test_localSubFolders.js
new revision: 1.3; previous revision: 1.2