Bug 952997 - Fix OS.File large file support. r=Yoric

dom/system/OSFileConstants.cpp

@@ -303,6 +303,17 @@ void CleanupOSFileConstants()
 #define INT_CONSTANT(name)      \
   { #name, INT_TO_JSVAL(name) }
 
+/**
+ * Define a simple read-only property holding an unsigned integer.
+ *
+ * @param name The name of the constant. Used both as the JS name for the
+ * constant and to access its value. Must be defined.
+ *
+ * Produces a |ConstantSpec|.
+ */
+#define UINT_CONSTANT(name)      \
+  { #name, UINT_TO_JSVAL((name)) }
+
 /**
  * End marker for ConstantSpec
  */
@@ -649,7 +660,7 @@ static const dom::ConstantSpec gWinProperties[] =
   INT_CONSTANT(FILE_END),
 
   // SetFilePointer error constant
-  INT_CONSTANT(INVALID_SET_FILE_POINTER),
+  UINT_CONSTANT(INVALID_SET_FILE_POINTER),
 
   // File attributes
   INT_CONSTANT(FILE_ATTRIBUTE_DIRECTORY),

toolkit/components/osfile/modules/osfile_shared_allthreads.jsm

@@ -454,10 +454,15 @@ exports.Type = Type;
  */
 
 let projectLargeInt = function projectLargeInt(x) {
-  return parseInt(x.toString(), 10);
+  let str = x.toString();
+  let rv = parseInt(str, 10);
+  if (rv.toString() !== str) {
+    throw new TypeError("Number " + str + " cannot be projected to a double");
+  }
+  return rv;
 };
 let projectLargeUInt = function projectLargeUInt(x) {
-  return parseInt(x.toString(), 10);
+  return projectLargeInt(x);
 };
 let projectValue = function projectValue(x) {
   if (!(x instanceof ctypes.CData)) {

toolkit/components/osfile/modules/osfile_win_back.jsm

@@ -98,14 +98,18 @@
            return SysFile._FindClose;
          });
 
-       Type.DWORD = Type.int32_t.withName("DWORD");
+       Type.DWORD = Type.uint32_t.withName("DWORD");
 
        /**
-        * A C integer holding -1 in case of error or a positive integer
+        * A special type used to represent flags passed as DWORDs to a function.
-        * in case of success.
+        * In JavaScript, bitwise manipulation of numbers, such as or-ing flags,
+        * can produce negative numbers. Since DWORD is unsigned, these negative
+        * numbers simply cannot be converted to DWORD. For this reason, whenever
+        * bit manipulation is called for, you should rather use DWORD_FLAGS,
+        * which is represented as a signed integer, hence has the correct
+        * semantics.
         */
-       Type.negative_or_DWORD =
+       Type.DWORD_FLAGS = Type.int32_t.withName("DWORD_FLAGS");
-         Type.DWORD.withName("negative_or_DWORD");
 
        /**
         * A C integer holding 0 in case of error or a positive integer
@@ -219,11 +223,11 @@
          "CreateFileW", ctypes.winapi_abi,
                     /*return*/  Type.file_HANDLE,
                     /*name*/    Type.path,
-                    /*access*/  Type.DWORD,
+                    /*access*/  Type.DWORD_FLAGS,
-                    /*share*/   Type.DWORD,
+                    /*share*/   Type.DWORD_FLAGS,
                     /*security*/Type.SECURITY_ATTRIBUTES.in_ptr,
-                    /*creation*/Type.DWORD,
+                    /*creation*/Type.DWORD_FLAGS,
-                    /*flags*/   Type.DWORD,
+                    /*flags*/   Type.DWORD_FLAGS,
                     /*template*/Type.HANDLE);
 
        declareLazyFFI(SysFile, "DeleteFile", libc,
@@ -258,10 +262,10 @@
        declareLazyFFI(SysFile, "FormatMessage", libc,
          "FormatMessageW", ctypes.winapi_abi,
                     /*return*/ Type.DWORD,
-                    /*flags*/  Type.DWORD,
+                    /*flags*/  Type.DWORD_FLAGS,
                     /*source*/ Type.void_t.in_ptr,
-                    /*msgid*/  Type.DWORD,
+                    /*msgid*/  Type.DWORD_FLAGS,
-                    /*langid*/ Type.DWORD,
+                    /*langid*/ Type.DWORD_FLAGS,
                     /*buf*/    Type.out_wstring,
                     /*size*/   Type.DWORD,
                     /*Arguments*/Type.void_t.in_ptr
@@ -285,7 +289,7 @@
                     /*return*/   Type.zero_or_nothing,
                     /*sourcePath*/ Type.path,
                     /*destPath*/ Type.path,
-                    /*flags*/    Type.DWORD
+                    /*flags*/    Type.DWORD_FLAGS
                    );
 
        declareLazyFFI(SysFile, "ReadFile", libc,
@@ -316,7 +320,7 @@
 
        declareLazyFFI(SysFile, "SetFilePointer", libc,
          "SetFilePointer", ctypes.winapi_abi,
-                    /*return*/ Type.negative_or_DWORD,
+                    /*return*/ Type.DWORD,
                     /*file*/   Type.HANDLE,
                     /*distlow*/Type.long,
                     /*disthi*/ Type.long.in_ptr,
@@ -348,14 +352,14 @@
 
         declareLazyFFI(SysFile, "GetFileAttributes", libc,
           "GetFileAttributesW", ctypes.winapi_abi,
-                     /*return*/   Type.DWORD,
+                     /*return*/   Type.DWORD_FLAGS,
                      /*fileName*/ Type.path);
 
         declareLazyFFI(SysFile, "SetFileAttributes", libc,
           "SetFileAttributesW", ctypes.winapi_abi,
                      /*return*/         Type.zero_or_nothing,
                      /*fileName*/       Type.path,
-                     /*fileAttributes*/ Type.DWORD);
+                     /*fileAttributes*/ Type.DWORD_FLAGS);
      };
 
      exports.OS.Win = {

toolkit/components/osfile/modules/osfile_win_front.jsm

@@ -35,16 +35,16 @@
 
      // Mutable thread-global data
      // In the Windows implementation, methods |read| and |write|
-     // require passing a pointer to an int32 to determine how many
+     // require passing a pointer to an uint32 to determine how many
      // bytes have been read/written. In C, this is a benigne operation,
      // but in js-ctypes, this has a cost. Rather than re-allocating a
-     // C int32 and a C int32* for each |read|/|write|, we take advantage
+     // C uint32 and a C uint32* for each |read|/|write|, we take advantage
      // of the fact that the state is thread-private -- hence that two
      // |read|/|write| operations cannot take place at the same time --
      // and we use the following global mutable values:
-     let gBytesRead = new ctypes.int32_t(-1);
+     let gBytesRead = new ctypes.uint32_t(0);
      let gBytesReadPtr = gBytesRead.address();
-     let gBytesWritten = new ctypes.int32_t(-1);
+     let gBytesWritten = new ctypes.uint32_t(0);
      let gBytesWrittenPtr = gBytesWritten.address();
 
      // Same story for GetFileInformationByHandle
@@ -173,8 +173,24 @@
        if (whence === undefined) {
          whence = Const.FILE_BEGIN;
        }
-       return throw_on_negative("setPosition",
+       let pos64 = ctypes.Int64(pos);
-         WinFile.SetFilePointer(this.fd, pos, null, whence));
+       // Per MSDN, while |lDistanceToMove| (low) is declared as int32_t, when
+       // providing |lDistanceToMoveHigh| as well, it should countain the
+       // bottom 32 bits of the 64-bit integer. Hence the following |posLo|
+       // cast is OK.
+       let posLo = new ctypes.uint32_t(ctypes.Int64.lo(pos64));
+       posLo = ctypes.cast(posLo, ctypes.int32_t);
+       let posHi = new ctypes.int32_t(ctypes.Int64.hi(pos64));
+       let result = WinFile.SetFilePointer(
+         this.fd, posLo.value, posHi.address(), whence);
+       // INVALID_SET_FILE_POINTER might be still a valid result, as it
+       // represents the lower 32 bit of the int64 result. MSDN says to check
+       // both, INVALID_SET_FILE_POINTER and a non-zero winLastError.
+       if (result == Const.INVALID_SET_FILE_POINTER && ctypes.winLastError) {
+         throw new File.Error("setPosition");
+       }
+       pos64 = ctypes.Int64.join(posHi.value, result);
+       return Type.int64_t.project(pos64);
      };
 
      /**

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

@@ -103,10 +103,10 @@ function test_ReadWrite()
      null);
   isnot(output, OS.Constants.Win.INVALID_HANDLE_VALUE, "test_ReadWrite: output file opened");
   let array = new (ctypes.ArrayType(ctypes.char, 4096))();
-  let bytes_read = new ctypes.int32_t(-1);
+  let bytes_read = new ctypes.uint32_t(0);
   let bytes_read_ptr = bytes_read.address();
   log("We have a pointer for bytes read: "+bytes_read_ptr);
-  let bytes_written = new ctypes.int32_t(-1);
+  let bytes_written = new ctypes.uint32_t(0);
   let bytes_written_ptr = bytes_written.address();
   log("We have a pointer for bytes written: "+bytes_written_ptr);
   log("test_ReadWrite: buffer and pointers ready");
@@ -141,7 +141,7 @@ function test_ReadWrite()
   isnot (result, OS.Constants.Win.INVALID_SET_FILE_POINTER, "test_ReadWrite: output reset");
 
   let array2 = new (ctypes.ArrayType(ctypes.char, 4096))();
-  let bytes_read2 = new ctypes.int32_t(-1);
+  let bytes_read2 = new ctypes.uint32_t(0);
   let bytes_read2_ptr = bytes_read2.address();
   let pos = 0;
   while (true) {

toolkit/components/osfile/tests/xpcshell/test_osfile_async_largefiles.js

@@ -0,0 +1,153 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+Components.utils.import("resource://gre/modules/ctypes.jsm");
+Components.utils.import("resource://gre/modules/osfile.jsm");
+Components.utils.import("resource://gre/modules/Task.jsm");
+
+/**
+ * A test to check that .getPosition/.setPosition work with large files.
+ * (see bug 952997)
+ */
+
+// Test setPosition/getPosition.
+function test_setPosition(forward, current, backward) {
+  let path = OS.Path.join(OS.Constants.Path.tmpDir,
+                          "test_osfile_async_largefiles.tmp");
+
+  // Clear any left-over files from previous runs.
+  try {
+    yield OS.File.remove(path);
+  } catch (ex if ex.becauseNoSuchFile) {
+    // ignore
+  }
+
+  try {
+    let file = yield OS.File.open(path, {write:true, append:false});
+    try {
+      let pos = 0;
+
+      // 1. seek forward from start
+      do_print("Moving forward: " + forward);
+      yield file.setPosition(forward, OS.File.POS_START);
+      pos += forward;
+      do_check_eq((yield file.getPosition()), pos);
+
+      // 2. seek forward from current position
+      do_print("Moving current: " + current);
+      yield file.setPosition(current, OS.File.POS_CURRENT);
+      pos += current;
+      do_check_eq((yield file.getPosition()), pos);
+
+      // 3. seek backward from current position
+      do_print("Moving current backward: " + backward);
+      yield file.setPosition(-backward, OS.File.POS_CURRENT);
+      pos -= backward;
+      do_check_eq((yield file.getPosition()), pos);
+
+    } finally {
+      yield file.setPosition(0, OS.File.POS_START);
+      yield file.close();
+    }
+  } catch(ex) {
+    try {
+      yield OS.File.remove(path);
+    } catch (ex if ex.becauseNoSuchFile) {
+      // ignore.
+    }
+    do_throw(ex);
+  }
+}
+
+// Test setPosition/getPosition expected failures.
+function test_setPosition_failures() {
+  let path = OS.Path.join(OS.Constants.Path.tmpDir,
+                          "test_osfile_async_largefiles.tmp");
+
+  // Clear any left-over files from previous runs.
+  try {
+    yield OS.File.remove(path);
+  } catch (ex if ex.becauseNoSuchFile) {
+    // ignore
+  }
+
+  try {
+    let file = yield OS.File.open(path, {write:true, append:false});
+    try {
+      let pos = 0;
+
+      // 1. Use an invalid position value
+      try {
+        yield file.setPosition(0.5, OS.File.POS_START);
+        do_throw("Shouldn't have succeeded");
+      } catch (ex) {
+        do_check_true(ex.toString().contains("expected type"));
+      }
+      // Since setPosition should have bailed, it shouldn't have moved the
+      // file pointer at all.
+      do_check_eq((yield file.getPosition()), 0);
+
+      // 2. Use an invalid position value
+      try {
+        yield file.setPosition(0xffffffff + 0.5, OS.File.POS_START);
+        do_throw("Shouldn't have succeeded");
+      } catch (ex) {
+        do_check_true(ex.toString().contains("expected type"));
+      }
+      // Since setPosition should have bailed, it shouldn't have moved the
+      // file pointer at all.
+      do_check_eq((yield file.getPosition()), 0);
+
+      // 3. Use a position that cannot be represented as a double
+      try {
+        // Not all numbers after 9007199254740992 can be represented as a
+        // double. E.g. in js 9007199254740992 + 1 == 9007199254740992
+        yield file.setPosition(9007199254740992, OS.File.POS_START);
+        yield file.setPosition(1, OS.File.POS_CURRENT);
+        do_throw("Shouldn't have succeeded");
+      } catch (ex) {
+        do_print(ex.toString());
+        do_check_true(!!ex.message);
+      }
+
+    } finally {
+      yield file.setPosition(0, OS.File.POS_START);
+      yield file.close();
+      try {
+        yield OS.File.remove(path);
+      } catch (ex if ex.becauseNoSuchFile) {
+        // ignore.
+      }
+    }
+  } catch(ex) {
+    do_throw(ex);
+  }
+}
+
+function run_test() {
+  // First verify stuff works for small values.
+  add_task(test_setPosition.bind(null, 0, 100, 50));
+  add_task(test_setPosition.bind(null, 1000, 100, 50));
+  add_task(test_setPosition.bind(null, 1000, -100, -50));
+
+  if (OS.Constants.Win || ctypes.off_t.size >= 8) {
+    // Now verify stuff still works for large values.
+    // 1. Multiple small seeks, which add up to > MAXINT32
+    add_task(test_setPosition.bind(null, 0x7fffffff, 0x7fffffff, 0));
+    // 2. Plain large seek, that should end up at 0 again.
+    // 0xffffffff also happens to be the INVALID_SET_FILE_POINTER value on
+    // Windows, so this also tests the error handling
+    add_task(test_setPosition.bind(null, 0, 0xffffffff, 0xffffffff));
+    // 3. Multiple large seeks that should end up > MAXINT32.
+    add_task(test_setPosition.bind(null, 0xffffffff, 0xffffffff, 0xffffffff));
+    // 5. Multiple large seeks with negative offsets.
+    add_task(test_setPosition.bind(null, 0xffffffff, -0x7fffffff, 0x7fffffff));
+
+    // 6. Check failures
+    add_task(test_setPosition_failures);
+  }
+
+  run_next_test();
+}

toolkit/components/osfile/tests/xpcshell/xpcshell.ini

@@ -9,6 +9,7 @@ tail =
 [test_osfile_async_bytes.js]
 [test_osfile_async_copy.js]
 [test_osfile_async_flush.js]
+[test_osfile_async_largefiles.js]
 [test_osfile_async_setDates.js]
 [test_removeEmptyDir.js]
 [test_makeDir.js]