439494 - folderRenamed nsIMsgFolderListener notification in local folders broken

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: patch

The folderRenamed notification in local folders is broken right now, as it doesn't work when there are no subfolders, and it calls once for all subfolders when there are.

The patch fixes them by moving the code into ::Rename() instead of ::RenameSubFolders().

As nsMsgFolderNotificationService::NotifyFolderRenamed is a no-op on current trunk, this really can't cause any adverse effects, so this patch can be checked in.

Comment 1

[Rain [:rain1] (inactive)]

Attachment: uncomment test

This has to be checked in after the test is checked in (bug 439225).

Comment 2

[Rain [:rain1] (inactive)]

Please check in "patch" for now.

Comment 3

[Rain [:rain1] (inactive)]

OK, please don't check it in, i'm not very confident of it applying correctly.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 325298 [details] [diff] [review]
uncomment test

I think it we need to get patches updated first. Note, you don't have to ask me for review on all test cases, the reviewer of the patch is sufficient in most cases.

I would like to look at patches that change the architecture/have issues with how they are being tested (e.g. your other two bugs).

Comment 5

[Rain [:rain1] (inactive)]

Attachment: new patch, combines patch and test, and fixes bitrot

Combines the two patches. Thanks Mark, I'll keep that in mind in the future.

Comment 6

[Mark Banner (:standard8)]

So when I run the test with this patch applied, I notice some extra warnings on the log:

WARNING: NS_ENSURE_TRUE(!pathKey.IsEmpty()) failed: file /Users/moztest/mozilla/hg/mozilla/mailnews/base/src/nsMsgFolderCache.cpp, line 291

repeated three times.

Is this expected/known?

Comment 7

[Mark Banner (:standard8)]

Here's the vital part of the stack:

#0  nsMsgFolderCache::GetCacheElement (this=0x5bd750, pathKey=@0xbfffac8c, createIfMissing=0, result=0xbfffad14) at /Users/moztest/mozilla/hg/mozilla/mailnews/base/src/nsMsgFolderCache.cpp:291
#1  0x01015598 in nsMsgDBFolder::GetFolderCacheElemFromFile (this=0x5cfe00, file=0x5d4d90, cacheElement=0xbfffad14) at /Users/moztest/mozilla/hg/mozilla/mailnews/base/util/nsMsgDBFolder.cpp:490
#2  0x0101594a in nsMsgDBFolder::GetStringProperty (this=0x5cfe00, propertyName=0x13f3366 "folderName", propertyValue=@0xbfffadd8) at /Users/moztest/mozilla/hg/mozilla/mailnews/base/util/nsMsgDBFolder.cpp:1704
#3  0x01168194 in nsMsgLocalMailFolder::SetPrettyName (this=0x5cfe00, aName=@0xbfffae38) at /Users/moztest/mozilla/hg/mozilla/mailnews/local/src/nsLocalMailFolder.cpp:1216
#4  0x0116cbeb in nsMsgLocalMailFolder::RenameSubFolders (this=0x533250, msgWindow=0x0, oldFolder=0x532d14) at /Users/moztest/mozilla/hg/mozilla/mailnews/local/src/nsLocalMailFolder.cpp:1195
#5  0x01172d91 in nsMsgLocalMailFolder::Rename (this=0x532cf0, aNewName=@0xbfffb590, msgWindow=0x0) at /Users/moztest/mozilla/hg/mozilla/mailnews/local/src/nsLocalMailFolder.cpp:1144

So this is happening after the first rename has occurred, so my guess is the folder cache hasn't been written, and the call to GetFolderCacheKey in nsMsgDBFolder::GetStringProperty fails, but because we don't check the return value - it ends up calling GetFolderCacheElemFromFile and then we get the warning in GetCacheElement.

I think this ends up passing because the return statement in nsMsgLocalMailFolder::SetPrettyName is wrong - I think the return statement doesn't do what you expect from looking at it and calls SetStringProperty even if NS_FAILED(rv) is true. 

Comment 8

[Rain [:rain1] (inactive)]

Thanks for debugging this.

(In reply to comment #7)
> 
> So this is happening after the first rename has occurred, so my guess is the
> folder cache hasn't been written, and the call to GetFolderCacheKey in
> nsMsgDBFolder::GetStringProperty fails, but because we don't check the return
> value - it ends up calling GetFolderCacheElemFromFile and then we get the
> warning in GetCacheElement.
Actually the folder cache doesn't seem to come into the picture at all.

http://mxr.mozilla.org/mozilla/source/mailnews/base/util/nsMsgDBFolder.cpp#1044
- I'd be interested to know what the paths of "path" and "summaryName" are, here. If I'm reading the source correctly, they're empty! Obviously that shouldn't happen.

http://mxr.mozilla.org/mozilla/source/mailnews/base/util/nsMsgDBFolder.cpp#1695
dbPath is just passed down to GetFolderCacheElemFromFile, and its path passed to GetCacheElement -- this path is supposedly empty, which leads to our assertion.

For me on Windows, the path is always meaningful and points to the already-renamed summary file of the parent directory -- eg "obj-i686-pc-mingw32\_tests\mailtest\Mail\Local Folders\Inbox.sbd\folder2.msf".

If they *are* empty, I'd want to know exactly what is happening inside GetFilePath() and GetSummaryFileLocation(). Are they somehow failing somewhere? It'd be great if you could step through the code.

Of course, another thing to do is to try with a 250ms timeout between tests ;)

