From 2b61449bc2f4788f5a8b5ffa999eb7e0c1b61ef8 Mon Sep 17 00:00:00 2001 From: Nils Maier Date: Mon, 21 Oct 2013 06:50:00 +0100 Subject: [PATCH] Bug 928239 - Fix pump_userland fallback in OS.File.copy. r=yoric --- .../osfile/modules/osfile_unix_front.jsm | 13 ++- .../tests/mochi/main_test_osfile_async.js | 35 ------ .../tests/xpcshell/test_osfile_async_copy.js | 109 ++++++++++++++++++ .../osfile/tests/xpcshell/xpcshell.ini | 1 + 4 files changed, 120 insertions(+), 38 deletions(-) create mode 100644 toolkit/components/osfile/tests/xpcshell/test_osfile_async_copy.js diff --git a/toolkit/components/osfile/modules/osfile_unix_front.jsm b/toolkit/components/osfile/modules/osfile_unix_front.jsm index 12472ffb0c5..c7971ec9107 100644 --- a/toolkit/components/osfile/modules/osfile_unix_front.jsm +++ b/toolkit/components/osfile/modules/osfile_unix_front.jsm @@ -94,7 +94,7 @@ * the end of the file has been reached. * @throws {OS.File.Error} In case of I/O error. */ - File.prototype._read = function _read(buffer, nbytes, options) { + File.prototype._read = function _read(buffer, nbytes, options = {}) { // Populate the page cache with data from a file so the subsequent reads // from that file will not block on disk I/O. if (typeof(UnixFile.posix_fadvise) === 'function' && @@ -120,7 +120,7 @@ * @return {number} The number of bytes effectively written. * @throws {OS.File.Error} In case of I/O error. */ - File.prototype._write = function _write(buffer, nbytes, options) { + File.prototype._write = function _write(buffer, nbytes, options = {}) { return throw_on_negative("write", UnixFile.write(this.fd, buffer, nbytes) ); @@ -431,6 +431,9 @@ * @option {number} bufSize A hint regarding the size of the * buffer to use for copying. The implementation may decide to * ignore this hint. + * @option {bool} unixUserland Will force the copy operation to be + * caried out in user land, instead of using optimized syscalls such + * as splice(2). * * @throws {OS.File.Error} In case of error. */ @@ -550,7 +553,11 @@ } else { dest = File.open(destPath, {trunc:true}); } - result = pump(source, dest, options); + if (options.unixUserland) { + result = pump_userland(source, dest, options); + } else { + result = pump(source, dest, options); + } } catch (x) { if (dest) { dest.close(); diff --git a/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js b/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js index 5cf1500593b..e286288b62f 100644 --- a/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js +++ b/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js @@ -158,7 +158,6 @@ let test = maketest("Main", function main(test) { yield test_read_write(); yield test_read_write_all(); yield test_position(); - yield test_copy(); yield test_mkdir(); yield test_iter(); yield test_exists(); @@ -476,40 +475,6 @@ let test_position = maketest("position", function position(test) { }); }); -/** - * Test OS.File.prototype.{copy, move} - */ -let test_copy = maketest("copy", function copy(test) { - return Task.spawn(function() { - let currentDir = yield OS.File.getCurrentDirectory(); - let pathSource = OS.Path.join(currentDir, EXISTING_FILE); - let pathDest = OS.Path.join(OS.Constants.Path.tmpDir, - "osfile async test 2.tmp"); - yield OS.File.copy(pathSource, pathDest); - test.info("Copy complete"); - yield reference_compare_files(pathSource, pathDest, test); - test.info("First compare complete"); - - let pathDest2 = OS.Path.join(OS.Constants.Path.tmpDir, - "osfile async test 3.tmp"); - yield OS.File.move(pathDest, pathDest2); - test.info("Move complete"); - yield reference_compare_files(pathSource, pathDest2, test); - test.info("Second compare complete"); - OS.File.remove(pathDest2); - - try { - let field = yield OS.File.open(pathDest); - test.fail("I should not have been able to open " + pathDest); - file.close(); - } catch (err) { - test.ok(err, "Could not open a file after it has been moved away " + err); - test.ok(err instanceof OS.File.Error, "Error is an OS.File.Error"); - test.ok(err.becauseNoSuchFile, "Error mentions that the file does not exist"); - } - }); -}); - /** * Test OS.File.{removeEmptyDir, makeDir} */ diff --git a/toolkit/components/osfile/tests/xpcshell/test_osfile_async_copy.js b/toolkit/components/osfile/tests/xpcshell/test_osfile_async_copy.js new file mode 100644 index 00000000000..c8238910473 --- /dev/null +++ b/toolkit/components/osfile/tests/xpcshell/test_osfile_async_copy.js @@ -0,0 +1,109 @@ +"use strict"; + +Components.utils.import("resource://gre/modules/osfile.jsm"); +Components.utils.import("resource://gre/modules/FileUtils.jsm"); +Components.utils.import("resource://gre/modules/NetUtil.jsm"); +Components.utils.import("resource://gre/modules/Promise.jsm"); +Components.utils.import("resource://gre/modules/Task.jsm"); + +function run_test() { + do_test_pending(); + run_next_test(); +} + +/** + * A file that we know exists and that can be used for reading. + */ +let EXISTING_FILE = "test_osfile_async_copy.js"; + +/** + * Fetch asynchronously the contents of a file using xpcom. + * + * Used for comparing xpcom-based results to os.file-based results. + * + * @param {string} path The _absolute_ path to the file. + * @return {promise} + * @resolves {string} The contents of the file. + */ +let reference_fetch_file = function reference_fetch_file(path) { + let promise = Promise.defer(); + let file = new FileUtils.File(path); + NetUtil.asyncFetch(file, + function(stream, status) { + if (!Components.isSuccessCode(status)) { + promise.reject(status); + return; + } + let result, reject; + try { + result = NetUtil.readInputStreamToString(stream, stream.available()); + } catch (x) { + reject = x; + } + stream.close(); + if (reject) { + promise.reject(reject); + } else { + promise.resolve(result); + } + }); + return promise.promise; +}; + +/** + * Compare asynchronously the contents two files using xpcom. + * + * Used for comparing xpcom-based results to os.file-based results. + * + * @param {string} a The _absolute_ path to the first file. + * @param {string} b The _absolute_ path to the second file. + * + * @resolves {null} + */ +let reference_compare_files = function reference_compare_files(a, b) { + let a_contents = yield reference_fetch_file(a); + let b_contents = yield reference_fetch_file(b); + // Not using do_check_eq to avoid dumping the whole file to the log. + // It is OK to === compare here, as both variables contain a string. + do_check_true(a_contents === b_contents); +}; + +/** + * Test to ensure that OS.File.copy works. + */ +function test_copymove(options = {}) { + let source = OS.Path.join((yield OS.File.getCurrentDirectory()), + EXISTING_FILE); + let dest = OS.Path.join(OS.Constants.Path.tmpDir, + "test_osfile_async_copy_dest.tmp"); + let dest2 = OS.Path.join(OS.Constants.Path.tmpDir, + "test_osfile_async_copy_dest2.tmp"); + try { + // 1. Test copy. + yield OS.File.copy(source, dest, options); + yield reference_compare_files(source, dest); + // 2. Test subsequent move. + yield OS.File.move(dest, dest2); + yield reference_compare_files(source, dest2); + // 3. Check that the moved file was really moved. + do_check_eq((yield OS.File.exists(dest)), false); + } finally { + try { + yield OS.File.remove(dest); + } catch (ex if ex.becauseNoSuchFile) { + // ignore + } + try { + yield OS.File.remove(dest2); + } catch (ex if ex.becauseNoSuchFile) { + // ignore + } + } +} + +// Regular copy test. +add_task(test_copymove); +// Userland copy test. +add_task(test_copymove.bind(null, {unixUserland: true})); + +add_task(do_test_finished); diff --git a/toolkit/components/osfile/tests/xpcshell/xpcshell.ini b/toolkit/components/osfile/tests/xpcshell/xpcshell.ini index d9594b56d49..34376d7fa31 100644 --- a/toolkit/components/osfile/tests/xpcshell/xpcshell.ini +++ b/toolkit/components/osfile/tests/xpcshell/xpcshell.ini @@ -6,6 +6,7 @@ tail = [test_path.js] [test_osfile_async.js] [test_osfile_async_bytes.js] +[test_osfile_async_copy.js] [test_profiledir.js] [test_logging.js] [test_creationDate.js]