Closed Bug 434708 Opened 14 years ago Closed 14 years ago
When creating folders don't iterate the directory whilst we are changing its contents, and remove a couple of bogus warnings
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 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?
(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.
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.
(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.
Patch is now checked in -> fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
You need to log in before you can comment on or make changes to this bug.