317674 - rename folder fails for Unicode folder names

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Dimitri Sverdlov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Thunderbird version 1.5 RC1 (20051025)

Renaming folders fails if the old and/or the new folder name use Unicode characters.

Reproducible: Always

Steps to Reproduce:
1. Create a folder "test"
2. Try to rename it to "тест"


Actual Results:  
Alert: "The folder could not be renamed. Perhaps the folder is being reparsed, or the new name in not a valid folder name."

Note that creating a *new* folder named "тест" works fine. An other bug here is that a newly created folder "тест" can't be deleted.

Other examples:
works:
* renaming "test тест" to "test" 

doesn't work:
* renaming "test тест" to "тест" 
* renaming "test тест" to "testtest тест" 
* renaming "тест" то "test тест"
* renaming "тест" то "тесттест"
etc.

Comment 1

[Dimitri Sverdlov]

P.S. - Sorry for the HTML-encoded characters, i didn't know bugzilla would convert them to that. But I hope you get the idea.

Comment 2

[David :Bienvenu]

this is a local folder you're trying to rename?

Comment 3

[David :Bienvenu]

if this is true in general (and I suspect it is), it would be bad...

Comment 4

[Dimitri Sverdlov]

Yes, that's true in general - local folders, POP3 folders, RSS folders.

Comment 5

[David :Bienvenu]

This is failing in PR_Rename - nsMsgI18NCopyUTF16ToNative is giving us a string back with '?' in it, and I suspect that PR_Rename is not happy with that. It calls MoveFile, which I guess is a win32api? The weird thing is that creating a folder with the same name works fine...

Comment 6

[Jungshik Shin]

Dimitri, what's your OS locale? Are you running a Russian Windows XP? Next time you write a comment with any character outside the US-ASCII (e.g. Cyrillic letters), please, set View | Character Encoding to UTF-8 *before* filling in the textarea for 'additional comments'.

Hopefully, when I begin to work on bug 162361 (this weekend), I'll be able to come up with a rough idea to fix this as well.

Comment 7

[Jungshik Shin]

Apparently, when you create a new folder with character outside the current locale (ANSI-based locale), those characters are replaced by '_' *on* the disk (this replacement is done by Windows 'A' APIs). When removing or renaming, we go through UTF16->Native converter which replaces those characters with '?'. Therefore, we can't rename or remove it while we can create it with '_' in its on-disk filename.

BTW, I realized that that part of mailnews still relies on nsFileSpec... That means fixing bug 162361 will not automatically get 'propagated' there...

