Skip to content

Commit

Permalink
FSA: Make removeEntry() take an exclusive lock
Browse files Browse the repository at this point in the history 
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 whatwg/fs#39

TODO: link blink-dev PSA

Fixed: 1254078
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
  • Loading branch information
@a-sully @chromium-wpt-export-bot
a-sully authored and chromium-wpt-export-bot committed Nov 17, 2022
1 parent 3cab330 commit 3de3386
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 53 deletions.
84 changes: 84 additions & 0 deletions fs/script-tests/FileSystemDirectoryHandle-removeEntry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
35 changes: 2 additions & 33 deletions fs/script-tests/FileSystemWritableFileStream-write.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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) => {
Expand All @@ -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) => {
Expand All @@ -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) => {
Expand Down
20 changes: 0 additions & 20 deletions fs/script-tests/FileSystemWritableFileStream.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 3de3386

Please sign in to comment.