41944 - Local Folder: Unknown error selecting new folder with "?" in name.

Status: RESOLVED WORKSFORME

(SeaMonkey :: MailNews: Message Display, defect)

Description

[laurel]

Using jun8 commercial build

Steps:
1.  From mail window, File|New Folder.
2.  Select Local Folders as the parent for the new folder.
3.  Give the folder a name with a question mark in it, such as "What?". Confirm
OK to create the folder, folder created and appears in Local Folder hierarchy.
4.  Select the new folder.

Result:  Alert dialog appears "unknown error".  After OKing the  alert, I can
copy messages to the folder, folder remains through exit.

Comment 1

[scottputterman]

moving to M18.

Comment 2

[scottputterman]

nominating for nsbeta3

Comment 3

[lchiang]

- per mail triage

Comment 4

[scottputterman]

sorry for the extra email. Removing mail2 keyword.

Comment 5

[Håkan Waara]

It won't accept an "OK" for me, if the proposed foldername starts with a
questionmark.

Comment 6

[timeless]

? is an illegal symbol in fat16, it's also generally a high risk symbol for 
file systems.

I'd suggest wontfix.

If we do intend to fix this, it means that mailboxes will have to store 
internal names that can differ from external names. and then what do you do 
when you have 2 folders w/ the same internal name?

IE is cute.

I'll attach a picture just for the sake of arguement.

Comment 7

[timeless]

Attachment: oe foo? cute but um, wrong.

Comment 8

[laurel]

cc Esther.  She's seen with a comma "," in the folder name.

Comment 9

[esther]

Additional information.  When a user creates a Local folder with a comma in the 
name, we create a duplicate folder with a randomly generated name after closing 
and reopening app.

1. Launch mail
2. Create a new folder under Local Folders called:  comma, this
3. Open the folder, you get the "unknown error 80520012", OK this error
4. Drag messge in that folder
5. Exit app
6. Launch mail again and view your Local Folder 

Result:  You will have a duplicate of comma, this .  The dup folder has a 
randome name something like this 8c798a9.   It will have the message in it too.
Expected:  If the comma was illegal, we shouldn't allow the name.  If the comma 
is legal, we shouldn't create a dup folder.  

Note: If this folder was migrated from 4.7 it will substitue the comma for an 
Underscore (example comma_this).

This confuses the user, and I have found dataloss when you delete the random 
folder, it deleted the message in the original folder.  If the user had done a 
shift delete or deleted the trash the message is not retrievable.

Comment 10

[Jaime Rodriguez, Jr.]

Adding Jenm.

Comment 11

[scottputterman]

reassigning to cavin, cc naving. We should handle all illegal characters.

Comment 12

[Karen Huang]

IMAP new folder with "?" in name has been covered from bug 20324.
This is for Local Folder -> Ccing sheela.

Comment 13

[scottputterman]

moving to 0.9.3 per pdt

Comment 14

[(not reading, please use [e-mail] instead)]

updating summary.  there is a related migration bug that I'm logging now.

Comment 15

[(not reading, please use [e-mail] instead)]

migration issue spun into bug #87380

Comment 16

[(not reading, please use [e-mail] instead)]

I believe we have to follow the same fix I made for news, see nsNewsFolder.cpp, 
CreateSubfolder().

you have to hash the name before you create the file for the db, and then set 
the name of the folder [folder->Name()] to be the "real name".

We'll have to fix Rename(), CopyFolderLocal() CheckIfFolderExists()

Comment 17

[Cavin Song]

Attachment: Patch file.

Comment 18

[Cavin Song]

The patch fixes the problem described in this bug (ie, creation of local folders 
with illegal chars). There are a few bugs related to local folder rename 
problems (64462, 87872, etc) which will be addressed by Navin. Just want to be 
clear that these problems are not caused by the patch.

The way we implement the fix is that we 'hash' the illegal folder name and use 
it to create the physical files on disk. Then we store the illegal folder name 
in the folder cache file so that we can carry the name from one session to 
another. For example, assuming folder 'a,b' has the hashed folder name 
'3d350ca7', then files 'a4eb90c2' and '3d350ca7.msf' are created on disk and the 
name 'a,b' is stored in the folder cache file (panacea.dat).

