93666 - Can't get MsgHeader when trying to access mail in a folder named test%25test

Status: RESOLVED WORKSFORME

(MailNews Core :: Backend, defect)

Description

[Andreas Otte]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.2) Gecko/20010725
BuildID:    2001072504

The messages in a folder named test%25test are not displayed in detail.

Reproducible: Always
Steps to Reproduce:
1. Create a new subfolder named test%25test (Yes, I know it is evil, but ...)
2. Move a message into this folder
3. Try to display the message in the new subfolder

Actual Results:  The message is not displayed. 

Expected Results:  Display the message.

This also happens on the trunk, where I see the following messages:

###!!! ASSERTION: couldn't get message header: 'PR_FALSE', file
nsMailboxProtocol.cpp, line 389
###!!! Break: at file nsMailboxProtocol.cpp, line 389
Error reading file [...ommited...]/test%test

Seems to me the url is unescaped when it should not.

Comment 1

[Andreas Otte]

I stand corrected. It is not unescaped to much, we do not escape to much.

In nsMailboxService.cpp there is in at least three times code that looks like this:

rv = nsParseLocalMessageURI(aSrcMsgMailboxURI, folderURI, &msgKey);
rv = nsLocalURI2Path(kMailboxRootURI, folderURI, folderPath);
if (NS_SUCCEEDED(rv))
{
    // set up the url spec and initialize the url with it.
    nsFilePath filePath(folderPath); // convert to file url representation...
    urlSpec = PR_smprintf("mailbox://%s?number=%d", (const char *) filePath,msgKey);
    nsCOMPtr <nsIMsgMailNewsUrl> url = do_QueryInterface(*aMailboxUrl);
    url->SetSpec(urlSpec);
    PR_FREEIF(urlSpec);
}

An URI is converted into a local Filepath and then back into an URI. Up to the
folderuri the uri is properly escaped, after the conversion to a FileSpec it is
unescaped (as it should), then the unescaped filePath is directly used to build
the new urispec. That is wrong, it has again to be escaped before called with
SetSpec.

Inserting this code (quick hack, copied from nsPop3Service) after the
construction of filepath

    nsXPIDLCString escapedFilePath;
    *((char**)getter_Copies(escapedFilePath)) =
                            nsEscape(filePath, url_Path);

and then using escapedFilePath instead of filePath, fixes the problem. I can
only guess that there are much more places inside mailnews where this url ->
filepath -> url conversion is done. Alls this need to be looked at.

Comment 2

[David :Bienvenu]

not a database problem. It's a folder unescaping problem. You're talking about
local mail, I assume. Seth, who was working on these issues? Was it Cavin?

Comment 3

[David :Bienvenu]

reassigning to Cavin. he's got other bugs related to issues like this.

Comment 4

[Cavin Song]

Local folder 'a#b' has the exactly same problem.  In this case, the error msg 
indicates that the folder it tried to open was 'a' (instead of 'a#b'):

Error reading file [...omitted...]/a

The reason why the resulting folder is 'a' is that the parsing code in 
nsStdURLParser::ParseAtDirectory() uses ';', '?' and '#' as delimiters to figure 
out file and directory names. So looks like we need to escape the folder name 
before calling:

  url->SetSpec(urlSpec);

Comment 5

[Cavin Song]

I just noticed that nsLocalURI2Path() was added the code to return unescaped 
folder name in the output parameter 'pathResult'. The changed was made on 
09/26/2000 by nhotta to fix 52165 (in file local/src/nsLocalUtils.cpp). So it 
looks we may have a dead-lock situation here because we need the unescaped 
folder names in one case but in other cases we don't need it.  We'll have to try 
both 7 bit and 8 bit folder names when we have a fix for this.  Need to 
investigate it more.

Comment 6

[Andreas Otte]

I feared it would not be that easy. Also I found it very interesting what
happend when my first attempt at the problem (described above) got accidentally
checked in with my fix for 40670. I tested on linux and it worked fine, but it
didn't apparently on windows: With an escaping issue????? Really strange!

When creating a new folder the name is typed normally. The folder is created as
typed. In the middle is a mailbox url where the normal url rules apply, so
escaping is necessary with SetSpec. That is absolutely nesessary. No way around
that.

The message database seems to use the real name as key. So unescaping the
(previously escaped) url is necessary to access the message.

Is the problem in bug 52165 a porting issue from 4.x to mozilla? Were the
folders differently encoded on 4.x? If it is, maybe the deadlock can be solved
by renaming the folder when moving from 4.x to mozilla.

Comment 7

[Cavin Song]

