197379 - file:// URLs from CFM mozilla don't work with Mach-O mozilla

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Conrad Carlen (not reading bugmail)]

rephrased from bug 197283

For the CFM build, the file:// URL format was:

file:///volume-name/path-name

For Mach-O, it's the Unix form:

file:///path-name

More from bug 197283:

------- Additional Comment #1 From Darin Fisher  2003-03-04 20:55 -------

sounds like something nsLocalFileOSX should maybe handle automagically??


------- Additional Comment #2 From Conrad Carlen 2003-03-05 07:07 -------

I don't think so. There would be a perf hit which we can ill-afford if
nsLocalFile::InitWith[Native]Path() were to do this conversion. If anything, the
file:// handler could do it since only file:// URLs have this problem.

-> Networking:File since it's a problem with file:// URLs in general.


------- Additional Comment #3 From Darin Fisher 2003-03-05 10:56 -------

yeah, i suppose something could be added to nsFileChannel::EnsureStream.


This bug is to do what Darin suggested. I think the answer is to allow new
builds to read the old file:// url format but to only write the current format.
This leaves older builds high and dry, but I think we'll have to live with that.
The new format is much better in that it supports long file names, and is UTF-8.

Comment 1

[benc]

We've got a couple bugs on this problem already. 

People were unhappy before w/ CFM b/c IE used UNIX style. Now ex-CFM people will
be unhappy b/c we use UNIX style.

You could look at the place where the volume name shows up and check it w/ the
list of mounted volumes... Is that what Darin meant in more codish terms?

Comment 2

[Matthias Versen [:Matti]]

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

Comment 3

[benc]

...was a bit confused, because the quoted comments appear to come from bug 195984.

Comment 4

[Conrad Carlen (not reading bugmail)]

Attachment: makefile patch to build new file

Comment 5

[Conrad Carlen (not reading bugmail)]

Attachment: new file

Adding a impl of this code for OSX because I didn't want to contaminate the
Unix impl with a bunch of Mac-specific #ifdefs. I started by copying the Unix
impl, and then added the code in net_GetFileFromURLSpec and supporting code to
sniff for and convert HFS paths to POSIX. The additional runtime expense of
calling pathBeginsWithVolName() should be offset by creating the file with
NS_NewLocalFile() instead of the component mgr, as the existing code did.

Comment 6

[Conrad Carlen (not reading bugmail)]

This fixes bug 169093 (tested), and should fix the other dependent bug listed
here. It will also fix the bug where chrome packages installed with a CFM build
can't be used by a Mach-O build.

Comment 7

[Conrad Carlen (not reading bugmail)]

Comment on attachment 124998 [details]
new file

and the makefile patch too - didn't want to add the file just yet and make one
patch.

Comment 8

[Simon Fraser [no longer active]]

Would this break if I had a volume called 'usr' or 'System' when I made some CFM
paths?

Comment 9

[Conrad Carlen (not reading bugmail)]

Such a path would still be treated as a POSIX path, in the old code and new.
That's why there's code to check for that. It's conservative to avoid HFS path
false-positives.

Comment 10

[Alec Flett]

Comment on attachment 124998 [details]
new file

static PRBool pathBeginsWithVolName(const nsACString& path, nsACString&
firstPathComponent)

maybe document this a bit to explain where the last  string came from

    static nsCStringArray gVolumeList; // We will leak this - one for the life
of the app :-/

I remember you mentioning this on IRC.. this is ok since its mac-only.. but
what if someone changes their drive name while mozilla is running? or inserts a
new disk into a removable drive? if those are real issues, we should document
them in this code and/or file a bug about it. (or just fix those issues in this
bug)

static void SwapSlashColon(char *s)

document this - there's more punctuation than code in this method - not quite
self-documenting :)

nsresult
net_GetFileFromURLSpec(const nsACString &aURL, nsIFile **result)

hmm.. how is this different from the one in necko? why do we need a seperate
one (or should we just fix the one in necko)? should it be static?

too many open issues - I'm holding off on marking this r=alecf until we have
reasons/comments in the code explaining some of these issues.

Comment 11

[Alec Flett]

ah, I just read comment 5 - I guess I'm still in favor of the #ifdefs to avoid
duplicate code/etc - partially for footprint reasons, but partially because
things like path-parsing are security sensitive areas, and if we have to fix a
security bug in one place, it would suck if we didn't fix it in the other.

can you maybe abstract out some of the mac-specific stuff like

#ifdef XP_OSX
    void TransmogrifyStringInAMacWay();
#endif

so that the #ifdefs are kept to a minimum?

Comment 12

[Conrad Carlen (not reading bugmail)]

> maybe document this a bit to explain where the last string came from
OK