One problem I found and still don't a solution to is the following scenario:

 Folder 'a,b' contains a subfolder 'a?b' and you rename 'a,b' to 'ab'.

After the rename is done you'll see that folder 'ab' now contains subfolder 
'a4eb90c2' which is the hashed name for 'a?b'. The reason why this is happening 
is that we remove the cache info for all the subfolder of 'a,b' when we rename 
'a,b' to 'ab'. So by the time we rebuild the subfolders for 'ab' we have no idea 
about the subfolders' original names and we can't hash 'a4eb90c2' back to 'a?b'.
Same problem when you lose the folder name info in the cache file (panacea.dat).

Another problem is that if you try to name 'a,b' to '3d350ca7' it'll fail 
because we hash 'a,b' to '3d350ca7' and then check if file '3d350ca7' exists. 
Not much we can do about this and since it's not very likely that this will 
happen so we should be fine with the issue.

Comment 19

[(not reading, please use [e-mail] instead)]

> For example, assuming folder 'a,b' has the hashed folder name 
> '3d350ca7', then files 'a4eb90c2' and '3d350ca7.msf' are created on disk and 
> the name 'a,b' is stored in the folder cache file (panacea.dat).

you mean:  "then files '3d350ca7' and '3d350ca7.msf' are created", right?

> One problem I found and still don't a solution to is the following scenario:
> Folder 'a,b' contains a subfolder 'a?b' and you rename 'a,b' to 'ab'.

start a new bug on this.  I think it would be ok to check in with that problem 
outstanding.  in the new bug we can talk about how to fix it.  (somehow we're 
going to have to recursively remove the old folder cache elements and add new 
ones.)

> Another problem is that if you try to name 'a,b' to '3d350ca7' it'll fail 
> because we hash 'a,b' to '3d350ca7' and then check if file '3d350ca7' exists. 
> Not much we can do about this and since it's not very likely that this will 
> happen so we should be fine with the issue.

when it fails, does the user get an alert saying "you can't create that 
folders, since that folder already exists"?
we can't just call "MakeUnique()" blindly, since that would allow users
to create folders with the same (legal) name over and over.  but, we might be 
able to get fancy and do "MakeUnique()" if it turns out the parent folder 
doesn't have immediate children with the same pretty name.

but I agree it is not very likely, but start a new bug (and we'll punt on it), 
since it is not worth the effort right now.

I'll go review the patch.

Comment 20

[Cavin Song]

1. Yes, I meant "then files '3d350ca7' and '3d350ca7.msf' are created" (cut and 
paste error).
2. Yes, we do prompt users for the error (indicating that the folder already 
exists).

I'll log these two problems. 

Comment 21

[(not reading, please use [e-mail] instead)]

comments:

1) I don't think onlineName is appropriate for local folders.  local folders 
aren't online.  (it makes sense for imap folders, which are actually online).  
I see that dbfolderInfo uses "onlineName", so we can't change that.  but 
perhaps in localmailfolder.cpp, we can come up with a different name.

2)

+  if (NS_SUCCEEDED(rv) && (const char *) onlineName && nsCRT::strlen((const 
char *) onlineName))

do use .get() instead of casting.

3) 

to be safe, in WriteToFolderCacheElem() and ReadFromFolderCacheElem() you might 
want NS_ENSURE_ARG_POINTER(element); 

unless the caller checks for null before calling.

4)

+      nsXPIDLCString onlineFullUtf7Name;
+      rv1 = cacheElement->GetStringProperty("onlineName", getter_Copies
(onlineFullUtf7Name));
+      if (NS_SUCCEEDED(rv1) && onlineFullUtf7Name.get() && nsCRT::strlen
(onlineFullUtf7Name.get()))
+       currentFolderNameStr.AssignWithConversion(onlineFullUtf7Name.get());

this code (looks copied from the imap implementation) is misleading. onlineName 
is not really a UTF7 string.  

some of the string converting that is going on (from CString to String and 
back) has me worried for the non ASCII case.

you've made the "onlineName" attribute a string property, like it is for imap.
that works for imap, since the online name is in modified UTF7, which is 7 bit 
clean and we convert it later before coming up with the PRUnichar * version of 
the name.  (note, for dbfolderInfo, it is a PRUnichar * property)

