Bug 840436 - OS.File.writeAtomic without flush doesn't rename anymore. r=froydnj

toolkit/components/osfile/osfile_shared_front.jsm

@@ -308,17 +308,15 @@ AbstractFile.read = function read(path, bytes) {
 };
 
 /**
- * Write a file, atomically.
+ * Write a file in one operation.
  *
- * By opposition to a regular |write|, this operation ensures that,
- * until the contents are fully written, the destination file is
- * not modified.
- *
- * By default, files are flushed for additional safety, i.e. to lower
- * the risks of losing data in case the device is suddenly removed or
- * in case of sudden shutdown. This additional safety is important
- * for user-critical data (e.g. preferences, application data, etc.)
- * but comes at a performance cost. For non-critical data (e.g. cache,
+ * By default, this operation ensures that, until the contents are
+ * fully written, the destination file is not modified. By default,
+ * files are flushed for additional safety, i.e. to lower the risks of
+ * losing data in case the device is suddenly removed or in case of
+ * sudden shutdown. This additional safety is important for
+ * user-critical data (e.g. preferences, application data, etc.) but
+ * comes at a performance cost. For non-critical data (e.g. cache,
  * thumbnails, etc.), you may wish to deactivate flushing by passing
  * option |flush: false|.
  *
@@ -347,23 +345,34 @@ AbstractFile.writeAtomic =
      function writeAtomic(path, buffer, options) {
   options = options || noOptions;
 
-  let tmpPath = options.tmpPath;
-  if (!tmpPath) {
-    throw new TypeError("Expected option tmpPath");
-  }
-
   let noOverwrite = options.noOverwrite;
   if (noOverwrite && OS.File.exists(path)) {
     throw OS.File.Error.exists("writeAtomic");
   }
 
+  if ("flush" in options && !options.flush) {
+    // Just write, without any renaming trick
+    let dest;
+    try {
+      dest = OS.File.open(path, {write: true, truncate: true});
+      return dest.write(buffer, options);
+    } finally {
+      dest.close();
+    }
+  }
+
+
+  let tmpPath = options.tmpPath;
+  if (!tmpPath) {
+    throw new TypeError("Expected option tmpPath");
+  }
+
+
   let tmpFile = OS.File.open(tmpPath, {write: true, truncate: true});
   let bytesWritten;
   try {
     bytesWritten = tmpFile.write(buffer, options);
-    if ("flush" in options && options.flush) {
-      tmpFile.flush();
-    }
+    tmpFile.flush();
   } catch (x) {
     OS.File.remove(tmpPath);
     throw x;

toolkit/components/osfile/tests/mochi/main_test_osfile_async.js

@@ -402,6 +402,60 @@ let test_read_write_all = maketest("read_write_all", function read_write_all(tes
 
     // Cleanup.
     OS.File.remove(pathDest);
+
+    // Same tests with |flush: false|
+    // Check that read + writeAtomic performs a correct copy
+    options = {tmpPath: tmpPath, flush: false};
+    optionsBackup = {tmpPath: tmpPath, flush: false};
+    bytesWritten = yield OS.File.writeAtomic(pathDest, contents, options);
+    test.is(contents.byteLength, bytesWritten, "Wrote the correct number of bytes (without flush)");
+
+    // Check that options are not altered
+    test.is(Object.keys(options).length, Object.keys(optionsBackup).length,
+            "The number of options was not changed (without flush)");
+    for (let k in options) {
+      test.is(options[k], optionsBackup[k], "Option was not changed (without flush)");
+    }
+    yield reference_compare_files(pathSource, pathDest, test);
+
+    // Check that temporary file was removed
+    test.info("Compare complete (without flush)");
+    test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed (without flush)");
+
+    // Check that writeAtomic fails if noOverwrite is true and the destination
+    // file already exists!
+    view = new Uint8Array(contents.buffer, 10, 200);
+    try {
+      options = {tmpPath: tmpPath, noOverwrite: true, flush: false};
+      yield OS.File.writeAtomic(pathDest, view, options);
+      test.fail("With noOverwrite, writeAtomic should have refused to overwrite file (without flush)");
+    } catch (err) {
+      test.info("With noOverwrite, writeAtomic correctly failed (without flush)");
+      test.ok(err instanceof OS.File.Error, "writeAtomic correctly failed with a file error (without flush)");
+      test.ok(err.becauseExists, "writeAtomic file error confirmed that the file already exists (without flush)");
+    }
+    yield reference_compare_files(pathSource, pathDest, test);
+    test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed (without flush)");
+
+    // Now write a subset
+    START = 10;
+    LENGTH = 100;
+    view = new Uint8Array(contents.buffer, START, LENGTH);
+    bytesWritten = yield OS.File.writeAtomic(pathDest, view, {tmpPath: tmpPath, flush: false});
+    test.is(bytesWritten, LENGTH, "Partial write wrote the correct number of bytes (without flush)");
+    array2 = yield OS.File.read(pathDest);
+    view1 = new Uint8Array(contents.buffer, START, LENGTH);
+    test.is(view1.length, array2.length, "Re-read partial write with the correct number of bytes (without flush)");
+    for (let i = 0; i < LENGTH; ++i) {
+      if (view1[i] != array2[i]) {
+        test.is(view1[i], array2[i], "Offset " + i + " is correct (without flush)");
+      }
+      test.ok(true, "Compared re-read of partial write (without flush)");
+    }
+
+    // Cleanup.
+    OS.File.remove(pathDest);
+
   });
 });