15069 - File type url not correctly parsed and created

Status: VERIFIED FIXED

(MailNews Core :: Networking, defect, P3)

Description

[jefft]

Calling nsIIOService::NewURI("file://c:/temp/nsmail8.hml", nsnull, returnPtr)
gets a returned URL with "file://C:0/temp/nsmail8.hml" spec. Looks like with
treated the second ':' delimiter as host:port separator.

Comment 1

[jefft]

Take it back. For windows, the client should turn ':' inot '|'. Mark it as
invalid.

Comment 2

[Warren Harris]

So, is GetSpec returning file://C|/temp/nsmail8.hml or
file://C:0/temp/nsmail8.hml?

Comment 3

[jefft]

GetSpec() returns file://C:0/temp/nsmail8.html. It shouldn't I think.

Comment 4

[jefft]

More ... if I pass in file://C:/temp/nsmail8.htm GetSpec() returns
file://C:0/temp/nsmail8.htm. if I pass in file:://C|/temp/nsmail8.htm GetSpec()
returns file://C|/temp/nsmail8.htm.

Comment 5

[Warren Harris]

I'm reopening this bug, because both forms should return the spec
file:://C|/temp/nsmail8.htm.

Comment 6

[leger]

Clearing INVALID resolution due to reopen of this bug.

Comment 7

[Andreas Otte]

Currently there is no special file-url-parser. So file://c:/temp/nsmail8.hml is
"correctly" parsed with c as host and : as the port separator. If you use
file:///c:/temp/nsmail8.hml, you get no host, no port and /c:/temp/nsmail8.hml
as path. Is that valid? Currently there are also no OS-dependend parts in the
url-parser. So : ist not converted to |.

Maybe we need a special file-urlparser? Or only on windows? warren?

Comment 8

[Warren Harris]

Hmmm. At the very least, file://c:/foo should probably print out as file://c/foo
(no port number specified which should be -1, not 0). However, can't we special
case this -- either XP_PC or in general?

Maybe when the port number is missing in this way, we treat <host>: as part of
the path (just an idea). Or maybe a missing port number could map to -2 (another
illegal port value), and when GetSpec was called it could print out
file://c:/foo again. If we do this, we need to make sure that the nsFileURL code
(in xpcom) gets the native path right for a URL specified like this.

For reference, Communicator and IE both get this right. IE also permits
"file:C:\..."

Comment 9

[Andreas Otte]

I have a fix for this. Now it converts for example

file:c:/temp/nsmail8.hml
file:/c:/temp/nsmail8.hml
file://c:/temp/nsmail8.hml
file:///c:/temp/nsmail8.hml

to

scheme:  file
host:
port:
path:    /c|/temp/nsmail8.hml

Unfortunaly this fix is part of nsStdURL.cpp, which includes locally (on my
build) much more fixes to a number of other bugs. Any volunteers for the whole
package?

Comment 10

[Warren Harris]

Andreas,

Can you post the diffs to npm.netlib? Perhaps Gagan or I can look at them.
Thanks,

Warren

Comment 11

[Andreas Otte]

Attachment: this is a clean (without the other stuff I changed on the file) patch to nsStdURL.cpp to fix this. Please review this.

Comment 12

[Gagan]

Argh... catching up on this now. I am not if favor of special-casing file in
nsStdURL parsing. I think the simple solution is just to check for zero as a
port number and skip it. Or if the caller knows its a file: spec then add the
additional // before the path, before handing it to the NewURI call. Fixing this
now.

Comment 13

[Andreas Otte]

I didn't like that too, but I couldn't think of something else as effective. Be
sure to get all that stuff in the path, not C as host or something else.

Comment 14

[Gagan]

actually the fix that I have should work fine. I basically throw an
NS_ERROR_MALFORMED_URI on detecting a zero port (which is being triggered by
C:\) on receiving this the creator of the URI can check to see if its a file:
URI and appropriately handle it. (as in add the slashes and try again)

Comment 15

[Andreas Otte]

This is a previous comment from warren:

> Maybe when the port number is missing in this way, we treat <host>: as part of
> the path (just an idea). Or maybe a missing port number could map to -2 >
(another
> illegal port value), and when GetSpec was called it could print out
> file://c:/foo again. If we do this, we need to make sure that the nsFileURL >
code
> (in xpcom) gets the native path right for a URL specified like this.
>
> For reference, Communicator and IE both get this right. IE also permits
> "file:C:\..."

I still think the parser should be able to parse this without complaining. Maybe
we could do a kind of preparsing in the file-handler that uses as much as
possible from nsStdURL.cpp or write a special file-parser class. The file
protocol has no prehost, host and port and the parser must know that to get it
right.

Comment 16

[jefft]

Today's build I am failing in creating a file url with
"file://C:/TEMP/nsmail-2.html" Can we get this fixed for M11?

Comment 17

[Andreas Otte]

gagans last changes will not allow file:c:\bla or file://c:\bla. You get an
error and have the chance to use the correct file:///c:\bla ...

Comment 18

[lchiang]

Should this be a m11 stopper bug?  If not, pls move target milestone to M12.
Thanks.

Comment 19

[jefft]

I have a M11 stopper bug 12345 relies on this one. Maybe it should be a M11
stopper too.

Comment 20

[Gagan]

Jeff: as Andreas pointed out when you fail to create a URL from file: you will
need to modify the spec to make sure it has the 3 slashes. Since I am not sure
where you are doing this (and I would gladly have fixed it for you there) I am
assigning this back to you. Let me know if you need any help with this.

Comment 21

[jefft]

Okay, gagan and andreas thanks for looking into this. I'll change the file spec
to file:/// instead of file:// and see how it goes.

Comment 22

[jefft]

I have changed mimedrft.cpp to pass in "file:///" instead of "file://"
Everything works fine. Thanks much for help. I am marking this bug as fixed.

Comment 23

[lchiang]

Par, pls verify. Thanks.

Comment 24

[ppandit]

Line 1668 in mimedrft.cpp is:
char *tmpSpecStr = PR_smprintf("file:///%s", tmpSpec->GetNativePathCString(
);

VERIFIED BY LXR.
Checked only mimedrft.cpp.
nsStdURL.cpp version 1.28 has the fix.

Comment 25

[Warren Harris]

This is completely the wrong way to do this. The right way is to do something
like this:

  nsFileURL fileURL(tempSpec);
  char *tmpSpecStr = fileURL.GetURLString();

... that is if you even want tempSpecStr at all. Usually holding a URL as a
string is wrong. Reopening.

Comment 26

[leger]

Clearing FIXED resolutions due to reopen

Comment 27

[leger]

Moving from M11 to M12 since reopened and M11 is outta here.

Comment 28

[Phil Peterson]

Jeff, does this effect anything visible to the user? Trying to decide if it's a
beta stopper.

Comment 29

[jefft]

No, but this should be easy fix. We just didn't write code right as warren
mentioned.

Comment 30

[jefft]

Fix checked in as suggest by warren. Thanks warren.

Comment 31

[ppandit]

mimedrft.cpp now shows:

1668   // This needs to be done so the attachment structure has a handle 
1669   // on the temp file for this attachment...
1670   // if ( (tmpSpec) && (!bodyPart) )
1671   if (tmpSpec)
1672   {
1673       nsFileURL fileURL(*tmpSpec);
1674       const char * tempSpecStr = fileURL.GetURLString();
1675 
1676       nsMimeNewURI(&(newAttachment->orig_url), tempSpecStr, nsnull);
1677       NS_IF_ADDREF(newAttachment->orig_url);
1678   }

Mark as VERIFIED