434708 - When creating folders don't iterate the directory whilst we are changing its contents, and remove a couple of bogus warnings.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

I'm splitting this out from bug 421050. Patch originally by Håkan Waara, updated by me, test cases by me.

From bug 421050 comment 2:

1. Squash a bogus warning that we get in sMsgDBService::OpenFolderDB() for each
new local folder:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80550006: file
/Users/hakan/Programmering/mozilla/hg/applemail/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp,
line 153

We shouldn't use ENSURE_SUCCESS (which warns) here, because there are two error
values that are "expected" (and handled by the caller) at that point in the
flow and those are NS_MSG_ERROR_FOLDER_SUMMARY_MISSING and
NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE.

2. The second fix is that while creating subfolders - in this case the local
folders "Sent Messages", "Thrash", etc - we are iterating a local directory
while with nsIDirectoryEnumerator while changing its contents. The behaviour
for that is undefined! 

In my case, it consistently found "Sent Messages" twice and bailed out because
of that, causing the creation of the "Apple Mail Import" folder to fail, and by
extension the whole import test.

The fix is to first store away the contents of the directory in an array, and
then iterate through the array while creating the subfolders.



I've done various tests like creating new folders/accounts/profiles and haven't found any problems with it, the unit test also passes as well ;-)

Comment 1

[David :Bienvenu]

Comment on attachment 321735 [details] [diff] [review]
The fix

+   * @param aFolderName     The name of the folder to create.
+   * @param aCreate         Whether or not the file should be created.

instead of "folder to create", I think this should read something like "the name of the folder to open a database for"

and

"Whether or not the file"

should be something like "Whether or not the database should be created if missing"

(this goes for both methods).

I'm curious how CreateSubfolder ends up modifying the directory out from underneath the enumerator. AddSubfolder shouldn't do anything in the case that it's called from CreateSubfolders, because it's only calling AddSubfolder with files that exist...Is it nsMsgDBFolder::AddSubfolder that's being naughty? Or is this something specific to Import?

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> I'm curious how CreateSubfolder ends up modifying the directory out from
> underneath the enumerator. AddSubfolder shouldn't do anything in the case that
> it's called from CreateSubfolders, because it's only calling AddSubfolder with
> files that exist...Is it nsMsgDBFolder::AddSubfolder that's being naughty? Or
> is this something specific to Import?

I've just taken a look at this. It is actually nsMsgLocalMailFolder::AddSubfolder that is ensuring the folders exist:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/local/src/nsLocalMailFolder.cpp&rev=1.581&mark=264-280#264

From the bugs that have touched that, it looks there may be various reasons for ensuring the files exist.

Comment 3

[David :Bienvenu]

right, I saw that code, but CreateSubfolders only calls AddSubfolder with files that exist, because it's iterating over the contents of the directory. There are probably other code paths that call AddSubfolder in cases that the file doesn't exist.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> right, I saw that code, but CreateSubfolders only calls AddSubfolder with files
> that exist, because it's iterating over the contents of the directory. There
> are probably other code paths that call AddSubfolder in cases that the file
> doesn't exist.
> 
Per discussion on irc, this is actually do to the .msf files for the folders being created whilst we are running CreateSubfolders and somewhere below AddSubfolder.

The side effects of correcting that are hard to tell, but even if we did, its a good idea to put this patch in anyway as it'll be more robust.

Comment 5

[Mark Banner (:standard8)]

Patch is now checked in -> fixed.