Skip to content

Commit

Permalink
Bug 952997 - Fix OS.File large file support. r=yoric
Browse files Browse the repository at this point in the history
  • Loading branch information
nmaier committed Apr 8, 2014
1 parent 74850db commit 86cfc9c
Show file tree
Hide file tree
Showing 7 changed files with 218 additions and 30 deletions.
13 changes: 12 additions & 1 deletion dom/system/OSFileConstants.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,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
*/
Expand Down Expand Up @@ -682,7 +693,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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,10 +474,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)) {
Expand Down
37 changes: 20 additions & 17 deletions toolkit/components/osfile/modules/osfile_win_back.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,17 @@
return SysFile._FindClose;
});

Type.DWORD = Type.int32_t.withName("DWORD");

/**
* A C integer holding -1 in case of error or a positive integer
* in case of success.
Type.DWORD = Type.uint32_t.withName("DWORD");

/* 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
Expand Down Expand Up @@ -241,11 +244,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);

libc.declareLazyFFI(SysFile, "DeleteFile",
Expand Down Expand Up @@ -280,10 +283,10 @@
libc.declareLazyFFI(SysFile, "FormatMessage",
"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
Expand Down Expand Up @@ -346,7 +349,7 @@

libc.declareLazyFFI(SysFile, "SetFilePointer",
"SetFilePointer", ctypes.winapi_abi,
/*return*/ Type.negative_or_DWORD,
/*return*/ Type.DWORD,
/*file*/ Type.HANDLE,
/*distlow*/Type.long,
/*disthi*/ Type.long.in_ptr,
Expand Down Expand Up @@ -378,14 +381,14 @@

libc.declareLazyFFI(SysFile, "GetFileAttributes",
"GetFileAttributesW", ctypes.winapi_abi,
/*return*/ Type.DWORD,
/*return*/ Type.DWORD_FLAGS,
/*fileName*/ Type.path);

libc.declareLazyFFI(SysFile, "SetFileAttributes",
"SetFileAttributesW", ctypes.winapi_abi,
/*return*/ Type.zero_or_nothing,
/*fileName*/ Type.path,
/*fileAttributes*/ Type.DWORD);
/*fileAttributes*/ Type.DWORD_FLAGS);

advapi32.declareLazyFFI(SysFile, "GetNamedSecurityInfo",
"GetNamedSecurityInfoW", ctypes.winapi_abi,
Expand Down
29 changes: 22 additions & 7 deletions toolkit/components/osfile/modules/osfile_win_front.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -176,9 +176,24 @@
if (whence === undefined) {
whence = Const.FILE_BEGIN;
}
return throw_on_negative("setPosition",
WinFile.SetFilePointer(this.fd, pos, null, whence),
this._path);
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", ctypes.winLastError, this._path);
}
pos64 = ctypes.Int64.join(posHi.value, result);
return Type.int64_t.project(pos64);
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}

} 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();
}
1 change: 1 addition & 0 deletions toolkit/components/osfile/tests/xpcshell/xpcshell.ini
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,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]
Expand Down

0 comments on commit 86cfc9c

Please sign in to comment.