224320 - remove bogus IsDirectory canonicalization

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

remove bogus IsDirectory canonicalization.  this code:

    if (escPath[escPath.Length() - 1] != '/') {
        PRBool dir;
        rv = aFile->IsDirectory(&dir);
        if (NS_FAILED(rv))
            NS_WARNING(PromiseFlatCString(
                NS_LITERAL_CSTRING("Cannot tell if ") + escPath +
                NS_LITERAL_CSTRING(" is a directory or file")).get());
        else if (dir) {
            // make sure we have a trailing slash
            escPath += "/";
        }
    }

is completely bogus.  if the nsIFile points to a non-existant file or directory
(that has not yet been created), then we will definitely hit the NS_WARNING. 
more importantly, it means that we will sometimes be adding a trailing slash to
directories and sometimes not.  anyways, this code shouldn't have to be here. 
the code which loads the file URL is already doing a stat to figure out the type
of the file so it knows how to load it (directory index stream or regular file). 

that said, there may be some obscure code somewhere that checks for the trailing
'/' in a file:// URL.  that code would be anyways broken when dealing with files
that don't yet exist.

Comment 1

[Darin Fisher]

forgot to mention that this code is duplicated in all nsURLHelperXXX.cpp files,
and we hit the NS_WARNING a lot when saving a file to a location where the file
does not yet exist.

Comment 2

[Darin Fisher]

Attachment: v0 preliminary patch

this is just a sample of what the changes should look like.  this patch only
includes changes to nsURLHelperUnix.cpp, but changes to the other files will
look very similar if not identical.

i spent some time investigating this issue, and it turns out that the trailing
slash canonicalization for directories is very important.  since nsLocalFile
normalizes all pathes to not end with a slash, we always end up with
directories being given as URLs without trailing slashes.  this on the surface
should be fine; however, the lack of a trailing slash, makes nsIURI::resolve
much less useful.  consider this common snipet of pseudo-code:

  {
    var dir = directoryservice.get("TmpD");
    var spec = fileprotocolhandler.getURLSpecFromFile(dir);
    
    // ...
    
    var baseUrl = ioservice.newURI(spec, null, null);
    var url = baseUrl.resolve("foo.txt");
  }

now, if |spec| ends with a slash, then |url| will be file:///tmp/foo.txt, but
if |spec| does not end with a slash, then |url| will be file:///foo.txt! :-(

given a base URL like file:///tmp, the only way to form the absolute URL
file:///tmp/foo.txt is to call baseUrl.resolve("tmp/foo.txt"), but that implies
that the code would need to know the name of the temp folder (the leaf name of
the directory given by "TmpD").

so, not doing this trailing slash canonicalization would be inconvenient in
many cases.  while i've always disliked having to stat files in the nsIFile to
URL conversion code, i now believe it is the right thing to do.

so, this patch just removes the bogus warning and adds a comment.

Comment 3

[Darin Fisher]

Comment on attachment 134870 [details] [diff] [review]
v0 preliminary patch

doug: if you agree with this, then i'll go and do the same for the other
platform specific files.  thx!

Comment 4

[Darin Fisher]

fixed-on-trunk