407222 - Save as HTML complete doesn't work when a "/" is in name

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Isaac Engel]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.7) Gecko/20070930 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.7) Gecko/20070930 Firefox/2.0.0.7

When I try  to save sites I have renamed as HTML complete  I can get a download failed error.  I'm fairly certain that the problem is caused by adding a "/" to the file name.

Reproducible: Always

Steps to Reproduce:
1.With open window in Camino, click Save as (or command S)
2.Change title to include a /.  Leave save option as HTML complete
3. Click Save, or return
Actual Results:  
Get a download failed error message.

Expected Results:  
Should have saved file and accompanying folder to designated directory.

I began the bug reporting process when I thought the problem was more generally linked to any attempt to save as HTML complete.  I subsequently linked the problem to including a / in the file name, so the problem isn't as serious as I first thought.  Still, it's something that, if not worth fixing, at the very least users should be alerted to.

Comment 1

[Chris Lawson (gone)]

Well, the *reason* this is happening is because Camino saves the related files (aside from the HTML itself) in a directory called "[filename] Files" (where [filename] is the name you give the HTML file in the Save dialog), and / is not a legal character in a directory name. (Oddly, Mac OS X *does* allow it in filenames, though it substitutes a : whenever such a filename is used in Terminal.)

Whether we can do anything about this or if we just want to call it INVALID, I dunno.

Comment 2

[Stuart Morgan]

(In reply to comment #1)
> / is not a legal character in a directory name. (Oddly, Mac OS X *does* allow
> it in filenames

/ is a legal character for both folders and files from an OS X UI perspective; there is no such directory/filename distinction. Failure to correctly handle standard system behavior is a valid bug; the fact that a character translation has to happen for the path to be passed between APIs does not make it invalid.

The only reason I'm not confirming this is that we need to figure out if the bug is in Camino code, or core code (and if the latter, if it's a dup of an existing core bug).

Comment 3

[philippe (part-time)]

Minefield: replaces the '/' in the filename by a '_' (underscore) and saves the file happily (file/name.html becomes file_name.html).

Bon Echo/Fx2: fails silently to save, error message in the console:
Error: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIPrefBranch.setComplexValue]
Source file: chrome://global/content/contentAreaUtils.js
Line: 545

(similarly, Minefield saves the resources for that a html file in a folder labelled 'filename_files', whereas Camino (branch and trunk) saves them in 'filename files' (with a space)).

Comment 4

[Stuart Morgan]

Confirming since this is likely something we need to handle ourselves; if it turns out to be core we can move/dup this later.

Comment 7

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

