629291 - Using Safe File OutputStreams to overwrite file with unicode characters in file name results in file name change.

Status: RESOLVED FIXED

(Core :: Networking: File, defect, P2)

Description

[Michael Kraft [:morac]]

Attachment: Test module demostrating issue

I have an add-on that allows the user to specify a file name and as such the file name can contain unicode characters.  I'm seeing inconsistencies between how safe file output streams and normal file output streams behave when set to overwrite a file whose filename contains unicode characters when used with the NetUtil.jsm routines.

Here's what I've seen when writing to files:

1. If the file does not exist, using both safe and non-safe file output streams will create the file with the correct name.
2. If the file exists, but contains no unicode characters in the file name, then both safe and non-safe file output streams will properly overwrite the name.
3. If the file exists and contains unicode characters, non-safe file output streams will properly overwrite the name.
4. If the file exists and contains unicode characters, safe file output streams will create a new file with all the unicode characters replaced with underscores and leave the original file untouched.

So the problem is only with safe file output stream writes for existing files whose filename contains unicode characters.

I can't find any documentation on safe file writes indicating that this is expected and since the behavior is different depending on the stream type and whether unicode is used or not, I'm filing this bug.


I attached an example module file containing a simple write file function with a hard coded filename of धधधध.tmp (U-2343) which obviously contains unicode characters. The write function takes a parameter which if true does a safe write, otherwise it does a non-safe write.  Both using the NetUtil.asyncCopy to do the actual write.

Calling Test.write(true) once results in the धधधध.tmp file being created and filled correctly.  Calling it a second time results in a ____.tmp file being created and filled "correctly".  The original file is not updated.  Calling Test.write(false) will update the original file.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is an XPCOM issue.  nsSafeFileOutputStream::Finish does this for the case when the target file already existed:
 
740             rv = mTargetFile->GetNativeLeafName(targetFilename);
741             if (NS_SUCCEEDED(rv)) {
742                 // This will replace target.
743                 rv = mTempFile->MoveToNative(nsnull, targetFilename);

This code seems to date back to the initial implementation of this stream.

The problem is that on Windows GetNativeLeafName will try to encode the filename in whatever the system-default codepage is.  And any characters not expressible in that codepage will become '_'.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Michael, I assume it's ok if I crib shamelessly from your testcase for the automated test for this, right

Comment 3

[Michael Kraft [:morac]]

(In reply to comment #2)
> Michael, I assume it's ok if I crib shamelessly from your testcase for the
> automated test for this, right

Fine with me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

I've verified that on try the test fails without the patch and passes with the patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 507938 [details] [diff] [review]
Proposed fix

Requesting approval.  This should be a very safe fix for a potential dataloss problem.

Comment 6

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/9c815db836e3