462181 - Fix extension-generation code in SaveHeaderSniffer's PerformSave (Save code produces WARNING: malformed url: no scheme: error in debug builds)

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Downloading, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

See bug 409679 comment 4 and bug 409679 comment 10-14:

When we save a file, we're doing something wrong and ending up without a scheme towards the end.  It doesn't seem to cause any problems for saving files, but it shows up (in debug builds) every time the Save code is called.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

From irc: 

I wonder if this comment explains the reason we get one good spec and one spec error in the save code: 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/camino/src/download/nsDownloadListener.mm&rev=1.39&mark=752-753#749

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

When I was working on bug 576978, I saw something suspicious in nsHeaderSniffer:PerformSave, but now I don't remember what it was.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> When I was working on bug 576978, I saw something suspicious in
> nsHeaderSniffer:PerformSave, but now I don't remember what it was.

This:

293     // Make sure the appropriate extension is appended to the suggested file name.
294     nsCOMPtr<nsIURI> fileURI(do_CreateInstance("@mozilla.org/network/standard-url;1"));
295     nsCOMPtr<nsIURL> fileURL(do_QueryInterface(fileURI, &rv));
296     if (!fileURL)
297         return rv;
298     fileURL->SetFilePath(NS_ConvertUTF16toUTF8(defaultFileName));
299     
300     nsCAutoString fileExtension;
301     fileURL->GetFileExtension(fileExtension);

I'm guessing it's that line 298; but that's too much GeckXPCOM C++ for me to diagnose exactly what's going on.

I think, though, what's happening is that we're calling an nsStandardURL method on simply a filename ("bob.jpg") in order to be able to get the extension, rather than providing a full nsURL.  It seems like there should be a better way to get the extension, though.  Anyone know why smfr didn't just use Cocoa?  For i18n reasons (e.g. something like bug 233798)?

Indeed, the NSPR logging I did in bug 409679 comment 11 et seq. shows that's exactly the problem (my comment in comment 1 here is entirely spurious in this case/for this bug).

Also, similar to bug 643094, it seems like we should only do this "what is the extension" check in certain cases; when we're using the document *title* or the host (or our fallback filename), we know that the "extension", if any, is going to be at best bogus.

In the other cases, we sometimes have an actual URL where the end-of-URL matches the filename (possibly in name-from-content-disposition, but not always; view-source, name-from-file-URLs) and sometimes don't (caller-provided file name, cases where content-disposition≠URL-name); in the good cases, we should pass the actual URL in to nsStandardURL if we wanted to continue using it.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Another option would be to construct the future on-disk file:/// URL (what eventually becomes NSSavePanel's |file|) ourselves, right here, but that's messy, probably hard, and likely error-prone to do at this stage of "saving", since we don't have all that info (and, in fact, use this check to try and generate some of that info :P ).