(In reply to comment #3)
> (similarly, Minefield saves the resources for that a html file in a folder
> labelled 'filename_files', whereas Camino (branch and trunk) saves them in
> 'filename files' (with a space)).

We do filename sanitization of the default suggested filename here in SaveHeaderSniffer::PerformSave:

288     // Validate the file name to ensure legality.
289     for (PRUint32 i = 0; i < defaultFileName.Length(); i++)
290         if (defaultFileName[i] == ':' || defaultFileName[i] == '/')
291             defaultFileName.SetCharAt(' ', i);

which must be how we're getting the space when philippe actually got something to save in comment 3 (based on a simple test, some page/file with a / or : already in its name before the user touches the name).

However, we never fix up the user-chosen name, which is what comment 0 is about:

373     nsAutoString dstString;
374     [[savePanel filename] assignTo_nsAString:dstString];
375 
376     return InitiateDownload(sourceData, dstString, inOriginalURI);

defaultFileName and dstString are both nsAutoString, so I inserted a copy of the existing validation routine between line 375 and 376 there and changed the variable names…
and broke all saving, for reasons that are quite beyond me.

Then, I tried to fix the name with Cocoa (stringByReplacingCharactersInSet:), and that also broke all saving.  (I tend to think we really should fix it in Cocoa, so that we can also strip out control and invalid characters, which Firefox ultimately did, though that probably requires fixing bug 619166 or code-duplication in the interim.)

I get no errors, so I must be missing something, but it's too late to safely/sanely try and debug right now.

Comment 8

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

Attachment: Revalidate filenames after the user has touched them

The reason I couldn't fix it before is because NSSavePanel's "filename" really returns not the filename, but rather the complete POSIX path :P  So I was actually stripping all the "/" out of the path!

This patch revalidates the filename after the user has been allowed to modify it, thereby fixing this bug.  It also adds a bit of bullet-proofing in the form of checking for control characters (which bug 619166 should later uplift to control+invalid).

As I mentioned in the other bug, the insane whitespace in this file has already been filed as bug 482163.

Comment 9

[Ilya Sherman]

Comment on attachment 508059 [details] [diff] [review]
Revalidate filenames after the user has touched them

>+    NSCharacterSet* pathDelimiterSet = [NSCharacterSet characterSetWithCharactersInString:@"/:"];
>+    NSString* legalFilename = [[[savePanel filename] lastPathComponent] stringByReplacingCharactersInSet:pathDelimiterSet withString:@" "];
>+    legalFilename = [legalFilename stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "];

It would be nice to refactor things so that this logic is shared with the defaultFileName case.  |defaultFileName| is an |nsAutoString| though, and I don't know offhand how exactly that works.  Maybe that class difference makes it not worth refactoring this?  Also, would we ever need to strip out control characters from |defaultFileName|?

>+    NSString* fileNameWithPath = [[savePanel filename] stringByDeletingLastPathComponent];
>+    fileNameWithPath = [fileNameWithPath stringByAppendingPathComponent:legalFilename];

nit: If you want to split this across two lines, I would prefer to introduce a separate variable like "NSString* filePath" for the first line -- otherwise that line looks rather surprising before you read on to the next one.


r=ilya with the above addressed (which for the first point might amount to no code changes)

Comment 10

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

Comment on attachment 508059 [details] [diff] [review]
Revalidate filenames after the user has touched them

> Comment on attachment 508059 [details] [diff] [review]
> Revalidate filenames after the user has touched them
> 
> >+    NSCharacterSet* pathDelimiterSet = [NSCharacterSet characterSetWithCharactersInString:@"/:"];
> >+    NSString* legalFilename = [[[savePanel filename] lastPathComponent] stringByReplacingCharactersInSet:pathDelimiterSet withString:@" "];
> >+    legalFilename = [legalFilename stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "];
> 
> It would be nice to refactor things so that this logic is shared with the
> defaultFileName case.  |defaultFileName| is an |nsAutoString| though, and I
> don't know offhand how exactly that works.  Maybe that class difference makes
> it not worth refactoring this?  Also, would we ever need to strip out control
> characters from |defaultFileName|?

As I mentioned to Ilya in the channel, smontagu fixed a Firefox bug with control characters solely by adding some excluded characters to a header in XPCOM somewhere, so I'm not really sure what Gecko sanitizes for us and what it doesn't (and I also don't know the what-which-Gecko-strings-do rules).

Upon further testing, it looks like maybe NSSavePanel filters out user-entered control characters automatically (I tried a couple of times in 2.1a1 to insert a return in a filename, and it turned into a space), so I guess we can just leave out the control character replacement.

Comment 11

[Stuart Morgan]

Comment on attachment 508059 [details] [diff] [review]
Revalidate filenames after the user has touched them

sr=smorgan

>+    NSString* legalFilename = [[[savePanel filename] lastPathComponent] stringByReplacingCharactersInSet:pathDelimiterSet withString:@" "];

This is a pretty long line; how about:

NSString* legalFilename = [[[savePanel filename] lastPathComponent]
    stringByReplacingCharactersInSet:pathDelimiterSet withString:@" "];

>+    legalFilename = [legalFilename stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "];

Remove, per your comment above?

Comment 12

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

Landed as http://hg.mozilla.org/camino/rev/3a7ebd16ba53 with Stuart's comments and Ilya's remaining comment addressed.