in nsLocalMailFolder.cpp, when the user creates a folder, folderName is a 
PRUnichar *, and we convert that to a CString.   if I tried to create a non 
ASCII local folder name, would would happen?

we might have to take the PRUnichar *, and turn it into escaped UTF8 (which is 
7bit clean), store that as the "onlineName" property, and then pulling it out 
of the folder cache, turn it back into a PRUnichar *.

Comment 22

[scottputterman]

This fix would be nice to have.  My biggest concern is that it affects how we
create name for local folders (which is most of the users out there) and the
patch is relatively big.  This is one of the ones I think we need to feel very
confident about before taking onto the branch.

Comment 23

[(not reading, please use [e-mail] instead)]

the "rename, start" bug that cavin is going to log is very similar to a bug 
naving owns for imap folders.  (#70517)

Comment 24

[Cavin Song]

Attachment: Patch for ASCII only fix. Also fixed missing panacea.dat problem.

Comment 25

[Cavin Song]

This patch only supports 7bit ASCII and the 1st problem described above is also 
fixed:

  Folder 'a,b' contains a subfolder 'a?b' and you rename 'a,b' to 'ab'.

In addtion, the problem of not showing the correct folder display names when 
panacea.dat is missing or corrupted is fixed.

One new problem that was discovered and has not been fixed is folder names with 
slash(es) (like 'a/b'). For now, code is added to disallow folder names with 
slash(es) to be created or renamed. An error dialog is displayed in this case. 
I'll file a new bug against it.

I'll also have to try the patch on linux and mac to see if I need to change the 
special char list in CheckIfContainsSpecialChars() (ie, to see if there are any 
chars that don't work on these two platforms).  Loading the latest code now.

Comment 26

[(not reading, please use [e-mail] instead)]

minor comments.  over all, it is looking good.

1)  cavin and I decided to move the existing bug (the "a/b" problem) out to 
another bug.  see #89986

so the plan is to remove that part of the fix from this patch.

2)

+  rv = GetDBFolderInfoAndDB(getter_AddRefs(folderInfo), getter_AddRefs(db));
+  // do this after GetDBFolderInfoAndDB, because it crunches 
m_displayFolderName (not sure why)

any idea yet why GetDBFolderInfoAndDB() stomps on m_displayFolderName()?

3)

+  folderInfo = null_nsCOMPtr();

folderInfo is a COMPtr, so once the method returns and folderInfo goes out of 
scope, it will release its ref.  I don't think you have to do it manually.

4)

+NS_IMETHODIMP nsMsgLocalMailFolder::GetDisplayName(PRUnichar ** aDisplayName)
+{
+  if (!aDisplayName)
+    return NS_ERROR_NULL_POINTER;

just do NS_ENSURE_ARG_POINTER(aDisplayName);

5)

+  ReadDBFolderInfo(PR_FALSE); // update cache first.

ReadDBFolderInfo() returns a nsresult.  we don't care about the return result?

if we really don't care what the return value is, we should do this:

(void)ReadDBFolderInfo(PR_FALSE);

6)

before landing, we should double check that we haven't broken any existing 
functionality for users on with non latin-1 or non ASCII system charsets.

it looks like we should be ok, but it would be bad if this fix makes it so 
those users who could create non-ASCII folders are now unable too.

nhotta, can you help test or help review when we've got a final patch?

Comment 27

[Navin Gupta]

I can review the next patch. I am wondering why aren't we fixing to support 
the unicode folder names or is it taken care of in another bug. 

Comment 28

[Håkan Waara]

FYI: |null_nsCOMPtr| is deprecated. If you plan to use it, just use 0 instead.

Comment 29

[(not reading, please use [e-mail] instead)]

> I am wondering why aren't we fixing to support 
> the unicode folder names or is it taken care of in another bug. 

we'll spin that issue of to another bug.

currently, we only support local folders that have ASCII names or names in the
system char set.

cavin and I see a way to extend his fix to allow for local folder names to be in
ANY languange no matter what the system charset, but that will take more work
and more testing, so we'll wait on it until after this fix is in.  (assuming
folders in the system charset still works.)

Comment 30