> what if someone changes their drive name while mozilla is running?
We would have to live with that. Building the volume list on every call to this
would make it unbearably slow (not worth fixing this). The reason it uses a
static list is to check the name against a volume name and eliminate most cases
without a stat - just a search through a string list.

> I guess I'm still in favor of the #ifdefs to avoid
duplicate code/etc - partially for footprint reasons
Either one file is compiled in the build or the other, never both. So it's not a
footprint issue - unless you mean footprint of the src. As far as the
sensitivity of parsing code, it's still done with net_ParseFileURL().

> static void SwapSlashColon(char *s)
document this
OK. It was copied from existing nsURLHelperMac.cpp, though.

> hmm.. how is this different from the one in necko?
This is necko. It's making OSX use it's own version rather than XP_UNIX

I can #ifdef it into nsURLHelperUnix.cpp, but - We have an implementation of
this code for each platform and each impl differs maybe less than this one
differs from nsURLHelperUnix.cpp.

Comment 13

[Conrad Carlen (not reading bugmail)]

Attachment: patch

After discussion with Alec, OSX has its own separate implementation. It's just
too much new code that's not relevant to any other XP_UNIX. There are comments
in the new file and in nsURLHelperUnix.cpp pointing out where they match, for
the sake of maintainence. While I was in nsURLHelperUnix, I changed it also to
use NS_NewLocalFile() instead of the component mgr to create a file.

Comment 14

[Conrad Carlen (not reading bugmail)]

Attachment: patch

whoops - attached wrong file.

Comment 15

[Alec Flett]

Comment on attachment 125447 [details] [diff] [review]
patch

oh, I meant to say that this:
+  if (escPath[escPath.Length() - 1] != '/') {

should be
if (escPath.Last() != '/')

Last() should be faster, and avoid at least one vtable call.


thanks for the extra comments
sr=alecf with the above change...

Comment 16

[Simon Fraser [no longer active]]

I think we need to measure what impact this patch has on startup and pageload.

Comment 17

[Conrad Carlen (not reading bugmail)]

Simon, no significant perf change.

before patch: Ts = 4149, Tp = 1351
after patch:  Ts = 4131, Tp = 1357

The cost of the patch is (in the normal case of not being an HFS path) one
string comparison for each mounted volume. I had 3 for this test. But, changing
the way the file gets created (not thru createInstance, but NS_NewLocalFile)
should easily make up for a few string comparisons.

Comment 18

[Conrad Carlen (not reading bugmail)]

Requesting 1.4 blockage. See the list of bugs this blocks - this patch fixes
them. Some of them are quite serious and all are regressions from CFM releases.

Comment 19

[Conrad Carlen (not reading bugmail)]

Fixed.

Comment 20

[Conrad Carlen (not reading bugmail)]

Comment on attachment 125447 [details] [diff] [review]
patch

Requesting approval for 1.4. See comment 18.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 125447 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to the 1.4 branch.

Comment 22

[Samir Gehani]

adt: nsbeta1+/adt2

a=adt to land this on the 1.4 branch by tonight (before 2003-06-19 5am and no
later please).  Please mark this with the fixed1.4 keyword once this lands. 
Thanks.  

Comment 23

[Conrad Carlen (not reading bugmail)]

Checked into 1.4.

Comment 24

[bmartin]

I'm trying to verify this in latest 1.4 branch for MacOSX, should both paths
work and display file contents within the browser?:

file:///volume-name/path-name

and

file:///path-name

Currently, only file:///path-name works.

Comment 25

[Conrad Carlen (not reading bugmail)]

bmartin: Is there a space in your HD name? Ben discovered yesterday, it's not
working in that case.

Comment 26

[bmartin]

My HD name contains a space.  "Macintosh HD"  if I remove the space, it still
does not work.

Will not work in either case:
file:///Macintosh HD/PathToFile/ (file:///Macintosh%20HD/PathToFile/)
or
file:///MacintoshHD/PathToFile/

Comment 27

[bmartin]

HD with a space in the name will fail.

HD without a space in the name will work.  If 1.4 is running while space in HD
name is removed, you'll need to quit 1.4 then restart for it to work properly.

ccarlen has a patch to fix this, should a respin be in order? Or, should we just
leave as is?

Comment 28

[Conrad Carlen (not reading bugmail)]

Attachment: fixes spaces, etc. in volume name

Comment 29

[Darin Fisher]

Comment on attachment 126526 [details] [diff] [review]
fixes spaces, etc. in volume name

sr=darin

Comment 30

[Conrad Carlen (not reading bugmail)]

attachment 126526 [details] [diff] [review] was checked into the trunk.

Comment 31

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.

Comment 32

[benc]

smooth, someone took me off the qa w/o making me cc...