Question mark ? in folder names makes the folder useless

moving to M18.

nominating for nsbeta3

- per mail triage

sorry for the extra email. Removing mail2 keyword.

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

? 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.

Created attachment 24386
oe foo? cute but um, wrong.

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

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.

Adding Jenm.

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

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

moving to 0.9.3 per pdt

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

migration issue spun into bug #87380

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()

Created attachment 40399
Patch file.

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.

> 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.

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.

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 *.

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.

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

Created attachment 41494
Patch for ASCII only fix. Also fixed missing panacea.dat problem.

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.

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?

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.

FYI: |null_nsCOMPtr| is deprecated. If you plan to use it, just use 0 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.)

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.

Created attachment 41674
Another patch based on comments.

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

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

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.

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

Created attachment 41697
Patch with minor changes.

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?

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.

Created attachment 42135
Patch per naving comments.

>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.

sorry for the delay.

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

-sspitzer

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.

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

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

Created attachment 42816
updated patch, there are still some i18n problems though

Created attachment 42907
this patch (while not cleaned up yet) fixes the i18n issues.

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

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.

Created attachment 43075
Added 2 fixes. Still has problems with create & open folders.

Created attachment 43310
Comment out the code to check if the name will convert to the underlying OS.

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.

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.

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.)

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);

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.

>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.

moving to 0.9.4

adding nsenterprise-

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 '|'?

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

moving to 0.9.6

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.

moving to 0.9.8

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

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

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

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.

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)

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

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)

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

Based on Comment 69 => WFM