>I feared it would not be that easy. Also I found it very interesting what
>happend when my first attempt at the problem (described above) got accidentally
>checked in with my fix for 40670. I tested on linux and it worked fine, but it
>didn't apparently on windows: With an escaping issue????? Really strange!
>
My guess is that it's because we also escape "C|" at the beginning of 
'filePath'. "C:" is turned into "C|" in the url.  Since nsLocalURI2Path() is 
called only by PrepareMessageUrl() we can comment out the unescaping folder name 
code in nsLocalURI2Path() to achieve the same goal.  I have not debugged the 
problem yet so I don't know if this is going to work.

Comment 8

[Andreas Otte]

As far as I know the C: <-> C| stuff is not part of escaping. Maybe the real
root of the problem is bug 33451.

Comment 9

[esther]

Update: this still happens with current build on linux and mac local mail
subfolder with test%25test name.  I'm not sure if it's mentioned in this bug or
not, but I can see the msgs in the thread pane, but nothing (no header or msg
body) shows up in the Message pane.  

Comment 10

[Andreas Otte]

I started to look into this again and I think all we need to do is replace the
call for nsEscape with a call to NS_EscapeURL (with esc_Forced) in
NS_MsgEscapeEncodeURLPath located in mozilla/mailnews/base/util/nsMsgUtil.cpp to
get the right escaping on already escaped looking strings.

Comment 11

[Andreas Otte]

No it is not enough, the other problem from comment #1 still needs to be adressed.  

Comment 12

[Andreas Otte]

Attachment: patch that allows me to see the messages

This patch also fixes problems with foldernames like a%20b or a#b. The escaping
used is very minimal so hopefully nothing breaks. I tested it on linux an could
not find any regressions. Can anybody with a windows build test it.

Comment 13

[Andreas Otte]

Attachment: recreated patch to prevent bitrot

Comment 14

[Andreas Otte]

Comment on attachment 127959 [details] [diff] [review]
recreated patch to prevent bitrot

trying another reviewer

Comment 15

[David :Bienvenu]

I can try this on windows.

Comment 16

[WADA:World Anti-bad-Duping Agency]

Andreas Otte, will your patch resolve following "#" related 2 bugs too?
 Bug 94124 Local Folder: Can create folder "#ab" but can't see it on restart..
 Bug 115091 Mail subfolder can not have a # sign in the name..

Comment 17

[Andreas Otte]

Yes, I think so ...

Comment 18

[Andreas Otte]

David, any test results?

Comment 19

[David :Bienvenu]

I applied the patch and rebuilt, and I still have the problem with a#b.
Test%25Test worked, however.

Comment 20

[Andreas Otte]

You are correct, it does not work anymore with a#b. I'm sure it did at one point
in time. Thanks for testing, will try to fix it.

Comment 21

[Andreas Otte]

bienvenu: Apparently you checked in this changes by accident while working on
bug 137006. 

Status of this patch: Works for %25 or similar, does not work for a#b. I had no
time to look after this ...

Comment 22

[David :Bienvenu]

ah, thx for pointing that out - I couldn't remember what bug # that patch was
for. If I get a chance, I'll try to figure out the a#b problem.

Comment 23

[David :Bienvenu]

some or all of this patch will need to get backed out. The problem is that the
uri's for msg folders are stored in the msg filters.dat file, and any change in
the escaping of uri's is going to break some msg filters. This caused the
regression bug 229345 - my bad. I'll need to figure out if we can upgrade the
filter uri's, perhaps by unescaping them and then re-escaping them correctly.

Comment 24

[Andreas Otte]

Hmmm ... changes in escaping/unescaping or fixing the order of
escaping/unescaping or fixing missing escapes or unescapes can happen every time
... so those filter uris really need to be fixed, backing this out can only be
temporary.

Comment 25

[David :Bienvenu]

I agree that it should be only temporary, but we store uri's in pref.js as well
as msgFilterRules.dat - if the URI's can change at any time, they're no good as
a persistent representation of folders. So far, this is the first time anyone
has broken us, so I'm a little more optimistic that they won't be changing
frequently.

Comment 26

[Andreas Otte]

Maybe the only problem is that we are missing there an escape or unescape as it
was the case with this problem. Adding it here might force adding it there.

Comment 27

[WADA:World Anti-bad-Duping Agency]

How about changing folder name and real file name on folder creation.

Netscape 4.x avoided special character in folder name problem by changing file name.
For example, if folder name contains "\" in MS Win environment, Netscape 4.x
changes it to string like "_A", "_B" if "_A" exists, in file name.

