Can't delete \NoSelect folders on IMAP server cascadely

VERIFIED FIXED in mozilla1.0.1

Status

MailNews Core
Networking: IMAP
--
major
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: Henry Jia, Assigned: Henry Jia)

Tracking

Trunk
mozilla1.0.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

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

Comment 2

17 years ago
reassigning.
Assignee: mscott → Henry.Jia
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

16 years ago
Created attachment 67368 [details] [diff] [review]
suggested patch

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

Comment 4

16 years ago
Created attachment 68508 [details] [diff] [review]
new suggested fix

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

Comment 5

16 years ago
mscott@netcape.com:  Would you review the patch for Henry please?  Thanks.

Comment 6

16 years ago
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

16 years ago
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

16 years ago
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

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

9) Instead of using PL_strcmp, use strcmp directly.

(Assignee)

Comment 10

16 years ago
Created attachment 71624 [details] [diff] [review]
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.

Updated

16 years ago
Keywords: patch, review

Comment 11

16 years ago
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.
Attachment #71624 - Flags: review+

Comment 12

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

sr=bienvenu
Attachment #71624 - Flags: superreview+
(Assignee)

Comment 13

16 years ago
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.

Updated

16 years ago
Attachment #67368 - Attachment is obsolete: true

Updated

16 years ago
Attachment #68508 - Attachment is obsolete: true

Comment 14

16 years ago
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

16 years ago
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
mscott@netscape.com'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
(Assignee)

Comment 16

16 years ago
Created attachment 75908 [details] [diff] [review]
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.
(Assignee)

Comment 17

16 years ago
Created attachment 77400 [details] [diff] [review]
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.
Attachment #75908 - Attachment is obsolete: true

Comment 18

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

r=bienvenu on last patch
Attachment #77400 - Flags: review+

Comment 19

16 years ago
Nominating (but I don't know if this will be accepted - it's a bit late)
Keywords: nsbeta1
(Assignee)

Comment 20

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

Comment 21

16 years ago
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
Attachment #77400 - Flags: superreview+
(Assignee)

Comment 22

16 years ago
Thanks, mscott and bienvenu.

a= and checkin wanted
(Assignee)

Updated

16 years ago
Keywords: approval

Comment 23

16 years ago
Fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Comment on attachment 77400 [details] [diff] [review]
the patch for checkin

a=shaver for 1.0 branch.
Attachment #77400 - Flags: approval+

Updated

16 years ago
Target Milestone: --- → mozilla1.0.1
(Assignee)

Comment 25

16 years ago
Adding keyword mozilla1.0.1
Keywords: mozilla1.0.1

Comment 26

16 years ago
please checkin to the 1.0.1 branch ASAP. once there, remove the mozilla1.0.1+
keyword and add the fixed1.0.1 keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+

Comment 27

16 years ago
checked in to 1.0 branch.

Comment 28

16 years ago
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. 
Keywords: mozilla1.0.1+ → fixed1.0.0

Comment 29

16 years ago
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".
Keywords: fixed1.0.0 → verified1.0.1

Updated

16 years ago
Summary: Can't delete folders on IMAP server cascadely → Can't delete \NoSelect folders on IMAP server cascadely

Comment 30

16 years ago
Verified.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.