89698 - hang when renaming folder to a folder name with slash & back slash

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Karen Huang]

Used 07-06-13-0.9.2 build

hang when renaming folder to a folder name with slash

1)Login to an IMAP mail account
2)Select a normal folder name (e.g.family)
3)Tried to rename the folder name for adding a slash to "family/friend"
4)Actual Results: Application will hang, need to use the Task Manager to end the 
task

Expected Results: Shouldn't hang when renaming folder to a folder name with 
slash

Comment 1

[Karen Huang]

Attachment: Attach the alert should display instead of hang on 6.1....

Comment 2

[Karen Huang]

Nominating nsbeta1, adding keywords hang.

Comment 3

[Karen Huang]

adding back slash on the summary since application also hang when renaming 
folder to a folder name with back slash too.

Additional Info: 
Problem is occurring for *RENAMING* Folder name with slash & back slash, problem 
is NOT occurring when CREATING folder name with slash & back slash......

Comment 4

[Navin Gupta]

reassigning to self. 

Comment 5

[Navin Gupta]

Attachment: proposed fix.

Comment 6

[Navin Gupta]

If the newName has hierarchyDelimiter then do not issue a rename. This is what
4x does. Alert string is also a replica of 4x alert. 

cc bienvenu for review. 

Comment 7

[David :Bienvenu]

Shouldn't you check if msgWindow is null? And that error message needs work,
even if it was in 4.x, cc'ing jglick for input. At least say "reserved by the
imap server" if nothing else.

Also, you shouldn't be calling nsTextFormatter directly - instead, you should
get the string bundle and use FormatStringFromID, which can take an
nsXPIDLString. You might want to add a utility method IMAPGetStringBundle to
nsImapStringBundle, use that to get the bundle, and then format the string - you
can use lxr to see how to use FormatStringFromID.

Comment 8

[Navin Gupta]

Attachment: proposed fix, v2

Comment 9

[David :Bienvenu]

is this cast needed? +             (const PRUnichar*) m_hierarchyDelimiter


Also, IMAPGetStringByID should use the new method to get the bundle - otherwise,
it's just copied code, right?

Comment 10

[Navin Gupta]

that cast is needed.

I 'll make that change.

Comment 11

[Navin Gupta]

Attachment: proposed fix w/ little more clean up

Comment 12

[Navin Gupta]

I have tried to use nsXPIDLString wherever possible and for passwordPrompt
FormatStringFromID was not working, so left it as such. Also I have tested
the changes I have made. 

Comment 13

[David :Bienvenu]

good job, one nit, in +IMAPGetStringBundle(nsIStringBundle **aBundle)
+

if the string service is null but for some reason rv is NS_OK, we should still
return NS_ERROR_FAILURE or some error code (now we'd return NS_OK).

What do you mean, "for passwordPrompt FormatStringFromID was not working"? Is
there a bug out there that we should be aware of?

r=bienvenu, if you fix the nit, but I'd like to know a little more about the
password prompt problem.

Comment 14

[Navin Gupta]

I have made that minor nit change. So if you look at PromptForPassword()
we use nsTextFormatter, so I tried to change it to use FormatStringFromID
but it didn't work. Only the first char of username and hostname were 
being displayed. for example "Enter your password for n@n" instead of 
"naving@nsmail-2". So I didn't change it. 

Comment 15

[David :Bienvenu]

that's a sign that someone has got confused between unicode and ascii strings -
it might be fixed by just changing the format string to take unicode strings.

Comment 16

[Suresh]

r=suresh

Comment 17

[Navin Gupta]

cc jatin for alert wording review. jatin, see the alert wording here 
"------- Additional Comments From Karen Huang 2001-07-06 16:22 -------"
in this bug. 

Comment 18

[Navin Gupta]

fix checked in, will make changes to alert wording, if needed. 

Comment 19

[jglick]

I don't think most users care that the char is reserved, they just need to 
know they need to pick a different name.  How about:

"The folder name can not contain the %c character. Please choose another name."

Robin, this ok? Is it "can not" or "cannot"? Thanks. 

Comment 20

[robinf]

"cannot" is correct. Jen - your changes are fine with me, thanks.

Comment 21

[Navin Gupta]

alert wording changes checked in. Marking fixed. 

Comment 22

[Karen Huang]

>I don't think most users care that the char is reserved, they just need to 
>know they need to pick a different name.  How about:

>"The folder name can not contain the %c character. Please choose another name."

No. I disagree.
I still prefer 4.7 alert since we need to consider other IMAP servers..... 

Comment 23

[jglick]

Karen, what are your concerns? That users need to know its an IMAP server? Or 
that this particular IMAP server can't accept "/"?

Comment 24

[Karen Huang]

Users should be able to rename the folder name with "/" for those IMAP servers 
whose hiearchy delimiter is not '/'....(e.g. Cyrus & Lotus).

I tried on Cyrus and it works (I don't have account for Lotus....but I suspect 
that it will work....)

It is really depend on the IMAP servers. I will agree to make it more sense for 
users, but I am thinking that shouldn't use "The folder name can not contain the 
%c character" for global users.....

Comment 25

[jglick]

"The / character cannot be used in the folder name for this server. Please 
choose another name." 

Better?

Comment 26

[robinf]

I'd recommend: "For this server, the / character cannot be used in a folder
name. Please choose another folder name."

Comment 27

[Karen Huang]

Attachment: Attach the alert displays on Outlook Express

Comment 28

[Karen Huang]

Attachment: Attach the alert displays on Outlook Express for Cyrus server

Comment 29

[Karen Huang]

I don't know why our client did handle Cyrus for renaming folder with slash, and 
Outlook Express didn't - is this their bug? 

Anyway, I still like the way on 4.7 - since users still can use folder with 
slash (e.g. They can create the fodler with slash - our client will treat it for 
folder/subfodler, but just cannot rename folder with the slash!)

Comment 30

[Navin Gupta]

For cyrus the hierarchyDelimiter is '.' so '/' should be acceptable. 

Comment 31

[Karen Huang]

Ccing John for get the suggestion from him....

Comment 32

[jglick]

"The "/" character is reserved on this IMAP server. Please choose another folder 
name."

Is at least better than the current 4.x:  "The / character is reserved on imap 
server. Please choose another name."

I think David just wanted this alert to at least us proper English.

Comment 33

[David :Bienvenu]

Yes, thanks, Jennifer, proper english was what I was driving at :-)

