120485 - Can't delete \NoSelect folders on IMAP server cascadely

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Henry Jia]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4) Gecko/20011207
BuildID:    2002011510

Mozilla can't delete folders on IMAP server cascadely. From the communication
and the code, I'm sure that mozilla didn't delete these folders successfully.
Test Enviroment: mozilla nightly build 2002011510, mozilla 0.9.7 on solaris & win2k

Reproducible: Always
Steps to Reproduce:
1. Clear the default settings 'Edit'->'Mail & Newsgroup Account
Settings...'->'Server Settings'->'Advanced...'->'Server supports folders that
contain sub-folders and messages' for IMAP server that supports folders that can
contain either sub-folders or messages
2. Create folders that can only contain sub-folders cascadely
3. Delete the toppest parent folder
4. Mozilla shows it's OK. But in fact, when you refresh the folder list of the
mail window, you'll see the deleted folders.

Actual Results:  The folders can't be deleted cascadely.

Expected Results:  The folders should be deleted cascadely. And from the code,
mozilla tries to do this.

Comment 1

[Henry Jia]

mscott, I'm trying to solve this bug, can this bug assigned to me.

Comment 2

[scottputterman]

reassigning.

Comment 3

[Henry Jia]

Attachment: suggested patch

I test this patch on solaris & win2k professional. I test IMAP 4 and IMAP 4 Rev
1 servers(including UW server).

Comment 4

[Henry Jia]

Attachment: new suggested fix

I made a stupid mistake last time. Now, I give the correct patch.

Comment 5

[Margaret Chan]

[e-mail]:  Would you review the patch for Henry please?  Thanks.

Comment 6

[Scott MacGregor]

I'm sorry I haven't done this yet. I started Friday and then accidentally killed
the window I was typing in =). I'll finish this on Monday for sure. 

Comment 7

[Scott MacGregor]

Comment on attachment 68508 [details] [diff] [review]
new suggested fix

Thanks for the patch Henry. A couple comments, most of them are very minor
nits.

1) we tend to not use the notation where you put a 'b' before booleans, a 'p'
before pointers, etc. In the case of method arguments like bDeleteMyself we do
encourage using an 'a' for argument.

i.e.
DeleteSubFolders(const char * aMailboxName, aDeleteSelf);

instead of using myself, how about self: aDeleteSelf. myself makes it sound
like we are deleting a person =). 

2) small typo in your comment for DeleteSubFolders on: "dicide". the last part
of the comment should be "and provide feedback...."

3) Can you move the comment for DeleteSubFolders above the method instead of
below it?

4) instead of using PL_strlen, people are trying to convert existing code to
just strlen. This is a recent change you wouldn't know about with out me
telling you. For more details: Bug #124536

5) I'd get rid of variables like iLen and pSelectedMailboxDir and
bMyselfInTheSubFolderList and just rename them to things like length,
selectedMailboxDir, etc.

6) PL_strcpy can now be just strcpy

7) I'd express equalties with zero as:
!PL_strcmp(currentName, selectedMailbox)

8) instead of bMyselfInTheSubfolderList
how about folderInSubfolderList
instead of bMyselfDelted how about folderDeleted

9) Instead of using PL_strcmp, use nsCRT::strcmp

Thanks for the patch!

Comment 8

[David :Bienvenu]

In addition to Scott's comments, I have some comments as well:

+    PR_FREEIF(pSelectedMailboxDir);

This can just be PR_Free(pSelectedMailboxDir) since PR_Free checks for null - 
PR_FREEIF checks for null, and also sets pSelectedMailboxDir to null, which I
don't think you need, so it's just extra code. Or, you could use an
nsCAutoString, which will get cleaned up automatically.

Also, the variable named "b" - that's not a useful name for a variable. In fact,
can't you just pass in PR_FALSE directly and get rid of the variable completely?

Comment 9

[Jerry]

according to the bug 124536
sccott's comments 9 may changed to 

9) Instead of using PL_strcmp, use strcmp directly.

Comment 10

[Henry Jia]

Attachment: the revised patch

Thanks, mscott, bienvenu, margaret and jerry. I've revised the patch according
to your suggestions.
Bienvenu, since the argument 'aDeleteSelf' in the function DeleteSubFolders is
not a general parameter, it's a reference parameter and wants to return
something, so we can't pass PR_FALSE directly to the function.

Comment 11

[Scott MacGregor]

Comment on attachment 71624 [details] [diff] [review]
the revised patch

one small nit, should we check for an allocation failure:
selectedMailboxDir = (char *)PR_MALLOC(length+2);

r=mscott

You still need to get an sr= on this. I'd suggest bienvenu.

Comment 12

[David :Bienvenu]

Comment on attachment 71624 [details] [diff] [review]
the revised patch

sr=bienvenu

Comment 13

[Henry Jia]

Should I give a new patch according to mscott's comment 2002-03-05 or the patch 
that has passed r= and sr= will be checked in?
Another thing is that I want to obsolete the patch(attachment 67368 [details] [diff] [review]) and the 
patch(attachment 68508 [details] [diff] [review]), but I don't have the right. Anyone can obsolete them 
to make it clear? How can I get this right? Thanks.

