Closed Bug 15069 Opened 21 years ago Closed 21 years ago

File type url not correctly parsed and created

Categories

(MailNews Core :: Networking, defect, P3)

x86
Windows NT
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jefft, Assigned: jefft)

References

Details

Attachments

(1 file)

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.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → INVALID
Take it back. For windows, the client should turn ':' inot '|'. Mark it as
invalid.
Status: RESOLVED → VERIFIED
So, is GetSpec returning file://C|/temp/nsmail8.hml or
file://C:0/temp/nsmail8.hml?
GetSpec() returns file://C:0/temp/nsmail8.html. It shouldn't I think.
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.
Status: VERIFIED → REOPENED
Target Milestone: M14
I'm reopening this bug, because both forms should return the spec
file:://C|/temp/nsmail8.htm.
Assignee: warren → gagan
Blocks: 13449
Status: REOPENED → NEW
Resolution: INVALID → ---
Clearing INVALID resolution due to reopen of this bug.
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?
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:\..."
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?
Andreas,

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

Warren
Status: NEW → ASSIGNED
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.
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.
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)
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.
Blocks: 12345
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?
Target Milestone: M14 → M11
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 ...
Should this be a m11 stopper bug?  If not, pls move target milestone to M12.
Thanks.
I have a M11 stopper bug 12345 relies on this one. Maybe it should be a M11
stopper too.
Assignee: gagan → jefft
Status: ASSIGNED → NEW
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.
Status: NEW → ASSIGNED
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.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
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.
QA Contact: lchiang → ppandit
Par, pls verify. Thanks.
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
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.
Resolution: FIXED → ---
Clearing FIXED resolutions due to reopen
Target Milestone: M11 → M12
Moving from M11 to M12 since reopened and M11 is outta here.
Target Milestone: M12 → M13
Jeff, does this effect anything visible to the user? Trying to decide if it's a
beta stopper.
No, but this should be easy fix. We just didn't write code right as warren
mentioned.
Status: REOPENED → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Fix checked in as suggest by warren. Thanks warren.
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
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.