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

RESOLVED FIXED in mozilla2.0b12



Networking: File
6 years ago
6 years ago


(Reporter: morac, Assigned: bz)


Windows XP
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)



(2 attachments)



6 years ago
Created attachment 507367 [details]
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.
Attachment #507367 - Attachment mime type: application/octet-stream → text/plain
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 '_'.
Component: Networking: File → XPCOM
QA Contact: networking.file → xpcom
Michael, I assume it's ok if I crib shamelessly from your testcase for the automated test for this, right
Assignee: nobody → bzbarsky
Component: XPCOM → Networking: File
QA Contact: xpcom → networking.file

Comment 3

6 years ago
(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.
Priority: -- → P2
Created attachment 507938 [details] [diff] [review]
Proposed fix

I've verified that on try the test fails without the patch and passes with the patch
Attachment #507938 - Flags: review?(benjamin)
Whiteboard: [need review]
Attachment #507938 - Flags: review?(benjamin) → review+
Whiteboard: [need review] → [need approval]
Comment on attachment 507938 [details] [diff] [review]
Proposed fix

Requesting approval.  This should be a very safe fix for a potential dataloss problem.
Attachment #507938 - Flags: approval2.0?


6 years ago
Attachment #507938 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Last Resolved: 6 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.