1844663 - atomicFileOutputStream deletes its output file on close

Status: NEW

(Core :: Networking, defect, P2)

Description

[Robin Steuber (she/her) [:bytesized]]

Steps to Reproduce:

1. Open the browser console and run:

let l = {}; 
XPCOMUtils.defineLazyGetter(l, "BinaryOutputStream", () => {
  return Components.Constructor(
    "@mozilla.org/binaryoutputstream;1",
    "nsIBinaryOutputStream",
    "setOutputStream"
  );
});
let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
file.initWithPath("C:\\Users\\<user>\\test.txt");
let stream = FileUtils.openAtomicFileOutputStream(file);
let bstream = new l.BinaryOutputStream(stream);
let encoded = new TextEncoder().encode("testing 123\n");
bstream.writeByteArray(encoded);

2. Observe that test.txt exists and contains the right contents
3. Run stream.close();
4. Observe that test.txt no longer exists.

I took a peek at the implementation of nsAtomicFileOutputStream and noticed that the handling is a bit different if the file already exists. So I tried the above steps again after creating an existing test.txt. This time I see that test-1.txt is created with the expected output, but it is also deleted when the stream is closed and test.txt does not contain the new contents afterwards.

I spent a little time in a debugger investigating, and the file is being deleted here. It seems like it probably works properly if nsAtomicFileOutputStream::Finish() is called first, but neither my example code nor the production code that I'm trying to fix (Bug 1830368) ever seem to hit that function.

Comment 1

[Robin Steuber (she/her) [:bytesized]]

It seems like it works properly if instead of stream.close() you run

stream.QueryInterface(Ci.nsISafeOutputStream);
stream.finish();

After rereading this comment, it kinda seems like the implementation expects that the caller will call finish() if to indicate that the file writing is complete and close() to indicate that we actually don't want that file anymore and just want to delete it? I guess that technically there is nothing wrong with this, but it's pretty different from normal file usage conventions. That certainly isn't how fclose works, for example. So if that is how it's meant to work, some explaining comments might be good. Especially here.