[(not reading, please use [e-mail] instead)]

to elaborate on 

+  ReadDBFolderInfo(PR_FALSE);

unless there is a good reason not to check rv (for example, we expect to fail 
in certain cases, so we don't want to return error if that call fails)

this should be:

rv = ReadDBFolderInfo(PR_FALSE);  // update cache first.
NS_ENSURE_SUCCESS(rv,rv);

otherwise, do this:

// ignore failure, because...
(void)ReadDBFolderInfo(PR_FALSE);  // update cache first.

Comment 31

[Cavin Song]

Attachment: Another patch based on comments.

Comment 32

[Cavin Song]

Looks like we don't need the ReadDBFolderInfo() call in GetDisplayName(), so 
I'll remove it before I check in the code.

Comment 33

[Navin Gupta]

why don't we use nsresult nsMsgDBFolder::CreateFileSpecForDB 
for hashing for msf file rather than adding new code here. 

Comment 34

[Cavin Song]

Navin, you're right and I already changed the code to make use of 
CreateFileSpecForDB().

So can I get a review from Navin and Seth so that I can check in the code?  Let 
me know if you want to see the updated patch again.

Comment 35

[Navin Gupta]

please attach the updated patch. I would like to take a look 

Comment 36

[Cavin Song]

Attachment: Patch with minor changes.

Comment 37

[(not reading, please use [e-mail] instead)]

Is this safe?

-
if (NS_FAILED(ConvertFromUnicode(fileCharset, nsAutoString(aNewName),
&convertedNewName)))
+  nsAutoString tmpAutoString;
+  tmpAutoString.AssignWithConversion(newSafeName);
+  if (NS_FAILED(ConvertFromUnicode(fileCharset, tmpAutoString, &convertedNewName)))

before your fix, we used aNewName, which was the PRUnichar * that they user
typed in.  (could have been the folder name, in chinese.)

now, we are converting newSafeName, which is going to be a hashed version that
might be altered because of illegal chars or illegal length.

what do we do with convertedNewName after this? 

Comment 38

[Cavin Song]

convertedNewName is the new folder name to be renamed later in the code. 
Actually, newNameStr and convertedNewName are used interchangeably but I have 
changed the code so that only newNameStr is used.

We need to hash the folder name in order to support folder name like 'a,b'. Not 
sure if we need to call ConvertFromUnicode() if we only support ASCII chars for 
now. To support non ASCII chars like Chinese we need to do something different 
which will be addressed in a different bug.

I'll attach a different patch per naving comments.

Comment 39

[Cavin Song]

Attachment: Patch per naving comments.

Comment 40

[nhottanscp]

>Not sure if we need to call ConvertFromUnicode() if we only support
>ASCII chars for now.
Is this about local folder name? Non ASCII is supported for local folder name.

Comment 41

[Roy Yokoyama]

sorry for the delay.

I'm testing the latest patch on roy's japanese win2k machine.

-sspitzer

Comment 42

[(not reading, please use [e-mail] instead)]

actually, roy showed me how to change the system char set on my win2k box.

I'll be testing the patch out there, and I'll see that migration of 4.x non-
ASCII local folder names still works.

Comment 43

[(not reading, please use [e-mail] instead)]

cavin and I found some problems on non-ASCII operating systems.

we're fixing it now, new patch coming soon.

Comment 44

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, there are still some i18n problems though

Comment 45

[(not reading, please use [e-mail] instead)]

Attachment: this patch (while not cleaned up yet) fixes the i18n issues.

Comment 46

[(not reading, please use [e-mail] instead)]

here's a todo list for that patch.

1) not sure why rename of non ascii local folders works (see 1302 in 
nsLocalMailFolder.cpp)

2) name of file on disk if not in same char set (see "XXX TODO" in 
hashifnecessary).  (not necessary for this fix, but getting that work should 
allow users to name local folders to non-ASCII and non-system charset values.
japanese folder names on a US-ASCII or Big5 OS)

3) Get/Set MailboxName not wstring friendly, so we use escaped UTF8

4) no Get/Set PRUnicharStringProperty on folder cache element, so again use 
escaped UTF8

5) code cleanup

Comment 47

[(not reading, please use [e-mail] instead)]