Comment 34

[Karen Huang]

Navin, have you fixed this bug for back slash as well?
Hang is not occurring on renaming folder name with slash, but hang is still 
occurring on folder name with back slash.

Also, are we going to change the alert to the new alert that described above?
It seems that it still displays: "The folder name cannot contain the / 
character. Please choose another name."..........

Comment 35

[Karen Huang]

I also logged bug 99150 for addressing Cyrus renaming folder name with slash 
problem......

Comment 36

[Karen Huang]

Based on the summary including "back slash" scenario.
Reopening this bug since hang is still occurring when renaming the folder name 
with the "back slash"....

Comment 37

[Navin Gupta]

Well, the description and summary do not match. accepting.

Comment 38

[Karen Huang]

Please see above 2001-07-06 16:59 my comments.
I added "back slash" scenario at that time....

---------------------------------------------------------------------------
adding back slash on the summary since application also hang when renaming 
folder to a folder name with back slash too.

Additional Info: 
Problem is occurring for *RENAMING* Folder name with slash & back slash, problem 
is NOT occurring when CREATING folder name with slash & back slash......
--------------------------------------------------------------------------------

Comment 39

[Navin Gupta]

Attachment: proposed fix for backslash part.

Comment 40

[Navin Gupta]

The fix is to escape the folderName before passing url to necko so that 
necko won't replace '\' with '/' and also use Msg_hashIfNecessary to 
get the disk name. 

I have made changes to the alert dialog yet another time, as per last suggestion 
from jglick. david, please review. 

Comment 41

[David :Bienvenu]

what tests have you run? This kind of change requires lots of testing. Have you
tried things like renaming folders to something like "a&b" (this requires a
mod-utf7 encoding) or other characters that require escaping?

Comment 42

[Navin Gupta]

Well, I was trying mostly to fix the backslash problem. The fix also works 
for a&b. I have tried chinese chars as well and it works. 

Comment 43

[David :Bienvenu]

OK, r=bienvenu

Comment 44

[Scott MacGregor]

Comment on attachment 48994 [details] [diff] [review]
proposed fix for backslash part.

Don't we leak excapedNewName?

+            char *escapedNewName = nsEscape(utfNewName, url_Path);
+            urlSpec.Append(escapedNewName);


nsEscape allocates a new string for the escaped string since an escaped character takes up more bytes than a non-escaped character.

Comment 45

[Navin Gupta]

ok, I have added.  

+            nsCRT::free(escapedNewName);

Comment 46

[Scott MacGregor]

Comment on attachment 48994 [details] [diff] [review]
proposed fix for backslash part.

One last thing, what if nsEscape fails and returns null? We should probably check for that before calling .Append and before freeing a null string using nsCRT::free

Comment 47

[Navin Gupta]

Well, nsCRT::free should handle null string. But I will make it safe and 
change it to PR_FREEIF. 

Comment 48

[Scott MacGregor]

no what you had is okay. But we should check for a null string after our
nsEscape call before we call .Append 

Comment 49

[Navin Gupta]

ok, did that as well 

+if (!escapedNewName) return NS_ERROR_NULL_POINTER;

Comment 50

[Scott MacGregor]

Comment on attachment 48994 [details] [diff] [review]
proposed fix for backslash part.

sr=mscott

Comment 51

[timeless]

shouldn't you return outofmemory instead of nullpointer?

Comment 52

[Navin Gupta]

fix checked in. 

Comment 53

[Karen Huang]

Since this kind of change requires lots of testing as David mentioned in this 
bug -- "slash" actually is fixed on branch, but "back slash" only fixed on the 
trunk build. I am adding vtrunk for the keywords, will verify this bug on trunk 
after RTM.