296113 - Thunderbird seems to refuse to display e-mails in Local Folders if profile path contains # character

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Timwi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

I'm posting this for a friend. Steps to reproduce:

* Create a new profile (using ProfileManager) in some folder which has a # in
  its name
* Connect to some email account (I admit it's trickier with POP than IMAP since
  your e-mails are moved to the temporary profile)
* Copy an e-mail into a Local Folder
* Try to view that e-mail

Reproducible: Always

Steps to Reproduce:

Actual Results:  
E-mail is displayed blank

Expected Results:  
E-mail is displayed properly :)

Comment 1

[Jo Hermans]

bug 96934 ?

Comment 2

[Jeongkyu Kim]

Attachment: a patch to fix base url parser

I could reproduce this issue on my windows box. And I found that cause of the
issue is failure in parsing URL of mailbox when there is '#' in profile path.
The parser assume that the rest of '#' in URL is reference fragment, which is
incorrect in the case of this issue (see nsBaseURLParser::ParsePath() in
/netwerk/base/src/nsURLParsers.cpp).
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsURLParsers.cpp#230

The best way to idenfity '#' in file path is to check if '/' exists in the rest
of the string. If then, '#' can be determined as part of file path, not as
reference delimeter.

Comment 3

[Jeongkyu Kim]

seeking r?

Comment 4

[David :Bienvenu]

I wonder if the '#' should be escaped. Anyway, cc'ing a couple relevant folks.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> The parser assume that the rest of '#' in URL is reference fragment

Correct.  It is.

> to check if '/' exists in the rest of the string.

'/' is a valid character in a fragment identifier.  See
http://www.faqs.org/rfcs/rfc2396.html

David is right -- whoever is passing this string in to NewURI needs to be
URI-escaping the '#'.  Where is the string coming from?

(for future reference, you should look at http://www.mozilla.org/owners.html to
figure out whom to ask for review).

Comment 6

[Jeongkyu Kim]

(In reply to comment #4)
> I wonder if the '#' should be escaped. Anyway, cc'ing a couple relevant folks.

David, would you give me a brief idea on how escaping help to fix this
situation? Then I will research it further. :-)

Comment 7

[Jeongkyu Kim]

(In reply to comment #5)

> '/' is a valid character in a fragment identifier.  See
> http://www.faqs.org/rfcs/rfc2396.html

Oops, I got it :-)

> (for future reference, you should look at http://www.mozilla.org/owners.html to
> figure out whom to ask for review).

Thanks for the information.

Comment 8

[Jeongkyu Kim]

Comment on attachment 185104 [details] [diff] [review]
a patch to fix base url parser

marking obsolete, see comment #5

Comment 9

[David :Bienvenu]

As Boris suggested, set a breakpoint there, and see who's passing in the uri.
Then, work backwards to figure out where the uri was created, and see why
they're not escaping the '#' correctly.

Comment 10

[Jeongkyu Kim]

Boris,

Here is the execution path. The folder path is created in <1> and pasred in <2>.
Would you take a look at this function?

http://lxr.mozilla.org/seamonkey/source/mailnews/local/src/nsMailboxService.cpp#367

nsMessenger::OpenURL() ->
nsMailboxService::DisplayMessage() ->
nsMailboxService::FetchMessage() ->
nsMailboxService::PrepareMessageUrl()
{
...
    NS_ENSURE_SUCCESS(rv,rv);
<1> rv = nsLocalURI2Path(kMailboxRootURI, folderURI.get(), folderPath);

    if (NS_SUCCEEDED(rv))
    {
      // set up the url spec and initialize the url with it.
      nsFilePath filePath(folderPath); // convert to file url representation...
      nsCAutoString buf;
      NS_EscapeURL((const char *)filePath,-1,
                   esc_Minimal|esc_Forced|esc_AlwaysCopy,buf);
      if (mPrintingOperation)
      ...
      else
        urlSpec = PR_smprintf("mailbox://%s?number=%d", buf.get(), msgKey);
      
      nsCOMPtr <nsIMsgMailNewsUrl> url = do_QueryInterface(*aMailboxUrl);
<2>   url->SetSpec(nsDependentCString(urlSpec));
      PR_Free(urlSpec);
      
      (*aMailboxUrl)->SetMailboxAction(aMailboxAction);
...
}

Comment 11

[Boris Zbarsky [:bzbarsky]]

So what's the string in |buf| after the NS_EscapeURL call?

Fwiw, the flags passed in look wrong; given the way the string is being used and
the documentation for NS_EscapeURL, I think they should be
esc_Directory|esc_Forced|esc_AlwaysCopy

Comment 12

[Jeongkyu Kim]

It works great!!!

file path ->         "/C|/temp/profile#test/Mail/Local Folders/Inbox"
current escaping ->  "/C|/temp/profile#test/Mail/Local%20Folders/Inbox"
fixed escaping ->    "/C|/temp/profile%23test/Mail/Local%20Folders/Inbox"

Thanks Boris :-)

Comment 13

[Boris Zbarsky [:bzbarsky]]

I bet the same problem appears if the profile path contains a '?', then.  ;)

Comment 14

[Jeongkyu Kim]

Attachment: a patch to fix escape flags

Here comes new patch. see comment #11

David, would you take care of this?

Comment 15

[Jeongkyu Kim]

(In reply to comment #13)
> I bet the same problem appears if the profile path contains a '?', then.  ;)

I really want to try but there is such a OS that does not allow '?' in
folder/file name. Is there anybody who knows why? ;-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

You could try ';' as well.  ;)

Comment 17

[Jeongkyu Kim]

(In reply to comment #16)
> You could try ';' as well.  ;)

I tried ';' but it seems to be ok with current version.

Comment 18

[Scott MacGregor]

you should ask Darin Fisher to look at this patch. He's the module owner for
networking and should be the one to decide if this is the right thing to do. 

Comment 19

[Jeongkyu Kim]

(In reply to comment #18)
> you should ask Darin Fisher to look at this patch. He's the module owner for
> networking and should be the one to decide if this is the right thing to do. 

Hi Scott,

My first patch was made against network module, but Boris pointed out that it
was wrong (see comment #5 ). And Boris also found that NS_EscapeURL() has wrong
flag in nsMailboxService::PrepareMessageUrl(). The second patch was made for
fixing the bug in mozilla/mailnews/local/src/nsMailboxService.cpp . Because it
is mailnews mdoule, you or David could take a look at my patch, correct? :-)

Comment 20

[OstGote!]

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

Comment 21

[zug_treno]

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

Comment 22

[David :Bienvenu]

Comment on attachment 185177 [details] [diff] [review]
a patch to fix escape flags

I've run with this a bit - I made sure that folders with '#' in the name still work...

Comment 23

[David :Bienvenu]

fixed on trunk - we'll let this bake on the trunk, since it's always scary to change uri escaping. Thx for the patch!

Comment 24

[David :Bienvenu]

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

Comment 25

[David :Bienvenu]

landed on 2.0 branch