heads up, cavin has found a problem with my last patch.

StringHash() needs to be updated to handle null bytes (since it is now hashing 
a PRUnichar *, and not a char *.

he's working on that.

Comment 48

[Cavin Song]

Attachment: Added 2 fixes. Still has problems with create & open folders.

Comment 49

[Cavin Song]

Attachment: Comment out the code to check if the name will convert to the underlying OS.

Comment 50

[(not reading, please use [e-mail] instead)]

cavin, can you elaborate (for nhotta benefit and others) on why we had to
comment out that code?

I'm concerned that certain strings in japanese will still cause the same problem
we saw when we turned that code on.
 
I'm most interested in the second code path, the one that goes through
NS_MsgHashIfNecessary() instead of the new NS_MsgHashAutoStringIfNecessary()

I started looking into making the changes we'd need to deal with that (switching
more code to nsAutoString and nsILocalFile) and it quickly got out of hand.

Comment 51

[(not reading, please use [e-mail] instead)]

last night, I tried make the changes necessary to fix the problem we ran into 
where we'd use NS_MsgHashIfNecessary() in one code path, and 
NS_MsgHashAutoStringIfNecessary() in another.

most of it involved replacing nsFileSpec / nsIFileSpec with nsILocalFile, so I 
could use nsILocalFile's appendRelativeUnicodePath() method.

I stopped when I got here:

399         rv = nsLocalURI2Path(kMailboxRootURI, folderURI, folderPath);
400 
401         if (NS_SUCCEEDED(rv))
402         {
403             // set up the url spec and initialize the url with it.
404             nsFilePath filePath(folderPath); // convert to file url 
representation...
405 
406       if (mPrintingOperation)
407           urlSpec = PR_smprintf("mailbox://%s?number=%d&header=print", 
(const char *) filePath, msgKey);

I'm not sure what to do there yet.  filePath is a unix style path to the 
folder.  If we've got a nsILocalFile, I think we'd have to use nsIFile's path 
or URL attributes to get what we want, but I'd need to check that it does the 
right thing on all platforms.  I'm also nervous about what it will return if 
the file is non-ASCII.  I'd also need to see how we parse that url back out.

assuming cavin's last patch doesn't break any existing functionality on 
japanese systems, we should work towards landing it, and later resolving the 
NS_MsgHashIfNecessary() / NS_MsgHashAutoStringIfNecessary() issue that we found.

Comment 52

[(not reading, please use [e-mail] instead)]

see bug #68993 "filename containing Shift-JIS trail byte 7C"

I'm worried about that happening with the current patch.

if the folder name has that, the call to NS_MsgHashIfNecessary() will hash it, 
but the call to NS_MsgHashAutoStringIfNecessary() will not, so we'll fail to 
open the folder when we click on it.  (we'll get that same error that cavin saw 
when he uncomments that code.)

Comment 53

[nhottanscp]

I am not sure why there are two versions. But is that possible to convert to the
OS charset instead of AssignWithConversion? If the conversion fails then the
name has to be hashed.

 nsresult NS_MsgHashIfNecessary(nsCAutoString &name)
 {
+  nsAutoString autoStr;
+  autoStr.AssignWithConversion(name);
+  nsresult rv = NS_MsgHashAutoStringIfNecessary(autoStr);

Comment 54

[Cavin Song]

OK, here are the reasons why:
1. In nsMsgLocalMailFolder::CreateSubfolder() we call
NS_MsgHashAutoStringIfNecessary() directly to make sure that we have a safe
folder name to create .msf file and the msg file (ie, 'aa.msg' and 'aa').  The
string passed to NS_MsgHashAutoStringIfNecessary() is the one entered by the
users (ie, no conversion is performed on it) and for this reason
nsMsgI18Ncheck_data_in_charset_range() returns TRUE because Chinese string is
supported by Chinese windows.  At this point we have created .msf and msg files
on disk. So far so good.

2. Then in nsMsgLocalMailFolder::CreateSubfolder() we call AddSubfolder() which
eventually calls NS_MsgCreatePathStringFromFolderURI(). The latter does some
string manipulations and calls NS_MsgHashIfNecessary().  
NS_MsgHashIfNecessary() then converts the input C string to nsAUtoString before
calling NS_MsgHashAutoStringIfNecessary(). So by the time we try to check if the
folder name will convert to the underlying OS the string is altered and the call
to nsMsgI18Ncheck_data_in_charset_range()fails!  Since the call fails the name
is hashed and all the subsequent calls to open the folder database fails as
well. This is because in step one we create the .msf file with the original name
the users entered and now we try to open the database with a different (hashed)
name. The problem is really that the string passed to
NS_MsgHashAutoStringIfNecessary() (called by NS_MsgHashIfNecessary()) does not
represent a correct Chinese string.

To fix the problem we need to make NS_MsgCreatePathStringFromFolderURI() call
NS_MsgHashAutoStringIfNecessary() directly (instead of NS_MsgHashIfNecessary()).
But then we need to change NS_MsgCreatePathStringFromFolderURI() to use
nsAutoString instead of nsCAutoSting.

Commenting out the call to nsMsgI18Ncheck_data_in_charset_range(), the patch
works for the following (tested) cases of folder creation:
1. ASCII folders in English windows.
2. Chiness folders in Chiness windows.

However, with the call to nsMsgI18Ncheck_data_in_charset_range() it does not
work for the following cases of folder creation:
1. Chiness folders in English windows.
2. Chiness folders in Chiness windows.

Comment 55

[nhottanscp]

>NS_MsgHashIfNecessary() then converts the input C string to nsAUtoString before
>calling NS_MsgHashAutoStringIfNecessary(). 
This is done by AssignWithConversion as I mentioned in my last comment This may
lose the data because it just casts instead of applying charset conversion. I
think utility like ConvertToUnicode() can be used there to convert to unicode
from the OS charset. If the conversion fails then the name needs hashing.

Comment 56

[scottputterman]

moving to 0.9.4

Comment 57

[scottputterman]

adding nsenterprise-

Comment 58

[Cavin Song]

Found anther issue when running on the Japanese windows. If the folder name 
contains a Japanese char whose double byte code is '837C' (note that '7C' is 
'|' which is an illegal char on win32 in the code) then we'll end up hashing the 
folder name and we'll get 'Unknown error' msg when opening the folder.  So in 
this case we should not hash the folder name. But if I enter 'a|b' then we'll 
want to hash the name. Question is how does the code tell if a particular '7C' 
char comes from a Japanese char or it's from the ASCII '|'?

Comment 59

[Scott MacGregor]

we'd like this for Mach V but it really isn't something we need for eMojo. Mail
news triage meeting --> .9.5

Comment 60

[scottputterman]

moving to 0.9.6

Comment 61

[Jean-Francois Ducarroz]

I need an unicode version of NS_MsgHasIfNecessary for when we save an
attachment. It will be used in nsMessenger::SaveAttachment and in
nsMessenger::SaveAllAttachments. Let me know when you are done with this bug.

Comment 62

[scottputterman]

moving to 0.9.8

Comment 63

[Sheela Ravindran]

*** Bug 115777 has been marked as a duplicate of this bug. ***

Comment 64

[Bill Mason]

*** Bug 125868 has been marked as a duplicate of this bug. ***

Comment 65

[Jo Hermans]

*** Bug 188390 has been marked as a duplicate of this bug. ***

Comment 66

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 67

[WADA:World Anti-bad-Duping Agency]

Folder of "?" could be created and used with no error by both Mozilla Suite
latest-trunk(1.8a5) and Thunderbird 0.9(and latest nightly) on Win-2K.
( file set of "HEXA-string" and "HEXA-string.msf" was created )
Mscott has successfuly resolved this lo----nglived bug, I think, by fix for Bug
219586 and Bug 264467, post fix of Bug 264071 is required though.
Is it right?
(Note : "/" and "?" problem disappered, but "#" problem still exists)

Comment 68

[NextGen$]

*** Bug 239502 has been marked as a duplicate of this bug. ***

Comment 69

[Magnus Melin [:mkmelin]]

WFM on thunderbird 2.0 beta 1/windows and Thunderbird/2.0b1 ID:20070108 on linux.
(? in imap folder names seems fine too, but someone here said that's bug 20324)

Comment 71

[Philip Chee]

Based on Comment 69 => WFM