diff --git a/dom/system/OSFileConstants.cpp b/dom/system/OSFileConstants.cpp index 12e3c428398..5011a990db7 100644 --- a/dom/system/OSFileConstants.cpp +++ b/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), diff --git a/toolkit/components/osfile/modules/osfile_shared_allthreads.jsm b/toolkit/components/osfile/modules/osfile_shared_allthreads.jsm index 3c98c2978c1..8c775b599b0 100644 --- a/toolkit/components/osfile/modules/osfile_shared_allthreads.jsm +++ b/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)) { diff --git a/toolkit/components/osfile/modules/osfile_win_back.jsm b/toolkit/components/osfile/modules/osfile_win_back.jsm index 6a4b8586146..8d43347255c 100644 --- a/toolkit/components/osfile/modules/osfile_win_back.jsm +++ b/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 - * in case of success. + * A special type used to represent flags passed as DWORDs to a function. + * 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.withName("negative_or_DWORD"); + Type.DWORD_FLAGS = Type.int32_t.withName("DWORD_FLAGS"); /** * 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, - /*share*/ Type.DWORD, + /*access*/ Type.DWORD_FLAGS, + /*share*/ Type.DWORD_FLAGS, /*security*/Type.SECURITY_ATTRIBUTES.in_ptr, - /*creation*/Type.DWORD, - /*flags*/ Type.DWORD, + /*creation*/Type.DWORD_FLAGS, + /*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, - /*langid*/ Type.DWORD, + /*msgid*/ Type.DWORD_FLAGS, + /*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 = { diff --git a/toolkit/components/osfile/modules/osfile_win_front.jsm b/toolkit/components/osfile/modules/osfile_win_front.jsm index b424f30966a..6677ca7c8dd 100644 --- a/toolkit/components/osfile/modules/osfile_win_front.jsm +++ b/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", - WinFile.SetFilePointer(this.fd, pos, null, whence)); + let pos64 = ctypes.Int64(pos); + // 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); }; /** diff --git a/toolkit/components/osfile/tests/mochi/worker_test_osfile_win.js b/toolkit/components/osfile/tests/mochi/worker_test_osfile_win.js index c7219075618..0efa64704b1 100644 --- a/toolkit/components/osfile/tests/mochi/worker_test_osfile_win.js +++ b/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) { diff --git a/toolkit/components/osfile/tests/xpcshell/test_osfile_async_largefiles.js b/toolkit/components/osfile/tests/xpcshell/test_osfile_async_largefiles.js new file mode 100644 index 00000000000..c642bc887bb --- /dev/null +++ b/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(); +} diff --git a/toolkit/components/osfile/tests/xpcshell/xpcshell.ini b/toolkit/components/osfile/tests/xpcshell/xpcshell.ini index 29bc2b02e04..8e9825be762 100644 --- a/toolkit/components/osfile/tests/xpcshell/xpcshell.ini +++ b/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]