> 
> I think this ends up passing because the return statement in
> nsMsgLocalMailFolder::SetPrettyName is wrong - I think the return statement
> doesn't do what you expect from looking at it and calls SetStringProperty even
> if NS_FAILED(rv) is true. 
> 

That's correct according to bienvenu -- set the folderName if we failed to get it, or if the old and new ones aren't equal.

Comment 9

[Rain [:rain1] (inactive)]

(Thanks Jeff.) WRT the first instance of that warning (in testRename3()):

The call stack at that point is:
GetCacheElement
WriteToFolderCache
FlushToFolderCache
UpdateSummaryTotals
GetSubFolders <- this, and everything above this point, is called for the "old folder"
RenameSubFolders
RenameSubFolders <- this is for the new folder

This is happening after the folders have been successfully moved. What's happening is that it's trying to update the cache for a non-existent summary file.

Now, on Windows (and Linux I believe), the "pathDescriptor"/"pathKey" [1] is the path of the summary file itself. ("...\_tests\mailtest\Mail\Local Folders\Inbox.sbd\folder2.sbd\folder3.msf" on Windows.)

[1] http://mxr.mozilla.org/mozilla/source/mailnews/base/util/nsMsgDBFolder.cpp#1110

Therefore it is never empty. However, on Mac it is some base64 encoded internal representation, which only occurs when the file it's pointing to actually exists. Since the file doesn't exist (it was moved to folder4.sbd/folder3.msf a long time ago) -- the pathKey is empty.

Now the very reason that is even goes down this stack is because for the old folder (that is Inbox.sbd/folder2.sbd/folder3), mInitialized is false [2]. That's probably because I'm not doing any operations on the folder that could set it to true.

[2] http://mxr.mozilla.org/mozilla/source/mailnews/local/src/nsLocalMailFolder.cpp#359

This is what also leads to the weird directory structure:
-- START --
Inbox         Trash                  Unsent Messages.msf
Inbox.msf              Trash.msf            msgFilterRules.dat
Inbox.sbd              Unsent Messages
 
./Inbox.sbd:
folder2.sbd     folder3.msf  folder4.msf
folder3  folder4    folder4.sbd
 
./Inbox.sbd/folder2.sbd:
folder3
 
./Inbox.sbd/folder2.sbd/folder3:
 
./Inbox.sbd/folder4.sbd:
folder3  folder3.msf
-- END --

I'm not sure it's a cause for concern, given this.

Comment 10

[Rain [:rain1] (inactive)]

Attachment: Patch to ensure that subfolders are discovered immediately on rename

mInitialized is actually "whether the subfolders have been discovered".

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 327331 [details] [diff] [review]
Patch to ensure that subfolders are discovered immediately on rename

+      // Discover the subfolders inside this folder (this is recursive)
+      nsCOMPtr<nsISimpleEnumerator> enumerator;
+      newFolder->GetSubFolders(getter_AddRefs(enumerator));

Neil changed GetSubFolders so that you can call it without a parameter, i.e. GetSubFolders(nsnull).

r=me with that changed, as I know this is a common construct that we use for initialising folder trees. Please get David to ok it as well.

Comment 12

[David :Bienvenu]

Comment on attachment 327331 [details] [diff] [review]
Patch to ensure that subfolders are discovered immediately on rename

yes, it was my suggestion over irc to try this, so sr=me, modulo Standard8's comment.

Comment 13

[Rain [:rain1] (inactive)]

Attachment: update addressing review comment

carrying forward r=Standard8, sr=bienvenu

Comment 14

[Mark Banner (:standard8)]

I checked both patches in as one:

Checking in mailnews/base/test/resources/msgFolderListenerSetup.js;
/cvsroot/mozilla/mailnews/base/test/resources/msgFolderListenerSetup.js,v  <--  msgFolderListenerSetup.js
new revision: 1.3; previous revision: 1.2
done
Checking in mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js;
/cvsroot/mozilla/mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js,v  <--  test_nsIMsgFolderListenerLocal.js
new revision: 1.3; previous revision: 1.2
done
Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.594; previous revision: 1.593
done