Mozilla has already mechanism for different folder name and file name, keeping
folder name in ".msf", and this mechanism is already used on illegal character
in folder name case.
Since internal mail folder path uses real file name, this bug's case, character
string same as escaped character sequence, can be avoided if file name is
changed to different string, for example "%_25".

I believe this Netscape 4.x technique is applicable on all remaining problems of
illegal or special character in folder name, problems for "/?#" and "~" and ".",
if someone have submarine patent right.

Comment 28

[WADA:World Anti-bad-Duping Agency]

Correction. Sorry for spam.
(Invalid) > if someone have submarine patent right.
(Valid)   > if nobody has submarine patent right.

Comment 29

[WADA:World Anti-bad-Duping Agency]

In addition, if above Netscape 4.x technique is used, "hex string file name"
problems can be avoided.

When folder name contains "\", Mozilla currentry creates file name of hex string
based on Unicode under MS Windows-Me.
For example, for folder name of "A\B", Mozilla 20031228-trunk/Win-Me created
file/directry named "6e127717", "6e127717.msf" and "6e127717.sbd".
This ununderstandable filename sometimes causes confusion of users.
(1) When user deleted ".msf" file instead of using "Compact this folder"
    in order to re-create ".msf" file, folder name becomes the hex string.
(2) When user imports mails from othe Mailer and mail folder name contains
    illegal characters, file name for the folder becomes/contains hex string.

User impact on these situations can be reduced by Netscape 4.x technique.
Even if "A\B" is changed to "A_|_B", many users can guess this file is for
folder "A\B", or was for folder "A\B".

Comment 30

[WADA:World Anti-bad-Duping Agency]

Sorry but Netscape 4.x technique in Comment #29 does not applicable to IMAP
folder created by oter than Mozilla.

Comment 31

[David :Bienvenu]

I did back out this patch - making the filter code handle changes in uri
escaping wasn't trivial, and we need to fix that before we can check this in.

Comment 32

[WADA:World Anti-bad-Duping Agency]

How about to comletely separate (A) "Folder Name" and (B) "Folder File
Name(Physical Path Name)" and (C) "Internal Mail Folder Path Name(URI format)?

(A) and (B) (almost equal to (C) currently) are separated already - (A) is saved
".msf" file if folder name is different from file name (illegal file name
character case).
Separate folder file name("%25" in this bug) and folder internal path name(URI
format).

For example, assign unique mail folder number and use this number in internal
mail path(URI format) used in prefs.js or msgFilterRules.dat.
Needless to say, relationship between folder number and pysical file name should
be kept in somewhere and conversion from one to another should be done easily.

This unique identifier can be "Short Folder Name" or "Folder Nick Name".

If unique short name(nick name) of the folder does not contain any unsafe
special characters, it can be used in both file names and URL with no problem.
And short folder name forces user to convert special characters in folder name
to safe file name characters and safe URI characters by himself instead of
automatic conversion by Mozilla.
It is clever way, I think.
In addition, this can easily resolve not only all special URI character related
problems("/","#","?" in addition to "%") but also starting "." and ending "." in
file name problems.

However, if folder is IMAP folder and is defined by other mailer, folder file
name may contain unsafe characters.
So (A) "Folder Name" and (B) "Folder File Name(Physical Path Name)" and (C)
"Internal Mail Folder Path Name(URL format) should be distinguished. 

Since current mechanism, real folder name in ".msf", may causes difficulty in
mail folder recovery, relationship among "Short name" and  "File Name" and "Long
Name" should be kept in somewhere else.
I think text file in Mail directry is better place than prefs.js.

Changes required to support :
(1) New folder creation (POP and IMAP)
   Force user to enter "Short Folder Name".
     Short folder name  : used in URI
     Physical file name : same as Short Name 
     Long folder name   : Already user entered
(2) Import
   Add promt logic for "Short Folder Name"
   if file name contains unsafe characters.
     Short folder name  : used in URI
     (New) Physical file name : same as Short Name 
     Long folder name   : can be changed from old physical file name
(3) Subscribing already defined IMAP folder
   Add promt logic for "Short Folder Name"
     Short folder name  : used in URI
     Physical file name : as is
     Long folder name   : can be changed from physical file name
   Note: 
     Starting "." and ending "." in file name problems
     may not be resolved if folder is already defined at server.
(4) When unsafe file name is found in mail directry on restart.
    (User can manualy copy or rename folder file)
   Ignoring and warning message is sufficient, I think.
   But for migration, renaming support while restart is preferable.

Comment 33

[WADA:World Anti-bad-Duping Agency]

Added this bug to dependency tree of bug 124287 for ease of access.

Comment 34

[David :Bienvenu]

this seems to work for me today with a trunk build on windows.