From 3de3386b0b3d35a51edde2afeb7226084ff8a778 Mon Sep 17 00:00:00 2001 From: Austin Sullivan Date: Thu, 17 Nov 2022 13:10:21 -0800 Subject: [PATCH] FSA: Make removeEntry() take an exclusive lock removeEntry() currently takes a shared lock to allow for removal of a file with an open writable. Taking an exclusive lock makes the behavior consistent with move() and remove(), the other file-altering operations. Discussed in https://github.com/whatwg/fs/issues/39 TODO: link blink-dev PSA Fixed: 1254078 Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3 --- .../FileSystemDirectoryHandle-removeEntry.js | 84 +++++++++++++++++++ .../FileSystemWritableFileStream-write.js | 35 +------- .../FileSystemWritableFileStream.js | 20 ----- 3 files changed, 86 insertions(+), 53 deletions(-) diff --git a/fs/script-tests/FileSystemDirectoryHandle-removeEntry.js b/fs/script-tests/FileSystemDirectoryHandle-removeEntry.js index 108b9135f973fa..c0a5274709eb00 100644 --- a/fs/script-tests/FileSystemDirectoryHandle-removeEntry.js +++ b/fs/script-tests/FileSystemDirectoryHandle-removeEntry.js @@ -16,3 +16,87 @@ directory_test(async (t, root) => { await getSortedDirectoryEntries(root), ['file-to-keep']); }, 'removeEntry() while the file has an open writable fails'); + + +directory_test(async (t, root) => { + const handle = + await createFileWithContents(t, 'file-to-remove', '12345', root); + await createFileWithContents(t, 'file-to-keep', 'abc', root); + + const writable = await cleanup_writable(t, await handle.createWritable()); + await promise_rejects_dom( + t, 'InvalidModificationError', root.removeEntry('dir-to-remove')); + assert_array_equals( + await getSortedDirectoryEntries(root), ['dir-to-remove/']); + assert_array_equals(await getSortedDirectoryEntries(dir), ['file-in-dir']); +}, 'removeEntry() on a non-empty directory should fail'); + +directory_test(async (t, root) => { + // root + // ├──file-to-keep + // ├──dir-to-remove + // ├── file0 + // ├── dir1-in-dir + // │   └── file1 + // └── dir2 + const dir = await root.getDirectoryHandle('dir-to-remove', {create: true}); + await createFileWithContents(t, 'file-to-keep', 'abc', root); + await createEmptyFile(t, 'file0', dir); + const dir1_in_dir = await createDirectory(t, 'dir1-in-dir', dir); + await createEmptyFile(t, 'file1', dir1_in_dir); + await createDirectory(t, 'dir2-in-dir', dir); + + await root.removeEntry('dir-to-remove', {recursive: true}); + assert_array_equals(await getSortedDirectoryEntries(root), ['file-to-keep']); +}, 'removeEntry() on a directory recursively should delete all sub-items'); + +directory_test(async (t, root) => { + const dir = await createDirectory(t, 'dir', root); + await promise_rejects_js(t, TypeError, dir.removeEntry('')); +}, 'removeEntry() with empty name should fail'); + +directory_test(async (t, root) => { + const dir = await createDirectory(t, 'dir', root); + await promise_rejects_js(t, TypeError, dir.removeEntry(kCurrentDirectory)); +}, `removeEntry() with "${kCurrentDirectory}" name should fail`); + +directory_test(async (t, root) => { + const dir = await createDirectory(t, 'dir', root); + await promise_rejects_js(t, TypeError, dir.removeEntry(kParentDirectory)); +}, `removeEntry() with "${kParentDirectory}" name should fail`); + +directory_test(async (t, root) => { + const dir_name = 'dir-name'; + const dir = await createDirectory(t, dir_name, root); + + const file_name = 'file-name'; + await createEmptyFile(t, file_name, dir); + + for (let i = 0; i < kPathSeparators.length; ++i) { + const path_with_separator = `${dir_name}${kPathSeparators[i]}${file_name}`; + await promise_rejects_js( + t, TypeError, root.removeEntry(path_with_separator), + `removeEntry() must reject names containing "${kPathSeparators[i]}"`); + } +}, 'removeEntry() with a path separator should fail.'); + +directory_test(async (t, root) => { + const dir_name = 'dir-name'; + const dir = await createDirectory(t, dir_name, root); + + const handle = + await createFileWithContents(t, 'file-to-remove', '12345', dir); + await createFileWithContents(t, 'file-to-keep', 'abc', dir); + + const writable = await handle.createWritable(); + await promise_rejects_dom( + t, 'NoModificationAllowedError', root.removeEntry(dir_name)); + + await writable.close(); + assert_array_equals( + await getSortedDirectoryEntries(dir_name), + ['file-to-keep', 'file-to-remove']); + + await dir.removeEntry('file-to-remove'); + assert_array_equals(await getSortedDirectoryEntries(dir), ['file-to-keep']); +}, 'removeEntry() of a directory while a containing file has an open writable fails'); \ No newline at end of file diff --git a/fs/script-tests/FileSystemWritableFileStream-write.js b/fs/script-tests/FileSystemWritableFileStream-write.js index 70ba72f2245d39..1bab255666c0a4 100644 --- a/fs/script-tests/FileSystemWritableFileStream-write.js +++ b/fs/script-tests/FileSystemWritableFileStream-write.js @@ -194,17 +194,6 @@ directory_test(async (t, root) => { assert_equals(await getFileSize(handle), 3); }, 'write() with a valid typed array buffer'); -directory_test(async (t, root) => { - const dir = await createDirectory(t, 'parent_dir', root); - const file_name = 'close_fails_when_dir_removed.txt'; - const handle = await createEmptyFile(t, file_name, dir); - const stream = await handle.createWritable(); - await stream.write('foo'); - - await root.removeEntry('parent_dir', {recursive: true}); - await promise_rejects_dom(t, 'NotFoundError', stream.close()); -}, 'atomic writes: close() fails when parent directory is removed'); - directory_test(async (t, root) => { const handle = await createEmptyFile(t, 'atomic_writes.txt', root); const stream = await handle.createWritable(); @@ -276,22 +265,6 @@ directory_test(async (t, root) => { assert_equals(success_count, 1); }, 'atomic writes: only one close() operation may succeed'); -directory_test(async (t, root) => { - const dir = await createDirectory(t, 'parent_dir', root); - const file_name = 'atomic_writable_file_stream_persists_removed.txt'; - const handle = await createFileWithContents(t, file_name, 'foo', dir); - - const stream = await handle.createWritable(); - await stream.write('bar'); - - await dir.removeEntry(file_name); - await promise_rejects_dom(t, 'NotFoundError', getFileContents(handle)); - - await stream.close(); - assert_equals(await getFileContents(handle), 'bar'); - assert_equals(await getFileSize(handle), 3); -}, 'atomic writes: writable file stream persists file on close, even if file is removed'); - directory_test(async (t, root) => { const handle = await createEmptyFile(t, 'writer_written', root); const stream = await handle.createWritable(); @@ -316,7 +289,6 @@ directory_test(async (t, root) => { await promise_rejects_dom( t, "SyntaxError", stream.write({type: 'truncate'}), 'truncate without size'); - }, 'WriteParams: truncate missing size param'); directory_test(async (t, root) => { @@ -325,7 +297,6 @@ directory_test(async (t, root) => { await promise_rejects_dom( t, "SyntaxError", stream.write({type: 'write'}), 'write without data'); - }, 'WriteParams: write missing data param'); directory_test(async (t, root) => { @@ -334,17 +305,15 @@ directory_test(async (t, root) => { await promise_rejects_js( t, TypeError, stream.write({type: 'write', data: null}), 'write with null data'); - }, 'WriteParams: write null data param'); directory_test(async (t, root) => { - const handle = await createFileWithContents( - t, 'content.txt', 'seekable', root); + const handle = + await createFileWithContents(t, 'content.txt', 'seekable', root); const stream = await handle.createWritable(); await promise_rejects_dom( t, "SyntaxError", stream.write({type: 'seek'}), 'seek without position'); - }, 'WriteParams: seek missing position param'); directory_test(async (t, root) => { diff --git a/fs/script-tests/FileSystemWritableFileStream.js b/fs/script-tests/FileSystemWritableFileStream.js index d9c4f35335cd94..53e4fc1f28e4ae 100644 --- a/fs/script-tests/FileSystemWritableFileStream.js +++ b/fs/script-tests/FileSystemWritableFileStream.js @@ -33,26 +33,6 @@ directory_test(async (t, root) => { await promise_rejects_dom(t, 'NotFoundError', handle.createWritable()); }, 'createWritable() fails when parent directory is removed'); -directory_test(async (t, root) => { - const dir = await createDirectory(t, 'parent_dir', root); - const file_name = 'write_fails_when_dir_removed.txt'; - const handle = await createEmptyFile(t, file_name, dir); - const stream = await handle.createWritable(); - - await root.removeEntry('parent_dir', {recursive: true}); - await promise_rejects_dom(t, 'NotFoundError', stream.write('foo')); -}, 'write() fails when parent directory is removed'); - -directory_test(async (t, root) => { - const dir = await createDirectory(t, 'parent_dir', root); - const file_name = 'truncate_fails_when_dir_removed.txt'; - const handle = await createEmptyFile(t, file_name, dir); - const stream = await handle.createWritable(); - - await root.removeEntry('parent_dir', {recursive: true}); - await promise_rejects_dom(t, 'NotFoundError', stream.truncate(0)); -}, 'truncate() fails when parent directory is removed'); - directory_test(async (t, root) => { const handle = await createFileWithContents( t, 'atomic_file_is_copied.txt', 'fooks', root);