We need to fix this problem, but if we can't soon, we'd better block the creation of a folder with characters not representable on the current file system (well, NTFS uses Unicode, but our code doesn't yet fully exploit that)

Comment 8

[Dimitri Sverdlov]

(In reply to comment #6)
> Dimitri, what's your OS locale? Are you running a Russian Windows XP?

No, I'm running a German XP with a German locale, but I just often type in Russian.
So that should be the problem, as you wrote.

Comment 9

[David :Bienvenu]

Jungshik, c7 explains why we wouldn't be able to rename a folder that already had unicode characters, but what I also saw was that we couldn't rename an ascii folder name to a name that had unicode characters. Does that same explanation hold?

We're working on getting rid of uses of nsFileSpec but I'm pretty sure nsLocalMailFolder will be the last user to go.

Comment 10

[Jungshik Shin]

(In reply to comment #9)
> Jungshik, c7 explains why we wouldn't be able to rename a folder that already
> had unicode characters, but what I also saw was that we couldn't rename an
> ascii folder name to a name that had unicode characters. Does that same
> explanation hold?

I took a closer look and found the cause. We use 'nsMsgI18NCopyUTF16ToNative' to convert a new name to the native file system name which replaces unconvertible characters with '?'. However, '?' is invalid on Windows file system. We do check for invalid characters, but we do it *before* the conversion so that '?' passes through to lead to an error from a Windows API. A very quick fix would be to use CopyUnicodeToNative that takes care of invalid characters. A better fix would be to check if there's any unconvertible character in a folder name, in which case we can use a hashed name. 

> We're working on getting rid of uses of nsFileSpec but I'm pretty sure
> nsLocalMailFolder will be the last user to go.

This way, we can allow local folders whose names contain characters not covered by the file system character encoding even before we fix bug 162361 or convert nsLocalMailFolder to nsILocalFile.

One problem with this approach is that there may be a migration issue when we eventually support the full Unicode range on Windows. This is not a concern on Mac OS X and modern Linux (where UTF-8 is the default). 

David, what do you think? 



 

Comment 11

[Jungshik Shin]

Attachment: patch

I've just tested this patch on Linux with a non-UTF-8 locale. It seems to work well. I'll make further tests on Windows. With this patch, any Unicode character can be used for local folder names. If any character cannot be covered by the file system character encoding, its hash value is calculated and used instead.

Comment 12

[Jungshik Shin]

Comment on attachment 207970 [details] [diff] [review]
patch

I've also tested this on Windows and it worked as intended. 
As for  the migration issue (when we can use the full Unicode-repertoire on Windows), it appears that there shouldn't be any as long as the mapping between human-readable fold name (shown in the folder list) and on-disk name (hashed if necessary) is well-kept.

Comment 13

[David :Bienvenu]

Comment on attachment 207970 [details] [diff] [review]
patch

This patch changes the name of the .msf file on disk, for a given folder with intl characters, right? For existing folders, I think that means the old .msf file will get orphaned, and we'll generate a new one when you click on the folder - have you tried that?

Comment 14

[Jungshik Shin]

> file will get orphaned, and we'll generate a new one when you click on the
> folder - have you tried that? 

This patch only affects characters NOT covered by the file system encoding (e.g. Cyrillic on English Windows, Arabic on Japanese windows). That is, Kanji on Japanese Windows, Arabic on Arabic Windows, O-umlaut on English windows etc are NOT affected. 

Currently, characters not covered by the current file system encoding are converted to '_' when a folder is created on disk. However, our internal folder DB(?) keeps its original name *along with* its on-disk name(both folder itself and the corresponding msf file), right? As long as the name with uncovered characters replaced by '_'  is *unique*, this is fine, I think. When a user upgrades to a new version with my patch, this should continue to work if there is a separate folder DB in which our internal folder name (which is also shown to a user in the folder list)  is mapped to its on-disk name. 

Is the last 'if ...' the case or not? I thought that's the case because otherwise 'hashed' on-disk names wouldn't have worked, but your question and my memory of some 'msf' file mix-up in the past make me less certain. However, my test with 1.5RC2 and a build with my patch applied indicates that it's the case (i.e. we don't rely on on-disk names to get the list of folders, but we do have a separate folder DB). What I did was:

1. create a folder with two Devanagari characters followed by 'ab' ( सवab ) with 1.5 RC2
2. add some messages to it
3. go to the folder and read a couple of messages I just copied there
4. quit 1.5RC2
5. Check the on-disk name. It's '__ab' and '__ab.msf' because Devanagari characters are not covered by my current file system encoding.
6. launch my build with the patch applied
7. go to the folder (सवab) and make certain that the folder name (सवab) is intact
8. add a couple of more messages to the folder
9. see if there's any change in the names of on-disk folder and msf file
10. quit my build and check the on-disk file names once more.
11. launch 1.5RC2 and repeat 6 though 9
12. rename 'सवab' to 'usedtobehindi'
13. check the on-disk name to make sure that there's any stale *msf file ('__ab.msf') 

Comment 15

[Jungshik Shin]

David, do you have any test scenario other than what I described in the previous comment? 

Comment 16

[David :Bienvenu]

Junshik, terribly sorry, I didn't see your last comment - that test scenario looks fine.

Comment 17

[Jungshik Shin]

Comment on attachment 207970 [details] [diff] [review]
patch

David, thank you. No need to be sorry because it's me who forgot to add you to Cc when adding the description of my test.

BTW, do I have to ask for r separately from Scott? Or would you double as reviewer as well? I don't recall canceling 'review request', but somehow it's cancelled.

Comment 18

[David :Bienvenu]

you can put me down as r/sr

Comment 19

[Jungshik Shin]

thanks, David. patch landed on the trunk. i'll let it bake for a while on the trunk before asking for a branch approval. 

Comment 20

[Frederic Bezies]

sorry to spam this bug, but is it related to this display problem in newsgroups name, see bug 327037

Comment 21

[Scott MacGregor]

Comment on attachment 207970 [details] [diff] [review]
patch

has this baked long enough for branch consideration?

Comment 22

[Scott MacGregor]

Comment on attachment 207970 [details] [diff] [review]
patch

jshin, I think this has baked long enough and is ready for the branch. lemme know if you feel differently. Thanks!

Comment 23

[Jungshik Shin]

(In reply to comment #22)
> (From update of attachment 207970 [details] [diff] [review] [edit])
> jshin, I think this has baked long enough and is ready for the branch. lemme
> know if you feel differently. Thanks!

I agree that this should go in. I've just landed it on branch.