Comment 14

[Scott MacGregor]

just add a check for a failed memory allocation. No need to post a new patch.
Your good to go. 

Looks like someone already obsoleted those 2 patches for you. 

Comment 15

[Margaret Chan]

Unfortunately, Henry does not have checkin privileges yet.  In order for someone
to check in for him, I think it is better for him to add a new patch with the
check.  With that, I guess he will get automatic approval based on
[e-mail]'s comment.  It would be nice if someone can check it in for
him after he has provided the patch.  I am afraid that I may not have the time
to do so until next week.  Thanks, Margaret

Comment 16

[Henry Jia]

Attachment: the patch for checkin

According to Margaret & mscott's comments, I give this new patch. Just add a
malloc check. Can anyone reviewed it and checked in? Thanks very much.

Comment 17

[Henry Jia]

Attachment: the patch for checkin

Make the check more strict. Compare to attachment #71624 [details] [diff] [review], two checks are added.

1. use
	if( selectedMailboxDir )    // only do the intelligent test if there is
enough memory
	{
	    strcpy(selectedMailboxDir, selectedMailbox);
	    selectedMailboxDir[length] = onlineDirSeparator;
	    selectedMailboxDir[length+1] = '\0';
	    PRInt32 i;
	    for( i=0; i<numberToDelete && !folderInSubfolderList; i++ )
	    {
		char *currentName = (char *) m_deletableChildren->ElementAt(i);

		if( !strcmp(currentName, selectedMailbox) ||
!strcmp(currentName, selectedMailboxDir) )
		    folderInSubfolderList = PR_TRUE;
	    }
	}
instead of
	    strcpy(selectedMailboxDir, selectedMailbox);
	    selectedMailboxDir[length] = onlineDirSeparator;
	    selectedMailboxDir[length+1] = '\0';
	    PRInt32 i;
	    for( i=0; i<numberToDelete && !folderInSubfolderList; i++ )
	    {
		char *currentName = (char *) m_deletableChildren->ElementAt(i);

		if( !strcmp(currentName, selectedMailbox) ||
!strcmp(currentName, selectedMailboxDir) )
		    folderInSubfolderList = PR_TRUE;
	    }

2. use 
if( selectedMailboxDir && !strcmp(selectedMailboxDir, longestName) )	// just
myself
instead of 
if( !strcmp(selectedMailboxDir, longestName) )	// just myself

This should be final patch. Checkin wanted.

Comment 18

[David :Bienvenu]

Comment on attachment 77400 [details] [diff] [review]
the patch for checkin

r=bienvenu on last patch

Comment 19

[David :Bienvenu]

Nominating (but I don't know if this will be accepted - it's a bit late)

Comment 20

[Henry Jia]

Not too late, the patch is still fresh. The related files on trunk haven't been
changed these days.

Comment 21

[Scott MacGregor]

Comment on attachment 77400 [details] [diff] [review]
the patch for checkin

I thought I already reviewed this back in February. It sure looks familiar.
Sorry if you were waiting long.

sr=mscott

Comment 22

[Henry Jia]

Thanks, mscott and bienvenu.

a= and checkin wanted

Comment 23

[Pete Zha]

Fix checked in

Comment 24

[Mike Shaver (:shaver emeritus)]

Comment on attachment 77400 [details] [diff] [review]
the patch for checkin

a=shaver for 1.0 branch.

Comment 25

[Henry Jia]

Adding keyword mozilla1.0.1

Comment 26

[Judson Valeski]

please checkin to the 1.0.1 branch ASAP. once there, remove the mozilla1.0.1+
keyword and add the fixed1.0.1 keyword.

Comment 27

[Jay Yan]

checked in to 1.0 branch.

Comment 28

[Scott MacGregor]

if this is really in the 1.0 branch we should change the mozilla1.0.1+ keyword
into  a fixed1.0.0 keyword so QA will know to verify this on the branch. 

Comment 29

[Karen Huang]

Verified on all the platforms that we can delete \NoSelect folders cascadely:
--------------------------------------------------------------------------------
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 53 list "" "28_test1/*"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2/28_test3
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 53 OK LIST 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 54 list "" 
"28_test1/28_test2/28_test3/*"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2/28_test3/
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 54 OK LIST 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 55 delete 
"28_test1/28_test2/28_test3/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 55 OK DELETE 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 56 unsubscribe 
"28_test1/28_test2/28_test3/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 56 OK 
UNSUBSCRIBE completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 57 list "" 
"28_test1/28_test2/*"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2/
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 57 OK LIST 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 58 delete "28_test1/28_test2/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 58 OK DELETE 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 59 unsubscribe 
"28_test1/28_test2/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 59 OK 
UNSUBSCRIBE completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 60 delete "28_test1/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 60 OK DELETE 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 61 unsubscribe "28_test1/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 61 OK 
UNSUBSCRIBE completed
--------------------------------------------------------------------------------
Verified on Windows 2000 06-27-08-1.0 branch build
Verified on Linux 06-27-06-1.0 branch build
Verified on Mac 06-27-05-1.0 branch build

Changing keywords from "fixed1.0.0" to "verified1.0.1".

Comment 30

[Karen Huang]

Verified.